Eric Anholt <[email protected]> writes: > Summary: Tiny nitpicks, one functional concern. > > Keith Packard <[email protected]> writes: >> This extends the existing API to support options needed for render >> accleratoin, including an additional fragment, 'combine', (which > > acceleration > >> provides a place to perform the source IN mask operation before the >> final OP dest state) and an addtional 'defines' parameter which > > additional
Yeah, typos in the commit message fixed.
> This would be nice as:
> [glamor_program_alpha_normal] = " gl_FragColor = source *
> mask.a;\n",
> [glamor_program_alpha_ca_first] = " gl_FragColor = source.a *
> mask;\n",
> [glamor_program_alpha_ca_second] = " gl_FragColor = source *
> mask;\n"
Agreed.
> missing space
Fixed.
> I don't think the glamor_program_source_picture path is complete -- what
> about src->transform and src->alphaMap?
Oops. I've made it bail in that case for now. Transforms shouldn't be
terribly difficult, but we can pend that for now.
> I'd be just fine with the text path only doing solids, and glyph-masking
> a texture being something we fall back to the slow path for. If so, and
> we need to do 1x1 repeating src (I think that's how a lot of text is
> drawn still, right?), we might want to drop fill_pos in the facet and
> just texture2D(sampler, vec2(0.5)).
Checking for transform and alphaMap was easy to add, and leaving in the
large texture support isn't a huge burden. I did add a custom 1x1
version to see if that's faster (it isn't on my machine), but it might
be on something which executes more slowly?
> This was hard to read, and I think explicit might be nicer:
>
> prog = &program_render->progs[source_type][alpha];
> if (!glamor_setup_one_program_render(screen, prog, source_type, alpha,
> prim, defines))
> return NULL;
> if (alpha == glamor_program_alpha_ca_first) {
> /* Make sure we can also build the second program before deciding
> * to use this path.
> */
> if (!glamor_setup_one_program_render(screen,
>
> &program_render->progs[source_type][glamor_program_alpha_ca_second],
> source_type,
> glamor_program_alpha_ca_second, prim,
> defines)) {
> return NULL;
> }
> }
Your version is massively easier to read.
Here's a patch which addresses these issues on top of the current code;
if you like what it does, I'll squash it together with the original
patch.
From 16dced35ffdb65559a25a214d7f3b701903cca81 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Tue, 12 May 2015 16:06:18 -0700 Subject: [PATCH] glamor: Respond to Eric's review comments about new Render glamor_program API Signed-off-by: Keith Packard <[email protected]> --- glamor/glamor_copy.c | 4 +-- glamor/glamor_program.c | 89 +++++++++++++++++++++++++++++++---------------- glamor/glamor_program.h | 12 ++++--- glamor/glamor_transform.c | 18 +++++++--- glamor/glamor_transform.h | 3 ++ 5 files changed, 84 insertions(+), 42 deletions(-) diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c index 2ffe645..96273b8 100644 --- a/glamor/glamor_copy.c +++ b/glamor/glamor_copy.c @@ -53,7 +53,7 @@ static const glamor_facet glamor_facet_copyarea = { .vs_exec = (GLAMOR_POS(gl_Position, primitive.xy) " fill_pos = (fill_offset + primitive.xy) * fill_size_inv;\n"), .fs_exec = " gl_FragColor = texture2D(sampler, fill_pos);\n", - .locations = glamor_program_location_fill, + .locations = glamor_program_location_tex | glamor_program_location_texpos, .use = use_copyarea, }; @@ -140,7 +140,7 @@ static const glamor_facet glamor_facet_copyplane = { " gl_FragColor = fg;\n" " else\n" " gl_FragColor = bg;\n"), - .locations = glamor_program_location_fill|glamor_program_location_fg|glamor_program_location_bg|glamor_program_location_bitplane, + .locations = glamor_program_location_tex|glamor_program_location_texpos|glamor_program_location_fg|glamor_program_location_bg|glamor_program_location_bitplane, .use = use_copyplane, }; diff --git a/glamor/glamor_program.c b/glamor/glamor_program.c index 94b2b5c..d55b84b 100644 --- a/glamor/glamor_program.c +++ b/glamor/glamor_program.c @@ -47,7 +47,7 @@ static const glamor_facet glamor_fill_tile = { .name = "tile", .vs_exec = " fill_pos = (fill_offset + primitive.xy + pos) * fill_size_inv;\n", .fs_exec = " gl_FragColor = texture2D(sampler, fill_pos);\n", - .locations = glamor_program_location_fill, + .locations = glamor_program_location_tex | glamor_program_location_texpos, .use = use_tile, }; @@ -66,7 +66,7 @@ static const glamor_facet glamor_fill_stipple = { " if (a == 0.0)\n" " discard;\n" " gl_FragColor = fg;\n"), - .locations = glamor_program_location_fg | glamor_program_location_fill, + .locations = glamor_program_location_fg | glamor_program_location_tex | glamor_program_location_texpos, .use = use_stipple, }; @@ -87,7 +87,7 @@ static const glamor_facet glamor_fill_opaque_stipple = { " gl_FragColor = bg;\n" " else\n" " gl_FragColor = fg;\n"), - .locations = glamor_program_location_fg | glamor_program_location_bg | glamor_program_location_fill, + .locations = glamor_program_location_fg | glamor_program_location_bg | glamor_program_location_tex | glamor_program_location_texpos, .use = use_opaque_stipple }; @@ -114,12 +114,15 @@ static glamor_location_var location_vars[] = { .fs_vars = "uniform vec4 bg;\n" }, { - .location = glamor_program_location_fill, + .location = glamor_program_location_tex, + .fs_vars = "uniform sampler2D sampler;\n" + }, + { + .location = glamor_program_location_texpos, .vs_vars = ("uniform vec2 fill_offset;\n" "uniform vec2 fill_size_inv;\n" "varying vec2 fill_pos;\n"), - .fs_vars = ("uniform sampler2D sampler;\n" - "uniform vec2 fill_size_inv;\n" + .fs_vars = ("uniform vec2 fill_size_inv;\n" "varying vec2 fill_pos;\n") }, { @@ -222,7 +225,7 @@ static const glamor_facet facet_null_fill = { .name = "" }; -#define DBG 0 +#define DBG 1 static GLint glamor_get_uniform(glamor_program *prog, @@ -349,8 +352,8 @@ glamor_build_program(ScreenPtr screen, prog->matrix_uniform = glamor_get_uniform(prog, glamor_program_location_none, "v_matrix"); prog->fg_uniform = glamor_get_uniform(prog, glamor_program_location_fg, "fg"); prog->bg_uniform = glamor_get_uniform(prog, glamor_program_location_bg, "bg"); - prog->fill_offset_uniform = glamor_get_uniform(prog, glamor_program_location_fill, "fill_offset"); - prog->fill_size_inv_uniform = glamor_get_uniform(prog, glamor_program_location_fill, "fill_size_inv"); + prog->fill_offset_uniform = glamor_get_uniform(prog, glamor_program_location_texpos, "fill_offset"); + prog->fill_size_inv_uniform = glamor_get_uniform(prog, glamor_program_location_texpos, "fill_size_inv"); prog->font_uniform = glamor_get_uniform(prog, glamor_program_location_font, "font"); prog->bitplane_uniform = glamor_get_uniform(prog, glamor_program_location_bitplane, "bitplane"); prog->bitmul_uniform = glamor_get_uniform(prog, glamor_program_location_bitplane, "bitmul"); @@ -512,27 +515,41 @@ use_source_picture(CARD8 op, PicturePtr src, PicturePtr dst, glamor_program *pro 0, 0, prog->fill_offset_uniform, prog->fill_size_inv_uniform); - - return TRUE; } const glamor_facet glamor_source_picture = { .name = "render_picture", .vs_exec = " fill_pos = (fill_offset + primitive.xy + pos) * fill_size_inv;\n", .fs_exec = " vec4 source = texture2D(sampler, fill_pos);\n", - .locations = glamor_program_location_fill, + .locations = glamor_program_location_tex | glamor_program_location_texpos, .use_render = use_source_picture, }; +static Bool +use_source_1x1_picture(CARD8 op, PicturePtr src, PicturePtr dst, glamor_program *prog) +{ + glamor_set_blend(op, prog->alpha, dst); + + return glamor_set_texture_pixmap((PixmapPtr) src->pDrawable); +} + +const glamor_facet glamor_source_1x1_picture = { + .name = "render_picture", + .fs_exec = " vec4 source = texture2D(sampler, vec2(0.5));\n", + .locations = glamor_program_location_tex, + .use_render = use_source_1x1_picture, +}; + const glamor_facet *glamor_facet_source[glamor_program_source_count] = { [glamor_program_source_solid] = &glamor_source_solid, [glamor_program_source_picture] = &glamor_source_picture, + [glamor_program_source_1x1_picture] = &glamor_source_1x1_picture, }; static const char *glamor_combine[] = { - " gl_FragColor = source * mask.a;\n", - " gl_FragColor = source.a * mask;\n", - " gl_FragColor = source * mask;\n" + [glamor_program_alpha_normal] = " gl_FragColor = source * mask.a;\n", + [glamor_program_alpha_ca_first] = " gl_FragColor = source.a * mask;\n", + [glamor_program_alpha_ca_second] = " gl_FragColor = source * mask;\n" }; static Bool @@ -543,7 +560,7 @@ glamor_setup_one_program_render(ScreenPtr screen, const glamor_facet *prim, const char *defines) { - if(prog->failed) + if (prog->failed) return FALSE; if (!prog->prog) { @@ -572,9 +589,9 @@ glamor_setup_program_render(CARD8 op, ScreenPtr screen = dst->pDrawable->pScreen; glamor_program_alpha alpha; glamor_program_source source_type; - glamor_program *prog, *ret; + glamor_program *prog; - if (op > sizeof (composite_op_info)/sizeof (composite_op_info[0])) + if (op > ARRAY_SIZE(composite_op_info)) return NULL; if (glamor_is_component_alpha(mask)) { @@ -585,9 +602,17 @@ glamor_setup_program_render(CARD8 op, } else alpha = glamor_program_alpha_normal; - if (src->pDrawable) - source_type = glamor_program_source_picture; - else { + if (src->pDrawable) { + + /* Can't do transforms or alphamaps yet */ + if (src->transform || src->alphaMap) + return NULL; + + if (src->pDrawable->width == 1 && src->pDrawable->height == 1 && src->repeat) + source_type = glamor_program_source_1x1_picture; + else + source_type = glamor_program_source_picture; + } else { SourcePictPtr sp = src->pSourcePict; if (!sp) return NULL; @@ -600,18 +625,22 @@ glamor_setup_program_render(CARD8 op, } } - ret = &program_render->progs[source_type][alpha]; - for (;;) { - prog = &program_render->progs[source_type][alpha]; + prog = &program_render->progs[source_type][alpha]; + if (!glamor_setup_one_program_render(screen, prog, source_type, alpha, prim, defines)) + return NULL; - if (!glamor_setup_one_program_render(screen, prog, source_type, alpha, prim, defines)) - return NULL; + if (alpha == glamor_program_alpha_ca_first) { - if (alpha != glamor_program_alpha_ca_first) - break; - alpha++; + /* Make sure we can also build the second program before + * deciding to use this path. + */ + if (!glamor_setup_one_program_render(screen, + &program_render->progs[source_type][glamor_program_alpha_ca_second], + source_type, glamor_program_alpha_ca_second, prim, + defines)) + return NULL; } - return ret; + return prog; } Bool diff --git a/glamor/glamor_program.h b/glamor/glamor_program.h index f034420..894ccd3 100644 --- a/glamor/glamor_program.h +++ b/glamor/glamor_program.h @@ -27,11 +27,12 @@ typedef enum { glamor_program_location_none = 0, glamor_program_location_fg = 1, glamor_program_location_bg = 2, - glamor_program_location_fill = 4, - glamor_program_location_font = 8, - glamor_program_location_bitplane = 16, - glamor_program_location_dash = 32, - glamor_program_location_atlas = 64, + glamor_program_location_tex = 4, + glamor_program_location_texpos = 8, + glamor_program_location_font = 16, + glamor_program_location_bitplane = 32, + glamor_program_location_dash = 64, + glamor_program_location_atlas = 128, } glamor_program_location; typedef enum { @@ -119,6 +120,7 @@ glamor_use_program_fill(PixmapPtr pixmap, typedef enum { glamor_program_source_solid, glamor_program_source_picture, + glamor_program_source_1x1_picture, glamor_program_source_count, } glamor_program_source; diff --git a/glamor/glamor_transform.c b/glamor/glamor_transform.c index e94bca8..83855dc 100644 --- a/glamor/glamor_transform.c +++ b/glamor/glamor_transform.c @@ -155,11 +155,7 @@ glamor_set_solid(PixmapPtr pixmap, } Bool -glamor_set_texture(PixmapPtr texture, - int off_x, - int off_y, - GLint offset_uniform, - GLint size_inv_uniform) +glamor_set_texture_pixmap(PixmapPtr texture) { glamor_pixmap_private *texture_priv; @@ -173,6 +169,18 @@ glamor_set_texture(PixmapPtr texture, glActiveTexture(GL_TEXTURE0); glBindTexture(GL_TEXTURE_2D, texture_priv->fbo->tex); + return TRUE; +} + +Bool +glamor_set_texture(PixmapPtr texture, + int off_x, + int off_y, + GLint offset_uniform, + GLint size_inv_uniform) +{ + if (!glamor_set_texture_pixmap(texture)) + return FALSE; glUniform2f(offset_uniform, off_x, off_y); glUniform2f(size_inv_uniform, 1.0f/texture->drawable.width, 1.0f/texture->drawable.height); diff --git a/glamor/glamor_transform.h b/glamor/glamor_transform.h index 1d8eed4..dca6a26 100644 --- a/glamor/glamor_transform.h +++ b/glamor/glamor_transform.h @@ -39,6 +39,9 @@ glamor_set_color(PixmapPtr pixmap, GLint uniform); Bool +glamor_set_texture_pixmap(PixmapPtr texture); + +Bool glamor_set_texture(PixmapPtr texture, int off_x, int off_y, -- 2.1.4
-- -keith
signature.asc
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
