Closed Bug 1047477 Opened 5 years ago Closed 5 years ago

Add support for all blend mode to feBlend

Categories

(Core :: SVG, defect)

30 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: cabanier, Assigned: cabanier)

Details

Attachments

(2 files, 8 obsolete files)

Filter spec was updated with all of the blend modes that are defined in the CSS blending specification: http://dev.w3.org/fxtf/filters/#feBlendElement

New modes to be supported are:

  overlay,
  color-dodge,
  color-burn,
  hard-light,
  soft-light,
  difference,
  exclusion,
  hue,
  saturation,
  color and
  luminosity
Assignee: nobody → cabanier
Comment on attachment 8466265 [details] [diff] [review]
Work in progress. Not for review.

Thank you for attaching even a very early patch.

There are a lot of "if"s there in FilterProcessingScalar.cpp. You're probably aware of this requirement, but I'll mention it again anyway: The execution speed of a filter operation must not depend on the color values of the input pixels. Any conditional branch you introduce (where the input pixel color determines which branch is taken) poses the risk of an information leak.
(In reply to Markus Stange [:mstange] from comment #2)
> Comment on attachment 8466265 [details] [diff] [review]
> Work in progress. Not for review.
> 
> Thank you for attaching even a very early patch.
> 
> There are a lot of "if"s there in FilterProcessingScalar.cpp. You're
> probably aware of this requirement, but I'll mention it again anyway: The
> execution speed of a filter operation must not depend on the color values of
> the input pixels. Any conditional branch you introduce (where the input
> pixel color determines which branch is taken) poses the risk of an
> information leak.

I can likely remove most of the "if"'s. 
I am worried about the SIMD implementation though since even the basic blend modes look very complicated. Is is possible to just use the basic non-simd ones?
Flags: needinfo?(mstange)
Sure. We can still think about SIMD-accelerating the new blend modes later.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #4)
> Sure. We can still think about SIMD-accelerating the new blend modes later.

I finished the non-simd code and verified that it worked.
Now I'm looking how I can get rid of the if's or how I can make if/else execute in the same time. Do you have any suggestions?
Flags: needinfo?(mstange)
Usually the way to go is to compute both and then do some kind of bitmasking with the condition you test against.

However, let's step back for a moment. We already support all the same blend modes through CSS mix-blend-mode, right? On platforms where we use pixman for regular web content rendering (through DrawTargetCairo), the code that does the blending for inactive layers lives somewhere in pixman. Now there are two possibilities:
 1. If this pixman code is vulnerable against timing attacks, then we're already screwed, because attackers don't need to use filters to exploit the information leak. They can just use mix-blend-mode.
 2. If the pixman code is not vulnerable, then we can just make FilterNodeBlendSoftware use DrawTargetCairo instead of creating new code.

I've attached a patch to bug 1064875 that converts FilterNodeCompositeSoftware to use DrawTargetCairo. You could try whether doing something similar for FilterNodeBlendSoftware would work here.

I'm sorry that I didn't think of this sooner. It would probably have saved you a bunch of work.
Flags: needinfo?(mstange)
Attachment #8466265 - Attachment is obsolete: true
(In reply to Markus Stange [:mstange] from comment #6)
> 
> I'm sorry that I didn't think of this sooner. It would probably have saved
> you a bunch of work.

No problem! It was interesting to think about making it execute in the same amount of time.
I updated my implementation (which is now MUCH simpler) and created a test file.
The problem is that results are slightly different on some platforms (See the failures in the reftests in https://tbpl.mozilla.org/?tree=Try&rev=19337282014c)
Should I exclude those platforms?
Flags: needinfo?(mstange)
Since all of the failures have a maximum difference of 1, you can just make the test fuzzy(1,40000) for all platforms.
Flags: needinfo?(mstange)
Attached patch feblend.patch (obsolete) — Splinter Review
Attachment #8487338 - Attachment is obsolete: true
try run: https://tbpl.mozilla.org/?tree=Try&rev=0467a8f1b89d
Attachment #8488234 - Attachment is obsolete: true
Attachment #8488251 - Flags: review?(mstange)
Comment on attachment 8488251 [details] [diff] [review]
implementation of feblend + updated test file

Review of attachment 8488251 [details] [diff] [review]:
-----------------------------------------------------------------

Most of this is looking very good, but I'd like to see a new patch that addresses my comments.

::: gfx/2d/FilterProcessing.cpp
@@ +50,5 @@
>  {
>    if (Factory::HasSSE2()) {
>  #ifdef USE_SSE2
> +    RefPtr<DataSourceSurface> retval = ApplyBlending_SSE2(aInput1, aInput2, aBlendMode);
> +    if(retval != nullptr) {

if (retval) {

(also note the space after if)

::: gfx/2d/FilterProcessingScalar.cpp
@@ +89,4 @@
>  
> +  Rect r(0, 0, size.width, size.height);
> +  dt->ClearRect(r);
> +  dt->DrawSurface(aInput1, r, r, DrawSurfaceOptions());

It would probably be faster to copy aInput1 into target using CopyRect, before you create the drawtarget around "target". But CopyRect is local to FilterNodeSoftware.cpp, so I think you should move the whole contents of FilterProcessing::ApplyBlending_Scalar directly into FilterNodeBlendSoftware::Render.
(You can swap the order of case two and three in that function so that you don't have to indent the "both are non-transparent" case unnecessarily.)
Attachment #8488251 - Flags: review?(mstange)
Attachment #8488251 - Attachment is obsolete: true
New patch that addresses your comments + clean up
Attachment #8488436 - Attachment is obsolete: true
Attachment #8488438 - Flags: review?(mstange)
Comment on attachment 8488438 [details] [diff] [review]
implementation of feblend + updated test file

Review of attachment 8488438 [details] [diff] [review]:
-----------------------------------------------------------------

Let's keep the SSE2 code for the existing blend modes around for now. It looks like pixman doesn't have SIMD-accelerated versions for blending, but Skia does, so we can remove our SSE2 version when we switch to Skia.

Please do the following:
 - Keep FilterProcessing::ApplyBlending and FilterProcessing::ApplyBlending_SSE2.
 - In FilterProcessing::ApplyBlending, instead of calling ApplyBlending_Scalar (which you remove), just return nullptr.
 - In FilterNodeBlendSoftware::Render, first call FilterProcessing::ApplyBlending, and if that returns nullptr, execute the rest of the function.

::: gfx/2d/FilterNodeSoftware.cpp
@@ +1053,5 @@
>      return nullptr;
>    }
>  
> +  // Second case: one of them is transparent. Return the non-transparent one.
> +  if (static_cast<bool>(input1) != static_cast<bool>(input2)) {

if (!input1 || !input2) {
Attachment #8488438 - Flags: review?(mstange) → review-
Attachment #8488438 - Attachment is obsolete: true
Attachment #8488802 - Attachment is obsolete: true
Attachment #8488803 - Flags: review?(mstange)
Comment on attachment 8488803 [details] [diff] [review]
implementation of feblend + updated test file

Review of attachment 8488803 [details] [diff] [review]:
-----------------------------------------------------------------

Apart from the missing return this looks finished to me.

You should probably get some rubber-stamp reviews from other people that are peers of the code you touch. I can cover the files that start with Filter*, but maybe get a review from longsonr for the SVGBlendElement.cpp change, and you'll also need a dom peer to review the .webidl change (see https://groups.google.com/d/msg/mozilla.dev.platform/u2CLWN8MjDk/Zkm0GRjqNHYJ ).

::: gfx/2d/FilterNodeSoftware.cpp
@@ +1060,5 @@
>  
> +  // Third case: both are non-transparent.
> +  // Apply normal filtering.
> +  RefPtr<DataSourceSurface> target = FilterProcessing::ApplyBlending(input1, input2, mBlendMode);
> +  if(target != nullptr) {

if (target) {

@@ +1061,5 @@
> +  // Third case: both are non-transparent.
> +  // Apply normal filtering.
> +  RefPtr<DataSourceSurface> target = FilterProcessing::ApplyBlending(input1, input2, mBlendMode);
> +  if(target != nullptr) {
> +    target.forget();

You're missing a return here. (!)
Attachment #8488803 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #19)
> Comment on attachment 8488803 [details] [diff] [review]
> implementation of feblend + updated test file
> 
> Review of attachment 8488803 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Apart from the missing return this looks finished to me.
> 
> You should probably get some rubber-stamp reviews from other people that are
> peers of the code you touch. I can cover the files that start with Filter*,
> but maybe get a review from longsonr for the SVGBlendElement.cpp change, and
> you'll also need a dom peer to review the .webidl change (see
> https://groups.google.com/d/msg/mozilla.dev.platform/u2CLWN8MjDk/
> Zkm0GRjqNHYJ ).

Thanks. Will do!

> 
> ::: gfx/2d/FilterNodeSoftware.cpp
> @@ +1060,5 @@
> >  
> > +  // Third case: both are non-transparent.
> > +  // Apply normal filtering.
> > +  RefPtr<DataSourceSurface> target = FilterProcessing::ApplyBlending(input1, input2, mBlendMode);
> > +  if(target != nullptr) {
> 
> if (target) {
> 
> @@ +1061,5 @@
> > +  // Third case: both are non-transparent.
> > +  // Apply normal filtering.
> > +  RefPtr<DataSourceSurface> target = FilterProcessing::ApplyBlending(input1, input2, mBlendMode);
> > +  if(target != nullptr) {
> > +    target.forget();
> 
> You're missing a return here. (!)

Oops! :-)
Can you review the SVGBlendElement and WebIDL changes?
Attachment #8488803 - Attachment is obsolete: true
Attachment #8488898 - Flags: review?(longsonr)
Attachment #8488898 - Flags: review?(bzbarsky)
Comment on attachment 8488898 [details] [diff] [review]
implementation of feblend + updated test file

r=me on the content/ and dom/ changes
Attachment #8488898 - Flags: review?(bzbarsky) → review+
Attachment #8488898 - Flags: review?(longsonr) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d8af54b625de
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
These may have been added, but the rendering is incorrect.  Compare to Chrome on the following test file:
  failures on      Overlay/SoftLight/HardLight     ColorBurn     Hue/Saturation/Color/Luminosity 

Safari has problem rendering ColorBurn and Hue/Saturation, so don't look too closely at it's output.  The test image shows the correct rendering.
This illustrates the failure cases with Firefox.  Compare to Chrome or image in SVG file for correct blended output.
Thanks Alec, this is really useful. Can you please file a new bug with your testcase?
Submitted as: Bug 1181317
You need to log in before you can comment on or make changes to this bug.