Closed Bug 989230 Opened 8 years ago Closed 8 years ago
SVG gradient spread
Method="reflect" doesn't work
15.16 KB, image/svg+xml
9.45 KB, image/png
8.21 KB, image/png
1.92 KB, patch
|Details | Diff | Splinter Review|
2.98 KB, patch
|Details | Diff | Splinter Review|
3.06 KB, patch
|Details | Diff | Splinter Review|
557 bytes, image/svg+xml
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
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.
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+
(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
Attachment #8398966 - Flags: review?(longsonr) → review+
Address review comments.
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.
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?
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.
Can we please land this patch to fix the non-Mac platforms at least?
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
(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.
https://hg.mozilla.org/mozilla-central/rev/2009199a959c https://hg.mozilla.org/mozilla-central/rev/facca97fc84d https://hg.mozilla.org/mozilla-central/rev/ea822072022f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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+
You need to log in before you can comment on or make changes to this bug.