Bug 49799 – createImageBuffer fail should be handled in the same way as other fails

• Component: SVG
• Status: RESOLVED FIXED

[Zoltan Herczeg]

When the width of the absoluteDrawingRegion is less than 0.5 (and rounded to 0, but not empty), only a plain "return false" is executed.

[Zoltan Herczeg]

Created attachment 74375 [details]
patch

[Nikolas Zimmermann]

Comment on attachment 74375 [details]
patch

Patch looks great, one nitpick. I don't like the name of the test, filter-x.svg. How about 'filter-small-x-should-still-render.svg'? Or something alike...

[Dirk Schulze]

Comment on attachment 74375 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74375&action=review

> WebCore/ChangeLog:9
> +        When createImageBuffer fails (i.e the image width or height is less than
> +        0.5, which is rounded to 0) it just returs with false. Since the m_filter

How can this happen? enclosingIntRect should cause a ImageBuffer of 1x1 for a width and height of 0.5x0.5?

[Dirk Schulze]

Hmm, saw the rounding suff in SVGImageBufferTools, Niko shouldn't we use enclosingIntSize here?

[Zoltan Herczeg]

Landed in http://trac.webkit.org/changeset/72389
Closing bug.

[Zoltan Herczeg]

Dirk, see the example. absoluteDrawingRegion.width < 0.5 in this case

[Dirk Schulze]

Comment on attachment 74375 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74375&action=review

Maybe I  misunderstood the intention of the after reading the text in the ChangeLog.

> LayoutTests/svg/filters/filter-x.svg:7
> +<filter id="filt" filterUnits="userSpaceOnUse" x="9.51" y="5" width="10" height="10">
> +    <feFlood flood-color="red" />
> +</filter>
> +</defs>
> +<rect x="0" y="0" width="10" height="10" filter="url(#filt)"/>

Why does it test the case, where width or height are < 0.5 like in the bug description? Just x has a fractional part. How is it related to the described problem?

Also, the reference image is red. We prefer green rects of the size 100x100. (The size is not a dogma)

> WebCore/rendering/RenderSVGResourceFilter.cpp:235
> +    if (!SVGImageBufferTools::createImageBuffer(absoluteDrawingRegion, absoluteDrawingRegion, sourceGraphic, ColorSpaceLinearRGB)) {
> +        ASSERT(!m_filter.contains(object));
> +        filterData->savedContext = context;
> +        m_filter.set(object, filterData.leakPtr());
>          return false;
> +    }

I agree, that the failure handling wasn't correct here. Just a bit confused why you were talking about width or height < 0.5 and how the test checks the correct handling. Since the test should return true here?!? :-P

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 74375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74375&action=review
> 
> Maybe I  misunderstood the intention of the after reading the text in the ChangeLog.
> 
> > LayoutTests/svg/filters/filter-x.svg:7
> > +<filter id="filt" filterUnits="userSpaceOnUse" x="9.51" y="5" width="10" height="10">
> > +    <feFlood flood-color="red" />
> > +</filter>
> > +</defs>
> > +<rect x="0" y="0" width="10" height="10" filter="url(#filt)"/>
> 
> Why does it test the case, where width or height are < 0.5 like in the bug description? Just x has a fractional part. How is it related to the described problem?
width - x = 0.49 px.

> 
> Also, the reference image is red. We prefer green rects of the size 100x100. (The size is not a dogma)
True, we should have changed it to green.

> 
> > WebCore/rendering/RenderSVGResourceFilter.cpp:235
> > +    if (!SVGImageBufferTools::createImageBuffer(absoluteDrawingRegion, absoluteDrawingRegion, sourceGraphic, ColorSpaceLinearRGB)) {
> > +        ASSERT(!m_filter.contains(object));
> > +        filterData->savedContext = context;
> > +        m_filter.set(object, filterData.leakPtr());
> >          return false;
> > +    }
> 
> I agree, that the failure handling wasn't correct here. Just a bit confused why you were talking about width or height < 0.5 and how the test checks the correct handling. Since the test should return true here?!? :-P

[Dirk Schulze]

