1037340 - gfxFont::Draw is a hard-to-maintain mess of code

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

Currently, gfxFont::Draw is about 500 lines, with two separate copies of the "draw a sequence of glyphs" loop, two copies of code to read and interpret "simple" and "detailed" glyph records, track current position, draw missing-glyph boxes; and FOUR copies of code to handle possible color glyphs, SVG glyphs, and synthetic-bolding.

This is hard to read and maintain, and will make it harder to implement vertical textrun support as everything has to be done (and tested!) in all these copies of almost-equivalent code paths.

I propose to refactor this so that we have ONE loop over the glyphs in the textrun; ONE place where we interpret the glyph records; ONE place where we implement support for color glyphs and synthetic bolding.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: refactor gfxFont::Draw for better maintainability.

Tryserver says this appears to work: https://tbpl.mozilla.org/?tree=Try&rev=c51dc7614c44.

Comment 2

[Jonathan Kew [:jfkthame]]

Comment on attachment 8454314 [details] [diff] [review]
refactor gfxFont::Draw for better maintainability.

Turns out that some of this code has just been removed in bug 933019; however, I'd still like to do a refactoring of what's left so as to remove duplication of the color-glyph, svg-glyph and synthetic-bold support. New smaller :) patch will be forthcoming.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - refactor gfxFont::Draw for better maintainability.

(Updated for the post-bug-933019 world.) Refactor gfxFont::Draw to unify the handling of color and SVG glyphs and synthetic bolding for either SimpleGlyph or DetailedGlyph records. This will make it easier to implement the extension to vertical textruns, without having to patch as many places.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - hoist the setup of some unvarying parameters from gfxFont::Draw up to gfxTextRun::Draw.

Doing the above refactoring, it became obvious that we're doing a bunch of drawing setup in gfxFont::Draw that could actually be done once in gfxTextRun::Draw instead of repeated per-font when the textrun contains mixed fonts. If we split the drawing parameters into per-textrun and per-font records, so that we can hoist much of this setup out of the loop over GlyphRuns.

Comment 5

[Jonathan Kew [:jfkthame]]

BTW, besides making the code more manageable (and >100 lines shorter, though much of that is the removal of dead code left by bug 933019), these patches together appear to give me about a 5% boost to the overall score on http://people.mozilla.org/~jdaggett/textbench.

(Tested on OS X, averaging 5 runs each with and without the patches; warm caches. Although there's a few % variability in the result, *all* the patched runs scored higher than *any* of the unpatched.)

Comment 6

[John Daggett (:jtd)]

Comment on attachment 8454984 [details] [diff] [review]
pt 1 - refactor gfxFont::Draw for better maintainability.

In general, this looks great. The code is now actually readable! But given that it's more readable, it's also easier to spot places where we could be helping out the compiler to optimize the fast path better I think.

Within gfxFont::DrawGlyphs:

>     for (uint32_t i = 0; i < aCount; ++i, ++glyphData) {
>         if (glyphData->IsSimpleGlyph()) {
>             DrawOneGlyph(glyphData->GetSimpleGlyph(),
>                          glyphData->GetSimpleAdvance(),
>                          aPt, buffer, &emittedGlyphs);
>         } else {

I'm a little concerned here that pushing code into DrawOneGlyph is going to make it harder for the compiler to figure out how to streamline this loop. It also seems like we our current code and this code still hides the fast path case. 

I think (1) most textruns have only simple glyphs and (2) within DrawOneGlyph, haveSVGGlyphs, haveColorGlyphs, extraStrikes are all false or 0. So what falls out of the DrawGlyphs/DrawOneGlyph code above is roughly:

    for (uint32_t i = 0; i < aCount; ++i, ++glyphData) {
        aGlyphID = glyphData->GetSimpleGlyph();
        aAdvance = glyphData->GetSimpleAdvance();
        const TextRunDrawParams& runParams(buffer.mRunParams);

        double glyphX;
        if (runParams.isRTL) {
            aPt->x -= aAdvance;
            glyphX = aPt->x;
        } else {
            glyphX = aPt->x;
            aPt->x += aAdvance;
        }
        gfxPoint devPt(ToDeviceUnits(glyphX, runParams.devPerApp),
                       ToDeviceUnits(aPt->y, runParams.devPerApp));

        buffer.OutputGlyph(aGlyphID, devPt);
        emittedGlyphs = true;
    }

If this fast path case was called out, using the code you have for other cases would be fine I think.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - add a fast-path for drawing textruns with only simple glyphs using a simple font.

I've experimented with adding a fast-path for simple glyphs/simple fonts, but I'm not seeing an measureable improvement in textbench scores. Given that one of the aims here was to minimize the number of places where we'll need to modify code for vertical textrun support, I'd prefer not to do this unless there's a clear benefit. Fastpath patch attached if you want to give it a try... I've only tested it on OS X as yet.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - refactor gfxFont::Draw for better maintainability.

Rebased to current trunk.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - hoist the setup of some unvarying parameters from gfxFont::Draw up to gfxTextRun::Draw.

Rebased to current trunk.

Comment 10

[John Daggett (:jtd)]

Comment on attachment 8462012 [details] [diff] [review]
pt 1 - refactor gfxFont::Draw for better maintainability.

Review of attachment 8462012 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with suggested changes.

::: gfx/thebes/gfxFont.cpp
@@ +3114,4 @@
>          return &mGlyphBuffer[mNumGlyphs++];
>      }
>  
> +    void Flush(bool aFinish)

This actually really isn't a "flush" function, it's more like "AddToBufferAndFlushIfNeeded". Not a big deal but I think we should add a comment here explaining the significance of the aFinish parameter. Seeing a "flush" operation for every glyph initially raised concerns until I saw what this Flush method actually does.

@@ +3125,5 @@
>              Glyph *begin = &mGlyphBuffer[0];
>              Glyph *end = &mGlyphBuffer[mNumGlyphs];
>              std::reverse(begin, end);
>          }
>          

Remove whitespace

Comment 11

[John Daggett (:jtd)]

Comment on attachment 8462013 [details] [diff] [review]
pt 2 - hoist the setup of some unvarying parameters from gfxFont::Draw up to gfxTextRun::Draw.

Review of attachment 8462013 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits fixed.

::: gfx/thebes/gfxFont.cpp
@@ +7100,5 @@
> +    params.isRTL = IsRightToLeft();
> +    params.direction = direction;
> +    params.drawMode = aDrawMode;
> +    params.callbacks = aCallbacks;
> +    params.contextPaint = aContextPaint;    

Remove trailing whitespace.

@@ +7133,5 @@
>          uint32_t end = iter.GetStringEnd();
>          uint32_t ligatureRunStart = start;
>          uint32_t ligatureRunEnd = end;
>          ShrinkToLigatureBoundaries(&ligatureRunStart, &ligatureRunEnd);
>          

Ditto

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef8680643a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6758d669350

Comment 13

[Jonathan Kew [:jfkthame]]

Hmm, I notice you marked patch 3 as "r+" as well, although I hadn't requested review on that one. As per comment 7, I didn't see any measurable improvement from adding this. Unless there's a clear benefit, I'd prefer to avoid the extra codepath.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ef8680643a8
https://hg.mozilla.org/mozilla-central/rev/a6758d669350