Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24157005:
Moving 4 SkImageFilter derived classes from blink to skia (Closed)
|
Created:
7 years, 3 months ago by sugoi1 Modified:
7 years, 2 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionMoving 4 SkImageFilter derived classes from blink to skia
There were 4 classes in blink that derived from SkImageFilter :
- TileImageFilter -> SkTileImageFilter
- OffsetImageFilter -> SkOffsetImageFilter (already existed)
- FloodImageFilter -> SkFloodImageFilter
- CompositeImageFilter -> SkCompositeImageFilter
All functions were copied as is, without modification (except for warnings fixes), except for the offset filter, which was merged into the existing SkOffsetImageFilter class, as a special case when a crop rect is provided. Since the names won't clash with the names in blink, it should be easy to integrate them in blink later and fix issues, if needed.
BUG=
Committed: http://code.google.com/p/skia/source/detail?r=11475
Patch Set 1 #Patch Set 2 : Separate offset filter #Patch Set 3 : Merged offset filters together #
Total comments: 5
Patch Set 4 : Updated copyright #Patch Set 5 : Added new factory functions for Filter Effect image filters #Patch Set 6 : Simple rename to SkCSSImageFilters #Patch Set 7 : Merged new filters into existing ones #
Total comments: 10
Patch Set 8 : Added gm tests #
Total comments: 4
Patch Set 9 : Added missing SK_API #Messages
Total messages: 36 (0 generated)
I created separate filters from the skia filters for each case. We can rewrite them into more efficient filters later, but I just want to move these filters without any changes first, to avoid regressions.
I merged both offset filters together. https://codereview.chromium.org/24157005/diff/9001/src/effects/SkOffsetImageF... File src/effects/SkOffsetImageFilter.cpp (right): https://codereview.chromium.org/24157005/diff/9001/src/effects/SkOffsetImageF... src/effects/SkOffsetImageFilter.cpp:34: SkIPoint srcOffset = SkIPoint::Make(0, 0); @senorblanco : Was there a reason here why we aren't using "loc" directly and provide a (0,0) point instead ?
LGTM https://codereview.chromium.org/24157005/diff/9001/include/effects/SkTileImag... File include/effects/SkTileImageFilter.h (right): https://codereview.chromium.org/24157005/diff/9001/include/effects/SkTileImag... include/effects/SkTileImageFilter.h:14: typedef SkImageFilter INHERITED; Nit: you could also de-inline the constructor, in order to avoid putting this typedef at the top of the file. https://codereview.chromium.org/24157005/diff/9001/src/effects/SkOffsetImageF... File src/effects/SkOffsetImageFilter.cpp (right): https://codereview.chromium.org/24157005/diff/9001/src/effects/SkOffsetImageF... src/effects/SkOffsetImageFilter.cpp:34: SkIPoint srcOffset = SkIPoint::Make(0, 0); On 2013/09/23 18:37:30, sugoi1 wrote: > @senorblanco : Was there a reason here why we aren't using "loc" directly and > provide a (0,0) point instead ? Since this path draws its input into a buffer, it's applying the upstream offset at draw time. We don't want to return it via the outparam, since that would cause it to be applied twice. Note that other compositing filters (e.g., SkXfermodeImageFilter, SkMergeImageFilter) do the same.
https://codereview.chromium.org/24157005/diff/9001/include/effects/SkTileImag... File include/effects/SkTileImageFilter.h (right): https://codereview.chromium.org/24157005/diff/9001/include/effects/SkTileImag... include/effects/SkTileImageFilter.h:14: typedef SkImageFilter INHERITED; On 2013/09/24 01:13:19, Stephen White wrote: > Nit: you could also de-inline the constructor, in order to avoid putting this typedef at the top of the file. I'll probably need this for other reasons later on, so I'd rather make sure INHERITED is usable in the header file too. https://codereview.chromium.org/24157005/diff/9001/src/effects/SkOffsetImageF... File src/effects/SkOffsetImageFilter.cpp (right): https://codereview.chromium.org/24157005/diff/9001/src/effects/SkOffsetImageF... src/effects/SkOffsetImageFilter.cpp:34: SkIPoint srcOffset = SkIPoint::Make(0, 0); On 2013/09/24 01:13:19, Stephen White wrote: > On 2013/09/23 18:37:30, sugoi1 wrote: > > @senorblanco : Was there a reason here why we aren't using "loc" directly and > > provide a (0,0) point instead ? > > Since this path draws its input into a buffer, it's applying the upstream offset > at draw time. We don't want to return it via the outparam, since that would > cause it to be applied twice. Note that other compositing filters (e.g., > SkXfermodeImageFilter, SkMergeImageFilter) do the same. Thanks for the explanation, I wanted to make sure this was desired and not something on a TODO list :)
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/24157005/9001
Presubmit check for 24157005-9001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com') Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
Added bsalomon@ for approval (I will wait until the tree is green again, past M31, before I commit this).
On 2013/09/24 11:41:45, sugoi1 wrote: > Added bsalomon@ for approval (I will wait until the tree is green again, past > M31, before I commit this). rubber stamp lgtm w/ one exception. I think the copyright notices are wrong (Android instead of Google). Also, curious if there is a plan to gpu them.
On 2013/09/24 13:19:04, bsalomon wrote: > On 2013/09/24 11:41:45, sugoi1 wrote: > > Added bsalomon@ for approval (I will wait until the tree is green again, past > > M31, before I commit this). > > rubber stamp lgtm w/ one exception. I think the copyright notices are wrong > (Android instead of Google). Also, curious if there is a plan to gpu them. I updated the copyright notices. We should definitely add a gpu path to all these filters. I'll try to put that in my next quarter OKRs, if possible.
On 2013/09/24 14:00:27, sugoi1 wrote: > On 2013/09/24 13:19:04, bsalomon wrote: > > On 2013/09/24 11:41:45, sugoi1 wrote: > > > Added bsalomon@ for approval (I will wait until the tree is green again, > past > > > M31, before I commit this). > > > > rubber stamp lgtm w/ one exception. I think the copyright notices are wrong > > (Android instead of Google). Also, curious if there is a plan to gpu them. > > I updated the copyright notices. > > We should definitely add a gpu path to all these filters. I'll try to put that > in my next quarter OKRs, if possible. Since these filters all use generic Skia code, they work out of the box on the GPU. There's no need for a separate path.
On 2013/09/24 14:33:07, Stephen White wrote: > On 2013/09/24 14:00:27, sugoi1 wrote: > > On 2013/09/24 13:19:04, bsalomon wrote: > > > On 2013/09/24 11:41:45, sugoi1 wrote: > > > > Added bsalomon@ for approval (I will wait until the tree is green again, > > past > > > > M31, before I commit this). > > > > > > rubber stamp lgtm w/ one exception. I think the copyright notices are wrong > > > (Android instead of Google). Also, curious if there is a plan to gpu them. > > > > I updated the copyright notices. > > > > We should definitely add a gpu path to all these filters. I'll try to put that > > in my next quarter OKRs, if possible. > > Since these filters all use generic Skia code, they work out of the box on the > GPU. There's no need for a separate path. cool!
for scale and defensiveness (and possibly clarity), can we bucket a block of these CSS/SVG-specific filters into just a set of factories? (ala SkBlurMaskFilter). It also makes it easier to, under the hood, look for special-cases / code-folding and return secret subclasses w/o the client having to know.
Lets add some meaningful (i.e. one could write a test case from it) dox for each new filtertype.
On 2013/09/24 15:10:23, reed1 wrote: > for scale and defensiveness (and possibly clarity), can we bucket a block of > these CSS/SVG-specific filters into just a set of factories? (ala > SkBlurMaskFilter). It also makes it easier to, under the hood, look for > special-cases / code-folding and return secret subclasses w/o the client having > to know. Ok, I added SkFilterEffect so that we have factory functions for filters related to Filter Effects in blink. Hopefully, this is what you had in mind.
indeed, anything in that vein gives us more protection/options, and can encourage us to add lots of dox (e.g. urls pointing to specs as applicable, etc.)
SkCSSImageFilters?
do we already have unitests or gms (or benches) for these?
As I mentioned to Alexis privately, I think Flood can be implemented with an SkColorImageFilter (w/SrcIn Mode ColorFilter), and Composite should be doable with an SkXfermodeImageFilter. I'd actually prefer that we try that in Blink and see what breaks (layout test-wise), and keep Skia to the minimal set. Tile is really the only one that's not possible to express as other filters. However, it seems generic enough that it might be useful outside of SVG/CSS use.
On 2013/09/24 17:16:03, reed1 wrote: > do we already have unitests or gms (or benches) for these? I'll rename the class to SkCSSImageFilters. No tests on the skia side yet. My idea was to test these using the LayoutTests in blink first, and to add tests in Skia once I know everything is ok on the blink side to avoid rebaselining/bench regressions, if that's ok.
On 2013/09/24 17:21:09, Stephen White wrote: > As I mentioned to Alexis privately, I think Flood can be implemented with an > SkColorImageFilter (w/SrcIn Mode ColorFilter), and Composite should be doable > with an SkXfermodeImageFilter. I'd actually prefer that we try that in Blink and > see what breaks (layout test-wise), and keep Skia to the minimal set. > > Tile is really the only one that's not possible to express as other filters. > However, it seems generic enough that it might be useful outside of SVG/CSS use. Ok, the code is ready here, but I'll go test blink first to see if I can eliminate some of these filters before I commit this.
Would be really nice to have *some* sort of code exercising these when they land. gm's are pretty easy to create...
On 2013/09/24 17:24:03, sugoi1 wrote: > On 2013/09/24 17:16:03, reed1 wrote: > > do we already have unitests or gms (or benches) for these? > > I'll rename the class to SkCSSImageFilters. > > No tests on the skia side yet. My idea was to test these using the LayoutTests > in blink first, and to add tests in Skia once I know everything is ok on the > blink side to avoid rebaselining/bench regressions, if that's ok. Any code that's landed in Skia should really have tests in Skia, rather than depending on Blink for testing. That's the principle I've been using for the existing filters, anyway.
On 2013/09/24 17:34:13, Stephen White wrote: > On 2013/09/24 17:24:03, sugoi1 wrote: > > On 2013/09/24 17:16:03, reed1 wrote: > > > do we already have unitests or gms (or benches) for these? > > > > I'll rename the class to SkCSSImageFilters. > > > > No tests on the skia side yet. My idea was to test these using the LayoutTests > > in blink first, and to add tests in Skia once I know everything is ok on the > > blink side to avoid rebaselining/bench regressions, if that's ok. > > Any code that's landed in Skia should really have tests in Skia, rather than > depending on Blink for testing. That's the principle I've been using for the > existing filters, anyway. All right, I'll add tests right away for all the filters that will still be required after the blink tests.
Ok, so here's the current state : - Composite has been merged into SkXfermodeImageFilter. - Offset if still merged into SkOffsetImageFilter (no change) - Flood was removed, as it will be represented by SkColorFilter/SkColorFilterImageFilter in blink. - Tile was added as a new filter, without any wrapper since it's the only entirely new filter and since it may be useful by itself in Skia, as senorblanco@ pointed out. I'll add tests/gms to this cl if this is ok for you. https://codereview.chromium.org/24157005/diff/26001/include/effects/SkXfermod... File include/effects/SkXfermodeImageFilter.h (right): https://codereview.chromium.org/24157005/diff/26001/include/effects/SkXfermod... include/effects/SkXfermodeImageFilter.h:37: virtual bool canFilterImageGPU() const SK_OVERRIDE { return cropRect().isLargest(); } Are crop rects supported by default in GPU ? I conservatively assumed that it wasn't the case, but I can remove this change if it is.
sg, lets proceed w/ some tests. https://codereview.chromium.org/24157005/diff/26001/include/effects/SkTileIma... File include/effects/SkTileImageFilter.h (right): https://codereview.chromium.org/24157005/diff/26001/include/effects/SkTileIma... include/effects/SkTileImageFilter.h:17: SkTileImageFilter(const SkRect& srcRect, const SkRect& dstRect, SkImageFilter* input) Add some dox for what it does, and the two rects?
Just nits, couldn't help myself. https://codereview.chromium.org/24157005/diff/26001/include/effects/SkOffsetI... File include/effects/SkOffsetImageFilter.h (right): https://codereview.chromium.org/24157005/diff/26001/include/effects/SkOffsetI... include/effects/SkOffsetImageFilter.h:19: const SkIRect* cropRect = 0); = 0 -> = NULL? https://codereview.chromium.org/24157005/diff/26001/include/effects/SkXfermod... File include/effects/SkXfermodeImageFilter.h (right): https://codereview.chromium.org/24157005/diff/26001/include/effects/SkXfermod... include/effects/SkXfermodeImageFilter.h:25: SkImageFilter* foreground = NULL, const SkIRect* cropRect = 0); = 0 -> = NULL?
https://codereview.chromium.org/24157005/diff/26001/include/effects/SkXfermod... File include/effects/SkXfermodeImageFilter.h (right): https://codereview.chromium.org/24157005/diff/26001/include/effects/SkXfermod... include/effects/SkXfermodeImageFilter.h:37: virtual bool canFilterImageGPU() const SK_OVERRIDE { return cropRect().isLargest(); } On 2013/09/24 20:08:17, sugoi1 wrote: > Are crop rects supported by default in GPU ? I conservatively assumed that it > wasn't the case, but I can remove this change if it is. No, they're not. It looks like you've implemented them for the raster path, which is great. You should also add some tests for this. If you like, you can just copy xfermodeimagefilter GM and add a crop rect to each sample. You could also do a reduced number of modes, just to handle the two different code paths in the GPU code (is expressible as an effect, or a fixed-func blend mode). The rest of the code should be adequately covered by the existing GM. The GPU path can be left as-is for now, with the canFilterImageGPU() check as you wrote it. Please log a bug, though, since there is some opportunity for optimization: the GPU path applies the effect-style blending modes in a single pass, which the generic onFilterImage() path does not. Also check for any minor pixel diffs to layout tests in Chrome, and warn Rob if so. https://codereview.chromium.org/24157005/diff/26001/src/effects/SkXfermodeIma... File src/effects/SkXfermodeImageFilter.cpp (right): https://codereview.chromium.org/24157005/diff/26001/src/effects/SkXfermodeIma... src/effects/SkXfermodeImageFilter.cpp:64: if (cropRect().isLargest()) { I think the code you've added below should work for both crop-rect and non-crop-rect cases (tests should tell you). You should be able to delete the first stanza of code here.
I added 2 new gms and modified 1 : - offsetimagefilter.cpp : 4 tests for offset with crop - tileimagefilter.cpp : 4 tests for tiling with crop - xfermodeimagefilter : modified to add 3 crop tests I didn't feel the need to add more than this, but I can add more if you feel that the coverage is too thin. https://codereview.chromium.org/24157005/diff/26001/include/effects/SkOffsetI... File include/effects/SkOffsetImageFilter.h (right): https://codereview.chromium.org/24157005/diff/26001/include/effects/SkOffsetI... include/effects/SkOffsetImageFilter.h:19: const SkIRect* cropRect = 0); On 2013/09/24 20:30:34, mtklein wrote: > = 0 -> = NULL? Done. https://codereview.chromium.org/24157005/diff/26001/include/effects/SkTileIma... File include/effects/SkTileImageFilter.h (right): https://codereview.chromium.org/24157005/diff/26001/include/effects/SkTileIma... include/effects/SkTileImageFilter.h:17: SkTileImageFilter(const SkRect& srcRect, const SkRect& dstRect, SkImageFilter* input) On 2013/09/24 20:11:57, reed1 wrote: > Add some dox for what it does, and the two rects? Done. https://codereview.chromium.org/24157005/diff/26001/include/effects/SkXfermod... File include/effects/SkXfermodeImageFilter.h (right): https://codereview.chromium.org/24157005/diff/26001/include/effects/SkXfermod... include/effects/SkXfermodeImageFilter.h:25: SkImageFilter* foreground = NULL, const SkIRect* cropRect = 0); On 2013/09/24 20:30:34, mtklein wrote: > = 0 -> = NULL? Done. https://codereview.chromium.org/24157005/diff/26001/src/effects/SkXfermodeIma... File src/effects/SkXfermodeImageFilter.cpp (right): https://codereview.chromium.org/24157005/diff/26001/src/effects/SkXfermodeIma... src/effects/SkXfermodeImageFilter.cpp:64: if (cropRect().isLargest()) { On 2013/09/24 23:14:13, Stephen White wrote: > I think the code you've added below should work for both crop-rect and > non-crop-rect cases (tests should tell you). You should be able to delete the > first stanza of code here. Done.
https://codereview.chromium.org/24157005/diff/53001/gm/xfermodeimagefilter.cpp File gm/xfermodeimagefilter.cpp (right): https://codereview.chromium.org/24157005/diff/53001/gm/xfermodeimagefilter.cp... gm/xfermodeimagefilter.cpp:197: // Test cropping Do you need to expand the HEIGHT to accommodate these new test cases? I.e., are they included in the PNG? https://codereview.chromium.org/24157005/diff/53001/src/effects/SkOffsetImage... File src/effects/SkOffsetImageFilter.cpp (right): https://codereview.chromium.org/24157005/diff/53001/src/effects/SkOffsetImage... src/effects/SkOffsetImageFilter.cpp:22: if (cropRect().isLargest()) { So is this stanza still needed?
https://codereview.chromium.org/24157005/diff/53001/src/effects/SkOffsetImage... File src/effects/SkOffsetImageFilter.cpp (right): https://codereview.chromium.org/24157005/diff/53001/src/effects/SkOffsetImage... src/effects/SkOffsetImageFilter.cpp:22: if (cropRect().isLargest()) { On 2013/09/25 18:11:50, Stephen White wrote: > So is this stanza still needed? Er, ignore that. Not enough coffee...
https://codereview.chromium.org/24157005/diff/53001/gm/xfermodeimagefilter.cpp File gm/xfermodeimagefilter.cpp (right): https://codereview.chromium.org/24157005/diff/53001/gm/xfermodeimagefilter.cp... gm/xfermodeimagefilter.cpp:197: // Test cropping On 2013/09/25 18:11:50, Stephen White wrote: > Do you need to expand the HEIGHT to accommodate these new test cases? I.e., are > they included in the PNG? No, the extra 3 bring the count to 36, so it's a 6x6 grid, it has the same size as before, but the last 3 spots are now taken by the crop tests.
On 2013/09/25 18:21:26, sugoi1 wrote: > https://codereview.chromium.org/24157005/diff/53001/gm/xfermodeimagefilter.cpp > File gm/xfermodeimagefilter.cpp (right): > > https://codereview.chromium.org/24157005/diff/53001/gm/xfermodeimagefilter.cp... > gm/xfermodeimagefilter.cpp:197: // Test cropping > On 2013/09/25 18:11:50, Stephen White wrote: > > Do you need to expand the HEIGHT to accommodate these new test cases? I.e., > are > > they included in the PNG? > > No, the extra 3 bring the count to 36, so it's a 6x6 grid, it has the same size > as before, but the last 3 spots are now taken by the crop tests. OK, great. LGTM on the new version.
lgtm
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/24157005/53001
Message was sent while issue was closed.
Change committed as 11475 |
