Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1691403002:
Change SkUnitScalarClampToByte to more accurate implementation. (Closed)
DescriptionChange SkUnitScalarClampToByte to more accurate implementation.
The previous implementation was likely more efficient when SkScalar was SkFixed.
BUG=skia:4632
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1691403002
Committed: https://skia.googlesource.com/skia/+/f156ea3c3edf4208ebfcb7761135768a840185d0
Patch Set 1 : #
Total comments: 1
Patch Set 2 : Change to (U8CPU)(x*255+0.5). #Patch Set 3 : Rounding doesn't work well for GrCircleBlurFragmentProcessor. #
Total comments: 3
Patch Set 4 : Switch back to verbose form of SkScalarPin to test if it gives different results in dm. #Patch Set 5 : Revert to Patch 2. #Messages
Total messages: 59 (29 generated)
Description was changed from ========== Change SkUnitScalarClampToByte to more efficient implementation. The previous implementation was likely more efficient when SkScalar was SkFixed. BUG=skia:4632 ========== to ========== Change SkUnitScalarClampToByte to more efficient implementation. The previous implementation was likely more efficient when SkScalar was 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/1691403002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-Arm7...) 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_...)
Patchset #1 (id:1) 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/1691403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:20001) 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/1691403002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/40001
Patchset #1 (id:40001) 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/1691403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/60001
Patchset #1 (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/1691403002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/80001
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
Similar to https://codereview.chromium.org/1693683002
lgtm
reed@google.com changed reviewers: + reed@google.com
How different are the results from the prev impl? Seems like we may want either of these as alternatives... (int)(x * 255.9999f) -- evenly distributes all unit floats among the bytes (int)(x * 255 + 0.5f) -- gives equal distribution to 0 and 255
On 2016/02/13 at 12:01:49, reed wrote: > How different are the results from the prev impl? > > Seems like we may want either of these as alternatives... > > (int)(x * 255.9999f) -- evenly distributes all unit floats among the bytes > (int)(x * 255 + 0.5f) -- gives equal distribution to 0 and 255 Output from each are here: https://x20web.corp.google.com/~benjaminwagner/review/1691403002/ to-fixed-shift8 = before this CL x255 = after this CL x255.9999 = static_cast<U8CPU>(SkScalarPin(x, 0, 1) * 255.9999f) x255p0.5 = static_cast<U8CPU>(SkScalarPin(x, 0, 1) * 255 + 0.5f) The biggest difference is visible in ovals.png or roundrects.png, in the green shape second from top and second from right. The x255 impl is darker, but I can't see much difference between the others. There are almost no differences between to-fixed-shift8 and x255.9999.
https://codereview.chromium.org/1691403002/diff/80001/include/core/SkColorPriv.h File include/core/SkColorPriv.h (right): https://codereview.chromium.org/1691403002/diff/80001/include/core/SkColorPri... include/core/SkColorPriv.h:227: return static_cast<U8CPU>(SkScalarPin(x, 0, 1) * 255); In nearly all of our new Sk4f code, we do the following: value * 255 + 0.5 I wonder if we should to that here as well?
Description was changed from ========== Change SkUnitScalarClampToByte to more efficient implementation. The previous implementation was likely more efficient when SkScalar was SkFixed. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Change SkUnitScalarClampToByte to more accurate implementation. The previous implementation was likely more efficient when SkScalar was 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/1691403002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/100001
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 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/1691403002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
So, is this worth-while of a change, since (now) its only used by our HSV code (which we can probably delete too)? https://codereview.chromium.org/1691403002/diff/120001/include/core/SkColorPr... File include/core/SkColorPriv.h (right): https://codereview.chromium.org/1691403002/diff/120001/include/core/SkColorPr... include/core/SkColorPriv.h:227: return static_cast<U8CPU>(SkScalarPin(x, 0, 1) * 255 + 0.5); does this not generate warnings, since you're adding a double? (0.5)
https://codereview.chromium.org/1691403002/diff/120001/include/core/SkColorPr... File include/core/SkColorPriv.h (right): https://codereview.chromium.org/1691403002/diff/120001/include/core/SkColorPr... include/core/SkColorPriv.h:227: return static_cast<U8CPU>(SkScalarPin(x, 0, 1) * 255 + 0.5); On 2016/02/23 01:24:32, reed1 wrote: > does this not generate warnings, since you're adding a double? (0.5) (usually those warnings are only generated for literals whose float loses precision, e.g. 0.1)
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/1691403002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/140001
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 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/1691403002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/140001
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 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/1691403002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/02/23 at 01:24:32, reed wrote: > So, is this worth-while of a change, since (now) its only used by our HSV code (which we can probably delete too)? Sorry, I was uploading new patches to investigate strange rectangles, fixed in https://codereview.chromium.org/1723383002/ SkHSVToColor is used in Android, Chromium, Google3, and a couple GMs.
https://codereview.chromium.org/1691403002/diff/120001/include/core/SkColorPr... File include/core/SkColorPriv.h (right): https://codereview.chromium.org/1691403002/diff/120001/include/core/SkColorPr... include/core/SkColorPriv.h:227: return static_cast<U8CPU>(SkScalarPin(x, 0, 1) * 255 + 0.5); On 2016/02/23 at 02:09:19, mtklein wrote: > On 2016/02/23 01:24:32, reed1 wrote: > > does this not generate warnings, since you're adding a double? (0.5) > > (usually those warnings are only generated for literals whose float loses precision, e.g. 0.1) I could replace it with SK_ScalarHalf, but mtklein previously told me "These SkScalarFoo functions drive me nuts."
lgtm
The CQ bit was checked by benjaminwagner@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/1691403002/#ps160001 (title: "Revert to Patch 2.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1691403002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691403002/160001
Message was sent while issue was closed.
Description was changed from ========== Change SkUnitScalarClampToByte to more accurate implementation. The previous implementation was likely more efficient when SkScalar was SkFixed. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Change SkUnitScalarClampToByte to more accurate implementation. The previous implementation was likely more efficient when SkScalar was SkFixed. 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/+/f156ea3c3edf4208ebfcb7761135768a840185d0 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://skia.googlesource.com/skia/+/f156ea3c3edf4208ebfcb7761135768a840185d0 |
