[Bug 6274] text repainting does not account for glyphs which draw outside the typographic bounds of the font

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=6274

WebKit Review Bot <webkit.r...@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |dgla...@chromium.org,
| |webkit-bot-watchers@googleg
| |roups.com,
| |webkit.r...@gmail.com

--- Comment #22 from WebKit Review Bot <webkit.r...@gmail.com> 2010-04-05 15:21:46 PST ---
Attachment 52574 did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1688007

--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=6274

--- Comment #23 from WebKit Review Bot <webkit.r...@gmail.com> 2010-04-05 16:03:47 PST ---
Attachment 52577 did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1687016

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=6274

WebKit Review Bot <webkit.r...@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |g...@gnome.org,
| |xan....@gmail.com

--- Comment #24 from WebKit Review Bot <webkit.r...@gmail.com> 2010-04-05 16:30:36 PST ---
Attachment 52577 did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1685035

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=6274





--- Comment #25 from mi...@webkit.org 2010-04-05 16:41:12 PST ---
(From update of attachment 52577)
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 57096)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,104 @@
> +2010-04-05 Enrica Casucci <enr...@apple.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Text repainting does not account for glyphs which draw outside the typographic bounds of the font (6274).
> + <rdar://problem/6649734>
> + <https://bugs.webkit.org/show_bug.cgi?id=6274>
> +
> + In order to be able to handle successfully this case, it is necessary to change the glyph width cache to store
> + the bounding box for the glyph instead of the simply caching the glyph width.
> + Retrieving the bounding box for the glyph is expensive, therefore we do it only
> + when we are rendering text using the complex text path to minimize the performance impact.
> + To support characters with stacked diacritics, the method canUseGlyphCache has been modified to
> + return false for the range of characters with stacked diacritics.
> +
> + The original version of this patch has been written by Dan Bernstein.
> +
> + Test: fast/repaint/stacked-diacritics.html
> +
> + * WebCore.base.exp:
> + * platform/graphics/Font.cpp:
> + (WebCore::Font::floatWidth):
> + * platform/graphics/Font.h:
> + (WebCore::GlyphOverflow::GlyphOverflow):
> + (WebCore::Font::width):
> + * platform/graphics/FontFastPath.cpp:
> + (WebCore::Font::canUseGlyphCache):
> + (WebCore::Font::floatWidthForSimpleText):
> + * platform/graphics/GlyphWidthMap.cpp:
> + (WebCore::GlyphWidthMap::locatePageSlowCase):
> + * platform/graphics/GlyphWidthMap.h:
> + (WebCore::GlyphWidthMap::metricsForGlyph):
> + (WebCore::GlyphWidthMap::widthForGlyph):
> + (WebCore::GlyphWidthMap::setMetricsForGlyph):
> + (WebCore::GlyphWidthMap::GlyphWidthPage::metricsForGlyph):
> + (WebCore::GlyphWidthMap::GlyphWidthPage::setMetricsForGlyph):
> + (WebCore::GlyphWidthMap::GlyphWidthPage::setMetricsForIndex):
> + * platform/graphics/SimpleFontData.cpp:
> + (WebCore::SimpleFontData::platformGlyphInit):
> + * platform/graphics/SimpleFontData.h:
> + (WebCore::):
> + (WebCore::SimpleFontData::widthForGlyph):
> + (WebCore::SimpleFontData::metricsForGlyph):
> + * platform/graphics/WidthIterator.cpp:
> + (WebCore::WidthIterator::WidthIterator):
> + (WebCore::WidthIterator::advance):
> + * platform/graphics/WidthIterator.h:
> + (WebCore::WidthIterator::maxGlyphBoundingBoxY):
> + (WebCore::WidthIterator::minGlyphBoundingBoxY):
> + (WebCore::WidthIterator::firstGlyphOverflow):
> + (WebCore::WidthIterator::lastGlyphOverflow):
> + * platform/graphics/mac/ComplexTextController.cpp:
> + (WebCore::ComplexTextController::ComplexTextController):
> + (WebCore::ComplexTextController::adjustGlyphsAndAdvances):
> + * platform/graphics/mac/ComplexTextController.h:
> + (WebCore::ComplexTextController::minGlyphBoundingBoxX):
> + (WebCore::ComplexTextController::maxGlyphBoundingBoxX):
> + (WebCore::ComplexTextController::minGlyphBoundingBoxY):
> + (WebCore::ComplexTextController::maxGlyphBoundingBoxY):
> + * platform/graphics/mac/FontComplexTextMac.cpp:
> + (WebCore::Font::floatWidthForComplexText):
> + * platform/graphics/mac/SimpleFontDataMac.mm:
> + (WebCore::SimpleFontData::platformMetricsForGlyph):
> + * platform/graphics/win/FontWin.cpp:
> + (WebCore::Font::floatWidthForComplexText):
> + * platform/graphics/win/SimpleFontDataCGWin.cpp:
> + (WebCore::SimpleFontData::platformMetricsForGlyph):
> + * platform/graphics/win/SimpleFontDataWin.cpp:
> + (WebCore::SimpleFontData::metricsForGDIGlyph):
> + * platform/graphics/win/UniscribeController.cpp:
> + (WebCore::UniscribeController::UniscribeController):
> + (WebCore::UniscribeController::shapeAndPlaceItem):
> + * platform/graphics/win/UniscribeController.h:
> + (WebCore::UniscribeController::minGlyphBoundingBoxX):
> + (WebCore::UniscribeController::maxGlyphBoundingBoxX):
> + (WebCore::UniscribeController::minGlyphBoundingBoxY):
> + (WebCore::UniscribeController::maxGlyphBoundingBoxY):
> + * rendering/InlineFlowBox.cpp:
> + (WebCore::InlineFlowBox::placeBoxesHorizontally):
> + (WebCore::InlineFlowBox::computeLogicalBoxHeights):
> + (WebCore::InlineFlowBox::computeVerticalOverflow):
> + * rendering/InlineTextBox.cpp:
> + (WebCore::InlineTextBox::setFallbackFonts):
> + (WebCore::InlineTextBox::fallbackFonts):
> + (WebCore::InlineTextBox::setGlyphOverflow):
> + (WebCore::InlineTextBox::glyphOverflow):
> + * rendering/InlineTextBox.h:
> + (WebCore::InlineTextBox::clearGlyphOverflowAndFallbackFontMap):
> + * rendering/RenderBlockLineLayout.cpp:
> + (WebCore::RenderBlock::computeHorizontalPositionsForLine):
> + (WebCore::RenderBlock::createLineBoxesForResolver):
> + * rendering/RenderText.cpp:
> + (WebCore::RenderText::RenderText):
> + (WebCore::RenderText::styleDidChange):
> + (WebCore::RenderText::widthFromCache):
> + (WebCore::RenderText::trimmedPrefWidths):
> + (WebCore::RenderText::calcPrefWidths):
> + (WebCore::RenderText::setText):
> + (WebCore::RenderText::width):
> + * rendering/RenderText.h:

