If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

SVG feBlend rendering incorrect for some new blend mode types

RESOLVED FIXED in Firefox 43

Status

()

Core
SVG
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Alecazam, Assigned: Tom Klein)

Tracking

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Are we maybe just getting the order wrong? Or are these blend modes symmetric?
Status: UNCONFIRMED → NEW
Component: Untriaged → SVG
Ever confirmed: true
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86_64 → All
Version: 39 Branch → Trunk
(Reporter)

Comment 3

2 years ago
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.
(Reporter)

Comment 4

2 years ago
Created attachment 8630720 [details]
BrowserPassThroughFail.svg

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

Comment 5

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
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.
Assignee: nobody → twointofive
Attachment #8648278 - Flags: review?(longsonr)

Updated

2 years ago
Attachment #8648278 - Flags: review?(longsonr) → review?(mstange)
(Assignee)

Comment 7

2 years ago
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

2 years ago
Comment on attachment 8648278 [details] [diff] [review]
1181317.diff

I'm not a gfx reviewer but it looks right to me.
Attachment #8648278 - Flags: feedback+
(Reporter)

Comment 9

2 years ago
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.
Attachment #8648278 - Flags: review?(mstange) → review+
(Assignee)

Comment 11

2 years ago
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.
Attachment #8648278 - Attachment is obsolete: true
Attachment #8650652 - Flags: review?(mstange)
Hmm, wouldn't it better to just flip the inputs here? https://dxr.mozilla.org/mozilla-central/source/gfx/src/FilterSupport.cpp#750
(Assignee)

Comment 13

2 years ago
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).
Attachment #8650652 - Attachment is obsolete: true
Attachment #8650652 - Flags: review?(mstange)
Attachment #8654179 - Flags: review?(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?
Attachment #8654179 - Flags: review?(mstange) → review+
(Assignee)

Comment 15

2 years ago
(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?
(Assignee)

Comment 16

2 years ago
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.)
Keywords: checkin-needed
Yeah you're right, I was a little confused.
(Assignee)

Comment 18

2 years ago
(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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab7c7ce7027
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ab7c7ce7027
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.