Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 7 months ago by fs Modified:
5 years, 7 months ago Reviewers:
Stephen White, reed1 CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionDon't fail SkMergeImageFilter if one of the inputs are empty
If one of the inputs to a SkMergeImageFilter was clipped away or
otherwise caused the filterImage(...) invocation for it to return
false, the entire effect would be "failed" and return false --
regardless of if it had produced a result or not.
Instead of returning false directly if filterImage(...) for a source
returned false, consider all the inputs, and then only return false if
all of them do.
BUG=chromium:489046
Committed: https://skia.googlesource.com/skia/+/d8b57717793f069db2ccd7c8174d0822bd7de6e1
Patch Set 1 #
Created: 5 years, 7 months ago
Messages
Total messages: 17 (5 generated)
The CQ bit was checked by fs@opera.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/1133523006/1
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-05-19 17:52 UTC
fs@opera.com changed reviewers: + senorblanco@chromium.org
fs@opera.com changed reviewers: + reed@google.com
code seems logical, but Stephen should comment on the feature-correctness of the new behavior.
The CQ bit was unchecked by reed@google.com
LGTM
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133523006/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/d8b57717793f069db2ccd7c8174d0822bd7de6e1
Message was sent while issue was closed.
On 2015/05/20 07:52:24, I haz the power (commit-bot) wrote: > Committed patchset #1 (id:1) as > https://skia.googlesource.com/skia/+/d8b57717793f069db2ccd7c8174d0822bd7de6e1 This is causing a layout test failure when rolling into Blink. See https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... I'm currently looking, but is the new result for svg/filters/feMerge.svg more correct?
Message was sent while issue was closed.
On 2015/05/20 14:07:54, bungeman1 wrote: > On 2015/05/20 07:52:24, I haz the power (commit-bot) wrote: > > Committed patchset #1 (id:1) as > > https://skia.googlesource.com/skia/+/d8b57717793f069db2ccd7c8174d0822bd7de6e1 > > This is causing a layout test failure when rolling into Blink. See > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > I'm currently looking, but is the new result for svg/filters/feMerge.svg more > correct? Yes, I think this is a fix for https://code.google.com/p/chromium/issues/detail?id=473191 (GPU raster was correct in that case, since it has wider tiles, so the new CPU result should now match GPU).
Message was sent while issue was closed.
On 2015/05/20 14:24:25, Stephen White wrote: > On 2015/05/20 14:07:54, bungeman1 wrote: > > On 2015/05/20 07:52:24, I haz the power (commit-bot) wrote: > > > Committed patchset #1 (id:1) as > > > > https://skia.googlesource.com/skia/+/d8b57717793f069db2ccd7c8174d0822bd7de6e1 > > > > This is causing a layout test failure when rolling into Blink. See > > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > > > I'm currently looking, but is the new result for svg/filters/feMerge.svg more > > correct? > > Yes, I think this is a fix for > https://code.google.com/p/chromium/issues/detail?id=473191 > > (GPU raster was correct in that case, since it has wider tiles, so the new CPU > result should now match GPU). Yes, that appears to be exactly the same as the bug I was looking at (489046)... Will dupe or something.
Message was sent while issue was closed.
On 2015/05/20 14:29:18, fs wrote: > On 2015/05/20 14:24:25, Stephen White wrote: > > On 2015/05/20 14:07:54, bungeman1 wrote: > > > On 2015/05/20 07:52:24, I haz the power (commit-bot) wrote: > > > > Committed patchset #1 (id:1) as > > > > > > https://skia.googlesource.com/skia/+/d8b57717793f069db2ccd7c8174d0822bd7de6e1 > > > > > > This is causing a layout test failure when rolling into Blink. See > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > > > > > I'm currently looking, but is the new result for svg/filters/feMerge.svg > more > > > correct? > > > > Yes, I think this is a fix for > > https://code.google.com/p/chromium/issues/detail?id=473191 > > > > (GPU raster was correct in that case, since it has wider tiles, so the new CPU > > result should now match GPU). > > Yes, that appears to be exactly the same as the bug I was looking at (489046)... > Will dupe or something. Yes, I see now that this restores the expectations prior to https://codereview.chromium.org/769423008 . I will rebaseline this one for the Skia roll.
Message was sent while issue was closed.
On 2015/05/20 14:30:50, bungeman1 wrote: > On 2015/05/20 14:29:18, fs wrote: > > On 2015/05/20 14:24:25, Stephen White wrote: > > > On 2015/05/20 14:07:54, bungeman1 wrote: > > > > On 2015/05/20 07:52:24, I haz the power (commit-bot) wrote: > > > > > Committed patchset #1 (id:1) as > > > > > > > > > https://skia.googlesource.com/skia/+/d8b57717793f069db2ccd7c8174d0822bd7de6e1 > > > > > > > > This is causing a layout test failure when rolling into Blink. See > > > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > > > > > > > I'm currently looking, but is the new result for svg/filters/feMerge.svg > > more > > > > correct? > > > > > > Yes, I think this is a fix for > > > https://code.google.com/p/chromium/issues/detail?id=473191 > > > > > > (GPU raster was correct in that case, since it has wider tiles, so the new > CPU > > > result should now match GPU). > > > > Yes, that appears to be exactly the same as the bug I was looking at > (489046)... > > Will dupe or something. > > Yes, I see now that this restores the expectations prior to > https://codereview.chromium.org/769423008 . > I will rebaseline this one for the Skia roll. Great, thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