I think some specific comments on at least some of the functions would be
helpful.

> + float floatWidth(const TextRun&, HashSet<const SimpleFontData*>* fallbackFonts = 0, GlyphOverflow* glyphOverflow = 0) const;

Don’t need the argument name “glyphOverflow” here.

> @@ -234,6 +234,11 @@ bool Font::canUseGlyphCache(const TextRu
> if (c <= 0x194F)
> return false;
>
> + if (c < 0x1E00)
> + continue;
> + if (c <= 0x2000)
> + return false;
> +

It is important to add a comment about why this range is included (even more
than any of the other ranges, because the reason it’s included is unique).
There is also some trailing whitespace up there.


> class GlyphWidthMap : public Noncopyable {

Should probably rename this (and related files) GlyphMetricsMap.

> +ALWAYS_INLINE GlyphMetrics SimpleFontData::metricsForGlyph(Glyph glyph, GlyphMetricsMode metricsMode) const
> {
> - float width = m_glyphToWidthMap.widthForGlyph(glyph);
> - if (width != cGlyphWidthUnknown)
> - return width;
> -
> - width = platformWidthForGlyph(glyph);
> - m_glyphToWidthMap.setWidthForGlyph(glyph, width);
> -
> - return width;
> + GlyphMetrics metrics = m_glyphToWidthMap.metricsForGlyph(glyph);
> + if (metrics.horizontalAdvance != cGlyphWidthUnknown)
> + return metrics;
> +
> + metrics = platformMetricsForGlyph(glyph, metricsMode);
> + m_glyphToWidthMap.setMetricsForGlyph(glyph, metrics);
> +
> + return metrics;
> }

This confuses me. What’s to stop us from caching “width only” metrics in the
map at first, and then on a subsequent request for “bounding box” metrics
return the cached value instead of actually getting the bounding box? This
seems to be assuming that the fast path and the complex path will never hit the
same glyph. I don’t think it’s true, especially since the complex path can be
forced for any text by specifying text-rendering: optimizeLegibility.

> + , m_maxGlyphBoundingBoxY(numeric_limits<float>::min())
> + , m_minGlyphBoundingBoxY(numeric_limits<float>::max())
> + , m_firstGlyphOverflow(0)
> + , m_lastGlyphOverflow(0)

Seems like you don’t need this (nor any of the changes to WidthIterator) now
that this is limited to the complex code path.

Seeing as you have kept that code in, and just by avoiding actually getting the
glyph bounds you managed to have no performance impact on the fast path, I am
thinking that perhaps instead of forcing the complex path for the range of
glyphs with stacked diacritics, you could still use the fast path for those,
but get and use the real bounding rects in that case. This would essentially
make canUseGlyphCache() select between three modes: fast, fast with real glyph
bounds, and complex. For “fast with real glyph bounds”, Font::floatWidth()
would pass the glyphOverflow pointer through to floatWidthForSimpleText(). For
plain “fast”, it would pass 0, and the will instruct WidthIterator down the
line to not get real bounding boxes.

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=6274


Enrica Casucci <enr...@apple.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |enr...@apple.com




--- Comment #26 from Enrica Casucci <enr...@apple.com> 2010-04-05 18:53:10 PST ---
(In reply to comment #25)
> (From update of attachment 52577 [details])

Will do.

>
> > + float floatWidth(const TextRun&, HashSet<const SimpleFontData*>* fallbackFonts = 0, GlyphOverflow* glyphOverflow = 0) const;
>
> Don’t need the argument name “glyphOverflow” here.

Ok,

>
> > @@ -234,6 +234,11 @@ bool Font::canUseGlyphCache(const TextRu
> > if (c <= 0x194F)
> > return false;
> >
> > + if (c < 0x1E00)
> > + continue;
> > + if (c <= 0x2000)
> > + return false;
> > +
>
> It is important to add a comment about why this range is included (even more
> than any of the other ranges, because the reason it’s included is unique).
> There is also some trailing whitespace up there.
>

Will do,

>
> > class GlyphWidthMap : public Noncopyable {
>
> Should probably rename this (and related files) GlyphMetricsMap.
>

I know, it is just a pain :-)

> > +ALWAYS_INLINE GlyphMetrics SimpleFontData::metricsForGlyph(Glyph glyph, GlyphMetricsMode metricsMode) const
> > {
> > - float width = m_glyphToWidthMap.widthForGlyph(glyph);
> > - if (width != cGlyphWidthUnknown)
> > - return width;
> > -
> > - width = platformWidthForGlyph(glyph);
> > - m_glyphToWidthMap.setWidthForGlyph(glyph, width);
> > -
> > - return width;
> > + GlyphMetrics metrics = m_glyphToWidthMap.metricsForGlyph(glyph);
> > + if (metrics.horizontalAdvance != cGlyphWidthUnknown)
> > + return metrics;
> > +
> > + metrics = platformMetricsForGlyph(glyph, metricsMode);
> > + m_glyphToWidthMap.setMetricsForGlyph(glyph, metrics);
> > +
> > + return metrics;
> > }
>
> This confuses me. What’s to stop us from caching “width only” metrics in the
> map at first, and then on a subsequent request for “bounding box” metrics
> return the cached value instead of actually getting the bounding box? This
> seems to be assuming that the fast path and the complex path will never hit the
> same glyph. I don’t think it’s true, especially since the complex path can be
> forced for any text by specifying text-rendering: optimizeLegibility.
>

Then I'm not sure I've fully understood how this code works.

> > + , m_maxGlyphBoundingBoxY(numeric_limits<float>::min())
> > + , m_minGlyphBoundingBoxY(numeric_limits<float>::max())
> > + , m_firstGlyphOverflow(0)
> > + , m_lastGlyphOverflow(0)
>
> Seems like you don’t need this (nor any of the changes to WidthIterator) now
> that this is limited to the complex code path.
>
> Seeing as you have kept that code in, and just by avoiding actually getting the
> glyph bounds you managed to have no performance impact on the fast path, I am
> thinking that perhaps instead of forcing the complex path for the range of
> glyphs with stacked diacritics, you could still use the fast path for those,
> but get and use the real bounding rects in that case. This would essentially
> make canUseGlyphCache() select between three modes: fast, fast with real glyph
> bounds, and complex. For “fast with real glyph bounds”, Font::floatWidth()
> would pass the glyphOverflow pointer through to floatWidthForSimpleText(). For
> plain “fast”, it would pass 0, and the will instruct WidthIterator down the
> line to not get real bounding boxes.

I think I've tried that, and still had perf issues.
We'll talk in person about this.
Thanks.

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=6274


Eric Seidel <er...@webkit.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution| |FIXED




--- Comment #65 from Eric Seidel <er...@webkit.org> 2010-05-17 00:52:45 PST ---
If I'm reading the comments correctly, looks like this should be closed. Please re-open if I'm wrong.

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=6274


mi...@webkit.org changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |REOPENED
Resolution|FIXED |




--- Comment #66 from mi...@webkit.org 2010-05-17 07:51:37 PST ---
Not fixed in general. The test case from this report is still failing.