Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2177103002:
stages for most xfermodes (Closed)
Descriptionstages for most xfermodes
also assumes the clamp from https://codereview.chromium.org/2178793002/ is present
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2177103002
Committed: https://skia.googlesource.com/skia/+/64665440efefbb0dd8fb61dd4bdb6c8a0b4ea290
Patch Set 1 #Patch Set 2 : tidy up #Patch Set 3 : tidy up more #Patch Set 4 : more tweaks #Patch Set 5 : three more #Patch Set 6 : more reorg #Patch Set 7 : note #Patch Set 8 : but #Patch Set 9 : return false #Patch Set 10 : simpler without macro #
Total comments: 2
Messages
Total messages: 28 (21 generated)
Description was changed from ========== stages for most xfermodes BUG=skia: ========== to ========== stages for most xfermodes BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2177103002 ==========
Description was changed from ========== stages for most xfermodes BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2177103002 ========== to ========== stages for most xfermodes also assumes the clamp from https://codereview.chromium.org/2178793002/ is present BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2177103002 ==========
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) Build-Ubuntu-GCC-x86_64-Release-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mtklein@chromium.org changed reviewers: + reed@google.com - mtklein@google.com
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 mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
did you consider a class of these that uses the coefficients to perform the mode? Would be slower (as it'd have to read the coeff from memory) but would allow one code block for many. e.g. SkXfermode::ModeAsCoeff look up the src-coeff and dst-coeff, then return src_coeff * src + dst_coeff * dst https://codereview.chromium.org/2177103002/diff/180001/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/2177103002/diff/180001/src/core/SkXfermode.cp... src/core/SkXfermode.cpp:1439: template <Sk4f kernel(const Sk4f& s, const Sk4f& sa, const Sk4f& d, const Sk4f& da)> given vector_call, do we still need to pass by const& for this (and KERNEL below), or can we pass by value? And does it matter?
Yeah, I did take a brief look at a single function for coefficient modes. To work well, we really want the coefficients expressed as floats, not as an enum. I think that'd require us to go up to 6 coefficients to implement the porter duffs: float kS, kD, kSDa, kDSa, kSD, kSaDa They'd also all be rather slower... every blend would require 10 multiplies and 5 adds, where the worst here is 3 muls and 4 adds (multiply mode). https://codereview.chromium.org/2177103002/diff/180001/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/2177103002/diff/180001/src/core/SkXfermode.cp... src/core/SkXfermode.cpp:1439: template <Sk4f kernel(const Sk4f& s, const Sk4f& sa, const Sk4f& d, const Sk4f& da)> On 2016/07/25 16:40:28, reed1 wrote: > given vector_call, do we still need to pass by const& for this (and KERNEL > below), or can we pass by value? And does it matter? Yes we could require kernel's type to be Sk4f SK_VECTORCALL(Sk4f, Sk4f, Sk4f, Sk4f) I have been trying to limit the use of SK_VECTORCALL to only the functions that absolutely need it, mainly because it's weird. This keeps a sort of conventional distinction between stage functions and their expected-to-be-inlined helpers: stages are weird and use SK_VECTORCALL (and may or may not be static); inlined helpers can be ordinary static functions and pass by const&. As far as I'm aware, there wouldn't be any difference in the generated code in this case.
I see now that asCoeff isn't a magical simplification anyway, since many times the "coeff" involves src-a or dst-a. lgtm
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== stages for most xfermodes also assumes the clamp from https://codereview.chromium.org/2178793002/ is present BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2177103002 ========== to ========== stages for most xfermodes also assumes the clamp from https://codereview.chromium.org/2178793002/ is present BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2177103002 Committed: https://skia.googlesource.com/skia/+/64665440efefbb0dd8fb61dd4bdb6c8a0b4ea290 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/64665440efefbb0dd8fb61dd4bdb6c8a0b4ea290
Message was sent while issue was closed.
On 2016/07/25 18:00:50, commit-bot: I haz the power wrote: > Committed patchset #10 (id:180001) as > https://skia.googlesource.com/skia/+/64665440efefbb0dd8fb61dd4bdb6c8a0b4ea290 KERNEL(plus) seems to be conflicting with std::plus in some of our downstream customers? |
