Created attachment 8630694 [details] InnerShadowBlendTest Firefox.svg User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.130 Safari/537.36 Firefox for Android Steps to reproduce: Open the attached svg file which has a red inner shadow that uses feBlend to composite the inner shadow against a gray shape. The blend modes are taken from mix-blend-mode. This was tested on FirefoxNightly 42.0a1 (2015-07-05), and also Firefox 39.0 on OSX Yosemite 10.10.3. Actual results: Firefox incorrectly renders some tiles. The following cases overlay/softLight/hardLight, color-burn, and hue/saturation/color/luminosity all render incorrectly. Expected results: Drop the file into Chrome to see correct blending, or look at the enclosed image in the SVG file. Safari also renders a few of the elements incorrectly. The results should match the image and other browers (see Chrome and not Safari) for accurate blending results.
I should add that my mix-blend-mode tests seem to work fine for all blend modes. It's just the feBlend cases that don't look correct.
Are we maybe just getting the order wrong? Or are these blend modes symmetric?
Here's another bad blend case for Firefox. This one with mix-blend-mode. Chrome/Safari don't get pass-through mode (normal blend mode not isolated). This should be a lerp(dst, src(s) blend dst, opacity). Isolated is lerp(black, src(s) blend dst, opacity). At least Safari/Chrome don't render black, but they're still not correct.
Created attachment 8630720 [details] BrowserPassThroughFail.svg This illustrates bugs with pass-through blend mode. Normal not isolated blending.
(In reply to Markus Stange [:mstange] from comment #2) > Are we maybe just getting the order wrong? Or are these blend modes > symmetric? HardLight and SoftLight results looked flipped. Also Color/Luminosity looked flipped. Overlay and ColorBurn look too dark. Hue/Saturation look incorrect. src/dst are easy to pass to wrong inputs. Hue/Saturation/Color/Luminosity are non-separable blend modes, but are correct for mix-blend-mode usage.
Created attachment 8648278 [details] [diff] [review] 1181317.diff Markus, I think you were right about the order being switched. Since I'm new to filters, let me see if I'm getting it right, according to http://cairographics.org/operators/ : First, <feBlend in="input1" in2="input2"/> means we draw input1 over input2. In that case, in cairo's terminology, input2 is the destination and input1 is the source, so according to the cairo docs, we should draw the destination (input2) first and then draw input1 with the appropriate blend mode over that. Currently we draw input1 first and then input2. Drawing input2 first and then input1 puts us in line with chrome's rendering and the testcase static image. I added another reftest to feBlend-2.svg because the current one doesn't take the FilterNodeBlendSoftware::Render path where I made changes. I also changed the colors in feBlend-1.svg since (I guess) they were mostly incorrect with our incorrect behavior - they're now in line with what chrome renders, for what it's worth.
Comment on attachment 8648278 [details] [diff] [review] 1181317.diff I'm not a gfx reviewer but it looks right to me.
I've had similar bugs implementing these with flipped in1/in2. I filed a Chrome bug on comment 4 here https://code.google.com/p/chromium/issues/detail?id=507827, but I think it needs further classification from the SVG2 spec. isolation and knockout mean very specific things in Illustrator. Pass through is a normal blend onto bg and a linear lerp with layer opacity from that result to the original bg. I think that's what the spec was hoping to implement, but didn't specify it clearly enough.
Comment on attachment 8648278 [details] [diff] [review] 1181317.diff Thank you for fixing this! I haven't manually checked that this gives the correct result but I trust you on it.
Created attachment 8650652 [details] [diff] [review] Patch v2.diff Thanks for the review Markus. Unfortunately D2D filters didn't occur to me the first time around, and of course the inputs were reversed there as well, so I've added an feBlend inputs switch for D2D1 in this patch. I don't think there are any other filter drawers that need checking(?). I also noticed this time around that there were a couple of copy and paste errors in feBlend-1.svg causing some of the test patches to be drawn over others, so I fixed those as well.
Hmm, wouldn't it better to just flip the inputs here? https://dxr.mozilla.org/mozilla-central/source/gfx/src/FilterSupport.cpp#750
Created attachment 8654179 [details] [diff] [review] Patch v3.diff (In reply to Markus Stange [:mstange] from comment #12) > Hmm, wouldn't it better to just flip the inputs here? > https://dxr.mozilla.org/mozilla-central/source/gfx/src/FilterSupport.cpp#750 Might as well I guess (I haven't checked which order Skia will want (presuming skia will replace cairo in the software filter case), but the new reftest should catch that in any case).
Comment on attachment 8654179 [details] [diff] [review] Patch v3.diff Thanks! Are you going to push it to try to see what Skia is doing?
(In reply to Markus Stange [:mstange] from comment #14) > Comment on attachment 8654179 [details] [diff] [review] > Patch v3.diff > > Thanks! > > Are you going to push it to try to see what Skia is doing? Thanks! Yes, I'll push to try, but what platform should I run on to test Skia? (I'd like to know just for general info.) Also I thought cairo was used to draw filters for all backends in the software case, so in what sense would we be testing Skia? My reference to Skia in comment 13 was meant to say that when cairo eventually gets replaced by skia, so that skia does all the software filter drawing(?), maybe skia will actually want the inputs in their original order, unflipped - can that be tested by a try run now?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbe51effec10 (Regarding Skia, I locally tested using skia as the backend by setting gfx.canvas/content.azure.backends=skia, but the code to actually render filters using skia doesn't appear to exist yet, so that will have to wait.)
Yeah you're right, I was a little confused.
(In reply to Markus Stange [:mstange] from comment #17) > Yeah you're right, I was a little confused. Yeah, my comment 13 wasn't the clearest.