Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1707023002:
Add pinned versions of *ToFixed. (Closed)
DescriptionAdd pinned versions of *ToFixed.
BUG=skia:4632
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1707023002
Committed: https://skia.googlesource.com/skia/+/70f1a6c64ecb891d77aee576bd045f8b4a03f2a3
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Use simpler logic in *PinToFixed. Correctly format test. #Patch Set 3 : Rebase. #Patch Set 4 : Rebase. #Patch Set 5 : Rebase. #Messages
Total messages: 36 (19 generated)
Description was changed from ========== Add pinned versions of *ToFixed. BUG=skia:4632 ========== to ========== Add pinned versions of *ToFixed. 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/1707023002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707023002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/1707023002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707023002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) 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 #1 (id:20001) has been deleted
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/1707023002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707023002/60001
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
I've been avoiding adding "PinToFixed" because it seems like a hack, but I also don't want to make major fixes of minor problems for code that's going to be deleted in the not-to-distant future. This will likely only be used in src/effects/gradients/SkTwoPointConicalGradient.cpp; see https://codereview.chromium.org/1767163003 and email discussion with fmalita.
https://codereview.chromium.org/1707023002/diff/60001/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/1707023002/diff/60001/include/core/SkFixed.h#... include/core/SkFixed.h:34: // SkFixed has more precision than float near SK_FixedMax/SK_FixedMin. Values higher than Let's write "near SK_FixedMax and SK_FixedMin". Best to avoid confusion about division. https://codereview.chromium.org/1707023002/diff/60001/include/core/SkFixed.h#... include/core/SkFixed.h:37: #define SK_FloatToFixedMax SkFixedToFloat(SK_FixedMax - 0x7f) Walk me through this math / logic? I'm curious if we can write both float and double limits in terms of (1<<31), which is really the fundamental limit of the underlying int type, int32_t. Just a recap to make sure we're on the same page: Float to int conversion works by truncating the float's fractional part, then converting that remaining integral part to integer. It's undefined if that integral part does not fit into the underlying integer. So, for float -> int32_t, we get: 0x00000000 --> 0.0f --> 0, ok ... skip many boring values ... 0x4effffff --> 2147483520.0f --> 2147483520, which fits in int32_t 0x4f000000 --> 2147483648.0f --> 2147483648, does not fit in int32_t And in negative, we get 0x80000000 --> -0.0f --> 0, ok ... skip many boring values ... 0xcf000000 --> -2147483648.0f --> -2147483648, which fits in int32_t 0xcf000001 --> -2147483904.0f --> -2147483904, does not fit in int32_t This makes me wonder if we can write these like this: static inline SkFixed SkFloatPinToFixed(float x) { x *= SK_Fixed1; // We're trying to avoid UB, which happens if we truncate x and its integral part cannot fit into our underlying type (int32_t). if (x >= 2147483648) { return ... } if (x < -2147483648) { return ... } SkASSERT(truncf(x) == (float)(int32_t)x); return x; }
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/1707023002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707023002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1707023002/diff/60001/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/1707023002/diff/60001/include/core/SkFixed.h#... include/core/SkFixed.h:34: // SkFixed has more precision than float near SK_FixedMax/SK_FixedMin. Values higher than On 2016/03/11 at 15:01:29, mtklein wrote: > Let's write "near SK_FixedMax and SK_FixedMin". Best to avoid confusion about division. Removed. https://codereview.chromium.org/1707023002/diff/60001/include/core/SkFixed.h#... include/core/SkFixed.h:37: #define SK_FloatToFixedMax SkFixedToFloat(SK_FixedMax - 0x7f) On 2016/03/11 at 15:01:29, mtklein wrote: > Walk me through this math / logic? I'm curious if we can write both float and double limits in terms of (1<<31), which is really the fundamental limit of the underlying int type, int32_t. > > Just a recap to make sure we're on the same page: > Float to int conversion works by truncating the float's fractional part, then converting that remaining integral part to integer. > It's undefined if that integral part does not fit into the underlying integer. > > So, for float -> int32_t, we get: > 0x00000000 --> 0.0f --> 0, ok > ... skip many boring values ... > 0x4effffff --> 2147483520.0f --> 2147483520, which fits in int32_t > 0x4f000000 --> 2147483648.0f --> 2147483648, does not fit in int32_t > > And in negative, we get > 0x80000000 --> -0.0f --> 0, ok > ... skip many boring values ... > 0xcf000000 --> -2147483648.0f --> -2147483648, which fits in int32_t > 0xcf000001 --> -2147483904.0f --> -2147483904, does not fit in int32_t > > This makes me wonder if we can write these like this: > > static inline SkFixed SkFloatPinToFixed(float x) { > x *= SK_Fixed1; > // We're trying to avoid UB, which happens if we truncate x and its integral part cannot fit into our underlying type (int32_t). > if (x >= 2147483648) { return ... } > if (x < -2147483648) { return ... } > SkASSERT(truncf(x) == (float)(int32_t)x); > return x; > } Done, except it feels wrong to return values less than SK_FixedMin, so I changed the < to <=. My thinking was, "If I multiply first, I'll lose precision." But (obviously) although epsilon will be larger, the amount of precision is the same.
mtklein@google.com changed reviewers: + reed@google.com
+reed lgtm!
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/1707023002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707023002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
Oh, right, this does add to the public API; however, I hope to be able to move SkFixed.h to include/private soon.
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/1707023002/#ps140001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707023002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707023002/140001
Message was sent while issue was closed.
Description was changed from ========== Add pinned versions of *ToFixed. BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add pinned versions of *ToFixed. 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/+/70f1a6c64ecb891d77aee576bd045f8b4a03f2a3 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://skia.googlesource.com/skia/+/70f1a6c64ecb891d77aee576bd045f8b4a03f2a3 |
