[PATCH 10/10] glamor: Add glamor_program based poly_text and image_text

[Keith Packard]

Markus Wick <markus at selfnet.de> writes:


> more important to only get one mipmap level: max_level = 0

I copied this code from elsewhere; do you have a substitute suggestion?

> Here you hope that the driver detects that this texture isn't in use. 
> Else you maybe get some kind of async upload / stalling.
> How often do we create new fonts?

Applications using core fonts generally create them once at startup
time, or perhaps a few times while running. But, yeah, it should use a
temporary texture ID instead of smashing 0.

> Is a font always realized? I wonder as we reallize on font_get.

"realized" just means that I've uploaded the glyph data to the GPU; it's
per-GPU, and hence per-screen to us. I delay until the first font_get in
case you've got multiple screens and use a font on only a subset.

> In glamor.c, we always save the function pointers and reset them on 
> destruction. Is this needed?

We assume that the GL context is destroyed on server reset, which will
destroy all objects related to that, including the compiled functions.

> Also, all others are set there, but I like the idea to set them in each 
> _init function.

Building them all on the fly and letting GL clean up seems simplest.

>> +static void
>> +glamor_get_glyphs(FontPtr font, glamor_font_t *glamor_font,
>> +                  int count, char *chars, Bool sixteen, CharInfoPtr 
>> *charinfo)
>> +{
>> +    unsigned long n;
>
> Please don't use variables with one letter.
> I'm fine with loop indices, but what is "n"?

Yeah, number of glyphs returned from GetGlyphs. It's now 'nglyphs'

> I'd extract the format based on "sixteen" and merge the first if/else. 
> This is a bit much copy&paste.

Ok, I'll restructure it a bit then.

>> +#define GLYPHS_PER_DRAW        255
>
> This isn't used, is it?

Nope, good catch.

>> +    /* Set the font as texture 1 */
>> +
>
> Why texture 1? Texture 0 is the common one.

Texture 0 is the fill texture, texture 1 is use for the font.

>> +    glActiveTexture(GL_TEXTURE1);
>> +    glBindTexture(GL_TEXTURE_2D, glamor_font->texture_id);
>> +    glUniform1i(prog->font_uniform, 1);
>
> I guess you want to set the texture unit 1? As this is const, there is 
> no need to do this every time. Once per shader should be fine.

I'm sure I just don't understand the GL API somehow, but I'm not sure
what you're suggesting. There's one texture per font, and I need to
select the right font before drawing.

>> +    /* Set the vertex coordinates */
>> +    g = 0;
>
> What is "g"?

The number of glyphs actually rasterized. I've replaced that with
'nglyph', which seems a bit more descriptive.

>> +
>> +    for (c = 0; c < count; c++) {
>> +        if ((ci = *charinfo++)) {
>> +            int     x1 = x + ci->metrics.leftSideBearing;
>> +            int     y1 = y - ci->metrics.ascent;
>> +            int     width = GLYPHWIDTHPIXELS(ci);
>> +            int     height = GLYPHHEIGHTPIXELS(ci);
>> +            int     tx, ty = 0;
>
> As non of this variables is used again, I'd directly write into v[x] 
> instead.

The compiler will optimize that away; I like having the vertex elements
well identified instead of being just random looking myself.

>> +                    col += row << 8;
>> +                    row = 0;
>
> row won't be read again.

Yeah, that was leftover debug stuff. Removed.

> Do you think it's worth to precheck this box vs the current pixmap tile?
> As this scissor test (and maybe this precheck) is very common, shall we 
> move it into a util function (eg glamor_box_loop(gc, off_x, off_y))?

No, I don't expect the tiles to *ever* be used for real programs; the
tiles are set to the maximum size supported by the GPU, and the screen
is unlikely to be larger than that, so applications will presumably
always render to things smaller than the screen. And, the goal of all of
this code is to eliminate CPU overhead; a raft of extra compares isn't
going to help here.

>> +static const char vs_text[] =

> Isn't used any more.

Oops. Older version of the code; I've removed it.

Thanks for your review!

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140320/7244b6ae/attachment-0001.sig>