Bug 114949 – Initial advance of text runs should be taken into account

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antoine Quint]

In certain scenarii, text runs may have an initial advance that can have dramatic impact in the way the text is rendered. For instance, rendering the arabic text "مليئ حياةٍ مليئ" currently looks incorrect since the middle word, with double diactrics, is pushed down and does not share the same baseline as the surrounding words. We need to handle the possibility of a non-zero initial advance for such text runs.

[Antoine Quint]

<rdar://problem/13658313>

[Antoine Quint]

Created attachment 199018 [details]
Patch

[EFL EWS Bot]

Comment on attachment 199018 [details]
Patch

Attachment 199018 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/157423

[kov's GTK+ EWS bot]

Comment on attachment 199018 [details]
Patch

Attachment 199018 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/199152

[Early Warning System Bot]

Comment on attachment 199018 [details]
Patch

Attachment 199018 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/181244

[Early Warning System Bot]

Comment on attachment 199018 [details]
Patch

Attachment 199018 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/55533

[Darin Adler]

Comment on attachment 199018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=199018&action=review

> Source/WebCore/platform/graphics/GlyphBuffer.h:133
> +    void setInitialAdvance(CGSize anAdvance) { m_initialAdvance = anAdvance; }
> +    const CGSize initialAdvance() const { return m_initialAdvance; }

It’s not acceptable to use a Mac-specific type, CGSize, here in a platform independent class; maybe FloatSize instead? Also, the return type need not have a const. Further, WebKit coding style does not use prefixes like “an”.

[Antoine Quint]

(In reply to comment #7)
> (From update of attachment 199018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199018&action=review
> 
> > Source/WebCore/platform/graphics/GlyphBuffer.h:133
> > +    void setInitialAdvance(CGSize anAdvance) { m_initialAdvance = anAdvance; }
> > +    const CGSize initialAdvance() const { return m_initialAdvance; }
> 
> It’s not acceptable to use a Mac-specific type, CGSize, here in a platform independent class; maybe FloatSize instead? Also, the return type need not have a const. Further, WebKit coding style does not use prefixes like “an”.

Yes, I'm already working on updating GlyphBuffer.h. I'll change to use a GlyphBufferAdvance instead of a CGSize.

[Antoine Quint]

Created attachment 199033 [details]
Patch

[Darin Adler]

Comment on attachment 199033 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=199033&action=review

> Source/WebCore/ChangeLog:10
> +        Keep track of complex text runs' initial advance by adding a new
> +        member to GlyphBuffer and adding the initial advance of any non-first
> +        text run to the advance of the last glyph added for the previous text run.

Why doesn’t this mention the newly added support for advances with Y-axis changes? Isn’t that a separate thing we’re adding?

> Source/WebCore/platform/graphics/FontFastPath.cpp:482
> +    FloatPoint startPoint(point.x(), point.y() - glyphBuffer.initialAdvance().height());
> +    float nextX = startPoint.x() + glyphBuffer.advanceAt(0).width();
> +    float nextY = startPoint.y() + glyphBuffer.advanceAt(0).height();

Now that the other advances are being done both X and Y, why is the initial advance adjustment being done only for Y here?

> Source/WebCore/platform/graphics/GlyphBuffer.h:130
> -    float advanceAt(int index) const
> +    GlyphBufferAdvance advanceAt(int index) const

We might get slightly better performance if the return type here is const GlyphBufferAdvance&.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:504
> +        if (glyphBuffer && glyphBuffer->isEmpty())
> +            glyphBuffer->setInitialAdvance(complexTextRun.initialAdvance());

The logic here seems a little oblique. If there are complex text runs that don’t add a full glyph to the buffer, then the first run that does will set the initial advance. That seems a little strange. Maybe never happens. Would be nice to do this in a more direct way, but not sure that’s practical.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:576
> +        if (r > 0 && m_adjustedAdvances.size() > 0) {
> +            CGSize previousAdvance = m_adjustedAdvances.last();
> +            previousAdvance.width += complexTextRun.initialAdvance().width;
> +            previousAdvance.height -= complexTextRun.initialAdvance().height;
> +            m_adjustedAdvances[m_adjustedAdvances.size() - 1] = previousAdvance;
> +        }

This needs a comment. It’s not clear why this is the right thing to do. The change log comment explains it, but people won’t necessarily be reading the change log.

Also, normally we’d leave off those “> 0” in the conditional expression.

> Source/WebCore/platform/graphics/mac/ComplexTextController.h:119
> +        CGSize m_initialAdvance;

The missing glyphs run constructor does not initialize this. I don’t think it’s safe to leave it with an uninitialized value.

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:115
> +    m_initialAdvance = wkCTRunGetInitialAdvance(ctRun);

Would be better to use constructor syntax for this rather than assignment.

> LayoutTests/ChangeLog:9
> +        * fast/text/complex-initial-advance-expected.html: Added.
> +        * fast/text/complex-initial-advance.html: Added.

Do we need a fast path initial advance test case, too? If not, then why the code changes to the fast path? I think they’re untested.

[Antoine Quint]

(In reply to comment #10)
> (From update of attachment 199033 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199033&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Keep track of complex text runs' initial advance by adding a new
> > +        member to GlyphBuffer and adding the initial advance of any non-first
> > +        text run to the advance of the last glyph added for the previous text run.
> 
> Why doesn’t this mention the newly added support for advances with Y-axis changes? Isn’t that a separate thing we’re adding?

You're right, this is worthy of top-level mention in the ChangeLog. I will add this.

> > Source/WebCore/platform/graphics/FontFastPath.cpp:482
> > +    FloatPoint startPoint(point.x(), point.y() - glyphBuffer.initialAdvance().height());
> > +    float nextX = startPoint.x() + glyphBuffer.advanceAt(0).width();
> > +    float nextY = startPoint.y() + glyphBuffer.advanceAt(0).height();
> 
> Now that the other advances are being done both X and Y, why is the initial advance adjustment being done only for Y here?

There seems to already be specific code to deal with initial advances in the x-axis in Font::getGlyphsAndAdvancesForComplexText() that I was concerned would break things if changed. This seemed like a safer change, but it's quite possible this is a mistake and we should try to remove the existing code and use the initial advance provided by CoreText on the x-axis as well.

> > Source/WebCore/platform/graphics/GlyphBuffer.h:130
> > -    float advanceAt(int index) const
> > +    GlyphBufferAdvance advanceAt(int index) const
> 
> We might get slightly better performance if the return type here is const GlyphBufferAdvance&.

I'd have to trust you on that. It sounds like a good time to do this change as well since we're already changing all the call sites that use this method.

> > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:504
> > +        if (glyphBuffer && glyphBuffer->isEmpty())
> > +            glyphBuffer->setInitialAdvance(complexTextRun.initialAdvance());
> 
> The logic here seems a little oblique. If there are complex text runs that don’t add a full glyph to the buffer, then the first run that does will set the initial advance. That seems a little strange. Maybe never happens. Would be nice to do this in a more direct way, but not sure that’s practical.

Would it help to add a m_currentRun == 0 check here?

> > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:576
> > +        if (r > 0 && m_adjustedAdvances.size() > 0) {
> > +            CGSize previousAdvance = m_adjustedAdvances.last();
> > +            previousAdvance.width += complexTextRun.initialAdvance().width;
> > +            previousAdvance.height -= complexTextRun.initialAdvance().height;
> > +            m_adjustedAdvances[m_adjustedAdvances.size() - 1] = previousAdvance;
> > +        }
> 
> This needs a comment. It’s not clear why this is the right thing to do. The change log comment explains it, but people won’t necessarily be reading the change log.


Agreed, I will add one.
 
> Also, normally we’d leave off those “> 0” in the conditional expression.
> 
> > Source/WebCore/platform/graphics/mac/ComplexTextController.h:119
> > +        CGSize m_initialAdvance;
> 
> The missing glyphs run constructor does not initialize this. I don’t think it’s safe to leave it with an uninitialized value.

I will update the constructor in the .cpp file to set m_initialAdvance to CGSizeZero.
 
> > Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:115
> > +    m_initialAdvance = wkCTRunGetInitialAdvance(ctRun);
> 
> Would be better to use constructor syntax for this rather than assignment.

In order to get a copy?

> > LayoutTests/ChangeLog:9
> > +        * fast/text/complex-initial-advance-expected.html: Added.
> > +        * fast/text/complex-initial-advance.html: Added.
> 
> Do we need a fast path initial advance test case, too? If not, then why the code changes to the fast path? I think they’re untested.

Do you mean the changes in Font::drawGlyphBuffer()? This method is called for the test case for sure.

[Antoine Quint]

> > > Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:115
> > > +    m_initialAdvance = wkCTRunGetInitialAdvance(ctRun);
> > 
> > Would be better to use constructor syntax for this rather than assignment.
> 
> In order to get a copy?

Duh, you meant "m_initialAdvance(wkCTRunGetInitialAdvance(ctRun))". Fixing this in commit.

[Antoine Quint]

Created attachment 199203 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 199203 [details]
Patch for landing

Clearing flags on attachment: 199203

Committed r148956: <http://trac.webkit.org/changeset/148956>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.