Closed Bug 989230 Opened 7 years ago Closed 6 years ago

SVG gradient spreadMethod="reflect" doesn't work

Categories

(Core :: SVG, defect)

26 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
firefox-esr24 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: viktor.michna, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(7 files, 2 obsolete files)

Attached file svg_gradient_reflect.zip (obsolete) —
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
Attached image reporter's testcase
Attachment #8398397 - Attachment is obsolete: true
Attached image test_correct.png
Attached image test_firefox.png
Component: Untriaged → SVG
Product: Firefox → Core
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yup, I definitely broke this. Thanks for the bisect. Great bug report and test case, too. Should be an easy fix.
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.
Attached patch Likely fix, untested. (obsolete) — Splinter Review
Attached patch simpler fixSplinter Review
There's already a method to do this conversion.
Attachment #8398963 - Flags: review?(gal)
Can we have a reftest for this too?
How? We could have a reflect != repeat test but I don't know how to do reflect in another way.
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.
Attachment #8398963 - Flags: review?(gal) → review+
png?
(In reply to Andreas Gal :gal from comment #14)
> png?

Tends not to be cross-platform.
use a simple gradient and some fuzz, should work
Attached patch reftestSplinter Review
Attachment #8398966 - Flags: review?(longsonr)
Attachment #8398966 - Flags: review?(longsonr) → review+
Attached patch reflect.txtSplinter Review
Address review comments.
Attachment #8398960 - Attachment is obsolete: true
Attachment #8398963 - Attachment is obsolete: true
Assignee: nobody → longsonr
OS: Windows 7 → All
Hardware: x86_64 → All
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.)
Unfortunately, I can't look into OS X issues.
Assignee: longsonr → nobody
static CGPatternRef
CreateCGPattern(const Pattern &aPattern, CGAffineTransform aUserSpace)

    case ExtendMode::REFLECT:
      assert(0);

That explains that.
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?
Flags: needinfo?(milan)
SkiaGL on the Mac plans are part of the deliverables for the current work week.  Should have that by the end of the week.
(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.
(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.
(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.
Flags: needinfo?(milan)
Can we please land this patch to fix the non-Mac platforms at least?
Flags: needinfo?(jwatt)
Depends on: 1022624
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.
Attachment #8398963 - Attachment is obsolete: false
Flags: needinfo?(jwatt)
Blocks: 1023640
(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.
Assignee: nobody → longsonr
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
Attachment #8398963 - Flags: approval-mozilla-aurora?
Comment on attachment 8398963 [details] [diff] [review]
simpler  fix

Aurora approval granted.
Attachment #8398963 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1042872
You need to log in before you can comment on or make changes to this bug.