Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1813373002:
Make SkComposeImageFilter::onFilterImage filter the bounds given to the inner filter. (Closed)
DescriptionMake SkComposeImageFilter::onFilterImage filter the bounds given to the inner filter.
Previously, the bounds requested by the caller would be passed to both the
inner and outer filter, but since the outer filter may move pixels, this is not
necessarily sufficient to supply the pixels required by the outer filter to fill
the bounds.
Unit test included.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1813373002
Committed: https://skia.googlesource.com/skia/+/17a652017a0243c57c954662e08a7976b9990fee
Patch Set 1 #
Total comments: 4
Patch Set 2 : reed review comments #
Total comments: 4
Patch Set 3 : skip innerClipBounds initialization per senorblanco #
Messages
Total messages: 19 (7 generated)
Description was changed from ========== Make SkComposeImageFilter::onFilterImage filter the bounds given to the inner filter. Previously, the bounds requested by the caller would be passed to both the inner and outer filter, but since the outer filter may move pixels, this is not necessarily sufficient to supply the pixels required by the outer filter to fill the bounds. Unit test included. ========== to ========== Make SkComposeImageFilter::onFilterImage filter the bounds given to the inner filter. Previously, the bounds requested by the caller would be passed to both the inner and outer filter, but since the outer filter may move pixels, this is not necessarily sufficient to supply the pixels required by the outer filter to fill the bounds. Unit test included. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Make SkComposeImageFilter::onFilterImage filter the bounds given to the inner filter. Previously, the bounds requested by the caller would be passed to both the inner and outer filter, but since the outer filter may move pixels, this is not necessarily sufficient to supply the pixels required by the outer filter to fill the bounds. Unit test included. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make SkComposeImageFilter::onFilterImage filter the bounds given to the inner filter. Previously, the bounds requested by the caller would be passed to both the inner and outer filter, but since the outer filter may move pixels, this is not necessarily sufficient to supply the pixels required by the outer filter to fill the bounds. Unit test included. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
jbroman@chromium.org changed reviewers: + robertphillips@google.com, senorblanco@chromium.org
Ran into this while working on Blink.
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/1813373002/diff/1/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (right): https://codereview.chromium.org/1813373002/diff/1/tests/ImageFilterTest.cpp#n... tests/ImageFilterTest.cpp:1375: // The bounds passed to the inner filter must be filtered by the outer Some version of this comment might be useful in the SkComposeImageFilter class as well, to clarify why the method does what it does. https://codereview.chromium.org/1813373002/diff/1/tests/ImageFilterTest.cpp#n... tests/ImageFilterTest.cpp:1382: recorder.beginRecording(SkRect::MakeWH(200, 100)); btw -- beginRecording returns the canvas, so you don't have to call getRecordingCanvas... canvas = recorder.beginRecording(...)
https://codereview.chromium.org/1813373002/diff/1/tests/ImageFilterTest.cpp File tests/ImageFilterTest.cpp (right): https://codereview.chromium.org/1813373002/diff/1/tests/ImageFilterTest.cpp#n... tests/ImageFilterTest.cpp:1375: // The bounds passed to the inner filter must be filtered by the outer On 2016/03/19 at 14:28:49, reed1 wrote: > Some version of this comment might be useful in the SkComposeImageFilter class as well, to clarify why the method does what it does. Copied the first three lines to SkComposeImageFilter::onFilterImage. https://codereview.chromium.org/1813373002/diff/1/tests/ImageFilterTest.cpp#n... tests/ImageFilterTest.cpp:1382: recorder.beginRecording(SkRect::MakeWH(200, 100)); On 2016/03/19 at 14:28:49, reed1 wrote: > btw -- beginRecording returns the canvas, so you don't have to call getRecordingCanvas... > > canvas = recorder.beginRecording(...) Done.
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1813373002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813373002/20001
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 2016-03-21 20:37 UTC
Good catch! https://codereview.chromium.org/1813373002/diff/20001/src/effects/SkComposeIm... File src/effects/SkComposeImageFilter.cpp (right): https://codereview.chromium.org/1813373002/diff/20001/src/effects/SkComposeIm... src/effects/SkComposeImageFilter.cpp:29: SkIRect innerClipBounds = ctx.clipBounds(); I wouldn't bother initializing this, since the call to filterBounds() will overwrite it anyway.
https://codereview.chromium.org/1813373002/diff/20001/src/effects/SkComposeIm... File src/effects/SkComposeImageFilter.cpp (right): https://codereview.chromium.org/1813373002/diff/20001/src/effects/SkComposeIm... src/effects/SkComposeImageFilter.cpp:29: SkIRect innerClipBounds = ctx.clipBounds(); On 2016/03/21 at 15:09:18, Stephen White wrote: > I wouldn't bother initializing this, since the call to filterBounds() will overwrite it anyway. What I'm unsure about is the case where filterBounds() returns false; AFAICT it doesn't initialize the value in that case (though other call sites don't seem to handle this, so I'm a little confused). Should we allow an uninitialized SkIRect (doesn't even seem to be zero-initialized) to be used in that case, or should I just not worry about it?
https://codereview.chromium.org/1813373002/diff/20001/src/effects/SkComposeIm... File src/effects/SkComposeImageFilter.cpp (right): https://codereview.chromium.org/1813373002/diff/20001/src/effects/SkComposeIm... src/effects/SkComposeImageFilter.cpp:29: SkIRect innerClipBounds = ctx.clipBounds(); On 2016/03/21 15:13:11, jbroman wrote: > On 2016/03/21 at 15:09:18, Stephen White wrote: > > I wouldn't bother initializing this, since the call to filterBounds() will > overwrite it anyway. > > What I'm unsure about is the case where filterBounds() returns false; AFAICT it > doesn't initialize the value in that case (though other call sites don't seem to > handle this, so I'm a little confused). > > Should we allow an uninitialized SkIRect (doesn't even seem to be > zero-initialized) to be used in that case, or should I just not worry about it? Yeah, it's kind of a wart on the API: I don't think it's actually possible for filterBounds() to return false. It only can if onFilterBounds() returns false, which can only do so if filterBounds() does (recursively). It used to do so for an SkMatrixImageFilter with a non-invertible matrix, but we just return the source bounds in that case now. It's an edge case I'm not too worried about. We should probably remove the boolean at some point.
https://codereview.chromium.org/1813373002/diff/20001/src/effects/SkComposeIm... File src/effects/SkComposeImageFilter.cpp (right): https://codereview.chromium.org/1813373002/diff/20001/src/effects/SkComposeIm... src/effects/SkComposeImageFilter.cpp:29: SkIRect innerClipBounds = ctx.clipBounds(); On 2016/03/21 at 15:18:58, Stephen White wrote: > On 2016/03/21 15:13:11, jbroman wrote: > > On 2016/03/21 at 15:09:18, Stephen White wrote: > > > I wouldn't bother initializing this, since the call to filterBounds() will > > overwrite it anyway. > > > > What I'm unsure about is the case where filterBounds() returns false; AFAICT it > > doesn't initialize the value in that case (though other call sites don't seem to > > handle this, so I'm a little confused). > > > > Should we allow an uninitialized SkIRect (doesn't even seem to be > > zero-initialized) to be used in that case, or should I just not worry about it? > > Yeah, it's kind of a wart on the API: I don't think it's actually possible for filterBounds() to return false. It only can if onFilterBounds() returns false, which can only do so if filterBounds() does (recursively). It used to do so for an SkMatrixImageFilter with a non-invertible matrix, but we just return the source bounds in that case now. It's an edge case I'm not too worried about. We should probably remove the boolean at some point. OK, done.
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1813373002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813373002/40001
LGTM
Message was sent while issue was closed.
Description was changed from ========== Make SkComposeImageFilter::onFilterImage filter the bounds given to the inner filter. Previously, the bounds requested by the caller would be passed to both the inner and outer filter, but since the outer filter may move pixels, this is not necessarily sufficient to supply the pixels required by the outer filter to fill the bounds. Unit test included. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make SkComposeImageFilter::onFilterImage filter the bounds given to the inner filter. Previously, the bounds requested by the caller would be passed to both the inner and outer filter, but since the outer filter may move pixels, this is not necessarily sufficient to supply the pixels required by the outer filter to fill the bounds. Unit test included. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/17a652017a0243c57c954662e08a7976b9990fee ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/17a652017a0243c57c954662e08a7976b9990fee |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
