Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1388823002:
add LocalMatrixImageFilter
Descriptionadd LocalMatrixImageFilter
BUG=skia:
Patch Set 1 #
Total comments: 26
Patch Set 2 : rename new method to newWithLocalMatrix #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : attempted input-based approach (no success yet) #Patch Set 7 : add gm #
Messages
Total messages: 26 (6 generated)
reed@google.com changed reviewers: + robertphillips@google.com, senorblanco@google.com
The hacks to blurimagefilter are just there for testing. will remove before submitting.
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/1388823002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1388823002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1388823002/diff/1/include/core/SkImageFilter.h File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1388823002/diff/1/include/core/SkImageFilter.... include/core/SkImageFilter.h:221: /** rm "and, if not NULL, the localMatrix" ? https://codereview.chromium.org/1388823002/diff/1/include/core/SkImageFilter.... include/core/SkImageFilter.h:224: * "to balance to call" -> "to call" ? https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... File src/core/SkLocalMatrixImageFilter.cpp (right): https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:11: // Dox ? https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:18: if (localM.isIdentity()) { Why not do this ? https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:31: bool asFragmentProcessor(GrFragmentProcessor**, GrProcessorDataManager*, GrTexture*, align ? https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:37: bool onFilterImage(Proxy*, const SkBitmap& src, const Context&, align ?
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... File src/core/SkLocalMatrixImageFilter.cpp (right): https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:19: // return SkRef(base); Remove commented-out code if it doesn't work. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:30: bool onIsColorFilterNode(SkColorFilter** /*filterPtr*/) const override; I don't think this'll work, since you can't collapse a subsequent color filter into this one, due to the different local matrix. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:31: bool asFragmentProcessor(GrFragmentProcessor**, GrProcessorDataManager*, GrTexture*, Shouldn't need this if you call fBase->filterImage() instead of onFilterImage() -- see onFilterImage() below. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:34: bool canComputeFastBounds() const override; Shouldn't need this (see onFilterImage() below). https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:87: bool SkLocalMatrixImageFilter::canComputeFastBounds() const { You shouldn't need to virtualize this -- overriding affectsTransparentBlack() should be sufficent for the base class version of canComputeFastBounds to do the right thing. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:102: if (!fLocalM.mapRect(&tmp, src)) { This mapping should be done to the result of fBase->computeFastBounds(), not to its argument (an SkOffsetImageFilter upstream of an SkLocalMatrixImageFilter would probably show the difference). https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:111: return fBase->onFilterImage(proxy, src, localCtx, result, offset); If you change this to call fBase->filterImage() instead of onFilterImage(), I think it should take care of the GPU case as well. Then you could remove canFilterImageGPU() and filterImageGPU() from this class (it'll become a "generic skia" filter, same as SkColorFilterImageFilter for example -- notice there are no GPU calls there either).
https://codereview.chromium.org/1388823002/diff/1/include/core/SkImageFilter.h File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1388823002/diff/1/include/core/SkImageFilter.... include/core/SkImageFilter.h:228: virtual SkImageFilter* refWithLocalMatrix(const SkMatrix& localMatrix) const; I find the name confusing, since "ref" doesn't really capture everything it does. Maybe createWithLocalMatrix? It may be creating a new filter in some cases, if I read the above correctly. https://codereview.chromium.org/1388823002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1388823002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:238: uint32_t srcGenID = this->usesSrcInput() ? src.getGenerationID() : 0; If you recurse through filterImage() instead of onFilterImage(), I don't think you'll need to virtualize usesSrcInput(). The upstream filter will correctly cache itself, as will this one (with the same bitmap but a different key).
https://codereview.chromium.org/1388823002/diff/1/include/core/SkImageFilter.h File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1388823002/diff/1/include/core/SkImageFilter.... include/core/SkImageFilter.h:221: /** On 2015/10/05 20:50:11, robertphillips wrote: > rm "and, if not NULL, the localMatrix" ? Done. https://codereview.chromium.org/1388823002/diff/1/include/core/SkImageFilter.... include/core/SkImageFilter.h:224: * On 2015/10/05 20:50:11, robertphillips wrote: > "to balance to call" -> "to call" ? Done. https://codereview.chromium.org/1388823002/diff/1/include/core/SkImageFilter.... include/core/SkImageFilter.h:228: virtual SkImageFilter* refWithLocalMatrix(const SkMatrix& localMatrix) const; On 2015/10/05 21:06:23, Stephen White wrote: > I find the name confusing, since "ref" doesn't really capture everything it > does. Maybe createWithLocalMatrix? It may be creating a new filter in some > cases, if I read the above correctly. Changed to newWithLocalMatrix https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... File src/core/SkLocalMatrixImageFilter.cpp (right): https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:11: On 2015/10/05 20:50:11, robertphillips wrote: > // Dox ? Done. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:18: if (localM.isIdentity()) { On 2015/10/05 20:50:11, robertphillips wrote: > Why not do this ? Done. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:19: // return SkRef(base); On 2015/10/05 21:00:41, Stephen White wrote: > Remove commented-out code if it doesn't work. Done. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:30: bool onIsColorFilterNode(SkColorFilter** /*filterPtr*/) const override; On 2015/10/05 21:00:41, Stephen White wrote: > I don't think this'll work, since you can't collapse a subsequent color filter > into this one, due to the different local matrix. ColorFilters don't look at any matrix, so I think they are unaffected by our local matrix. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:31: bool asFragmentProcessor(GrFragmentProcessor**, GrProcessorDataManager*, GrTexture*, On 2015/10/05 20:50:11, robertphillips wrote: > align ? Done. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:31: bool asFragmentProcessor(GrFragmentProcessor**, GrProcessorDataManager*, GrTexture*, On 2015/10/05 20:50:11, robertphillips wrote: > align ? Done. https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:37: bool onFilterImage(Proxy*, const SkBitmap& src, const Context&, On 2015/10/05 20:50:11, robertphillips wrote: > align ? Done.
Stephen, I *think* I understand the direction of your comments : are you proposing that some of the methods I override (e.g. canFilterImageGPU) need not be overridden, because no one will call them (other than filterImage)? If so, that feels fragile unless we can really enforce that somehow. Is the current scheme incorrect, or not just as minimal as you think it could be?
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/1388823002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1388823002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/10/09 19:16:35, reed1 wrote: > Stephen, > > I *think* I understand the direction of your comments : are you proposing that > some of the methods I override (e.g. canFilterImageGPU) need not be overridden, > because no one will call them (other than filterImage)? If so, that feels > fragile unless we can really enforce that somehow. Is the current scheme > incorrect, or not just as minimal as you think it could be? Mostly the latter, but also the former in the colorfilter case (let me see if I can conjure an example for that). Mostly, I don't like the idea of making base class members virtual unnecessarily. Sort of similar to if someone virtualized a bunch of SkCanvas members just for a single subclass. And I think they can be avoided for the most part by calling through filterImage() instead of onFilterImage().
https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... File src/core/SkLocalMatrixImageFilter.cpp (right): https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... src/core/SkLocalMatrixImageFilter.cpp:30: bool onIsColorFilterNode(SkColorFilter** /*filterPtr*/) const override; On 2015/10/09 18:56:36, reed1 wrote: > On 2015/10/05 21:00:41, Stephen White wrote: > > I don't think this'll work, since you can't collapse a subsequent color filter > > into this one, due to the different local matrix. > > ColorFilters don't look at any matrix, so I think they are unaffected by our > local matrix. Consider the DAG: blur -> colorfilter -> localmatrixfilter. It looks like the current code would collapse this to blur -> colorfilter, so the localmatrix filter would no longer be applied to the blur params.
On 2015/10/09 19:45:22, Stephen White wrote: > On 2015/10/09 19:16:35, reed1 wrote: > > Stephen, > > > > I *think* I understand the direction of your comments : are you proposing that > > some of the methods I override (e.g. canFilterImageGPU) need not be > overridden, > > because no one will call them (other than filterImage)? If so, that feels > > fragile unless we can really enforce that somehow. Is the current scheme > > incorrect, or not just as minimal as you think it could be? > > Mostly the latter, but also the former in the colorfilter case (let me see if I > can > conjure an example for that). Mostly, I don't like the idea of making base class > > members virtual unnecessarily. Sort of similar to if someone virtualized a > bunch > of SkCanvas members just for a single subclass. And I think they can be avoided > for the most part by calling through filterImage() instead of onFilterImage(). When we have a proxy pattern (like NWayCanvas or DeferredCanvas) we make enough virtual so that the new wrapper-class can be used safely in all contexts. I think that is where we might be with LocalMatrixImageFilter, since code can ask it questions like canFilterImageGPU...
On 2015/10/09 19:54:13, Stephen White wrote: > https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... > File src/core/SkLocalMatrixImageFilter.cpp (right): > > https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... > src/core/SkLocalMatrixImageFilter.cpp:30: bool > onIsColorFilterNode(SkColorFilter** /*filterPtr*/) const override; > On 2015/10/09 18:56:36, reed1 wrote: > > On 2015/10/05 21:00:41, Stephen White wrote: > > > I don't think this'll work, since you can't collapse a subsequent color > filter > > > into this one, due to the different local matrix. > > > > ColorFilters don't look at any matrix, so I think they are unaffected by our > > local matrix. > > Consider the DAG: blur -> colorfilter -> localmatrixfilter. > > It looks like the current code would collapse this to blur -> colorfilter, so > the localmatrix filter would no longer be applied to the blur params. I can't tell without testing that. Lets do try to create some tests/DAGs to exercise this.
On 2015/10/09 20:02:35, reed1 wrote: > On 2015/10/09 19:54:13, Stephen White wrote: > > > https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... > > File src/core/SkLocalMatrixImageFilter.cpp (right): > > > > > https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... > > src/core/SkLocalMatrixImageFilter.cpp:30: bool > > onIsColorFilterNode(SkColorFilter** /*filterPtr*/) const override; > > On 2015/10/09 18:56:36, reed1 wrote: > > > On 2015/10/05 21:00:41, Stephen White wrote: > > > > I don't think this'll work, since you can't collapse a subsequent color > > filter > > > > into this one, due to the different local matrix. > > > > > > ColorFilters don't look at any matrix, so I think they are unaffected by our > > > local matrix. > > > > Consider the DAG: blur -> colorfilter -> localmatrixfilter. > > > > It looks like the current code would collapse this to blur -> colorfilter, so > > the localmatrix filter would no longer be applied to the blur params. > > I can't tell without testing that. Lets do try to create some tests/DAGs to > exercise this. In your diagram, what do the arrows mean? Is the blur an input to the colorfitler, which is wrapped by localmatrixfilter?
Ahh, I think I see part of my confusion. SkLocalMatrixImageFilter does not use an input, but the fBase pointer. I think if we rewrote it to use an actual input, then everything would fall out nicely, and we wouldn't need many of these overrides. The plumbing for filterImageGPU(), for fUsesSrcInput, for canComputeFastBounds is all set up to recurse on its inputs. Could you give that a try? On 2015/10/09 20:06:47, reed1 wrote: > On 2015/10/09 20:02:35, reed1 wrote: > > On 2015/10/09 19:54:13, Stephen White wrote: > > > > > > https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... > > > File src/core/SkLocalMatrixImageFilter.cpp (right): > > > > > > > > > https://codereview.chromium.org/1388823002/diff/1/src/core/SkLocalMatrixImage... > > > src/core/SkLocalMatrixImageFilter.cpp:30: bool > > > onIsColorFilterNode(SkColorFilter** /*filterPtr*/) const override; > > > On 2015/10/09 18:56:36, reed1 wrote: > > > > On 2015/10/05 21:00:41, Stephen White wrote: > > > > > I don't think this'll work, since you can't collapse a subsequent color > > > filter > > > > > into this one, due to the different local matrix. > > > > > > > > ColorFilters don't look at any matrix, so I think they are unaffected by > our > > > > local matrix. > > > > > > Consider the DAG: blur -> colorfilter -> localmatrixfilter. > > > > > > It looks like the current code would collapse this to blur -> colorfilter, > so > > > the localmatrix filter would no longer be applied to the blur params. > > > > I can't tell without testing that. Lets do try to create some tests/DAGs to > > exercise this. > > In your diagram, what do the arrows mean? > > Is the blur an input to the colorfitler, which is wrapped by localmatrixfilter?
I will try it.
I must be trying it wrong, because filterImage does not talk to its inputs, it relies on the subclasses to do that, so I don't get forwarded for free to the actual filter. I can upload my attempt, but since it doesn't work, it may not be useful. I'm happy to look at a different approach if you post one. That said, mine (so far) seems to be working, though I have yet to try your idea of the blur->colorfilter DAG test.
On 2015/10/09 21:17:17, reed1 wrote:
> I must be trying it wrong, because filterImage does not talk to its inputs, it
> relies on the subclasses to do that, so I don't get forwarded for free to the
> actual filter.
Does it not work if you override onFilterImage()?
Just create a new Context, as you did before, with the modified matrix,
and
return getInput(0)->filterImage(proxy, source, newCtx, ...)
does that not work?
> I can upload my attempt, but since it doesn't work, it may not be useful.
>
> I'm happy to look at a different approach if you post one. That said, mine (so
> far) seems to be working, though I have yet to try your idea of the
> blur->colorfilter DAG test.
On 2015/10/13 12:07:55, Stephen White wrote: > On 2015/10/09 21:17:17, reed1 wrote: > > I must be trying it wrong, because filterImage does not talk to its inputs, it > > relies on the subclasses to do that, so I don't get forwarded for free to the > > actual filter. > > Does it not work if you override onFilterImage()? > > Just create a new Context, as you did before, with the modified matrix, > and > > return getInput(0)->filterImage(proxy, source, newCtx, ...) > > does that not work? > > > I can upload my attempt, but since it doesn't work, it may not be useful. > > > > I'm happy to look at a different approach if you post one. That said, mine (so > > far) seems to be working, though I have yet to try your idea of the > > blur->colorfilter DAG test. Here's roughly what I had in mind: https://codereview.chromium.org/1392833005) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
