Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1693683002:
Change ScalarTo256 to more efficient implementation. (Closed)
DescriptionChange ScalarTo256 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&issue=1693683002
Committed: https://skia.googlesource.com/skia/+/e6af96a88901b3c6f0c27575197a93db6cb04042
Patch Set 1 #
Total comments: 2
Patch Set 2 : Assume SkScalar is floating point. #
Total comments: 1
Patch Set 3 : Indent. #Messages
Total messages: 26 (12 generated)
Description was changed from ========== Change ScalarTo256 to more efficient implementation. The previous implementation was likely more efficient when SkScalar was SkFixed. BUG=skia:4632 ========== to ========== Change ScalarTo256 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/1693683002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1693683002/1
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
https://codereview.chromium.org/1693683002/diff/1/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1693683002/diff/1/src/core/SkDraw.cpp#newcode... src/core/SkDraw.cpp:1764: return SkScalarTruncToInt(SkScalarMul(SkScalarPin(v, 0, SK_Scalar1), Let's write (int)(SkScalarPin(v,0,1) * 256) ? These SkScalarFoo functions drive me nuts.
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/1693683002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1693683002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1693683002/diff/1/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1693683002/diff/1/src/core/SkDraw.cpp#newcode... src/core/SkDraw.cpp:1764: return SkScalarTruncToInt(SkScalarMul(SkScalarPin(v, 0, SK_Scalar1), On 2016/02/12 at 17:19:57, mtklein wrote: > Let's write (int)(SkScalarPin(v,0,1) * 256) ? > > These SkScalarFoo functions drive me nuts. Done.
lgtm https://codereview.chromium.org/1693683002/diff/20001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1693683002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1764: return static_cast<int>(SkScalarPin(v, 0, 1) * 256); 4 space indent
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/1693683002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1693683002/40001
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
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/1693683002/#ps40001 (title: "Indent.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1693683002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1693683002/40001
Message was sent while issue was closed.
Description was changed from ========== Change ScalarTo256 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 ScalarTo256 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... Committed: https://skia.googlesource.com/skia/+/e6af96a88901b3c6f0c27575197a93db6cb04042 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/e6af96a88901b3c6f0c27575197a93db6cb04042
Message was sent while issue was closed.
reed@google.com changed reviewers: + reed@google.com
Message was sent while issue was closed.
what are the differences between the impls? Should we consider either of these... (int)(x * 256.999f) (int)(x * 256 + 0.5f)
Message was sent while issue was closed.
On 2016/02/13 at 12:04:15, reed wrote: > what are the differences between the impls? > > Should we consider either of these... > > (int)(x * 256.999f) > (int)(x * 256 + 0.5f) Here are the outputs from each of these impls: https://x20web.corp.google.com/~benjaminwagner/review/1693683002/ to-fixed-shift8-p1 = before this CL x256 = after this CL x256.999 = static_cast<int>(SkScalarPin(v, 0, 1) * 256.999f) x256p0.5 = static_cast<int>(SkScalarPin(v, 0, 1) * 256 + 0.5f) The differences are most noticeable in the saddles of 565/gm/patch_grid.png, and somewhat in the center of 565/gm/patch_primitive.png. To my eye, there's very little difference between the three x256* impls. |
