Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1684623002:
implement more xfermodeproc4f and add GM (Closed)
Descriptionimplement more xfermodeproc4f and add GM
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1677293004
Committed: https://skia.googlesource.com/skia/+/e71253298871403d37855c2c9e242469d4ed17cc
Patch Set 1 #
Total comments: 3
Patch Set 2 : remove unnecessary pins, and add assert #
Total comments: 19
Patch Set 3 : apply suggested expression simplifications #Patch Set 4 : finish refactoring from #12 #
Messages
Total messages: 23 (8 generated)
reed@google.com changed reviewers: + fmalita@chromium.org, herb@google.com, mtklein@google.com
ptal Next round can be to share/merge with the existing 16bit xfermodes.
The CQ bit was checked by reed@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/1684623002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684623002/1
https://codereview.chromium.org/1684623002/diff/1/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/1684623002/diff/1/src/core/SkXfermode.cpp#new... src/core/SkXfermode.cpp:198: return color_alpha(pin_0_1(tmp - s * d), tmp); defintely can't underflow (s - s*d) + (d - d*s)
https://codereview.chromium.org/1684623002/diff/1/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/1684623002/diff/1/src/core/SkXfermode.cpp#new... src/core/SkXfermode.cpp:198: return color_alpha(pin_0_1(tmp - s * d), tmp); On 2016/02/09 16:45:27, mtklein wrote: > defintely can't underflow (s - s*d) + (d - d*s) update: neither difference or exclusion can over or underflow
https://codereview.chromium.org/1684623002/diff/1/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/1684623002/diff/1/src/core/SkXfermode.cpp#new... src/core/SkXfermode.cpp:198: return color_alpha(pin_0_1(tmp - s * d), tmp); On 2016/02/09 16:49:01, mtklein wrote: > On 2016/02/09 16:45:27, mtklein wrote: > > defintely can't underflow (s - s*d) + (d - d*s) > > update: neither difference or exclusion can over or underflow Done.
The CQ bit was checked by reed@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/1684623002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684623002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:97: return color_alpha(s - s * da + d - d * sa + rc, sa + da - sa * da); I find things a little easier to read if you bind * more tightly than +/-: S44f rc = (two*d <= da).thenElse(two*s*d, sa*da - two*(da - d)*(sa-s)); return color_alpha(s - s*da + d - d*sa + rc, sa + da - sa*da); We can probably also pull terms out of color_alpha(), doing less redundant math: return s + d - s*da + color_alpha(rc - d*sa, 0); https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:103: return color_alpha(s + d - Sk4f::Max(s * da, d * sa), sa + da - sa * da); Aren't color and alpha actually the same math? return s + d - Sk4f::Max(s*alpha(d), d*alpha(s)); https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:109: return color_alpha(s + d - Sk4f::Min(s * da, d * sa), sa + da - sa * da); Same deal? return s + d - Sk4f::Min(s*alpha(d), d*alpha(s)); https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:112: static inline float colordodge_f(float sc, float dc, float sa, float da) { Let's steal my Sk4f version of this from SkXfermode_opts.h? https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:145: static Sk4f colorburn_4f(const Sk4f& s, const Sk4f& d) { Ditto... also got an Sk4f version in SkXfermode_opts.h https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:154: static Sk4f hardlight_4f(const Sk4f& s, const Sk4f& d) { Let's put overlay and hardlight closer together? It's not super obvious here that they're the same function with s and d swapped. https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:165: static inline float softlight_f(float sc, float dc, float sa, float da) { Also have a 4f version in SkXfermode_opts.h https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:191: return color_alpha(s + d - Sk4f(2) * Sk4f::Min(s * da, d * sa), I think this is clearer, and maybe cheaper, to pull shared work out of color_alpha(): auto m = Sk4f::Min(s*alpha(d), d*alpha(s)); return s + d - m - color_alpha(m, 0); https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:197: return color_alpha(tmp - s * d, tmp); Same sort of thing? Sk4f screen = s + d - s*d; return screen - color_alpha(s*d, 0); https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:200: //////////////////////////////////////////////////// FYI, didn't pay much attention below here...
https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:97: return color_alpha(s - s * da + d - d * sa + rc, sa + da - sa * da); On 2016/02/09 17:25:51, mtklein wrote: > I find things a little easier to read if you bind * more tightly than +/-: > > S44f rc = (two*d <= da).thenElse(two*s*d, > sa*da - two*(da - d)*(sa-s)); > return color_alpha(s - s*da + d - d*sa + rc, > sa + da - sa*da); > > We can probably also pull terms out of color_alpha(), doing less redundant math: > return s + d - s*da + color_alpha(rc - d*sa, 0); I like the shared-expression refactor: done. Not sure I agree that removing spaces around * helps yet. https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:103: return color_alpha(s + d - Sk4f::Max(s * da, d * sa), sa + da - sa * da); On 2016/02/09 17:25:51, mtklein wrote: > Aren't color and alpha actually the same math? > return s + d - Sk4f::Max(s*alpha(d), d*alpha(s)); Done. https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:109: return color_alpha(s + d - Sk4f::Min(s * da, d * sa), sa + da - sa * da); On 2016/02/09 17:25:51, mtklein wrote: > Same deal? > return s + d - Sk4f::Min(s*alpha(d), d*alpha(s)); Done. https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:154: static Sk4f hardlight_4f(const Sk4f& s, const Sk4f& d) { On 2016/02/09 17:25:51, mtklein wrote: > Let's put overlay and hardlight closer together? It's not super obvious here > that they're the same function with s and d swapped. Done.
https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:112: static inline float colordodge_f(float sc, float dc, float sa, float da) { On 2016/02/09 17:25:51, mtklein wrote: > Let's steal my Sk4f version of this from SkXfermode_opts.h? Done. https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:145: static Sk4f colorburn_4f(const Sk4f& s, const Sk4f& d) { On 2016/02/09 17:25:52, mtklein wrote: > Ditto... also got an Sk4f version in SkXfermode_opts.h Done. https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:165: static inline float softlight_f(float sc, float dc, float sa, float da) { On 2016/02/09 17:25:52, mtklein wrote: > Also have a 4f version in SkXfermode_opts.h Done. https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:191: return color_alpha(s + d - Sk4f(2) * Sk4f::Min(s * da, d * sa), On 2016/02/09 17:25:51, mtklein wrote: > I think this is clearer, and maybe cheaper, to pull shared work out of > color_alpha(): > > auto m = Sk4f::Min(s*alpha(d), d*alpha(s)); > return s + d - m - color_alpha(m, 0); Clever. done. https://codereview.chromium.org/1684623002/diff/20001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:197: return color_alpha(tmp - s * d, tmp); On 2016/02/09 17:25:51, mtklein wrote: > Same sort of thing? > > Sk4f screen = s + d - s*d; > return screen - color_alpha(s*d, 0); Done.
The CQ bit was checked by reed@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/1684623002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684623002/60001
lgtm
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 reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684623002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684623002/60001
Message was sent while issue was closed.
Description was changed from ========== implement more xfermodeproc4f and add GM BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== implement more xfermodeproc4f and add GM BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e71253298871403d37855c2c9e242469d4ed17cc ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/e71253298871403d37855c2c9e242469d4ed17cc |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
