358930 - Firefox 2.0 doesn't respect SVG gradient spreadMethod="pad"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Graeme Humphries]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20060601 Firefox/2.0 (Ubuntu-edgy)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20060601 Firefox/2.0 (Ubuntu-edgy)

New problem in 2.0, it looks like the SVG linear gradient rendering is ignoring spreadMethod="pad" from the SVG 1.1 spec (http://www.w3.org/TR/SVG11/pservers.html#LinearGradients). In the linked example, you can see that the bottom part of the letter shapes are transparent, instead of being padded with white (as Firefox 1.5 correctly did). It looks like start padding is working correctly just end padding is not.

Reproducible: Always

Steps to Reproduce:
1. Load SVG
2. View lack of end padding on fill gradient

Comment 1

[Graeme Humphries]

Attachment: Broken gradient redering in Firefox 2.0

Shows lack of padding in Firefox 2.0 rendering.

Comment 2

[Graeme Humphries]

Attachment: Correct gradient padding rendering by librsvg2.

Shows correct gradient padding rendering (but not correct placed image rendering) from librsvg2 on Linux.

Comment 3

[tor]

Works fine on my linux build here.  This sounds like a cairo version mismatch, and I see that Ubuntu uses the unsupported --enable-system-cairo build config option.

  https://launchpad.net/distros/ubuntu/+source/firefox/+bug/32561

Please test using the official linux build from mozilla.com.

Comment 4

[Graeme Humphries]

Yep, testing with official build shows no problems, it's a problem downstream. Sorry to waste your guys' time, I'll file a bug with Ubuntu.

Comment 5

[tor]

https://launchpad.net/distros/ubuntu/+source/firefox/+bug/69721

Comment 6

[Alexander Sack]

So why has this been resolved as invalid? If --enable-system-cairo has a bug, then its worth an open bug. You might not suppor that, but you apparently maintain that, because it exists their. So, though this bug might be less severe its still a bug in code maintained by you.

Comment 7

[Jonathan Watt [:jwatt]]

All --enable-system-cairo does is make Mozilla use the system cairo, and in that limited task it succeeds perfectly. However, the cairo binary installed on a system is of course outside Mozilla's control. Bugs in the version of cairo installed on the system are not bugs in Mozilla, and since the binary installed on a system is not (and should not be) controlled by Mozilla, --enable-system-cairo is naturally not something we can support for releases.

That aside, I'd have thought it a good idea for package maintainers to ensure their system version of cairo is the same (or at least as recent) as the version of cairo in the Mozilla tree before using such an option.

Comment 8

[Alexander Sack]

Jonathan, thanks clarifying that you think its a bug in our version of cairo and not that its due to the use of system cairo in general.

looking at cairo-features.h, you ship (from 1.8.1 branch):

#define CAIRO_VERSION_STRING "1.0.2"

We ship:

#define CAIRO_VERSION_STRING "1.0.2" (breezy)
#define CAIRO_VERSION_STRING "1.0.4" (dapper)
#define CAIRO_VERSION_STRING "1.2.4" (edgy - current stable)
#define CAIRO_VERSION_STRING "1.3.14" (feisty)

So the reporter has this issue with 1.2.4, which is more recent than cairo. Any chance that you might take a look if this is a regression in cairo, or if mozilla code base could be improved to support current cairo too?

Comment 9

[Jonathan Watt [:jwatt]]

Tim points out that at some point between cairo 1.0.2 and 1.2.4 cairo's gradient API was changed. To get "pad" behavior in the former you had to use CAIRO_EXTEND_NONE.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/cairo/cairo/src/cairo.h&rev=1.3.4.1&mark=1293#1291

Later CAIRO_EXTEND_PAD was added and the meaning of CAIRO_EXTEND_NONE was changed to "pixels outside of the source pattern are fully transparent".

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/cairo/cairo/src/cairo.h&rev=1.23&mark=1575,1578#1558

Comment 10

[Carl Worth]

(In reply to comment #9)
> Tim points out that at some point between cairo 1.0.2 and 1.2.4 cairo's
> gradient API was changed. To get "pad" behavior in the former you had to use
> CAIRO_EXTEND_NONE.
...
> Later CAIRO_EXTEND_PAD was added and the meaning of CAIRO_EXTEND_NONE was
> changed to "pixels outside of the source pattern are fully transparent".
...

Yes, that's fairly accurate.

What didn't change is that the default extend mode for gradients has always been a "pad" behavior, (even though we did not have the PAD enum value in 1.0).

And we hoped that not many people would be bitten by making NONE actually live up to its name in 1.2 since in 1.0 there's no real benefit to explicitly calling cairo_pattern_set_extend (gradient_pattern, CAIRO_EXTEND_NONE) --- that call doesn't actually change anything.

So, writing code to get compatible PAD behavior should be as simple as not calling cairo_pattern_set_extend at all, (or, if you really want to call it for some reason, then yes it will require a version check and using NONE for 1.0 but PAD for 1.2 and on).

Sorry for this little gotcha in the cairo API. I hope you will find that there really aren't a lot of these as far as backwards compatibility is concerned.

-Carl

Comment 11

[Alexander Sack]

I reopen the bug for now, as imo we should definitly try to get something that works across cairo versions. And comments from Carl sound like this should be doable with little or no tradeoff.

Comment 12

[Jonathan Watt [:jwatt]]

Yup, cool. Should be as simple as removing the two lines at:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp&rev=1.6.6.4&mark=182-183#179

I'll attach a patch later today. Would you be able to test it asac?

Comment 13

[Alexander Sack]

i already have the patch here ... testing.

Comment 14

[Alexander Sack]

Attachment: patch with comment

Comment 15

[Alexander Sack]

Attachment: patch without comment (candidate)

Comment 16

[Alexander Sack]

for me it indeed fixes this issue.

Comment 17

[Alexander Sack]

... have not yet tried with 1.0 cairo though.

Comment 18

[Jonathan Watt [:jwatt]]

Alexander, were you going to test 1.0?

Comment 21

[FReeZ]

I've applied the patch 257028 into FF 2.0.0.3 (my cairo version is 1.2.6) and now SVG images are finally rendereder fine. Good job guys =)

Comment 22

[Alexander Sack]

Jonathan, renders correctly with patch applied and built against 1.0 system cairo here. Who can review svg?

Comment 23

[Mike Connor [:mconnor]]

Johnathan can, and tor can SR...

Comment 24

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 25

[Carl Worth]

(In reply to comment #24)
> Created an attachment (id=260571) [details]
> patch

From a cairo point-of-view, the patch and the description look good to me.

-Carl

Comment 26

[Jonathan Watt [:jwatt]]

Thanks for looking Carl.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 260571 [details] [diff] [review]
patch

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 28

[Jonathan Watt [:jwatt]]

Checking in mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/Attic/nsSVGCairoGradient.cpp,v  <--  nsSVGCairoGradient.cpp
new revision: 1.6.6.5; previous revision: 1.6.6.4
done