Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2139703002:
Rotate emoji with FreeType. (Closed)
|
Created:
4 years, 5 months ago by bungeman-skia Modified:
4 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionRotate bitmap strikes with FreeType.
BUG=skia:3490
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002
Committed: https://skia.googlesource.com/skia/+/31e0c1379e6d0ce48196183e295b929af51fa74e
Committed: https://skia.googlesource.com/skia/+/401ae2d2a0c3f60129e689b922a070e7c367959c
Patch Set 1 #Patch Set 2 : Less invasive, update comments. #
Total comments: 1
Patch Set 3 : Now with subpixel goodness. #
Total comments: 1
Patch Set 4 : Modify subpixel condition. #
Total comments: 3
Patch Set 5 : Add more comments. #Patch Set 6 : Try to get more info. #Patch Set 7 : Remove double bolding of bitmaps. #Patch Set 8 : Remove whitespace. #
Messages
Total messages: 39 (27 generated)
Description was changed from ========== Rotate emoji with FreeType. BUG=skia:3490 ========== to ========== Rotate emoji with FreeType. BUG=skia:3490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002 ==========
bungeman@google.com changed reviewers: + mtklein@google.com, reed@google.com
https://codereview.chromium.org/2139703002/diff/20001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType_common.cpp (right): https://codereview.chromium.org/2139703002/diff/20001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType_common.cpp:468: canvas.concat(bitmapTransform); For the no-skew case everything is the same except right here. Now the transform is directly computed and applied. Before the ratio of the rounded out bounds were used to back form an approximation of a scale. This accounts for the changes to existing gms.
The CQ bit was checked by bungeman@google.com 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 bungeman@google.com
Description was changed from ========== Rotate emoji with FreeType. BUG=skia:3490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002 ========== to ========== Rotate bitmap strikes with FreeType. BUG=skia:3490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002 ==========
Now that there is a test for this, ptal.
The CQ bit was checked by bungeman@google.com 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 bungeman@google.com 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...
https://codereview.chromium.org/2139703002/diff/40001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/2139703002/diff/40001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1116: // If glyph is bitmap and subpixel requested and already transforming bitmap then do subpixel. Turns out I don't quite like this heuristic because it means that if we have a color bitmap font this subpixels all the strikes except those that are exact hits. In other words, the heuristic needs to incorporate font level (cross scalercontext) data to decide instead of just glyph or strike specific data.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2139703002/diff/60001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/2139703002/diff/60001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1117: // (font has no outlines or already transforming bitmap). might help to explain each of the subtle two heuristics a little even, bool can_we = ...; bool should_we = ...; return can_we && should_we;
https://codereview.chromium.org/2139703002/diff/60001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/2139703002/diff/60001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1117: // (font has no outlines or already transforming bitmap). On 2016/07/15 21:08:48, mtklein wrote: > might help to explain each of the subtle two heuristics a little > > even, > > bool can_we = ...; > bool should_we = ...; > return can_we && should_we; Done.
The CQ bit was checked by bungeman@google.com 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 bungeman@google.com
The CQ bit was checked by bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/2139703002/#ps80001 (title: "Add more comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Rotate bitmap strikes with FreeType. BUG=skia:3490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002 ========== to ========== Rotate bitmap strikes with FreeType. BUG=skia:3490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002 Committed: https://skia.googlesource.com/skia/+/31e0c1379e6d0ce48196183e295b929af51fa74e ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/31e0c1379e6d0ce48196183e295b929af51fa74e
Message was sent while issue was closed.
https://codereview.chromium.org/2139703002/diff/60001/src/ports/SkFontHost_Fr... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/2139703002/diff/60001/src/ports/SkFontHost_Fr... src/ports/SkFontHost_FreeType.cpp:1117: // (font has no outlines or already transforming bitmap). On 2016/07/15 21:30:29, bungeman-skia wrote: > On 2016/07/15 21:08:48, mtklein wrote: > > might help to explain each of the subtle two heuristics a little > > > > even, > > > > bool can_we = ...; > > bool should_we = ...; > > return can_we && should_we; > > Done. Thanks! This is awesome.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2149253005/ by bungeman@google.com. The reason for reverting is: Causing roll to fail on telemetry_perf_unittests (bencharks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_desktop.load:search:taobao (and baidu)) and browser_tests (FindInPageControllerTest.FindInPageSpecialURLS). This is due to triggering the assert in copyFTBitmap SkASSERT(dstMask.fBounds.width() == static_cast<int>(srcFTBitmap.width)); when called from inside the block guarded by if (bitmapTransform.isIdentity()).
Message was sent while issue was closed.
Description was changed from ========== Rotate bitmap strikes with FreeType. BUG=skia:3490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002 Committed: https://skia.googlesource.com/skia/+/31e0c1379e6d0ce48196183e295b929af51fa74e ========== to ========== Rotate bitmap strikes with FreeType. BUG=skia:3490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002 Committed: https://skia.googlesource.com/skia/+/31e0c1379e6d0ce48196183e295b929af51fa74e ==========
The CQ bit was checked by bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/2139703002/#ps120001 (title: "Remove double bolding of bitmaps.")
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 bungeman@google.com
The CQ bit was checked by bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/2139703002/#ps140001 (title: "Remove whitespace.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Rotate bitmap strikes with FreeType. BUG=skia:3490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002 Committed: https://skia.googlesource.com/skia/+/31e0c1379e6d0ce48196183e295b929af51fa74e ========== to ========== Rotate bitmap strikes with FreeType. BUG=skia:3490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2139703002 Committed: https://skia.googlesource.com/skia/+/31e0c1379e6d0ce48196183e295b929af51fa74e Committed: https://skia.googlesource.com/skia/+/401ae2d2a0c3f60129e689b922a070e7c367959c ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/401ae2d2a0c3f60129e689b922a070e7c367959c |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
