Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 3 months ago by f(malita) Modified:
5 years, 3 months ago Reviewers:
Stephen White, fs, Justin Novosad CC:
blink-reviews, krit, gyuyoung2, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, reed1, rwlbuis, Stephen Chennney Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionSwitch SVGFEImage from SkBitmapSource to SkImageSource
Use the new SkImage-based SkImageFilter subclass, and stop calling
Image::deprecatedBitmapForCurrentFrame().
BUG=449197
R=senorblanco@chromium.org,junov@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202450
Patch Set 1 #Patch Set 2 : explicit filter quality #
Total comments: 4
Patch Set 3 : avoid null SkImageFilters #Patch Set 4 : spec compliant + test #
Total comments: 2
Patch Set 5 : fixed-size <svg> #
Created: 5 years, 3 months ago
Messages
Total messages: 18 (3 generated)
Since we can't instantiate empty SkImages, and to avoid relying on SkBitmapSource + null SkBitmap, the new impl returns a null SkImageFilter ptr on error. Based on local testing, null SkImageFilters don't seem to cause problems downstream: they behave just as SkBitmapSource::Create(SkBitmap()) - i.e. they draw nothing.
PTAL. Thanks!
https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... File Source/core/svg/graphics/filters/SVGFEImage.cpp (right): https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... Source/core/svg/graphics/filters/SVGFEImage.cpp:195: return nullptr; I'm concerned about returning nullptr here, since that instead of rendering a black (or transparent black) frame, it means we'll be rendering the input primitive here (which is what a null input means in SkImageFilter-land). If there is a way of triggering this code path in a layout test (maybe a missing image/bad reference), could you add one that ensures that this code does the same thing as before? And if there isn't, could you turn this into an assert?
https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... File Source/core/svg/graphics/filters/SVGFEImage.cpp (right): https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... Source/core/svg/graphics/filters/SVGFEImage.cpp:195: return nullptr; On 2015/09/16 18:51:18, Stephen White wrote: > I'm concerned about returning nullptr here, since that instead of rendering a > black (or transparent black) frame, it means we'll be rendering the input > primitive here (which is what a null input means in SkImageFilter-land). I ran local tests forcing the return to always be nullptr, then compared with another run forcing SkBitmapSource::Create(SkBitmap()). The results/diffs, AFAICT, were identical. > > If there is a way of triggering this code path in a layout test (maybe a missing > image/bad reference), could you add one that ensures that this code does the > same thing as before? And if there isn't, could you turn this into an assert? Makes sense, let me play with that and get back to you.
https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... File Source/core/svg/graphics/filters/SVGFEImage.cpp (right): https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... Source/core/svg/graphics/filters/SVGFEImage.cpp:195: return nullptr; On 2015/09/16 18:57:36, f(malita) wrote: > On 2015/09/16 18:51:18, Stephen White wrote: > > If there is a way of triggering this code path in a layout test (maybe a > missing > > image/bad reference), could you add one that ensures that this code does the > > same thing as before? And if there isn't, could you turn this into an assert? > > Makes sense, let me play with that and get back to you. I can trigger this code by skipping the image URI completely. The result is different, but not necessarily in the sense we expected: * currently, we're returning SkBitmapSource::Create(SkBitmap()) => empty/null SkBitmap, width == height == 0 => SkBitmapSource with fSrcRect == fDstRect == 0 => SkBitmapSource::computeFastBounds shrinks the bounds to nothing => the filter chain is not drawing anything * the new code returns a null SkImageFilter, and somehow the filter chain ends up drawing transparent black E.g. <feImage out="out1"/> <feColorMatrix type="matrix" in="out1" values="1 0 0 0 0 0 1 0 0 0 0 0 1 0 0 0 0 0 0 .5"/> used to not draw anything, but with this CL draws gray. Not sure whether this case is covered in the spec, which behavior do you think makes more sense?
https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... File Source/core/svg/graphics/filters/SVGFEImage.cpp (right): https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... Source/core/svg/graphics/filters/SVGFEImage.cpp:195: return nullptr; Had an idea: maybe the safest thing to do is return a "null" SkPictureImageFilter instead. Updated, let me know what you think.
On 2015/09/16 at 21:14:25, fmalita wrote: > https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... > File Source/core/svg/graphics/filters/SVGFEImage.cpp (right): > > https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... > Source/core/svg/graphics/filters/SVGFEImage.cpp:195: return nullptr; > On 2015/09/16 18:57:36, f(malita) wrote: > > On 2015/09/16 18:51:18, Stephen White wrote: > > > If there is a way of triggering this code path in a layout test (maybe a > > missing > > > image/bad reference), could you add one that ensures that this code does the > > > same thing as before? And if there isn't, could you turn this into an assert? > > > > Makes sense, let me play with that and get back to you. > > I can trigger this code by skipping the image URI completely. The result is different, but not necessarily in the sense we expected: > > * currently, we're returning SkBitmapSource::Create(SkBitmap()) > => empty/null SkBitmap, width == height == 0 > => SkBitmapSource with fSrcRect == fDstRect == 0 > => SkBitmapSource::computeFastBounds shrinks the bounds to nothing > => the filter chain is not drawing anything > > * the new code returns a null SkImageFilter, and somehow the filter chain ends up drawing transparent black > > E.g. > > <feImage out="out1"/> > <feColorMatrix type="matrix" in="out1" values="1 0 0 0 0 > 0 1 0 0 0 > 0 0 1 0 0 > 0 0 0 0 .5"/> > > used to not draw anything, but with this CL draws gray. > > Not sure whether this case is covered in the spec, which behavior do you think makes more sense? "A href reference that is an empty image (zero width or zero height), that fails to download, is non-existent, or that cannot be displayed (e.g. because it is not in a supported image format) fills the filter primitive subregion with transparent black." https://drafts.fxtf.org/filters/#elementdef-feimage (fourth paragraph after the blue box)
On 2015/09/17 07:40:55, fs wrote: > On 2015/09/16 at 21:14:25, fmalita wrote: > > > https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... > > File Source/core/svg/graphics/filters/SVGFEImage.cpp (right): > > > > > https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... > > Source/core/svg/graphics/filters/SVGFEImage.cpp:195: return nullptr; > > On 2015/09/16 18:57:36, f(malita) wrote: > > > On 2015/09/16 18:51:18, Stephen White wrote: > > > > If there is a way of triggering this code path in a layout test (maybe a > > > missing > > > > image/bad reference), could you add one that ensures that this code does > the > > > > same thing as before? And if there isn't, could you turn this into an > assert? > > > > > > Makes sense, let me play with that and get back to you. > > > > I can trigger this code by skipping the image URI completely. The result is > different, but not necessarily in the sense we expected: > > > > * currently, we're returning SkBitmapSource::Create(SkBitmap()) > > => empty/null SkBitmap, width == height == 0 > > => SkBitmapSource with fSrcRect == fDstRect == 0 > > => SkBitmapSource::computeFastBounds shrinks the bounds to nothing > > => the filter chain is not drawing anything > > > > * the new code returns a null SkImageFilter, and somehow the filter chain ends > up drawing transparent black > > > > E.g. > > > > <feImage out="out1"/> > > <feColorMatrix type="matrix" in="out1" values="1 0 0 0 0 > > 0 1 0 0 0 > > 0 0 1 0 0 > > 0 0 0 0 .5"/> > > > > used to not draw anything, but with this CL draws gray. > > > > Not sure whether this case is covered in the spec, which behavior do you think > makes more sense? > > "A href reference that is an empty image (zero width or zero height), that fails > to download, is non-existent, or that cannot be displayed (e.g. because it is > not in a supported image format) fills the filter primitive subregion with > transparent black." > > https://drafts.fxtf.org/filters/#elementdef-feimage (fourth paragraph after the > blue box) Thanks for the pointer! So that means we're currently non-compliant in at least a couple of cases: * missing href -> the filter chain bounds collapse and we don't draw anything at all * non-existent image / download failure -> we draw the broken image icon Let me see if I can update the CL to cover some of these.
fmalita@chromium.org changed reviewers: + fs@opera.com
On 2015/09/17 09:55:04, f(malita) wrote: > Let me see if I can update the CL to cover some of these. Updated to handle missing/broken images per spec (using a null SkPictureImageFilter with a real cropRect), and added a test. PTAL
lgtm
On 2015/09/17 09:55:04, f(malita) wrote: > On 2015/09/17 07:40:55, fs wrote: > > On 2015/09/16 at 21:14:25, fmalita wrote: > > > > > > https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... > > > File Source/core/svg/graphics/filters/SVGFEImage.cpp (right): > > > > > > > > > https://codereview.chromium.org/1334173004/diff/20001/Source/core/svg/graphic... > > > Source/core/svg/graphics/filters/SVGFEImage.cpp:195: return nullptr; > > > On 2015/09/16 18:57:36, f(malita) wrote: > > > > On 2015/09/16 18:51:18, Stephen White wrote: > > > > > If there is a way of triggering this code path in a layout test (maybe a > > > > missing > > > > > image/bad reference), could you add one that ensures that this code does > > the > > > > > same thing as before? And if there isn't, could you turn this into an > > assert? > > > > > > > > Makes sense, let me play with that and get back to you. > > > > > > I can trigger this code by skipping the image URI completely. The result is > > different, but not necessarily in the sense we expected: > > > > > > * currently, we're returning SkBitmapSource::Create(SkBitmap()) > > > => empty/null SkBitmap, width == height == 0 > > > => SkBitmapSource with fSrcRect == fDstRect == 0 > > > => SkBitmapSource::computeFastBounds shrinks the bounds to nothing > > > => the filter chain is not drawing anything > > > > > > * the new code returns a null SkImageFilter, and somehow the filter chain > ends > > up drawing transparent black > > > > > > E.g. > > > > > > <feImage out="out1"/> > > > <feColorMatrix type="matrix" in="out1" values="1 0 0 0 0 > > > 0 1 0 0 0 > > > 0 0 1 0 0 > > > 0 0 0 0 .5"/> > > > > > > used to not draw anything, but with this CL draws gray. > > > > > > Not sure whether this case is covered in the spec, which behavior do you > think > > makes more sense? > > > > "A href reference that is an empty image (zero width or zero height), that > fails > > to download, is non-existent, or that cannot be displayed (e.g. because it is > > not in a supported image format) fills the filter primitive subregion with > > transparent black." > > > > https://drafts.fxtf.org/filters/#elementdef-feimage (fourth paragraph after > the > > blue box) > > Thanks for the pointer! So that means we're currently non-compliant in at least > a couple of cases: > > * missing href -> the filter chain bounds collapse and we don't draw anything at > all > * non-existent image / download failure -> we draw the broken image icon > > Let me see if I can update the CL to cover some of these. Good catch!
LGTM https://codereview.chromium.org/1334173004/diff/60001/LayoutTests/svg/filters... File LayoutTests/svg/filters/feimage-invalid-image.svg (right): https://codereview.chromium.org/1334173004/diff/60001/LayoutTests/svg/filters... LayoutTests/svg/filters/feimage-invalid-image.svg:2: <svg width="100%" height="100%" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> Nit: suggest we specify a width/height that explicitly encloses all the content, so we're not dependent on window size.
https://codereview.chromium.org/1334173004/diff/60001/LayoutTests/svg/filters... File LayoutTests/svg/filters/feimage-invalid-image.svg (right): https://codereview.chromium.org/1334173004/diff/60001/LayoutTests/svg/filters... LayoutTests/svg/filters/feimage-invalid-image.svg:2: <svg width="100%" height="100%" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> On 2015/09/17 13:58:14, Stephen White wrote: > Nit: suggest we specify a width/height that explicitly encloses all the content, > so we're not dependent on window size. Done.
The CQ bit was checked by fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org, fs@opera.com Link to the patchset: https://codereview.chromium.org/1334173004/#ps80001 (title: "fixed-size <svg>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334173004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334173004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=202450 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
