SVG filter output not correct for big filter size

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: krit, Assigned: roc)

Tracking

(Depends on: 1 bug)

unspecified
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: opacity=0 is bug 519144)

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-de) AppleWebKit/531.2+ (KHTML, like Gecko) Safari/531.2+
Build Identifier: FF 3.5.3

Not sure if it is a problem of filterRegion calculation, feFlood or both. But as a test result I see a pink rect, with the size of the window. The result scales with the window. You'll see no scrollbars.

Reproducible: Always



Expected Results:  
red sqaure with: x="10" y="10" width="100" height="100"
(Reporter)

Comment 1

9 years ago
Created attachment 406359 [details]
Test for big filterRegion

test
Attachment #406359 - Attachment is patch: false
Attachment #406359 - Attachment mime type: text/plain → image/svg
Summary: SVF filter output not correct for big filter size → SVG filter output not correct for big filter size
Attachment #406359 - Attachment mime type: image/svg → image/svg+xml
Assignee: nobody → roc
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Actually the main part of this testcase is INVALID IMHO.

Here's what we do in this testcase:
1) Because the filter region is huge, we set the temporary surface to a maximum size of 16384x16384
2) When the bounding-box of the <rect> object is mapped into the coordinates of the temporary surface, it comes to (0.0016384,0.00016384)-(0.0180224,0.00180224)
3) The spec doesn't say how to handle non-pixel-aligned filter primitive regions. We round them out to the nearest pixel boundary.
4) So in this case, the top-left pixel of the temporary surface is filled with red.
5) We draw that back to the screen, and that one pixel covers the entire screen.

I honestly think that's all OK, although I'm willing to listen to arguments that we should do something else. In practice, don't use crazy filter region sizes!
Created attachment 406384 [details] [diff] [review]
fix assertion

We can remove the assertion. This patch makes nsSVGUtils::ConvertToSurfaceSize more robust, right now if you pass in a large value it can be converted to a negative number by the PRInt32() cast and that results in the filter not rendering at all (it's treated as "in error"). Unfortunately we still don't pass a reftest because scaling fails due to the extremely large scale factor, but oh well. At least I have some crashtests to detect crashes or asserts.
Attachment #406384 - Flags: review?(jwatt)
(Reporter)

Comment 4

9 years ago
(In reply to comment #2)
> I honestly think that's all OK, although I'm willing to listen to arguments
> that we should do something else. In practice, don't use crazy filter region
> sizes!

Yes, definitly. But there are users that create filters with this size. Maybe just to test the behavior of browsers :-P
But my understanding is that the spec are very clear at this point. Even if the filterRect is huge, the feFlood should have the size and position of the filtered object (in this case). I haven't found a hint on how to handle big sized filterRegions or big sized filterRegion in combination with big sized filter primitives. It's up to the implementer how to deal with it.

> Actually the main part of this testcase is INVALID IMHO.
Why? It's correct according to the spec. Even if it is difficult to handle.
I think the spec allows us to use whatever default filterRes we choose.

It does not say how to handle filter primitive regions that are not pixel-aligned with the pixels of the filter temporary surface(s).

So, I think choosing the filterRes we use and aligning the filter primitive region the way we do is permitted, even if it's unexpected.

Updated

9 years ago
Attachment #406384 - Flags: review?(jwatt) → review+
http://hg.mozilla.org/mozilla-central/rev/52c525d3a384

I'm closing this until/unless someone convinces me that there's a better thing to do here.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Blocks: 477929

Updated

8 years ago
Depends on: 601064

Updated

8 years ago
Depends on: 631720

Updated

7 years ago
No longer depends on: 601064
This bug was incorrectly cited in the checkin comment for bug 519144.
Whiteboard: opacity=0 is bug 519144
You need to log in before you can comment on or make changes to this bug.