[PATCH 7/7] glamor: Replace CompositeGlyphs code

[Keith Packard]

Eric Anholt <eric at anholt.net> 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 <eric at anholt.net>

Thanks for your review. As with the last review, here's a patch which
takes your advice in most cases:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-glamor-Respond-to-Eric-s-review-of-Replace-Composite.patch
Type: text/x-diff
Size: 8312 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20150512/5f7fe550/attachment-0001.patch>
-------------- next part --------------

-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20150512/5f7fe550/attachment-0001.sig>