989230 - SVG gradient spreadMethod="reflect" doesn't work

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Viktor Michna]

Attachment: svg_gradient_reflect.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

We are using reflecting gradients in our company visualization software for purpose of pipe visualization.


Actual results:

No reflecting gradients aren't working since FF 26. Tested on MS Windows. See attached archive - test_firefox.png. Source svg file is also included (created in Inkscape).


Expected results:

See attached archive / test_correct.png

Comment 1

[Robert Longson [:longsonr]]

Attachment: reporter's testcase

Comment 2

[Robert Longson [:longsonr]]

Attachment: test_correct.png

Comment 3

[Robert Longson [:longsonr]]

Attachment: test_firefox.png

Comment 4

[Robert Longson [:longsonr]]

Note that this: http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/pservers-grad-10-b.html does so some spreadMethod="reflect" cases do work.

Comment 5

[Robert Longson [:longsonr]]

Please ignore comment 4. I was looking at the wrong part of the testcase. http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/pservers-grad-10-b.html doesn't work either.

Comment 6

[Alice0775 White]

Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b40ccf163d82&tochange=1574b1da1773
Regressed by: Bug 930451

Comment 7

[Andreas Gal :gal]

Yup, I definitely broke this. Thanks for the bisect. Great bug report and test case, too. Should be an easy fix.

Comment 8

[Andreas Gal :gal]

The bug is here:

In nsSVGGradientFrame.cpp:

  uint16_t aSpread = GetSpreadMethod();
  if (aSpread == SVG_SPREADMETHOD_PAD)
    gradient->SetExtend(gfxPattern::EXTEND_PAD);
  else if (aSpread == SVG_SPREADMETHOD_REFLECT)
    gradient->SetExtend(gfxPattern::EXTEND_REFLECT);
  else if (aSpread == SVG_SPREADMETHOD_REPEAT)
    gradient->SetExtend(gfxPattern::EXTEND_REPEAT);

And the code I added in gfxPattern:

mStops = gfxGradientCache::GetOrCreateGradientStops(aDT,
  stops,
  (cairo_pattern_get_extend(mPattern) == CAIRO_EXTEND_REPEAT)
    ? mozilla::gfx::EXTEND_REPEAT
    : mozilla::gfx::EXTEND_CLAMP);

I am not handling EXTEND_REFLECT in the pattern caching code.

Comment 9

[Andreas Gal :gal]

Attachment: Likely fix, untested.

Comment 10

[Robert Longson [:longsonr]]

Attachment: simpler fix

There's already a method to do this conversion.

Comment 11

[Jonathan Watt [:jwatt]]

Can we have a reftest for this too?

Comment 12

[Robert Longson [:longsonr]]

How? We could have a reflect != repeat test but I don't know how to do reflect in another way.

Comment 13

[Andreas Gal :gal]

Comment on attachment 8398963 [details] [diff] [review]
simpler  fix

Review of attachment 8398963 [details] [diff] [review]:
-----------------------------------------------------------------

Bonus points if you turn the provided test case into a ref test. Thanks!

::: gfx/thebes/gfxPattern.cpp
@@ +119,4 @@
>        stops[n].offset = offset;
>      }
> +
> +    GraphicsExtend extend = (GraphicsExtend)cairo_pattern_get_extend(mPattern);

Do we need the cast here? If so it should be X() syntax. However I would just make the GetOrCreate call below a 3-liner, nexting cairo_pattern_get_extend(mPattern) inside ToExtendMode instead of this temporary here. Your choice.

Comment 14

[Andreas Gal :gal]

png?

Comment 15

[Robert Longson [:longsonr]]

