Bug 88654 – SVG filter's SourceGraphic is clipped to the screen size

• Component: SVG
• Status: RESOLVED INVALID

[Tamas Czene]

after opening an svg file that is greater than the screen itself only the visible parts of the image is shown properly, the other parts of the image (that cannot be seen at first but could be moved in to the screen) remain empty

[Tamas Czene]

Created attachment 146553 [details]
proposed patch

[Build Bot]

Comment on attachment 146553 [details]
proposed patch

Attachment 146553 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12907947

[WebKit Review Bot]

Comment on attachment 146553 [details]
proposed patch

Attachment 146553 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12935020

New failing tests:
svg/filters/large-filtered-SVGImage.svg

[WebKit Review Bot]

Created attachment 146601 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Zoltan Herczeg]

Niko, what do you think about this patch? Actually Tamas got a tricky bug in SourceGarphics.

[Tamas Czene]

Created attachment 146847 [details]
proposed patch

[WebKit Review Bot]

Comment on attachment 146847 [details]
proposed patch

Attachment 146847 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12941410

New failing tests:
svg/filters/large-filtered-SVGImage.svg
svg/dynamic-updates/SVGFEBlendElement-dom-mode-attr.html
svg/dynamic-updates/SVGFEBlendElement-dom-in-attr.html
svg/filters/filter-refresh.svg
svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html
svg/filters/filterRes.svg
svg/dynamic-updates/SVGFEBlendElement-svgdom-in-prop.html
svg/filters/feConvolveFilter-y-bounds.svg
svg/dynamic-updates/SVGFEBlendElement-svgdom-in2-prop.html
svg/dynamic-updates/SVGFEBlendElement-svgdom-mode-prop.html

[WebKit Review Bot]

Created attachment 146892 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tim Horton]

Comment on attachment 146553 [details]
proposed patch

Clearing flags and obsoleting the older patch.

[Tim Horton]

Comment on attachment 146847 [details]
proposed patch

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

> LayoutTests/ChangeLog:3
> +        svgfilter sourcegraphic is clipped out according to the screen rectangle

Let's fix up the title a little. I'll fix the bug's title if you update it here :D

> LayoutTests/ChangeLog:7
> +

This could use a minor explanation "Add a test to ensure that [whatever you're testing]".

> LayoutTests/svg/filters/large-filtered-SVGImage.svg:3
> +    <filter id="test" filterUnits="objectBoundingBox" x="0" y="0" width="1" height="1">

Perhaps make the filter's id something more descriptive (translateImageFilter?) than "test".

> Source/WebCore/ChangeLog:3
> +        svgfilter sourcegraphic is clipped out according to the screen rectangle

Ditto on the title.

> Source/WebCore/ChangeLog:7
> +

This *definitely* needs more explanation of what you changed and why.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:251
> +    rect = IntRect(IntPoint(), sourceGraphic->logicalSize());

Could we just use an IntSize instead of an IntRect? Does this out arg ever need to carry anything other than a size?

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.h:77
> +        UNUSED_PARAM(resourceMode);

You can leave the name of the resourceMode parameter off above instead of UNUSED_PARAM'ing it.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.h:83
> +        UNUSED_PARAM(resourceMode);

Ditto.

[Tim Horton]

Comment on attachment 146847 [details]
proposed patch

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

> LayoutTests/ChangeLog:3
> +        svgfilter sourcegraphic is clipped out according to the screen rectangle

Let's fix up the title a little. I'll fix the bug's title if you update it here :D

> LayoutTests/ChangeLog:7
> +

This could use a minor explanation "Add a test to ensure that [whatever you're testing]".

> LayoutTests/svg/filters/large-filtered-SVGImage.svg:3
> +    <filter id="test" filterUnits="objectBoundingBox" x="0" y="0" width="1" height="1">

Perhaps make the filter's id something more descriptive (translateImageFilter?) than "test".

> Source/WebCore/ChangeLog:3
> +        svgfilter sourcegraphic is clipped out according to the screen rectangle

Ditto on the title.

> Source/WebCore/ChangeLog:7
> +

This *definitely* needs more explanation of what you changed and why.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:251
> +    rect = IntRect(IntPoint(), sourceGraphic->logicalSize());

Could we just use an IntSize instead of an IntRect? Does this out arg ever need to carry anything other than a size?

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.h:77
> +        UNUSED_PARAM(resourceMode);

You can leave the name of the resourceMode parameter off above instead of UNUSED_PARAM'ing it.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.h:83
> +        UNUSED_PARAM(resourceMode);

Ditto.

[Dirk Schulze]

Comment on attachment 146847 [details]
proposed patch

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

This patch is missing some more detailed description of the intention. r- because of the missing description.

>>> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:251
>>> +    rect = IntRect(IntPoint(), sourceGraphic->logicalSize());
>> 
>> Could we just use an IntSize instead of an IntRect? Does this out arg ever need to carry anything other than a size?
> 
> Could we just use an IntSize instead of an IntRect? Does this out arg ever need to carry anything other than a size?

The passed argument is a rect, how do you want to change that? I just don't understand why you set this rect. Usually you should not do that. paintInfo gives you the  information of the screen. I assume that you want to limit the painted content to the size of the source graphic some how? But again, what is the intention. Do you want to make sure that the source graphic covers everything that the filter region need?

[Tamas Czene]

Created attachment 150890 [details]
proposed patch

If SourceGraphic gets the image by the information of paintInfo and the image is larger than the screen itself, then sourceGraphic gets the size of the original image but only the pixels that are visible are painted to sourceGraphic. So filters are applied on the size of the original image but the only valid information of that is the clipped image (that has the size of the screen). Therefore if the image is larger than the screen and we scroll to another part of the image we don't see it because it doesn't get into the sourceGraphic.
But if we set 
rect = IntRect(IntPoint(), sourceGraphic->logicalSize());
then sourceGraphic will get the whole image.

[WebKit Review Bot]

Attachment 150890 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:253:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tamas Czene]

Created attachment 150891 [details]
proposed patch

[WebKit Review Bot]

Comment on attachment 150891 [details]
proposed patch

Attachment 150891 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13133998

New failing tests:
svg/filters/filterRes.svg

[WebKit Review Bot]

Created attachment 150900 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 150891 [details]
proposed patch

Attachment 150891 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15884904

New failing tests:
svg/filters/large-filtered-SVGImage.svg

[Dirk Schulze]

I understand the problem of the rendering that is limited to the viewport, while the filtered area is bigger than the viewport. We have another bug report about backing store tiling that seemed to have a similar problem.

Adding Simon. Tim, do you have some input as well?

[Csaba Osztrogonác]

Is this bug still valid?

[Csaba Osztrogonác]

Is this bug still valid?

[Csaba Osztrogonác]

Comment on attachment 150891 [details]
proposed patch

r-, because there is no more Qt in the trunk and the new test failed on mac.

Otherwise is this bug still valid?

[Csaba Osztrogonác]

Is this bug still valid on any platform or can we close it?

[Brent Fulgham]

It's difficult to confirm this bug without a test case. We don't believe there is an ongoing issue here. If you do feel there is a problem, please REOPEN this bug and include a test case illustrating the issue.