[Webkit-unassigned] [Bug 6274] text repainting does not account for glyphs which draw outside the typographic bounds of the font
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 5 18:53:12 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=6274
Enrica Casucci <enrica at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |enrica at apple.com
--- Comment #26 from Enrica Casucci <enrica at apple.com> 2010-04-05 18:53:10 PST ---
(In reply to comment #25)
> (From update of attachment 52577 [details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog (revision 57096)
> > +++ WebCore/ChangeLog (working copy)
> > @@ -1,3 +1,104 @@
> > +2010-04-05 Enrica Casucci <enrica at 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.
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list