950312 - mix-blend-mode causes full page invalidation while scrolling

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Alex]

While looking at some SVG content that made use of mix-blend-mode I noticed that scrolling the page was laggier than normal, enabling paint flashing showed that while the SVG content was on screen, every blended part of the SVG image was being repainted on every scroll operation, along with the rest of the page.

The page linked shows the effect pretty well, whenever the second SVG image is on screen the entire page is repainted while scrolling, and as soon as it moves off screen the painting goes back to normal.

background-blend-mode also exhibits constant repainting while being scrolled on or off screen, but it stops as soon as the element is fully visible, and doesn't cause full page repaints.

Comment 1

[Matt Woodrow (:mattwoodrow)]

I'll look at this next week.

Comment 2

[Rik Cabanier]

mattwoodrow: 
what we really want to know is 'will the mix-blend-mode element end up in the same layer as the background it needs to be blend with'
and if not, force an inactive layer to flatten things to make that true

Comment 3

[Matt Woodrow (:mattwoodrow)]

This is because we put the entire stacking context that contains the blending object into an inactive layer to ensure that the blended object gets drawn into the same buffer as the background it needs to blend with.

Unfortunately for this page (and I guess most pages), this means the entire page ends up inside an inactive layer. This flattens our scroll content into the same buffer as everything attached to the viewport (which happens to be nothing in this case). This breaks our attempts to retain scrolled content, and we have to repaint everything on scroll.

