Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686503003:
Add support for shaping a substring to HarfBuzzShaper (Closed)
|
Created:
3 years, 10 months ago by eae Modified:
3 years, 10 months ago Reviewers:
drott 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, f(malita), jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for shaping a substring to HarfBuzzShaper
Add support to HarfBuzzShaper for shaping a portion of the supplied text
content as identified by a start and end offset. This allows an instance
of the shaper to be used to shape multiple portions, or segments, of the
string with different fonts, styles and font feature settings as needed.
This change also renames a couple of methods and fields to increase code
readability and to avoid confusion. Significant renames described below:
shapeResult() -> shape()
harfBuzzBuffer -> buffer
normalizedBuffer -> text
normalizedBufferLength -> textLength
Eventually this will also allow for context aware shaping by passing the
surrounding text as context to harfbuzz when invoking shaping algorithm.
TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp
BUG=689155
R=drott@chromium.org
Review-Url: https://codereview.chromium.org/2686503003
Cr-Commit-Position: refs/heads/master@{#449158}
Committed: https://chromium.googlesource.com/chromium/src/+/8fd9860a37236d0d424b33e344f71df12625a653
Patch Set 1 #Patch Set 2 : Fix windows compiler error #
Total comments: 3
Patch Set 3 : Don't restrict run segmentation #Patch Set 4 : Cleanup comments and adjust tolerance #
Total comments: 4
Patch Set 5 : Clarify API and tests #
Messages
Total messages: 38 (28 generated)
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...
Description was changed from
==========
Add support for shaping a substring to HarfBuzzShaper
Add support to HarfBuzzShaper for shaping a portion of the supplied text
content as identified by a start and end offset. This allows an instance
of the shaper to be used to shape multiple portions, or segments, of the
string with different fonts, styles and font feature settings as needed.
This change also renames a couple of methods and fields to increase code
readability and to avoid confusion. Significant renames described below:
shape() -> shapeResult()
harfBuzzBuffer -> buffer
normalizedBuffer -> text
normalizedBufferLength -> textLength
Eventually this will also allow for context aware shaping by passing the
surrounding text as context to harfbuzz when invoking shaping algorithm.
TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp
BUG=689155
R=drott@chromium.org
==========
to
==========
Add support for shaping a substring to HarfBuzzShaper
Add support to HarfBuzzShaper for shaping a portion of the supplied text
content as identified by a start and end offset. This allows an instance
of the shaper to be used to shape multiple portions, or segments, of the
string with different fonts, styles and font feature settings as needed.
This change also renames a couple of methods and fields to increase code
readability and to avoid confusion. Significant renames described below:
shapeResult() -> shape()
harfBuzzBuffer -> buffer
normalizedBuffer -> text
normalizedBufferLength -> textLength
Eventually this will also allow for context aware shaping by passing the
surrounding text as context to harfbuzz when invoking shaping algorithm.
TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp
BUG=689155
R=drott@chromium.org
==========
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The rename looks good to me. And generally the split into shape and shapeSegment is nice and helps readability. I am a little concerned about restricting the range before the RunSegmenter, as the results are not necessarily stable with less context, a longer explanation below. https://codereview.chromium.org/2686503003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/2686503003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:684: RunSegmenter runSegmenter(m_text + start, length, orientation); Offsetting the RunSegmenter to a start at an offset or restricting it to a certain length has side effects on script and emoji segmentation, so the result of shaping a sub-run is only ensured to be identical to its shaping result as part of the whole run without offsets if the offsets are not splitting emoji sequences, or if the offsets do not cut a string between for example brackets that would otherwise resolve to the same script or before/after SCRIPT_COMMON sequences that can no longer be resolved due to missing context. It might be tricky to ensure this stability above this interface. So one possible way to adress this would be to segment everything, e.g. fast forward the consume and only start start shaping at the first useful RunSegmenterRange that covers "start", and discard the further consume() results after "start + length". https://codereview.chromium.org/2686503003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.h (right): https://codereview.chromium.org/2686503003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.h:64: // Shapes a single seqment, as identified by the RunSegmenterRange parameter, Is this the codereview tool showing it in a funny way, or is this double slash indented less than the others? https://codereview.chromium.org/2686503003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp (right): https://codereview.chromium.org/2686503003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp:243: TEST_F(HarfBuzzShaperTest, DISABLED_ShapeArabicWithContext) { When do you plan to enable this, or what's breaking here atm? Could you perhaps add a TODO and bug?
Thanks for the detailed feedback Dominik! Restricting the range for run segmentation seemed like the easiest and most performance way to implement this but as you pointed out it wouldn't yield the correct results in cases where the specified range spans segments. I've reworked it to always segment the entire range and only shape the relevant segments. I also added a bunch of comments explaining the logic and new methods. Please take another look when you get a chance.
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 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 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_...)
Hi Emil, after initially thinking about the RunSegmenter stability, I believe we actually have a similar problem with the shaping, see below. We can discuss on Hangouts or loop in Behdad as well. It's difficult to pick the right start/end positions when shaping a sub-range. Splitting clusters can lead to incorrect results. What do you think? https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:641: // Clamp the start and end offsets of the queue item to the offsets What do you anticipate these shapeStart and shapeEnd values to be? It might be difficult to choose these correctly, without something like a safe-to-break API that we discussed with Behdad before. If index values for shapeStart and shapeEnd are used which point to the middle of a 3-character cluster for example, the result may become an unshaped .notdef at the beginning of the string, whereas if the whole cluster is contained within start and end it would work out. https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:694: // Only shape segments within the range indicated by start and end offsets. IIUC the condition currently means: // Only shape segments overlapping with the range indicated by start and end. Not only those strictly within. Is this what you meant? If yes, could you edit the comment? https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp (right): https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp:213: String string = to16Bit("Hello World!", 12); Thinking about this more, see also my comment above: I don't think it's generally safe to use arbitrary indices into a run here, but only safe-to-break positions would work reliably. If you have for example a string beginning with 'e + combining cedilla + combining breve + remaining text' and use start index 1 or 2, the shaping for the orphaned combining marks will either fail or look different from the e with them and procude a different shape result. Perhaps we need something like an API that can tell layout code safe to break positions in a whole paragraph or run? https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp:233: ASSERT_NEAR(firstReference->width(), first->width(), 0.1); I think the previous patchset had 0.01 here, was that not enough to make this pass?
On 2017/02/08 15:52:59, drott wrote: > Hi Emil, after initially thinking about the RunSegmenter stability, I believe we > actually have a similar problem with the shaping, see below. We can discuss on > Hangouts or loop in Behdad as well. It's difficult to pick the right start/end > positions when shaping a sub-range. Splitting clusters can lead to incorrect > results. What do you think? The idea is to only use this for ranges where we'd have to split it anyway. I.e. at element boundaries or at line breaks. In the first case the split happens regardless of how we do the shaping. In the second case we'd set the range based on breakable characters or, when available, the safe-to-break indication from harfbuzz. Splitting at arbitrary positions will lead to arbitrary results but that is not the intention behind this API. > > https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp > (right): > > https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:641: // > Clamp the start and end offsets of the queue item to the offsets > What do you anticipate these shapeStart and shapeEnd values to be? It might be > difficult to choose these correctly, without something like a safe-to-break API > that we discussed with Behdad before. If index values for shapeStart and > shapeEnd are used which point to the middle of a 3-character cluster for > example, the result may become an unshaped .notdef at the beginning of the > string, whereas if the whole cluster is contained within start and end it would > work out. > > https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp:694: // Only > shape segments within the range indicated by start and end offsets. > IIUC the condition currently means: // Only shape segments overlapping with the > range indicated by start and end. Not only those strictly within. > > Is this what you meant? If yes, could you edit the comment? Yes. Will do, thanks! > > https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp > (right): > > https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp:213: > String string = to16Bit("Hello World!", 12); > Thinking about this more, see also my comment above: I don't think it's > generally safe to use arbitrary indices into a run here, but only safe-to-break > positions would work reliably. If you have for example a string beginning with > 'e + combining cedilla + combining breve + remaining text' and use start index 1 > or 2, the shaping for the orphaned combining marks will either fail or look > different from the e with them and procude a different shape result. > > Perhaps we need something like an API that can tell layout code safe to break > positions in a whole paragraph or run? I think I touched on this in the first reply but yes, we do need a safe-to-break API. This API though is trying to solve the case of allowing shaping with context across element bounaries. > > https://codereview.chromium.org/2686503003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp:233: > ASSERT_NEAR(firstReference->width(), first->width(), 0.1); > I think the previous patchset had 0.01 here, was that not enough to make this > pass? It is not. Due to float imprecision it can be off by up to about 0.016.
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...
PTAL
Patchset #4 (id:60001) has been deleted
Patchset #5 (id:100001) has been deleted
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.
LGTM, thanks.
Did doing the whole range run segmentation fix the problems you were previously
observing?
After Behdad's feedback on AAT not supporting pre/post context I installed Noto
Naskh Arabic, and modified the test SetUp() experimentally:
blink::FontFamily family;
family.setFamily("Noto Naskh Arabic");
fontDescription.setFamily(family);
Then the Arabic test actually passes here without any further modifications to
CaseMappingHarfBuzzBufferFiller. (just to double check whether the font matching
succeeded, I removed the last "c" character from the font family name, and then
the test fails again).
On which platform(s) did it fail previously? Do you have a visual test case for
the Arabic with mixed color that would run in LayoutNG already or are you about
to hook it up first?
On 2017/02/08 23:31:26, drott wrote:
> LGTM, thanks.
>
> Did doing the whole range run segmentation fix the problems you were
previously
> observing?
It did indeed, thank you so much for pointing that out!
> After Behdad's feedback on AAT not supporting pre/post context I installed
Noto
> Naskh Arabic, and modified the test SetUp() experimentally:
>
> blink::FontFamily family;
> family.setFamily("Noto Naskh Arabic");
> fontDescription.setFamily(family);
>
> Then the Arabic test actually passes here without any further modifications to
> CaseMappingHarfBuzzBufferFiller. (just to double check whether the font
matching
> succeeded, I removed the last "c" character from the font family name, and
then
> the test fails again).
That is very interesting, I couldn't get it to pass on linux without the hack in
the other change. Will investigate further.
> On which platform(s) did it fail previously? Do you have a visual test case
for
> the Arabic with mixed color that would run in LayoutNG already or are you
about
> to hook it up first?
That's the next step, been working on that in parallel to this change as it
depends on these changes to HarfBuzzShaper.
Thank you for your detailed review and very helpful suggestions, as always!
The CQ bit was checked by eae@chromium.org
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": 1486599680969450,
"parent_rev": "94c4e6b642d971a31680fbb19858f56e1a4d8d7b", "commit_rev":
"8fd9860a37236d0d424b33e344f71df12625a653"}
Message was sent while issue was closed.
Description was changed from
==========
Add support for shaping a substring to HarfBuzzShaper
Add support to HarfBuzzShaper for shaping a portion of the supplied text
content as identified by a start and end offset. This allows an instance
of the shaper to be used to shape multiple portions, or segments, of the
string with different fonts, styles and font feature settings as needed.
This change also renames a couple of methods and fields to increase code
readability and to avoid confusion. Significant renames described below:
shapeResult() -> shape()
harfBuzzBuffer -> buffer
normalizedBuffer -> text
normalizedBufferLength -> textLength
Eventually this will also allow for context aware shaping by passing the
surrounding text as context to harfbuzz when invoking shaping algorithm.
TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp
BUG=689155
R=drott@chromium.org
==========
to
==========
Add support for shaping a substring to HarfBuzzShaper
Add support to HarfBuzzShaper for shaping a portion of the supplied text
content as identified by a start and end offset. This allows an instance
of the shaper to be used to shape multiple portions, or segments, of the
string with different fonts, styles and font feature settings as needed.
This change also renames a couple of methods and fields to increase code
readability and to avoid confusion. Significant renames described below:
shapeResult() -> shape()
harfBuzzBuffer -> buffer
normalizedBuffer -> text
normalizedBufferLength -> textLength
Eventually this will also allow for context aware shaping by passing the
surrounding text as context to harfbuzz when invoking shaping algorithm.
TEST=Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp
BUG=689155
R=drott@chromium.org
Review-Url: https://codereview.chromium.org/2686503003
Cr-Commit-Position: refs/heads/master@{#449158}
Committed:
https://chromium.googlesource.com/chromium/src/+/8fd9860a37236d0d424b33e344f7...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/8fd9860a37236d0d424b33e344f7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
