Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
967833003:
check for inputs before reporting asColorFilter (Closed)
DescriptionUse ComposeColorFilter in factory to collapse consecutive filters (when possible).
Change asColorFilter to reflect its reliance on the new factory behavior.
patch from issue 967143002 at patchset 80001 (http://crrev.com/967143002#ps80001)
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/dac843bf046c2cd79fd955cb177aee241d7a4b0c
Patch Set 1 #Patch Set 2 : use asserts in asColorFilter #
Total comments: 4
Patch Set 3 : restore (but reverse) brightness test #
Total comments: 9
Patch Set 4 : check for specifics of the combined colorfilters #
Total comments: 6
Patch Set 5 : fix comments in tests #Patch Set 6 : rebase #Patch Set 7 : check if the composecolorfilter succeeded #
Total comments: 1
Messages
Total messages: 28 (7 generated)
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/967833003/diff/20001/src/effects/SkColorFilte... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.chromium.org/967833003/diff/20001/src/effects/SkColorFilte... src/effects/SkColorFilterImageFilter.cpp:25: if (!cropRect && input && input->asColorFilter(&inputCF)) { I'm not sure this !cropRect check is correct. I think it should be valid to collapse a chain of CFIFs, even if the last one has a crop rect -- it'll get applied by this CFIF at draw time. It's only the intermediate (input) CFs that can't have crop rects, which is checked in asColorFilter() below. For your purposes, calling asColorFilter() on the final CFIF (and checking that it has a NULL input) should be enough, since it will do the crop rect check that we don't want here. https://codereview.chromium.org/967833003/diff/20001/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (left): https://codereview.chromium.org/967833003/diff/20001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:156: // Check that a clipping color matrix followed by a grayscale does not concatenate into a single filter. Why did this test go away?
https://codereview.chromium.org/967833003/diff/20001/src/effects/SkColorFilte... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.chromium.org/967833003/diff/20001/src/effects/SkColorFilte... src/effects/SkColorFilterImageFilter.cpp:25: if (!cropRect && input && input->asColorFilter(&inputCF)) { On 2015/03/04 19:38:37, Stephen White wrote: > I'm not sure this !cropRect check is correct. I think it should be valid to > collapse a chain of CFIFs, even if the last one has a crop rect -- it'll get > applied by this CFIF at draw time. It's only the intermediate (input) CFs that > can't have crop rects, which is checked in asColorFilter() below. > > For your purposes, calling asColorFilter() on the final CFIF (and checking that > it has a NULL input) should be enough, since it will do the crop rect check that > we don't want here. Agreed. https://codereview.chromium.org/967833003/diff/20001/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (left): https://codereview.chromium.org/967833003/diff/20001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:156: // Check that a clipping color matrix followed by a grayscale does not concatenate into a single filter. On 2015/03/04 19:38:37, Stephen White wrote: > Why did this test go away? With the change to the factory, we *do* fold the two together (using SkComposeColorFilter).
ptal
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/967833003/40001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-03-05 02:04 UTC
reed@google.com changed reviewers: + robertphillips@google.com
https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (right): https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:147: static SkImageFilter* make_blue(SkImageFilter* input, const SkImageFilter::CropRect* cropRect) { overlength ? https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:160: { // Check that a color matrix followed by another color matrix concatenates ? I'm not sure if, as it stands, this is useful given the prior test.
https://codereview.chromium.org/967833003/diff/40001/src/effects/SkColorFilte... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.chromium.org/967833003/diff/40001/src/effects/SkColorFilte... src/effects/SkColorFilterImageFilter.cpp:90: SkASSERT(1 == this->countInputs()); Nit: this seems kinda superfluous, since it's hardcoded in the constructor. But harmless I suppose. https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (right): https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:160: { On 2015/03/04 20:14:40, robertphillips wrote: > // Check that a color matrix followed by another color matrix concatenates ? > > I'm not sure if, as it stands, this is useful given the prior test. I wonder if we should modify the matrix test to check that the combined CF is a matrix CF, not a compose CF. https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:161: // Check that a clipping color matrix followed by a grayscale does concatenate ... and maybe we should check that this one *is* a compose colorfilter, and not a matrix CF?
https://codereview.chromium.org/967833003/diff/40001/src/effects/SkColorFilte... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.chromium.org/967833003/diff/40001/src/effects/SkColorFilte... src/effects/SkColorFilterImageFilter.cpp:90: SkASSERT(1 == this->countInputs()); On 2015/03/04 20:26:16, Stephen White wrote: > Nit: this seems kinda superfluous, since it's hardcoded in the constructor. But > harmless I suppose. But if we ever (accidentally or otherwise) change the constructor, then we could give false positives here w/o this assert. https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (right): https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:147: static SkImageFilter* make_blue(SkImageFilter* input, const SkImageFilter::CropRect* cropRect) { On 2015/03/04 20:14:40, robertphillips wrote: > overlength ? Done. https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:160: { On 2015/03/04 20:26:17, Stephen White wrote: > On 2015/03/04 20:14:40, robertphillips wrote: > > // Check that a color matrix followed by another color matrix concatenates ? > > > > I'm not sure if, as it stands, this is useful given the prior test. > > I wonder if we should modify the matrix test to check that the combined CF is a > matrix CF, not a compose CF. Done. https://codereview.chromium.org/967833003/diff/40001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:161: // Check that a clipping color matrix followed by a grayscale does concatenate On 2015/03/04 20:26:16, Stephen White wrote: > ... and maybe we should check that this one *is* a compose colorfilter, and not > a matrix CF? Done.
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/967833003/60001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-03-05 02:34 UTC
lgtm + nits https://codereview.chromium.org/967833003/diff/60001/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (right): https://codereview.chromium.org/967833003/diff/60001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:154: { single _color matrix_ filter ? https://codereview.chromium.org/967833003/diff/60001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:165: { grayscale -> color matrix ? becuase typo https://codereview.chromium.org/967833003/diff/60001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:186: // that is another colorfilterimage can be expressed as a colorfilter (composed). extra "//" ?
https://codereview.chromium.org/967833003/diff/60001/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (right): https://codereview.chromium.org/967833003/diff/60001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:154: { On 2015/03/04 20:39:48, robertphillips wrote: > single _color matrix_ filter ? Done. https://codereview.chromium.org/967833003/diff/60001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:165: { On 2015/03/04 20:39:48, robertphillips wrote: > grayscale -> color matrix ? > becuase typo Done. https://codereview.chromium.org/967833003/diff/60001/tests/ImageFilterTest.cp... tests/ImageFilterTest.cpp:186: // that is another colorfilterimage can be expressed as a colorfilter (composed). On 2015/03/04 20:39:48, robertphillips wrote: > extra "//" ? Done.
senorblanco@chromium.org changed reviewers: + bsalomon@chromium.org
This change looks ok on its own, but I'm concerned about the GPU implementation of SkComposeColorFilter (I'm not sure if it was enabled before this change, but it looks like it'll be required now, since we no longer check the result of ComposeColorFilter(), so we must concatenate CFs). 1) We need to clamp colors to the [0, 1] range between fragment processors, to preserve the same semantics as the CPU path. 2) More problematically, we may run out of GPU resources, such as uniforms, or even fragment shader instructions, if we attempt to concatenate too many colorfilters into a single shader. See https://code.google.com/p/skia/issues/detail?id=1389
#1 sounds like a bug that already exists in the colormatrix frag. Those stages should always return premul colors [0...1]. If that is not the case today, it should be fixed independent of this CL I think.
On 2015/03/04 21:01:35, reed1 wrote: > #1 sounds like a bug that already exists in the colormatrix frag. Those stages > should always return premul colors [0...1]. If that is not the case today, it > should be fixed independent of this CL I think. Hmm. The color matrix does do unpremultiply. Maybe that takes care of the clamping as well; I'd have to check.
On 2015/03/04 21:09:30, Stephen White wrote: > On 2015/03/04 21:01:35, reed1 wrote: > > #1 sounds like a bug that already exists in the colormatrix frag. Those stages > > should always return premul colors [0...1]. If that is not the case today, it > > should be fixed independent of this CL I think. > > Hmm. The color matrix does do unpremultiply. Maybe that takes care of the > clamping as well; I'd have to check. Yeah, looks like there's an explicit clamp, so #1 is not an issue: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... Brian suggests an arbitrary limit on the number of concatenable frag processors (say, 4) to deal with #2.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/967833003/#ps110001 (title: "check if the composecolorfilter succeeded")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967833003/110001
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as https://skia.googlesource.com/skia/+/dac843bf046c2cd79fd955cb177aee241d7a4b0c
Message was sent while issue was closed.
https://codereview.chromium.org/967833003/diff/110001/src/effects/SkColorFilt... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.chromium.org/967833003/diff/110001/src/effects/SkColorFilt... src/effects/SkColorFilterImageFilter.cpp:94: // if we have an input, it is *not* also a colorfilter. Since we're now limiting the amount of collapsing we do, I think this may no longer be true. I suspect this will assert with >4 color filters composed.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:110001) has been created in https://codereview.chromium.org/978923005/ by reed@google.com. The reason for reverting is: Need to suppress these for rebaselining, so reverting for now. Regressions: Unexpected image-only failures (5) css3/filters/effect-brightness-clamping-hw.html [ ImageOnlyFailure ] css3/filters/effect-combined-hw.html [ ImageOnlyFailure ] virtual/slimmingpaint/css3/filters/effect-brightness-clamping-hw.html [ ImageOnlyFailure ] virtual/slimmingpaint/css3/filters/effect-combined-hw.html [ ImageOnlyFailure ] . |
