1181317 - SVG feBlend rendering incorrect for some new blend mode types

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alecazam]

Attachment: 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.

Comment 1

[Alecazam]

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.

Comment 2

[Markus Stange [:mstange]]

Are we maybe just getting the order wrong? Or are these blend modes symmetric?

Comment 3

[Alecazam]

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.

Comment 4

[Alecazam]

Attachment: BrowserPassThroughFail.svg

This illustrates bugs with pass-through blend mode.  Normal not isolated blending.

Comment 5

[Alecazam]

(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.

Comment 6

[Tom Klein]

Attachment: 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 7

[Tom Klein]

Clarification: the patch from comment 6 fixes the testcase from comment 0, but our behavior on the testcase from comment 4 is unchanged, so depending on the review outcome we may want to make comment 4 a new bug.

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 8648278 [details] [diff] [review]
1181317.diff

I'm not a gfx reviewer but it looks right to me.

Comment 9

[Alecazam]

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 10

[Markus Stange [:mstange]]

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.

Comment 11

[Tom Klein]

Attachment: 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.

Comment 12

[Markus Stange [:mstange]]

Hmm, wouldn't it better to just flip the inputs here? https://dxr.mozilla.org/mozilla-central/source/gfx/src/FilterSupport.cpp#750

Comment 13

[Tom Klein]

Attachment: 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 14

[Markus Stange [:mstange]]

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?

Comment 15

[Tom Klein]

(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?

Comment 16

[Tom Klein]

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.)

Comment 17

[Markus Stange [:mstange]]

Yeah you're right, I was a little confused.

Comment 18

[Tom Klein]

(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.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab7c7ce7027

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ab7c7ce7027