Eric Anholt <[email protected]> writes:
> I think this explanation is what I wanted in this block: > > /* If we're dealing with 1-bit glyphs, we upload them to > * the cache as normal 8-bit alpha, since that's what GL > * can handle. > */ > assert(glyph_draw->bitsPerPixel == 1); > assert(atlas_draw->bitsPerPixel == 8); I think those should both be depth instead of bpp, but yes, that makes things clearer. > It looks like this picture is never used, just the pixmap it's > wrapping. Store the pixmap, instead? Nice. I was using render operations for a while, but that's since migrated to being just pixmap ops. Fixed. > Debugging errorf left over Yeah, I've been leaving this in my version while I was having the disappearing glyph problem. I think Dave Airlie found the actual bug (in an earlier patch in the series), so I can probably remove this now. > missing space Fixed. > Isn't vec2 pos unused here? Drop this line of source? Yes. Tested by forcing glsl version to 120; > duplicated line here You'd think the compiler would complain about that... > OK, this is the second copy of this loop, and it's still weird but I > don't really see a better way to do this. Could split out a helper function, but that'd involve a pile of parameters, I suspect? > I think an appropriate name for this variable would be "glyphs_queued" > (or glyphs_in_vbo). I like 'glyphs_queued'. > My feedback all seems minor to me. With however much of it you want to > take, > > Reviewed-by: Eric Anholt <[email protected]> Thanks for your review. As with the last review, here's a patch which takes your advice in most cases:
From 5dec4e5b0243f2b995fbae72e06c08b620112296 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Tue, 12 May 2015 16:38:18 -0700 Subject: [PATCH] glamor: Respond to Eric's review of Replace CompositeGlyphs code This patch is designed to be folded into the previous patch, but is shown separately here to make the response to the review clear. Signed-off-by: Keith Packard <[email protected]> --- glamor/glamor_composite_glyphs.c | 59 +++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/glamor/glamor_composite_glyphs.c b/glamor/glamor_composite_glyphs.c index 41c6758..ee3cd75 100644 --- a/glamor/glamor_composite_glyphs.c +++ b/glamor/glamor_composite_glyphs.c @@ -39,7 +39,7 @@ struct glamor_glyph_private { }; struct glamor_glyph_atlas { - PicturePtr atlas; + PixmapPtr atlas; PictFormatPtr format; int dim; int max_glyph_dim; @@ -80,6 +80,13 @@ glamor_copy_glyph(PixmapPtr glyph_pixmap, if (!scratch_gc) return; + /* If we're dealing with 1-bit glyphs, we upload them to + * the cache as normal 8-bit alpha, since that's what GL + * can handle. + */ + assert(glyph_draw->depth == 1); + assert(atlas_draw->depth == 8); + changes[0].val = 0xff; changes[1].val = 0x00; if (ChangeGC(NullClient, scratch_gc, @@ -103,21 +110,14 @@ glamor_copy_glyph(PixmapPtr glyph_pixmap, static Bool glamor_glyph_atlas_init(ScreenPtr screen, struct glamor_glyph_atlas *atlas) { - PixmapPtr pixmap; - int error; PictFormatPtr format = atlas->format; - pixmap = glamor_create_pixmap(screen, atlas->dim, atlas->dim, format->depth, 0); - if (!pixmap) - return FALSE; - atlas->atlas = CreatePicture(0, &pixmap->drawable, format, 0, NULL, serverClient, &error); + atlas->atlas = glamor_create_pixmap(screen, atlas->dim, atlas->dim, format->depth, 0); atlas->x = 0; atlas->y = 0; atlas->row_height = 0; atlas->serial++; - ErrorF("Create new atlas depth %d serial %d nglyph %d\n", format->depth, atlas->serial, atlas->nglyph); atlas->nglyph = 0; - glamor_destroy_pixmap(pixmap); return TRUE; } @@ -130,7 +130,7 @@ glamor_glyph_can_add(struct glamor_glyph_atlas *atlas, PicturePtr glyph) if (atlas->x + glyph_draw->width > atlas->dim) { atlas->x = 0; atlas->y += atlas->row_height; - atlas->row_height =0; + atlas->row_height = 0; } /* Check for overfull */ @@ -147,7 +147,7 @@ glamor_glyph_add(struct glamor_glyph_atlas *atlas, PicturePtr glyph) PixmapPtr glyph_pixmap = (PixmapPtr) glyph_draw; struct glamor_glyph_private *glyph_priv = glamor_get_glyph_private(glyph_pixmap); - glamor_copy_glyph(glyph_pixmap, atlas->atlas->pDrawable, atlas->x, atlas->y); + glamor_copy_glyph(glyph_pixmap, &atlas->atlas->drawable, atlas->x, atlas->y); glyph_priv->x = atlas->x; glyph_priv->y = atlas->y; @@ -182,12 +182,10 @@ static const glamor_facet glamor_facet_composite_glyphs_120 = { .vs_vars = ("attribute vec2 primitive;\n" "attribute vec2 source;\n" "varying vec2 glyph_pos;\n"), - .vs_exec = (" vec2 pos = vec2(0.0,0.0);\n" - GLAMOR_POS(gl_Position, primitive) + .vs_exec = (GLAMOR_POS(gl_Position, primitive) " glyph_pos = source.xy / ATLAS_DIM.0;\n"), .fs_vars = ("varying vec2 glyph_pos;\n"), .fs_exec = (" vec4 mask = texture2D(atlas, glyph_pos);\n"), - .fs_exec = (" vec4 mask = texture2D(atlas, glyph_pos);\n"), .source_name = "source", .locations = glamor_program_location_atlas, }; @@ -220,8 +218,7 @@ glamor_glyphs_flush(CARD8 op, PicturePtr src, PicturePtr dst, { DrawablePtr drawable = dst->pDrawable; glamor_screen_private *glamor_priv = glamor_get_screen_private(drawable->pScreen); - PicturePtr atlas_pict = atlas->atlas; - PixmapPtr atlas_pixmap = glamor_get_drawable_pixmap(atlas_pict->pDrawable); + PixmapPtr atlas_pixmap = atlas->atlas; glamor_pixmap_private *atlas_priv = glamor_get_pixmap_private(atlas_pixmap); glamor_pixmap_fbo *atlas_fbo = glamor_pixmap_fbo_at(atlas_priv, 0, 0); PixmapPtr pixmap = glamor_get_drawable_pixmap(drawable); @@ -333,7 +330,7 @@ glamor_composite_glyphs(CARD8 op, INT16 y_src, int nlist, GlyphListPtr list, GlyphPtr *glyphs) { - int this_time; + int glyphs_queued; GLshort *v = NULL; DrawablePtr drawable = dst->pDrawable; ScreenPtr screen = drawable->pScreen; @@ -352,7 +349,7 @@ glamor_composite_glyphs(CARD8 op, glamor_make_current(glamor_priv); - this_time = 0; + glyphs_queued = 0; while (nlist--) { x += list->xOff; @@ -374,9 +371,9 @@ glamor_composite_glyphs(CARD8 op, glyph_draw->height > next_atlas->max_glyph_dim || (glyph_pix_priv != 0 && glyph_pix_priv->type != GLAMOR_MEMORY))) { - if (this_time) { - glamor_glyphs_flush(op, src, dst, prog, glyph_atlas, this_time); - this_time = 0; + if (glyphs_queued) { + glamor_glyphs_flush(op, src, dst, prog, glyph_atlas, glyphs_queued); + glyphs_queued = 0; } bail_one: glamor_composite(op, src, glyph_pict, dst, @@ -388,17 +385,17 @@ glamor_composite_glyphs(CARD8 op, struct glamor_glyph_private *glyph_priv = glamor_get_glyph_private((PixmapPtr)(glyph_draw)); if (_X_UNLIKELY(next_atlas != glyph_atlas)) { - if (this_time) { - glamor_glyphs_flush(op, src, dst, prog, glyph_atlas, this_time); - this_time = 0; + if (glyphs_queued) { + glamor_glyphs_flush(op, src, dst, prog, glyph_atlas, glyphs_queued); + glyphs_queued = 0; } glyph_atlas = next_atlas; } if (_X_UNLIKELY(glyph_priv->serial != glyph_atlas->serial)) { if (!glamor_glyph_can_add(glyph_atlas, glyph_pict)) { - if (this_time) { - glamor_glyphs_flush(op, src, dst, prog, glyph_atlas, this_time); - this_time = 0; + if (glyphs_queued) { + glamor_glyphs_flush(op, src, dst, prog, glyph_atlas, glyphs_queued); + glyphs_queued = 0; } if (glyph_atlas->atlas) { FreePicture(glyph_atlas->atlas, 0); @@ -409,7 +406,7 @@ glamor_composite_glyphs(CARD8 op, glamor_glyph_atlas_init(screen, glyph_atlas); glamor_glyph_add(glyph_atlas, glyph_pict); } - if (_X_UNLIKELY(this_time == 0)) { + if (_X_UNLIKELY(glyphs_queued == 0)) { if (glamor_glyph_use_130(glamor_priv)) prog = glamor_setup_program_render(op, src, glyph_pict, dst, glyphs_program, @@ -470,7 +467,7 @@ glamor_composite_glyphs(CARD8 op, v[3] = glyph_priv->y + glyph_draw->height; v += 4; } - this_time++; + glyphs_queued++; } } x += glyph->info.xOff; @@ -479,8 +476,8 @@ glamor_composite_glyphs(CARD8 op, } } - if (this_time) - glamor_glyphs_flush(op, src, dst, prog, glyph_atlas, this_time); + if (glyphs_queued) + glamor_glyphs_flush(op, src, dst, prog, glyph_atlas, glyphs_queued); return; } -- 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
