Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1683743005:
Simple cleanups related to SkFixed. (Closed)
DescriptionCleanups related to SkFixed.
Remove SK_FixedNaN. It does not seem to be supported or used anywhere in Skia, Chromium, Android, or Google3, (except accidentally by TwoPtRadial::kDontDrawT). I think supporting it would erase any benefit of SkFixed over float.
Remove SkBitmapFilter::lookup. It does not appear to be used anywhere in Skia, Chromium, Android, or Google3.
Fix a bug in SkPaint::breakText that limited it to ~5kB of text. Now it can handle more than 1GB.
BUG=skia:4632
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1683743005
Committed: https://skia.googlesource.com/skia/+/7ea5cb18389db11a94175de95c9db2b44972630c
Patch Set 1 #
Total comments: 7
Patch Set 2 : Add test for Paint::breakText. Revert change to SK_FixedMin. #Patch Set 3 : Revert changes to SkTwoPointConicalGradient.h; actually add test. #Patch Set 4 : Revert changes to src/core/SkScan_Hairline.cpp; causes one layout test to regress. #
Messages
Total messages: 45 (22 generated)
Description was changed from ========== Simple cleanups related to SkFixed. BUG=skia:4632 ========== to ========== Simple cleanups related to SkFixed. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683743005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683743005/1
Description was changed from ========== Simple cleanups related to SkFixed. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Simple cleanups related to SkFixed. Remove SK_FixedNaN. It does not seem to be supported or used anywhere in Skia, Chromium, Android, or Google3, (except accidentally by TwoPtRadial::kDontDrawT). I think supporting it would erase any benefit of SkFixed over float. Updated SK_FixedMin to be the actual min. Remove SkBitmapFilter::lookup. It does not appear to be used anywhere in Skia, Chromium, Android, or Google3. Use SkScalarFloorToInt in SkScan::HairRect to handle larger values. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
benjaminwagner@google.com changed reviewers: + mtklein@google.com, reed@google.com
Can we make SkFixed entirely private? (i.e. do our clients ever need it?) https://codereview.chromium.org/1683743005/diff/1/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/1683743005/diff/1/include/core/SkFixed.h#newc... include/core/SkFixed.h:24: #define SK_FixedMin (0x80000000) Why this change? https://codereview.chromium.org/1683743005/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/1683743005/diff/1/src/core/SkPaint.cpp#newcod... src/core/SkPaint.cpp:1031: Sk48Dot16 max = SkScalarTo48Dot16(maxWidth); Wow, this seems like a bug bug. Is/was there a test that showed this bug?
Chromium uses SkFixed, mostly because it's the same as hb_position_t (HarfBuzz's fixed point type). There is some cleanup that could be done in that area. https://codereview.chromium.org/1683743005/diff/1/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/1683743005/diff/1/include/core/SkFixed.h#newc... include/core/SkFixed.h:24: #define SK_FixedMin (0x80000000) On 2016/02/11 at 21:43:44, reed1 wrote: > Why this change? Because it seemed odd to me that for some x, SkScalarToFixed(x) < SK_FixedMin. https://codereview.chromium.org/1683743005/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/1683743005/diff/1/src/core/SkPaint.cpp#newcod... src/core/SkPaint.cpp:1031: Sk48Dot16 max = SkScalarTo48Dot16(maxWidth); On 2016/02/11 at 21:43:44, reed1 wrote: > Wow, this seems like a bug bug. Is/was there a test that showed this bug? The previous code was actually fine, assuming maxWidth is within the range of SkFixed. Casting from SkFixed (aka Sk16Dot16) to Sk48Dot16 is just adding zeros (or ones) on the left.
https://codereview.chromium.org/1683743005/diff/1/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/1683743005/diff/1/include/core/SkFixed.h#newc... include/core/SkFixed.h:24: #define SK_FixedMin (0x80000000) On 2016/02/11 21:53:13, Ben Wagner wrote: > On 2016/02/11 at 21:43:44, reed1 wrote: > > Why this change? > > Because it seemed odd to me that for some x, SkScalarToFixed(x) < SK_FixedMin. I think its wacky either way: Now we have -SK_FixedMin == SK_FixedMin :( https://codereview.chromium.org/1683743005/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/1683743005/diff/1/src/core/SkPaint.cpp#newcod... src/core/SkPaint.cpp:1031: Sk48Dot16 max = SkScalarTo48Dot16(maxWidth); On 2016/02/11 21:53:13, Ben Wagner wrote: > On 2016/02/11 at 21:43:44, reed1 wrote: > > Wow, this seems like a bug bug. Is/was there a test that showed this bug? > > The previous code was actually fine, assuming maxWidth is within the range of > SkFixed. Casting from SkFixed (aka Sk16Dot16) to Sk48Dot16 is just adding zeros > (or ones) on the left. So we could write a test that measures a big string (e.g. at a large point-size), and see it overflow the SkFixed?
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683743005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683743005/20001
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Simple cleanups related to SkFixed. Remove SK_FixedNaN. It does not seem to be supported or used anywhere in Skia, Chromium, Android, or Google3, (except accidentally by TwoPtRadial::kDontDrawT). I think supporting it would erase any benefit of SkFixed over float. Updated SK_FixedMin to be the actual min. Remove SkBitmapFilter::lookup. It does not appear to be used anywhere in Skia, Chromium, Android, or Google3. Use SkScalarFloorToInt in SkScan::HairRect to handle larger values. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Cleanups related to SkFixed. Remove SK_FixedNaN. It does not seem to be supported or used anywhere in Skia, Chromium, Android, or Google3, (except accidentally by TwoPtRadial::kDontDrawT). I think supporting it would erase any benefit of SkFixed over float. Remove SkBitmapFilter::lookup. It does not appear to be used anywhere in Skia, Chromium, Android, or Google3. Use SkScalarFloorToInt in SkScan::HairRect to handle larger values. Fix a bug in SkPaint::breakText that limited it to ~5kB of text. Now it can handle more than 1GB. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1683743005/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/1683743005/diff/1/src/core/SkPaint.cpp#newcod... src/core/SkPaint.cpp:1031: Sk48Dot16 max = SkScalarTo48Dot16(maxWidth); On 2016/02/11 at 22:22:59, reed1 wrote: > On 2016/02/11 21:53:13, Ben Wagner wrote: > > On 2016/02/11 at 21:43:44, reed1 wrote: > > > Wow, this seems like a bug bug. Is/was there a test that showed this bug? > > > > The previous code was actually fine, assuming maxWidth is within the range of > > SkFixed. Casting from SkFixed (aka Sk16Dot16) to Sk48Dot16 is just adding zeros > > (or ones) on the left. > > So we could write a test that measures a big string (e.g. at a large point-size), and see it overflow the SkFixed? Oh! For some reason I was thinking that was the width of one glyph. Large point-size is handled by scaling down the font and the maxWidth, so that's not a problem. However, this method previously could only handle ~5kB of text at the default size. The fix allows it to handle more than 1GB.
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683743005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683743005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683743005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683743005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683743005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683743005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated diff. FWIW, SkPaint::breakText is not widely used at the moment.
PTAL
lgtm does any of this change Chrome layouttests?
Ben, this is the change to SkPaint::breakText that I mentioned to you.
On 2016/02/22 at 18:11:41, reed wrote: > lgtm > > does any of this change Chrome layouttests? Yes. I had to revert src/core/SkScan_Hairline.cpp to get fast/block/float/overhanging-tall-block.html to pass.
The CQ bit was checked by benjaminwagner@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1683743005/#ps120001 (title: "Revert changes to src/core/SkScan_Hairline.cpp; causes one layout test to regress.")
The CQ bit was unchecked by benjaminwagner@google.com
Description was changed from ========== Cleanups related to SkFixed. Remove SK_FixedNaN. It does not seem to be supported or used anywhere in Skia, Chromium, Android, or Google3, (except accidentally by TwoPtRadial::kDontDrawT). I think supporting it would erase any benefit of SkFixed over float. Remove SkBitmapFilter::lookup. It does not appear to be used anywhere in Skia, Chromium, Android, or Google3. Use SkScalarFloorToInt in SkScan::HairRect to handle larger values. Fix a bug in SkPaint::breakText that limited it to ~5kB of text. Now it can handle more than 1GB. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Cleanups related to SkFixed. Remove SK_FixedNaN. It does not seem to be supported or used anywhere in Skia, Chromium, Android, or Google3, (except accidentally by TwoPtRadial::kDontDrawT). I think supporting it would erase any benefit of SkFixed over float. Remove SkBitmapFilter::lookup. It does not appear to be used anywhere in Skia, Chromium, Android, or Google3. Fix a bug in SkPaint::breakText that limited it to ~5kB of text. Now it can handle more than 1GB. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by benjaminwagner@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683743005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683743005/120001
Message was sent while issue was closed.
Description was changed from ========== Cleanups related to SkFixed. Remove SK_FixedNaN. It does not seem to be supported or used anywhere in Skia, Chromium, Android, or Google3, (except accidentally by TwoPtRadial::kDontDrawT). I think supporting it would erase any benefit of SkFixed over float. Remove SkBitmapFilter::lookup. It does not appear to be used anywhere in Skia, Chromium, Android, or Google3. Fix a bug in SkPaint::breakText that limited it to ~5kB of text. Now it can handle more than 1GB. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Cleanups related to SkFixed. Remove SK_FixedNaN. It does not seem to be supported or used anywhere in Skia, Chromium, Android, or Google3, (except accidentally by TwoPtRadial::kDontDrawT). I think supporting it would erase any benefit of SkFixed over float. Remove SkBitmapFilter::lookup. It does not appear to be used anywhere in Skia, Chromium, Android, or Google3. Fix a bug in SkPaint::breakText that limited it to ~5kB of text. Now it can handle more than 1GB. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/7ea5cb18389db11a94175de95c9db2b44972630c ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://skia.googlesource.com/skia/+/7ea5cb18389db11a94175de95c9db2b44972630c
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:120001) has been created in https://codereview.chromium.org/1724283003/ by benjaminwagner@google.com. The reason for reverting is: New test is failing on Windows.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
