Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1810693003:
Switch SkTileImageFilter over to new onFilterImage interface (Closed)
|
Created:
4 years, 9 months ago by robertphillips Modified:
4 years, 8 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSwitch SkTileImageFilter over to new onFilterImage interface
This relies on: https://codereview.chromium.org/1816223002 (Update SkSpecialImage to be able to create tight SkImages and SkSurfaces)
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1810693003
Committed: https://skia.googlesource.com/skia/+/c14b978613c663b97ff0b08e0013dc72bb66d46d
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : Fix method naming #Patch Set 4 : update to ToT #Patch Set 5 : Clean up #
Total comments: 2
Patch Set 6 : Update to ToT and fix bug #Patch Set 7 : Fix other call site #Patch Set 8 : update to ToT #Patch Set 9 : Revert back to returning nullptr #Patch Set 10 : update to ToT #Patch Set 11 : update to ToT #Messages
Total messages: 25 (10 generated)
Description was changed from ========== Switch SkTileImageFilter over to new onFilterImage interface ========== to ========== Switch SkTileImageFilter over to new onFilterImage interface GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Switch SkTileImageFilter over to new onFilterImage interface GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkTileImageFilter over to new onFilterImage interface This relies on: https://codereview.chromium.org/1816223002 (Update SkSpecialImage to be able to create tight SkImages and SkSurfaces) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
robertphillips@google.com changed reviewers: + senorblanco@google.com
robertphillips@google.com changed reviewers: + bsalomon@google.com
lgtm, but Stephen would know better than I.
Stephen, could you also take a look at this CL if/when you get a chance?
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/1810693003/diff/80001/src/effects/SkTileImage... File src/effects/SkTileImageFilter.cpp (right): https://codereview.chromium.org/1810693003/diff/80001/src/effects/SkTileImage... src/effects/SkTileImageFilter.cpp:53: return nullptr; As in some other filters, I'm concerned that we're losing the distinction between "filter processing failed, abort" and "this node should output transparent black". It seems we're losing that distinction, which could prove problematic for downstream nodes which affect transparent black. Could we have some kind of canonical transparent black SkSpecialImage we could return in the latter case?
https://codereview.chromium.org/1810693003/diff/80001/src/effects/SkTileImage... File src/effects/SkTileImageFilter.cpp (right): https://codereview.chromium.org/1810693003/diff/80001/src/effects/SkTileImage... src/effects/SkTileImageFilter.cpp:53: return nullptr; On 2016/03/23 14:10:47, Stephen White wrote: > As in some other filters, I'm concerned that we're losing the distinction > between "filter processing failed, abort" and "this node should output > transparent black". It seems we're losing that distinction, which could prove > problematic for downstream nodes which affect transparent black. Could we have > some kind of canonical transparent black SkSpecialImage we could return in the > latter case? I have updated this site and the following one to replicate the prior behavior (i.e., returning the unmodified source).
On 2016/03/25 16:22:45, robertphillips wrote: > https://codereview.chromium.org/1810693003/diff/80001/src/effects/SkTileImage... > File src/effects/SkTileImageFilter.cpp (right): > > https://codereview.chromium.org/1810693003/diff/80001/src/effects/SkTileImage... > src/effects/SkTileImageFilter.cpp:53: return nullptr; > On 2016/03/23 14:10:47, Stephen White wrote: > > As in some other filters, I'm concerned that we're losing the distinction > > between "filter processing failed, abort" and "this node should output > > transparent black". It seems we're losing that distinction, which could prove > > problematic for downstream nodes which affect transparent black. Could we have > > some kind of canonical transparent black SkSpecialImage we could return in the > > latter case? > > I have updated this site and the following one to replicate the prior behavior > (i.e., returning the unmodified source). Returning the source seems really odd to me. Using the original source primitive might work in some cases, but it seems like you could conjure cases where it won't. E.g., imagine SkOffsetImageFilter -> SkTileImageFilter). The offset might bring the source primitive out of the tile's input rect, causing the intersection to fail here, but the next filter along could bring its pixels back into view. I'd feel much more comfortable with some kind of "empty SkSpecialImage", which has the same behaviour as an empty SkBitmap (zero width and height, but won't cause DAG processing to abort). Is there such a thing?
On 2016/03/29 18:15:36, Stephen White wrote: > On 2016/03/25 16:22:45, robertphillips wrote: > > > https://codereview.chromium.org/1810693003/diff/80001/src/effects/SkTileImage... > > File src/effects/SkTileImageFilter.cpp (right): > > > > > https://codereview.chromium.org/1810693003/diff/80001/src/effects/SkTileImage... > > src/effects/SkTileImageFilter.cpp:53: return nullptr; > > On 2016/03/23 14:10:47, Stephen White wrote: > > > As in some other filters, I'm concerned that we're losing the distinction > > > between "filter processing failed, abort" and "this node should output > > > transparent black". It seems we're losing that distinction, which could > prove > > > problematic for downstream nodes which affect transparent black. Could we > have > > > some kind of canonical transparent black SkSpecialImage we could return in > the > > > latter case? > > > > I have updated this site and the following one to replicate the prior behavior > > (i.e., returning the unmodified source). > > Returning the source seems really odd to me. Using the original source primitive > might work in some > cases, but it seems like you could conjure cases where it won't. E.g., imagine > SkOffsetImageFilter > -> SkTileImageFilter). The offset might bring the source primitive out of the > tile's input rect, causing > the intersection to fail here, but the next filter along could bring its pixels > back into view. > > I'd feel much more comfortable with some kind of "empty SkSpecialImage", which > has the same behaviour > as an empty SkBitmap (zero width and height, but won't cause DAG processing to > abort). Is there such a > thing? Thinking about it a bit more, I think a better approach might just be to treat nullptr as transparent black, and put the onus on the calling filters to treat it as such, since the number of filters which affect transparent black is small. Then we don't need another "transparent black" sentinel value. So e.g., SkColorFilterImageFilter w/affectsTransparentBlack() would check its input for nullptr, and do a fillPaint() in that case. This might grind a bit more in the low-memory case (where an allocation fails upstream and we get nullptr, an affects-transparent-black filter will try to do an allocation of its own and probably fail). But as long as it's robust, it should be ok.
> Thinking about it a bit more, I think a better approach might just be to treat > nullptr as transparent black, and put the onus on the calling filters to treat > it as such, since the number of filters which affect transparent black is small. > Then we don't need another "transparent black" sentinel value. > > So e.g., SkColorFilterImageFilter w/affectsTransparentBlack() would check its > input > for nullptr, and do a fillPaint() in that case. > > This might grind a bit more in the low-memory case (where an allocation fails > upstream and we get nullptr, an affects-transparent-black filter will try to do > an allocation of its own and probably fail). But as long as it's robust, it > should be ok. In this CL (and the other two) I'm just trying to match existing behavior. I agree that the current behavior (i.e., returning the source) is incorrect. I agree that having the filters that care about transparent black behave accordingly is the correct approach. Even in the error case (where some upstream node in the DAG fails) it is arguably better to have the colorfilterimagefilter perform its operation rather than drop the entire DAG.
On 2016/03/29 20:32:34, robertphillips wrote: > > Thinking about it a bit more, I think a better approach might just be to treat > > nullptr as transparent black, and put the onus on the calling filters to treat > > > it as such, since the number of filters which affect transparent black is > small. > > Then we don't need another "transparent black" sentinel value. > > > > So e.g., SkColorFilterImageFilter w/affectsTransparentBlack() would check its > > input > > for nullptr, and do a fillPaint() in that case. > > > > This might grind a bit more in the low-memory case (where an allocation fails > > upstream and we get nullptr, an affects-transparent-black filter will try to > do > > an allocation of its own and probably fail). But as long as it's robust, it > > should be ok. > > In this CL (and the other two) I'm just trying to match existing behavior. I > agree that the current behavior (i.e., returning the source) is incorrect. > > I agree that having the filters that care about transparent black behave > accordingly is the correct approach. Even in the error case (where some upstream > node in the DAG fails) it is arguably better to have the colorfilterimagefilter > perform its operation rather than drop the entire DAG. OK, so I've satisfied myself in https://codereview.chromium.org/1844593002/ that this approach is workable (using nullptr in all cases), although that patch can't land yet due to some Blink bugs that need to be fixed first. Given that, I think we should land your earlier version (with nullptr returns), as long as it passes all the Skia GMs and Blink layout tests.
This one should be good to go now that https://codereview.chromium.org/1844593002/ has landed. LGTM
I have verified locally that this CL shouldn't change the layout tests.
The CQ bit was checked by robertphillips@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/1810693003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1810693003/200001
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 robertphillips@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/1810693003/#ps200001 (title: "update to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1810693003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1810693003/200001
Message was sent while issue was closed.
Description was changed from ========== Switch SkTileImageFilter over to new onFilterImage interface This relies on: https://codereview.chromium.org/1816223002 (Update SkSpecialImage to be able to create tight SkImages and SkSurfaces) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkTileImageFilter over to new onFilterImage interface This relies on: https://codereview.chromium.org/1816223002 (Update SkSpecialImage to be able to create tight SkImages and SkSurfaces) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/c14b978613c663b97ff0b08e0013dc72bb66d46d ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://skia.googlesource.com/skia/+/c14b978613c663b97ff0b08e0013dc72bb66d46d |