(In reply to Andreas Gal :gal from comment #14)
> png?

Tends not to be cross-platform.

Comment 16

[Andreas Gal :gal]

use a simple gradient and some fuzz, should work

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: reftest

Comment 18

[Robert Longson [:longsonr]]

Attachment: reflect.txt

Address review comments.

Comment 19

[Jonathan Watt [:jwatt]]

Unfortunately the patch here causes spreadMethod="reflect" gradients to completely fail on OS X instead of just incorrectly using "pad":

https://tbpl.mozilla.org/?tree=Try&rev=a95ce98f0c25

(The failures there on other platforms are max-difference of 1, so we just need some fuzz for them.)

Comment 20

[Robert Longson [:longsonr]]

Unfortunately, I can't look into OS X issues.

Comment 21

[Andreas Gal :gal]

static CGPatternRef
CreateCGPattern(const Pattern &aPattern, CGAffineTransform aUserSpace)

    case ExtendMode::REFLECT:
      assert(0);

That explains that.

Comment 22

[Andreas Gal :gal]

We really, really, really, really have to delete the core graphics backend. In the meantime I recommend removing the assert and marking the test a known-fail on mac and then lets land the fix. Milan, whats our timeline to kill CG and replace it with SkiaGL?

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

SkiaGL on the Mac plans are part of the deliverables for the current work week.  Should have that by the end of the week.

Comment 24

[Jonathan Watt [:jwatt]]

(In reply to Andreas Gal :gal from comment #21)
> static CGPatternRef
> CreateCGPattern(const Pattern &aPattern, CGAffineTransform aUserSpace)
> 
>     case ExtendMode::REFLECT:
>       assert(0);
> 
> That explains that.

The code that's actually causing us to paint nothing on OS X is this code:

https://mxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCG.cpp?rev=7bacc9e903b0&mark=642-656,661-674#632

which doesn't handle ExtendMode::REFLECT, obviously because we don't want to hit the assert that you point out.

If reverting the gradient caching patches for OS X is not an option at this point, I think the least bad thing to do would be to make the code that I point out take the ExtendMode::REPEAT path if handling ExtendMode::REFLECT. It's not great, but it's better than painting nothing, and I think better than taking the ExtendMode::EXTEND path. I'll file a separate bug for that.

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: spreadMethod="repeat" testcase resulting in bad banding on OS X

(In reply to Jonathan Watt [:jwatt] from comment #24)
> I think the least bad thing to do would be to make the code that I
> point out take the ExtendMode::REPEAT path if handling ExtendMode::REFLECT.

Ugh, except that the REPEAT path is pretty broken too, in that it has really bad banding.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #23)
> SkiaGL on the Mac plans are part of the deliverables for the current work
> week.  Should have that by the end of the week.

We have this on the roadmap, but it is not a short term solution.  We probably want something that fixes CG for now.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we please land this patch to fix the non-Mac platforms at least?

Comment 28

[Jonathan Watt [:jwatt]]

Comment on attachment 8398963 [details] [diff] [review]
simpler  fix

At this point we might as well wait for Jeff's reviews for bug 1022624 and then we can take longsonr's patch as-is, I think, plus my reftest patch modulo some fuzzying that's yet to be determined.

Comment 29

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #25)
> Ugh, except that the REPEAT path is pretty broken too, in that it has really
> bad banding.

Filed bug 1023640 for that.

Comment 30

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2009199a959c
https://hg.mozilla.org/integration/mozilla-inbound/rev/facca97fc84d

Comment 31

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea822072022f

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2009199a959c
https://hg.mozilla.org/mozilla-central/rev/facca97fc84d
https://hg.mozilla.org/mozilla-central/rev/ea822072022f

Comment 33

[Jonathan Watt [:jwatt]]

Comment on attachment 8398963 [details] [diff] [review]
simpler  fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 930451
User impact if declined: Some SVG breakage when SVG contains gradients
Testing completed (on m-c, etc.): landed there
Risk to taking this patch (and alternatives if risky): should be low, and we're early in the cycle
String or IDL/UUID changes made by this patch: none

Comment 34

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8398963 [details] [diff] [review]
simpler  fix

Aurora approval granted.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5b8263f7b46c
https://hg.mozilla.org/releases/mozilla-aurora/rev/f916b5650b57