There's also a bug here (though it may never actually matter), in that we're ignoring the LAYER_ACTIVE_FORCE from the nsDislayOwnLayer around the scrollbar content. If someone used mix-blend-mode (Let's hope nobody does this) for drawing the scrollbars, we'd end up blending with the page content, which is probably undesirable.

In a lot of cases the blended content will already be in the same buffer as the background it needs to blend with, and we could skip wrapping everything in an inactive layers.

The best place to detect this would be during layer building, since that's the only place we actually *know* what layers get created and what the contents of them are. However, I don't see any easy way to respond to the case where the blended object is in a different layer to the background. We'd need to repeat the layer building process, but possibly have to move up multiple levels of recursion into BuildContainerLayerFor.

An easier way might be to analyze the display list, and detect cases where we can guarantee it will be safe to skip the inactive layer creation.

I think this works:

For each mix-blend-mode display item, walk up the display list (to items that are painted before it) and try find one that has an opaque area covering our bounds. If we cross a display item that has a different active geometry root (doesn't scroll/move with our item), or has an active layer, give up.

This should work for simple cases, but it won't catch complex ones, like where the blend item is within an inactive transform. I guess we could add a way to walk upwards through that too if we wanted.

roc, how does that sound? Ideas on how to do this properly in FrameLayerBuilder would be good too! :)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Some display list analysis is possible but it's likely to be a bit of a pain since it would have to detect that the mix-blend-mode content is not over any active layer sibling (e.g. a video element).

I think the best fix here is just to add mix-blend-mode support to layers and the compositor. Then we have to force a layer for the nearest stacking context ancestor and for the mix-blend-mode element, but those layers can be active or inactive.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Ok. I assumed that not supporting this in the compositor/layers system was an intentional choice.

Rik, do you want to take that?

Comment 6

[Rik Cabanier]

I believe Horia is signed up to do that. Horia, is that the case?
If not, I can start looking into this.

Comment 7

[Horia Iosif Olaru]

Yes, I will be looking in to this soon, but I think I will need some guidance. That area is really unfamiliar to me.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think this should block enabling mix-blend-mode by default.

Comment 9

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> I don't think this should block enabling mix-blend-mode by default.

Does this mean that you'd be ok shipping blending without support in the compositor?
Unfortunately, Horia was assigned to a different team :-( 
I've started looking at doing hardware blending but other things keep popping up.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Rik Cabanier from comment #9)
> Does this mean that you'd be ok shipping blending without support in the
> compositor?

Yes.

> Unfortunately, Horia was assigned to a different team :-( 
> I've started looking at doing hardware blending but other things keep
> popping up.

OK. Compositor support is still very much wanted, but I think most use-cases won't need it.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Copy mix-blend-mode proprties to the compositor layer tree

I had some down time over the long weekend, so decided to have a play with this.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Pass the blend mode to DrawQuad

I think this makes the most sense as a parameter (rather than being part of the Effect), it's very similar in nature to opacity/transform.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add Layers API to see what blend modes are supported

This may be overkill, but some of the blend modes are a lot easier to implement than others and I haven't had time to get to them all yet.

It's not much code, and it will let us improve the situation for some blend modes immediately, which seems worthwhile.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Create active layers for nsDisplayMixBlendMode and nsDisplayBlendContainer if the layer manager supports all contained blend modes

Comment 15

[Rik Cabanier]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Created attachment 8409906 [details] [diff] [review]
> Part 1: Copy mix-blend-mode proprties to the compositor layer tree
> 
> I had some down time over the long weekend, so decided to have a play with
> this.

thank you!

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Implement SCREEN and MULTIPLY for CompositorOGL

This stops us needing to invalidate the entire page when scrolling with both h/w acceleration disabled (BasicLayerManager), and with OpenGL OMTC (CompositorOGL).

It should also prevent the equivalent bug for all blend modes when we don't have hardware acceleration.

Comment 17

[Matt Woodrow (:mattwoodrow)]

I also noticed that we probably want some more tests:

* We only appear to test all the operators on background-blend-mode, not mix-blend-mode. They currently both get drawn with Moz2D, but doing the latter on the compositor will mean that it's not tested.

* WebGL layers can often contain un-premultiplied alpha, and we need to make sure that the compositor shaders for blend modes handle this correctly.

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8409915 [details] [diff] [review]
Part 5: Implement SCREEN and MULTIPLY for CompositorOGL

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

lgtm!

Comment 19

[Rik Cabanier]

Comment on attachment 8409915 [details] [diff] [review]
Part 5: Implement SCREEN and MULTIPLY for CompositorOGL

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

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +912,5 @@
> +    case gfx::CompositionOp::OP_MULTIPLY:
> +      // If the source data was un-premultiplied we should have already
> +      // asked the fragment shader to fix that.
> +      srcBlend = LOCAL_GL_DST_COLOR;
> +      dstBlend = LOCAL_GL_ONE_MINUS_SRC_ALPHA;

missing 'break' statement

Comment 20

[Rik Cabanier]

Seems to work great! Next step would be to wire up shaders that take as input the blended content and their backdrop.

Comment 21

[Matt Woodrow (:mattwoodrow)]

(In reply to Rik Cabanier from comment #20)
> Seems to work great! Next step would be to wire up shaders that take as
> input the blended content and their backdrop.

Indeed it is! I haven't stated on this, but it didn't look too difficult from what I could see.

The hardest bit is probably getting the backdrop available as an input, not sure if we'll have any issues doing that on GLES.

Comment 22

[Rik Cabanier]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> 
> Indeed it is! I haven't stated on this, but it didn't look too difficult
> from what I could see.
> 
> The hardest bit is probably getting the backdrop available as an input, not
> sure if we'll have any issues doing that on GLES.

It will probably work as Google (and likely Apple) are able to do the same. 
I've done this for blending in D2D already (DrawTargetD2D.cpp line 1758 and up). Not sure if that code helps.

In chrome, the shader does the compositing and the blending. In D2D the shader just blends and the output is composited as usual.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8409909 [details] [diff] [review]
Part 4: Create active layers for nsDisplayMixBlendMode and nsDisplayBlendContainer if the layer manager supports all contained blend modes

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

::: layout/base/nsDisplayList.cpp
@@ +3459,5 @@
> +  MOZ_COUNT_CTOR(nsDisplayBlendContainer);
> +}
> +
> +nsDisplayBlendContainer::nsDisplayBlendContainer(nsDisplayListBuilder* aBuilder,
> +                                                 nsIFrame* aFrame, nsDisplayList* aList)

Where is this constructor going to be called? I can't see what mCanBeActive==false is for.

::: layout/base/nsDisplayList.h
@@ +44,5 @@
>  class ImageContainer;
>  } //namepsace
>  } //namepsace
>  
> +typedef mozilla::EnumSet<mozilla::gfx::CompositionOp> BlendModeSet;

Specify somewhere, maybe here, that NORMAL is never contained in this set. And assert it where possible.

@@ +690,5 @@
>     * has a blend mode attached. We do this so we can insert a 
>     * nsDisplayBlendContainer in the parent stacking context.
>     */
> +  void SetContainsBlendMode(uint8_t aBlendMode);
> +  void SetContainsBlendMode(BlendModeSet& aModes) {

I'd prefer the name SetContainsBlendModes

@@ +693,5 @@
> +  void SetContainsBlendMode(uint8_t aBlendMode);
> +  void SetContainsBlendMode(BlendModeSet& aModes) {
> +    mContainedBlendModes = aModes;
> +  }
> +  void ResetContainsBlendMode() { mContainedBlendModes = BlendModeSet(); }

Can't we just call SetContainsBlendModes(BlendModeSet())?

@@ +2774,5 @@
>      NS_DISPLAY_DECL_NAME("BlendContainer", TYPE_BLEND_CONTAINER)
> +
> +private:
> +    BlendModeSet mContainedBlendModes;
> +    bool mCanBeActive;

Document that this is set to true when mContainedBlendModes is a set of blend modes that the compositor supports.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8409915 [details] [diff] [review]
Part 5: Implement SCREEN and MULTIPLY for CompositorOGL

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

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +903,5 @@
> +    case gfx::CompositionOp::OP_OVER:
> +        MOZ_ASSERT(!aIsPremultiplied);
> +        srcBlend = LOCAL_GL_SRC_ALPHA;
> +        dstBlend = LOCAL_GL_ONE_MINUS_SRC_ALPHA;
> +        break;

Fix indent

Comment 25

[Rik Cabanier]

Seems like there are compilation errors on other platforms: https://tbpl.mozilla.org/?tree=Try&rev=51e4a8ae27fd

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8409908 [details] [diff] [review]
Part 3: Add Layers API to see what blend modes are supported

Note bug 989033 which wants to remove EnumSet.

Comment 27

[Matt Woodrow (:mattwoodrow)]

Benoit, I think Part 3 is a fairly good use case for EnumSet. Is that enough to reconsider removing it, or should I do something else instead?

Comment 28

[Nicolas Silva [:nical]]

Comment on attachment 8409907 [details] [diff] [review]
Part 2: Pass the blend mode to DrawQuad

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

Bas will have a more precise idea than me about how he'd want blend mode to be exposed by the Compositor API, so I'll transfer the review to him. I personally think that the blend mode should be part of the EffectChain.

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #27)
> Benoit, I think Part 3 is a fairly good use case for EnumSet. Is that enough
> to reconsider removing it, or should I do something else instead?

Thanks for pinging me. Here I think that you should carry on using EnumSet here, which means that I won't land bug 989033 (I won't remove EnumSet) for the time being.

Here's the reasoning. First, I have yet to land bug 987290, which will provide a (IMHO better) replacement for a majority of EnumSet use cases; and I don't want to block you here. Second, the present patch actually is using EnumSet for exactly what it's good at, and for which my replacement (bug 987290) isn't adequate by itself: you have a contiguous, non-bitfield enum (CompositionOp) which you want (1) to keep using as such, and you also want (2) to use it as indexing a bit set. Now, MFBT typed enums cover (1), and bug 987290 will allow using them for (2) instead, but I don't have yet a good solution for doing both (1) and (2) at the same time; EnumSet does exactly that.

Comment 30

[Bas Schouten (:bas.schouten)]

Comment on attachment 8409907 [details] [diff] [review]
Part 2: Pass the blend mode to DrawQuad

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

We probably need to note this doesn't work for ComponentAlpha layers, in general this is a little bit scary. Since we can't just freely use the different blend modes with all effects (although we can with most), I wonder whether we should make it a secondary effect in the chain just for non-over. But that's a little ugly as well.

Comment 31

[Rik Cabanier]

(In reply to Bas Schouten (:bas.schouten) from comment #30)
> Comment on attachment 8409907 [details] [diff] [review]
> Part 2: Pass the blend mode to DrawQuad
> 
> Review of attachment 8409907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We probably need to note this doesn't work for ComponentAlpha layers, in
> general this is a little bit scary. Since we can't just freely use the
> different blend modes with all effects (although we can with most), I wonder
> whether we should make it a secondary effect in the chain just for non-over.
> But that's a little ugly as well.

Would blend modes ever be applied to component alpha layers?

Comment 32

[Rik Cabanier]

Attachment: Part 6: Implement SCREEN and MULTIPLY for CompositorD3D11.cpp

Comment 33

[Rik Cabanier]

Comment on attachment 8410585 [details] [diff] [review]
Part 6: Implement SCREEN and MULTIPLY for CompositorD3D11.cpp

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +594,5 @@
> +    case gfx::CompositionOp::OP_SCREEN:
> +      mContext->OMSetBlendState(aIsPremultiplied ? mAttachments->mPremulScreenBlendState : mAttachments->mScreenBlendState, sBlendFactor, 0xFFFFFFFF);
> +      break;
> +    case gfx::CompositionOp::OP_MULTIPLY:
> +      // If the source data was un-premultiplied we should have already

Not sure where I should do this...

Comment 34

[Matt Woodrow (:mattwoodrow)]

(In reply to Rik Cabanier from comment #33)
> Comment on attachment 8410585 [details] [diff] [review]
> Part 6: Implement SCREEN and MULTIPLY for CompositorD3D11.cpp
> 
> Review of attachment 8410585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +594,5 @@
> > +    case gfx::CompositionOp::OP_SCREEN:
> > +      mContext->OMSetBlendState(aIsPremultiplied ? mAttachments->mPremulScreenBlendState : mAttachments->mScreenBlendState, sBlendFactor, 0xFFFFFFFF);
> > +      break;
> > +    case gfx::CompositionOp::OP_MULTIPLY:
> > +      // If the source data was un-premultiplied we should have already
> 
> Not sure where I should do this...

I don't think we have code to generate shaders for d3d11 so this is a bit difficult. You'll have to figure out all the shaders we might use when drawing with MULTIPLY and create a second version of each of them that also appends color.rgb *= color.a;. Then you need to make sure we pick that shader if using MULTIPLY + unpremultiplied alpha.

Comment 35

[Bas Schouten (:bas.schouten)]

Comment on attachment 8410585 [details] [diff] [review]
Part 6: Implement SCREEN and MULTIPLY for CompositorD3D11.cpp

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +58,5 @@
>    RefPtr<ID3D11BlendState> mPremulBlendState;
>    RefPtr<ID3D11BlendState> mNonPremulBlendState;
>    RefPtr<ID3D11BlendState> mComponentBlendState;
>    RefPtr<ID3D11BlendState> mDisabledBlendState;
> +  RefPtr<ID3D11BlendState> mPremulScreenBlendState;

If we're expecting to get more of these I wonder if we should convert this to an array of ID3D11BlendStates indexed by CompositionOp. (And create them on demand, see gfx/2d/DrawTargetD2D for an example)) That would clean this code up a lot. Let's do that.

@@ +594,5 @@
> +    case gfx::CompositionOp::OP_SCREEN:
> +      mContext->OMSetBlendState(aIsPremultiplied ? mAttachments->mPremulScreenBlendState : mAttachments->mScreenBlendState, sBlendFactor, 0xFFFFFFFF);
> +      break;
> +    case gfx::CompositionOp::OP_MULTIPLY:
> +      // If the source data was un-premultiplied we should have already

Hrm, that's a little tricky, that would require a lot more shaders. I need to think about it.

Comment 36

[Rik Cabanier]

(In reply to Bas Schouten (:bas.schouten) from comment #35)
> Comment on attachment 8410585 [details] [diff] [review]
> Part 6: Implement SCREEN and MULTIPLY for CompositorD3D11.cpp
> 
> Review of attachment 8410585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +58,5 @@
> >    RefPtr<ID3D11BlendState> mPremulBlendState;
> >    RefPtr<ID3D11BlendState> mNonPremulBlendState;
> >    RefPtr<ID3D11BlendState> mComponentBlendState;
> >    RefPtr<ID3D11BlendState> mDisabledBlendState;
> > +  RefPtr<ID3D11BlendState> mPremulScreenBlendState;
> 
> If we're expecting to get more of these I wonder if we should convert this
> to an array of ID3D11BlendStates indexed by CompositionOp. (And create them
> on demand, see gfx/2d/DrawTargetD2D for an example)) That would clean this
> code up a lot. 

I don't think we'll get any more. The other blend modes have to be implemented with shaders and will use either a normal source-over blendstate or a source blendstate.

> Let's do that.
> 
> @@ +594,5 @@
> > +    case gfx::CompositionOp::OP_SCREEN:
> > +      mContext->OMSetBlendState(aIsPremultiplied ? mAttachments->mPremulScreenBlendState : mAttachments->mScreenBlendState, sBlendFactor, 0xFFFFFFFF);
> > +      break;
> > +    case gfx::CompositionOp::OP_MULTIPLY:
> > +      // If the source data was un-premultiplied we should have already
> 
> Hrm, that's a little tricky, that would require a lot more shaders. I need
> to think about it.

For the shaders in D2D, I made a shader program that took parameters to choose what blend operation to perform. It seems we can do the same here.

Comment 37

[Bas Schouten (:bas.schouten)]

(In reply to Rik Cabanier from comment #36)
> (In reply to Bas Schouten (:bas.schouten) from comment #35)
> > Comment on attachment 8410585 [details] [diff] [review]
> > Part 6: Implement SCREEN and MULTIPLY for CompositorD3D11.cpp
> > 
> > Review of attachment 8410585 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/d3d11/CompositorD3D11.cpp
> > @@ +58,5 @@
> > >    RefPtr<ID3D11BlendState> mPremulBlendState;
> > >    RefPtr<ID3D11BlendState> mNonPremulBlendState;
> > >    RefPtr<ID3D11BlendState> mComponentBlendState;
> > >    RefPtr<ID3D11BlendState> mDisabledBlendState;
> > > +  RefPtr<ID3D11BlendState> mPremulScreenBlendState;
> > 
> > If we're expecting to get more of these I wonder if we should convert this
> > to an array of ID3D11BlendStates indexed by CompositionOp. (And create them
> > on demand, see gfx/2d/DrawTargetD2D for an example)) That would clean this
> > code up a lot. 
> 
> I don't think we'll get any more. The other blend modes have to be
> implemented with shaders and will use either a normal source-over blendstate
> or a source blendstate.

We don't need anything like ADD or OUT or anything like that?

> > Let's do that.
> > 
> > @@ +594,5 @@
> > > +    case gfx::CompositionOp::OP_SCREEN:
> > > +      mContext->OMSetBlendState(aIsPremultiplied ? mAttachments->mPremulScreenBlendState : mAttachments->mScreenBlendState, sBlendFactor, 0xFFFFFFFF);
> > > +      break;
> > > +    case gfx::CompositionOp::OP_MULTIPLY:
> > > +      // If the source data was un-premultiplied we should have already
> > 
> > Hrm, that's a little tricky, that would require a lot more shaders. I need
> > to think about it.
> 
> For the shaders in D2D, I made a shader program that took parameters to
> choose what blend operation to perform. It seems we can do the same here.

That would slow down all compositions here, tricky.

Comment 38

[Rik Cabanier]

(In reply to Bas Schouten (:bas.schouten) from comment #37)
> (In reply to Rik Cabanier from comment #36)
> > (In reply to Bas Schouten (:bas.schouten) from comment #35)
> > > Comment on attachment 8410585 [details] [diff] [review]
> > > Part 6: Implement SCREEN and MULTIPLY for CompositorD3D11.cpp
> > > 
> > > Review of attachment 8410585 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: gfx/layers/d3d11/CompositorD3D11.cpp
> > > @@ +58,5 @@
> > > >    RefPtr<ID3D11BlendState> mPremulBlendState;
> > > >    RefPtr<ID3D11BlendState> mNonPremulBlendState;
> > > >    RefPtr<ID3D11BlendState> mComponentBlendState;
> > > >    RefPtr<ID3D11BlendState> mDisabledBlendState;
> > > > +  RefPtr<ID3D11BlendState> mPremulScreenBlendState;
> > > 
> > > If we're expecting to get more of these I wonder if we should convert this
> > > to an array of ID3D11BlendStates indexed by CompositionOp. (And create them
> > > on demand, see gfx/2d/DrawTargetD2D for an example)) That would clean this
> > > code up a lot. 
> > 
> > I don't think we'll get any more. The other blend modes have to be
> > implemented with shaders and will use either a normal source-over blendstate
> > or a source blendstate.
> 
> We don't need anything like ADD or OUT or anything like that?

No, not for this level of the CSS blending spec. It's TBD if (and how) this will be added in the future.
 
> > > Let's do that.
> > > 
> > > @@ +594,5 @@
> > > > +    case gfx::CompositionOp::OP_SCREEN:
> > > > +      mContext->OMSetBlendState(aIsPremultiplied ? mAttachments->mPremulScreenBlendState : mAttachments->mScreenBlendState, sBlendFactor, 0xFFFFFFFF);
> > > > +      break;
> > > > +    case gfx::CompositionOp::OP_MULTIPLY:
> > > > +      // If the source data was un-premultiplied we should have already
> > > 
> > > Hrm, that's a little tricky, that would require a lot more shaders. I need
> > > to think about it.
> > 
> > For the shaders in D2D, I made a shader program that took parameters to
> > choose what blend operation to perform. It seems we can do the same here.
> 
> That would slow down all compositions here, tricky.

Not sure why that would be. The shader would only run if blending was requested.

Comment 39

[Rik Cabanier]

Matt, do you think you will have time to wire up support for shaders to the OpenGL port soon?
Once that it is in, I can write the shaders.

If you think it will be a while (> 2 weeks), I might request to ship blending without compositor support.

Comment 40

[Matt Woodrow (:mattwoodrow)]

(In reply to Rik Cabanier from comment #39)
> Matt, do you think you will have time to wire up support for shaders to the
> OpenGL port soon?
> Once that it is in, I can write the shaders.
> 
> If you think it will be a while (> 2 weeks), I might request to ship
> blending without compositor support.

I have a WIP for DARKEN/LIGHTEN, and that handles most of the hard work for getting the shaders set up. The remaining work is just writing the GLSL for the other blend modes.

Getting the tests for mix-blend-mode added would be really useful, I don't have anything to test against currently.

However I don't think we should block on compositor support. Go ahead with shipping, and any wins we get from this bug can be a bonus.

Comment 41

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP: Implement LIGHTEN/DARKEN for OGL

Haven't tested this at all, but it compiles locally :)

Comment 42

[Rik Cabanier]

(In reply to Matt Woodrow (:mattwoodrow) from comment #40)
> (In reply to Rik Cabanier from comment #39)
> > Matt, do you think you will have time to wire up support for shaders to the
> > OpenGL port soon?
> > Once that it is in, I can write the shaders.
> > 
> > If you think it will be a while (> 2 weeks), I might request to ship
> > blending without compositor support.
> 
> I have a WIP for DARKEN/LIGHTEN, and that handles most of the hard work for
> getting the shaders set up. The remaining work is just writing the GLSL for
> the other blend modes.
> 
> Getting the tests for mix-blend-mode added would be really useful, I don't
> have anything to test against currently.

I have a whole bunch of ref tests that compare the various blend modes and different stacking contexts but because of small rendering differences, I can't get them to pass the try bots.
fuzzy(x,y) doesn't seem to work for x.

Comment 43

[Rik Cabanier]

(In reply to Matt Woodrow (:mattwoodrow) from comment #41)
> Created attachment 8412329 [details] [diff] [review]
> WIP: Implement LIGHTEN/DARKEN for OGL
> 
> Haven't tested this at all, but it compiles locally :)

woot! I will try it out asap.

Comment 44

[Rik Cabanier]

(In reply to Rik Cabanier from comment #43)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #41)
> > Created attachment 8412329 [details] [diff] [review]
> > WIP: Implement LIGHTEN/DARKEN for OGL
> > 
> > Haven't tested this at all, but it compiles locally :)
> 
> woot! I will try it out asap.

Is there a patch missing? It's not applying for me.

Comment 45

[Matt Woodrow (:mattwoodrow)]

(In reply to Rik Cabanier from comment #44)
> 
> Is there a patch missing? It's not applying for me.

I made a bunch of other local changes to address review comments. It should be easy to fix up?

I'll get the updated patches up soon.

Comment 46

[Rik Cabanier]

(In reply to Matt Woodrow (:mattwoodrow) from comment #45)
> (In reply to Rik Cabanier from comment #44)
> > 
> > Is there a patch missing? It's not applying for me.
> 
> I made a bunch of other local changes to address review comments. It should
> be easy to fix up?

some code around line 1000 is missing in CompositorOGL.cpp so I can't get it to compile. I'll wait until you redo the patches.

Comment 47

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Add Effect for BlendModes

Comment 48

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Create active layers for nsDisplayMixBlendMode and nsDisplayBlendContainer if the layer manager supports all contained blend modes v2

Comment 49

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Share code for computing layers component alpha support

The client and composite layer managers *need* to be on the same page for this calculation, so we should put the code in one place.

I think this also fixes a potential bug in ContainerLayerComposite where we weren't checking gfxPrefs::ComponentAlphaEnabled() for the very last condition.

Comment 50

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 7: Disable component alpha for layers with a mix-blend-mode

We can't support mix-blend-mode and component alpha on the same layer (and forcing an intermediate wouldn't help since it still wouldn't have an opaque background, nor could we copy up), so disable it.

Try push for parts 1-7: https://tbpl.mozilla.org/?tree=Try&rev=950a8afb343d

Comment 51

[Matt Woodrow (:mattwoodrow)]

This bug is starting to get overwhelming. I'll probably stop here and file new bugs for the remaining blend modes in CompositorOGL.

Comment 52

[Rik Cabanier]

(In reply to Matt Woodrow (:mattwoodrow) from comment #51)
> This bug is starting to get overwhelming. I'll probably stop here and file
> new bugs for the remaining blend modes in CompositorOGL.

I agree! 
Your patches didn't apply for me, but I was able to fix them locally.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8413617 [details] [diff] [review]
Part 6: Share code for computing layers component alpha support

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

::: gfx/layers/Layers.cpp
@@ +1009,5 @@
> +  bool needsSurfaceCopy = false;
> +  if (UseIntermediateSurface()) {
> +    if (GetEffectiveVisibleRegion().GetNumRects() == 1 &&
> +        (GetContentFlags() & Layer::CONTENT_OPAQUE))
> +    {

{ on previous line

@@ +1033,5 @@
> +    if (mSupportsComponentAlphaChildren && needsSurfaceCopy) {
> +      *aNeedsSurfaceCopy = true;
> +    } else {
> +      *aNeedsSurfaceCopy = false;
> +    }

*aNeedsSurfaceCopy = mSupportsComponentAlphaChildren && needsSurfaceCopy;

Comment 54

[Rik Cabanier]

Matt, are you going to land these patches? (I would do it but it seems that they're still different from your passing try run)

Comment 55

[Matt Woodrow (:mattwoodrow)]

Still waiting on a review from Bas

Comment 56

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d694a8a18c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/43b7bdb56dac
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9f8d5ba0cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b4665ce6b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f10f329a013
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a5699a9f66
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae862914302

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d694a8a18c8
https://hg.mozilla.org/mozilla-central/rev/43b7bdb56dac
https://hg.mozilla.org/mozilla-central/rev/ad9f8d5ba0cf
https://hg.mozilla.org/mozilla-central/rev/75b4665ce6b0
https://hg.mozilla.org/mozilla-central/rev/6f10f329a013
https://hg.mozilla.org/mozilla-central/rev/59a5699a9f66
https://hg.mozilla.org/mozilla-central/rev/dae862914302

Comment 58

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF32.0b8
Build Id:20140818191513
OS:Mac Os X 1.8.5