Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740083002:
Add support for shaper-driven line breaking to HarfBuzzShaper (Closed)
|
Created:
3 years, 9 months ago by eae Modified:
3 years, 9 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, fmalita+watch_chromium.org, jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for shaper-driven line breaking to HarfBuzzShaper
Shapes a line of text by finding the ideal break point based on existing
shape results for the paragraph and then looking backwards until a valid
and appropriate break opportunity is identified. Re-shapes the beginning
and/or end of the line as needed (if the break position affect shaping).
This allows for significantly faster and more efficient line breaking by
only re-shaping when absolutely needed and by only measuring between the
start/end of lines and the nearest suitable and valid break opportunity.
The nextSafeToBreakBefore and previousSafeToBreakAfter functions will be
updated to use safe-to-break information when exposed by HarfBuzz.
R=drott@chromium.org, kojii@chromium.org
BUG=609117
TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp
Review-Url: https://codereview.chromium.org/2740083002
Cr-Commit-Position: refs/heads/master@{#459953}
Committed: https://chromium.googlesource.com/chromium/src/+/b47c12e89f5134efe6a68cb45e28a92451fcdb3e
Patch Set 1 #Patch Set 2 : WIP #Patch Set 3 : Getting closer #Patch Set 4 : Fix null-termination issue #Patch Set 5 : [LayoutNG] Full on shaper-driven line breaking #
Total comments: 9
Patch Set 6 : Rebase w/HEAD and address comments #Patch Set 7 : Try to fix test failures #
Messages
Total messages: 36 (23 generated)
Description was changed from
==========
wip
BUG=
==========
to
==========
Full on shaper-driven line brekaing for LayoutNG.
Still very much WIP but it is getting there. As Harfbuzz doesn't yet
expose safe-to-break information this implementation has a dummy
implementation where only space characters are considered safe.
==========
Description was changed from
==========
Full on shaper-driven line brekaing for LayoutNG.
Still very much WIP but it is getting there. As Harfbuzz doesn't yet
expose safe-to-break information this implementation has a dummy
implementation where only space characters are considered safe.
==========
to
==========
[LayoutNG] Full on shaper-driven line breaking
Still very much WIP but it is getting there. As Harfbuzz doesn't yet
expose safe-to-break information this implementation has a dummy
implementation where only space characters are considered safe.
==========
The CQ bit was checked by eae@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by eae@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
eae@chromium.org changed reviewers: + drott@chromium.org, kojii@chromium.org
With the caveat that we do not yet have a proper safe-to-break implementation this now works. I'd like to support RTL runs without having to branch all the loops but haven't figured out a good way to do that yet. Ideas welcome.
Noticed one thing while passing by. https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.h (right): https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.h:75: const AtomicString locale, drive-by: const AtomicString&? (this currently unnecessarily bumps the ref count)
> I'd like to support RTL runs without having to branch all the loops but haven't figured out a good way to do that yet. Ideas welcome. Can you point where you think RTL isn't working well yet? By a glance, this uses functions that support RTL (offsetForPosition, forEachGlyphInRange etc.) and that it's not obvious to me. I might need to read a bit more. But I think I got the basic idea, and this looks great, I can foresee how much this can improve our line braeker. Thank you so much for this CL, this is by far the best explanation than any design docs. Do you want to play a bit more, or land-and-improve as we integrate to NG? lgtm if the latter. https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp (right): https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp:356: 56); nit: nowadays I think you can, unless you prefer to16Bit(): String string(u"...");
On 2017/03/22 16:15:29, kojii wrote: > > I'd like to support RTL runs without having to branch all the loops but > haven't figured out a good way to do that yet. Ideas welcome. > > Can you point where you think RTL isn't working well yet? By a glance, this uses > functions that support RTL (offsetForPosition, forEachGlyphInRange etc.) and > that it's not obvious to me. I might need to read a bit more. If the overall direction of the entire paragraph is rtl it breaks as it operates on each run left to right. I'll add a test demonstrating the failure. > But I think I got the basic idea, and this looks great, I can foresee how much > this can improve our line braeker. Thank you so much for this CL, this is by far > the best explanation than any design docs. Aw, thank you! > Do you want to play a bit more, or land-and-improve as we integrate to NG? lgtm > if the latter. I'd at least like to add a (failing) rtl test before landing but in general I like the iterate-in-place strategy! > > https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp > (right): > > https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp:356: > 56); > nit: nowadays I think you can, unless you prefer to16Bit(): > String string(u"..."); Oh, that used to fail on windows. Didn't know it worked now. Thanks!
On 2017/03/22 at 17:04:37, eae wrote: > > String string(u"..."); > > Oh, that used to fail on windows. Didn't know it worked now. Thanks! Fixed in https://codereview.chromium.org/2500723002
Innovation! Looking forward to our smarter line breaking, this looks very promising. LGTM, with some suggestions maybe. If there is more layout / iteration / break finding logic coming, I'd prefer to have something like an adjacent class to HarfBuzzShaper encapsulating the layout parts of the logic. Perhaps I didn't fully understand the code, could you explain why some boundaries / candidate positions are snapped? Can we avoid that? And the other thought I had: It's intentional that this is not going through the word cache? What are your forward looking thoughts on this? Do you plan to have it go through the word cache or will that not be needed anymore?
https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:740: // TODO: This should use harf-buzz safe to break info when available. For now s/harf-buzz/HarfBuzz/ https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:798: LayoutUnit startPosition = result->snappedStartPositionForOffset(startOffset); IIUC, this one is pixel snapped, why is that the case? Does it mean that words (or every piece of string between breaks) can only be positioned at full pixels? https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:800: // If the start offset is not at a safe-to-break boundary content between What do you mean by "content" here? Or probably there is a "the" missing? https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:832: unsigned previousSafe = std::max( Nice.
On 2017/03/22 21:01:00, drott wrote: > Innovation! Looking forward to our smarter line breaking, this looks very > promising. LGTM, with some suggestions maybe. Thanks! > If there is more layout / iteration / break finding logic coming, I'd prefer > to have something like an adjacent class to HarfBuzzShaper encapsulating the > layout parts of the logic. That is fair, it relies pretty heavily on internal state at the moment but if/when we add more layout and line breaking logic I'll break it out into a ShaperLineBreaker or similar class. > Perhaps I didn't fully understand the code, could you explain why some > boundaries / candidate positions are snapped? Can we avoid that? Lines are snapped when going from font metrics to layout, this attempts to make that snapping more apparent by making it part of the API. It is needed for line breaking as the available space is always snapped. This might be another one of those things that really should be in ShaperLineBreaker. Ideally I'd like to get rid of snapping all together and always use LayoutUnits for everything but the precision isn't there to use it for advances. > And the other > thought I had: It's intentional that this is not going through the word cache? > What are your forward looking thoughts on this? Do you plan to have it go > through the word cache or will that not be needed anymore? My hope is that it'll be fast enough without the word cache given that we'll retain shape results and only do selective re-shaping when really needed. With context aware shaping we can't use the word cache for fonts that kern with space (which is rather common) so it needs to fast even without the word cache. If it isn't fast enough we can always add it later.
https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:798: LayoutUnit startPosition = result->snappedStartPositionForOffset(startOffset); On 2017/03/22 21:01:11, drott wrote: > IIUC, this one is pixel snapped, why is that the case? Does it mean that words > (or every piece of string between breaks) can only be positioned at full pixels? No, this snaps to the nearest 1/64th of a pixel, not full pixels. It is needed as the layout system operates entirely with fixed point LayoutUnits. Any start and end position or length needs to (eventually) be snapped to LayoutUnits. https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:800: // If the start offset is not at a safe-to-break boundary content between On 2017/03/22 21:01:11, drott wrote: > What do you mean by "content" here? Or probably there is a "the" missing? Missing "the", thank you.
https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/2740083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:798: LayoutUnit startPosition = result->snappedStartPositionForOffset(startOffset); On 2017/03/27 at 20:14:02, eae wrote: > On 2017/03/22 21:01:11, drott wrote: > > IIUC, this one is pixel snapped, why is that the case? Does it mean that words > > (or every piece of string between breaks) can only be positioned at full pixels? > > No, this snaps to the nearest 1/64th of a pixel, not full pixels. It is needed as the layout system operates entirely with fixed point LayoutUnits. > > Any start and end position or length needs to (eventually) be snapped to LayoutUnits. I see, that makes more sense, thank you for the explanation.
Description was changed from
==========
[LayoutNG] Full on shaper-driven line breaking
Still very much WIP but it is getting there. As Harfbuzz doesn't yet
expose safe-to-break information this implementation has a dummy
implementation where only space characters are considered safe.
==========
to
==========
Add support for shaper-driven line breaking to HarfbuzzShaper
Shapes a line of text by finding the ideal break point based on existing
shape results for the paragraph and then looking backwards until a valid
and appropriate break opportunity is identified. Re-shapes the beginning
and/or end of the line as needed (if the break position affect shaping).
This allows for significantly faster and more efficient line breaking by
only re-shaping when absolutely needed and by only measuring between the
start/end of lines and the nearest suitable and valid break opportunity.
The nextSafeToBreakBefore and previousSafeToBreakAfter functions will be
updated to use safe-to-break information when exposed by HarfBuzz.
R=drott@chromium.org, kojii@chromium.org
BUG=609117
TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp
==========
Description was changed from ========== Add support for shaper-driven line breaking to HarfbuzzShaper Shapes a line of text by finding the ideal break point based on existing shape results for the paragraph and then looking backwards until a valid and appropriate break opportunity is identified. Re-shapes the beginning and/or end of the line as needed (if the break position affect shaping). This allows for significantly faster and more efficient line breaking by only re-shaping when absolutely needed and by only measuring between the start/end of lines and the nearest suitable and valid break opportunity. The nextSafeToBreakBefore and previousSafeToBreakAfter functions will be updated to use safe-to-break information when exposed by HarfBuzz. R=drott@chromium.org, kojii@chromium.org BUG=609117 TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp ========== to ========== Add support for shaper-driven line breaking to HarfBuzzShaper Shapes a line of text by finding the ideal break point based on existing shape results for the paragraph and then looking backwards until a valid and appropriate break opportunity is identified. Re-shapes the beginning and/or end of the line as needed (if the break position affect shaping). This allows for significantly faster and more efficient line breaking by only re-shaping when absolutely needed and by only measuring between the start/end of lines and the nearest suitable and valid break opportunity. The nextSafeToBreakBefore and previousSafeToBreakAfter functions will be updated to use safe-to-break information when exposed by HarfBuzz. R=drott@chromium.org, kojii@chromium.org BUG=609117 TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp ==========
The CQ bit was checked by eae@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Interestingly HarfBuzzShaperTest.ShapeResultCopyRangeIntoArabicThaiHanLatin fails on Mac (and only on Mac), looking into that now.
The CQ bit was checked by eae@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eae@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from drott@chromium.org, kojii@chromium.org Link to the patchset: https://codereview.chromium.org/2740083002/#ps120001 (title: "Try to fix test failures")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490660876867760,
"parent_rev": "2c8645c1be63b83102853e2f74ce8e62c17542e2", "commit_rev":
"b47c12e89f5134efe6a68cb45e28a92451fcdb3e"}
Message was sent while issue was closed.
Description was changed from ========== Add support for shaper-driven line breaking to HarfBuzzShaper Shapes a line of text by finding the ideal break point based on existing shape results for the paragraph and then looking backwards until a valid and appropriate break opportunity is identified. Re-shapes the beginning and/or end of the line as needed (if the break position affect shaping). This allows for significantly faster and more efficient line breaking by only re-shaping when absolutely needed and by only measuring between the start/end of lines and the nearest suitable and valid break opportunity. The nextSafeToBreakBefore and previousSafeToBreakAfter functions will be updated to use safe-to-break information when exposed by HarfBuzz. R=drott@chromium.org, kojii@chromium.org BUG=609117 TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp ========== to ========== Add support for shaper-driven line breaking to HarfBuzzShaper Shapes a line of text by finding the ideal break point based on existing shape results for the paragraph and then looking backwards until a valid and appropriate break opportunity is identified. Re-shapes the beginning and/or end of the line as needed (if the break position affect shaping). This allows for significantly faster and more efficient line breaking by only re-shaping when absolutely needed and by only measuring between the start/end of lines and the nearest suitable and valid break opportunity. The nextSafeToBreakBefore and previousSafeToBreakAfter functions will be updated to use safe-to-break information when exposed by HarfBuzz. R=drott@chromium.org, kojii@chromium.org BUG=609117 TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp Review-Url: https://codereview.chromium.org/2740083002 Cr-Commit-Position: refs/heads/master@{#459953} Committed: https://chromium.googlesource.com/chromium/src/+/b47c12e89f5134efe6a68cb45e28... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/b47c12e89f5134efe6a68cb45e28... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