(In reply to comment #7)
> (From update of attachment 74375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74375&action=review
> 
> Maybe I  misunderstood the intention of the after reading the text in the ChangeLog.
> 
> > LayoutTests/svg/filters/filter-x.svg:7
> > +<filter id="filt" filterUnits="userSpaceOnUse" x="9.51" y="5" width="10" height="10">
> > +    <feFlood flood-color="red" />
> > +</filter>
> > +</defs>
> > +<rect x="0" y="0" width="10" height="10" filter="url(#filt)"/>
> 
> Why does it test the case, where width or height are < 0.5 like in the bug description? Just x has a fractional part. How is it related to the described problem?
Talked with Niko about the image size. Sorry the test _is_ correct for the check of the correct behavior _inside_ of SVGFilter.Nevertheless, a red reference image is not a good idea.

Not related to this bug: We should create a ImageBuffer for 0.49 sized images. This is why I was talking about enclosingIntRect/size.

[Nikolas Zimmermann]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 74375 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=74375&action=review
> > 
> > Maybe I  misunderstood the intention of the after reading the text in the ChangeLog.
> > 
> > > LayoutTests/svg/filters/filter-x.svg:7
> > > +<filter id="filt" filterUnits="userSpaceOnUse" x="9.51" y="5" width="10" height="10">
> > > +    <feFlood flood-color="red" />
> > > +</filter>
> > > +</defs>
> > > +<rect x="0" y="0" width="10" height="10" filter="url(#filt)"/>
> > 
> > Why does it test the case, where width or height are < 0.5 like in the bug description? Just x has a fractional part. How is it related to the described problem?
> Talked with Niko about the image size. Sorry the test _is_ correct for the check of the correct behavior _inside_ of SVGFilter.Nevertheless, a red reference image is not a good idea.
> 
> Not related to this bug: We should create a ImageBuffer for 0.49 sized images. This is why I was talking about enclosingIntRect/size.
Really, do you think there's any gain, filtering a 0.49px image buffer? :-)
Anyhow, however wants to work on it, be careful with the lroundfs, they're there on purpose.

Suppose you want a 199.51 px image buffer, we'd now allocate a 199px image buffer, and _scale the context_ accordingly by 199/199.51, so it perfectly fits into the buffer. Otherwhise you'll get artefacts on zooming.

[Dirk Schulze]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (From update of attachment 74375 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=74375&action=review
> > > 
> > > Maybe I  misunderstood the intention of the after reading the text in the ChangeLog.
> > > 
> > > > LayoutTests/svg/filters/filter-x.svg:7
> > > > +<filter id="filt" filterUnits="userSpaceOnUse" x="9.51" y="5" width="10" height="10">
> > > > +    <feFlood flood-color="red" />
> > > > +</filter>
> > > > +</defs>
> > > > +<rect x="0" y="0" width="10" height="10" filter="url(#filt)"/>
> > > 
> > > Why does it test the case, where width or height are < 0.5 like in the bug description? Just x has a fractional part. How is it related to the described problem?
> > Talked with Niko about the image size. Sorry the test _is_ correct for the check of the correct behavior _inside_ of SVGFilter.Nevertheless, a red reference image is not a good idea.
> > 
> > Not related to this bug: We should create a ImageBuffer for 0.49 sized images. This is why I was talking about enclosingIntRect/size.
> Really, do you think there's any gain, filtering a 0.49px image buffer? :-)
> Anyhow, however wants to work on it, be careful with the lroundfs, they're there on purpose.
> 
> Suppose you want a 199.51 px image buffer, we'd now allocate a 199px image buffer, and _scale the context_ accordingly by 199/199.51, so it perfectly fits into the buffer. Otherwhise you'll get artefacts on zooming.

But is it really a big problem (performance and allocation) to make it 1px larger and scale it up instead, but have a correct result, always?

[Zoltan Herczeg]

Sory for the red rect. Maybe this requires a rework, and when it is done, the image update can be submited with it. Or not?

[Dirk Schulze]

(In reply to comment #12)
> Sory for the red rect. Maybe this requires a rework, and when it is done, the image update can be submited with it. Or not?

Yes, would be great. Thanks!