1235995 - Add full mix-blend mode support to the OpenGL compositor

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: wip, v1

Not quite right yet, I think the backdrop sampling coordinates are bogus.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: wip v2

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

This patch implements all the remaining mix-blend modes for OpenGL, as per the spec[1]. The shader functions are the 'blend' step, and assume no premultiplied alpha (we un-premultiply and re-apply before and after if necessary). The composite step is handled by the default OP_OVER blend state.

The shader functions sample a copy of the current framebuffer to read the backdrop. This is unfortunately necessary since reading from the current output framebuffer is undefined in OpenGL. Note the copy is a full copy, I didn't make any attempt to bound it. If we want it minimized I can do that as a separate patch.

[1] https://drafts.fxtf.org/compositing/

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

This passes the css-blending reftests but I haven't done a try run yet.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8705574 [details] [diff] [review]
patch

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

::: gfx/2d/Types.h
@@ +205,5 @@
> +static inline bool
> +BlendOpReadsFromDest(CompositionOp aOp)
> +{
> +  switch (aOp) {
> +  case CompositionOp::OP_MULTIPLY:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style wants the case labels to be indented, too. Feel free to ignore this, but if you decide to follow it, please also do it for the other two switches you add in this patch.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1081,5 @@
>      blendMode = blendEffect->mBlendMode;
> +    if (BlendOpReadsFromDest(blendMode)) {
> +      gfx::IntRect rect(gfx::IntPoint(0, 0), mCurrentRenderTarget->GetSize());
> +
> +      CreateFBOWithTexture(rect, true, mCurrentRenderTarget->GetFBO(), nullptr, &mixBlendBackdrop);

It feels a little dirty to abuse a "CreateFBO" function when we don't even need an FBO, but I don't really see a good alternative.

@@ +1420,5 @@
>      gl()->fBlendFuncSeparate(LOCAL_GL_ONE, LOCAL_GL_ONE_MINUS_SRC_ALPHA,
>                               LOCAL_GL_ONE, LOCAL_GL_ONE_MINUS_SRC_ALPHA);
>    }
> +  if (mixBlendBackdrop) {
> +    gl()->fDeleteTextures(1, &mixBlendBackdrop);

I'm not too thrilled about one texture deletion per blending render call, but we can still work on performance after this is landed. Anything is better performance-wise than no blend mode support in the compositor.

::: gfx/layers/opengl/CompositorOGL.h
@@ +206,5 @@
>                                 GetMaxTextureSize(),
>                                 mFBOTextureTarget == LOCAL_GL_TEXTURE_2D,
>                                 SupportsPartialTextureUpdate());
>      result.mSupportedBlendModes += gfx::CompositionOp::OP_SOURCE;
> +    // All composition ops are supported in software.

remove "in software"

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ -459,5 @@
>      if (aConfig.mFeatures & ENABLE_OPACITY) {
>        fs << "  color *= uLayerOpacity;" << endl;
>      }
> -    if (aConfig.mFeatures & ENABLE_PREMULTIPLY) {
> -      fs << " color.rgb *= color.a;" << endl;

Oh, ENABLE_PREMULTIPLY was completely unused? Ok.

@@ +503,5 @@
>    }
> +  if (BlendOpReadsFromDest(blendOp)) {
> +    fs << "  vec4 backdrop = texture2D(uBackdropTexture, vBackdropCoord);" << endl;
> +
> +    // The spec assumes there is no premultiplied alpha. If the source texture

This comment was a little confusing to me, can you make it clearer? ENABLE_NO_PREMUL_ALPHA only describes the premultiplication of "color", i.e. the output from this shader. It does not describe whether backdrop is premultiplied or not; backdrop is always premultiplied.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Markus Stange [:mstange] from comment #5)
> It feels a little dirty to abuse a "CreateFBO" function when we don't even
> need an FBO, but I don't really see a good alternative.

I'll do a small follow-up patch to split this apart.

> @@ +503,5 @@
> >    }
> > +  if (BlendOpReadsFromDest(blendOp)) {
> > +    fs << "  vec4 backdrop = texture2D(uBackdropTexture, vBackdropCoord);" << endl;
> > +
> > +    // The spec assumes there is no premultiplied alpha. If the source texture
> 
> This comment was a little confusing to me, can you make it clearer?
> ENABLE_NO_PREMUL_ALPHA only describes the premultiplication of "color", i.e.
> the output from this shader. It does not describe whether backdrop is
> premultiplied or not; backdrop is always premultiplied.

Thanks, this was bogus. Fixed.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

The previous version didn't work with opacity in the backdrop, since it failed to take into account the mix step after blending.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8706101 [details] [diff] [review]
v2

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

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +752,5 @@
> +  if (!(aConfig.mFeatures & ENABLE_NO_PREMUL_ALPHA)) {
> +    fs << "  color.rgb /= color.a;" << endl;
> +  }
> +  fs << "  vec3 blended = blend(backdrop.rgb, color.rgb);" << endl;
> +  fs << "  color.rgb = (1.0 - backdrop.a) * color.rgb + backdrop.a * blended.rgb;" << endl;

Maybe this is obvious once I've read up on blending again, but it's not obvious to me right now why this is correct. You're mixing the two colors "color" and "blended", and you're using the backdrop's alpha as the mixing factor?
And in the next line, the result gets "color"'s original alpha applied to it. Can't we instead compute an opaque result color that already contains the blending with the backdrop? I think that would be easier to reason about. Then we'd just have one blending step, completely inside the shader.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Markus Stange [:mstange] from comment #8)
> Comment on attachment 8706101 [details] [diff] [review]
> v2
> 
> Review of attachment 8706101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/OGLShaderProgram.cpp
> @@ +752,5 @@
> > +  if (!(aConfig.mFeatures & ENABLE_NO_PREMUL_ALPHA)) {
> > +    fs << "  color.rgb /= color.a;" << endl;
> > +  }
> > +  fs << "  vec3 blended = blend(backdrop.rgb, color.rgb);" << endl;
> > +  fs << "  color.rgb = (1.0 - backdrop.a) * color.rgb + backdrop.a * blended.rgb;" << endl;
> 
> Maybe this is obvious once I've read up on blending again, but it's not
> obvious to me right now why this is correct. You're mixing the two colors
> "color" and "blended", and you're using the backdrop's alpha as the mixing
> factor?

Yeah, that is under "The result of the mixing function is modulated by the backdrop alpha" [1].

> And in the next line, the result gets "color"'s original alpha applied to
> it.

This is to regain premultiplied alpha for the OP_OVER parameters to glBlendFuncSeparate. AFAICT mix-blending doesn't change the alpha channel.

> Can't we instead compute an opaque result color that already contains
> the blending with the backdrop? I think that would be easier to reason
> about. Then we'd just have one blending step, completely inside the shader.

I tried to make it exactly like the spec to avoid spending time doing formula substitution, and to make it easier to follow against the spec. So I was hoping to leave collapsing everything as future performance work. But if you think it's worth doing now, I don't think it would take too long. What do you think?

[1] http://www.w3.org/TR/compositing-1/#blending

Comment 10

[Markus Stange [:mstange]]

Having it exactly like the spec is good. I'll read the section later.

I tested this patch on https://stripe.com/relay and it looks like the arrows in the blue section are too dark with your patch. Can you look into that?

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Markus Stange [:mstange] from comment #10)
> Having it exactly like the spec is good. I'll read the section later.
> 
> I tested this patch on https://stripe.com/relay and it looks like the arrows
> in the blue section are too dark with your patch. Can you look into that?

Good catch - this was a bug in "softlight", I had "(D(Cb) - Cs)" and it should have been "(D(Cb) - Cb)". Fixed locally.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v3

w/ softlight fix

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8706833 [details] [diff] [review]
v3

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

I went through the patch and checked all the math, and it looks good, except for the 1.1 thing.

The spec is quite clear that there is another OP_OVER composition step after the mixBlend step, so my reservations were unfounded.

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +605,5 @@
> +  case gfx::CompositionOp::OP_SATURATION:
> +  case gfx::CompositionOp::OP_COLOR:
> +  case gfx::CompositionOp::OP_LUMINOSITY:
> +    fs << "float Lum(vec3 c) {" << endl;
> +    fs << "  return 0.3 * c.r + 0.59 * c.g + 0.11 * c.b;" << endl;

It might be more efficient to write this as
  return dot(vec3(0.3, 0.59, 0.11), c);

@@ +614,5 @@
> +    fs << "  float x = max(max(c.r, c.g), c.b);" << endl;
> +    fs << "  if (n < 0.0) {" << endl;
> +    fs << "    c = L + (((c - L) * L) / (L - n));" << endl;
> +    fs << "  }" << endl;
> +    fs << "  if (x > 1.1) {" << endl;

Why 1.1 and not 1.0? Typo?

@@ +641,5 @@
> +    fs << "  }" << endl;
> +    fs << "  return vec3(0.0, c.gb);" << endl;
> +    fs << "}" << endl;
> +
> +    fs << "vec3 SetSat(vec3 c, float s) {" << endl;

I've tried to come up with something that looks better than what you have here but I was unsuccessful.

::: layout/reftests/css-blending/mix-blend-mode-soft-light.html
@@ +10,5 @@
> +    .child {
> +      width: 200px;
> +      height: 200px;
> +      mix-blend-mode: soft-light;
> +      opacity: 0.5;

Maybe set will-change:opacity here? As is, I don't think this test exercises blending in the compositor.

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8706833 [details] [diff] [review]
v3

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

::: gfx/2d/Types.h
@@ +201,5 @@
>    OP_COUNT
>  };
>  
> +static inline bool
> +BlendOpReadsFromDest(CompositionOp aOp)

Now that I read this again, I feel like this is not a good name or a good place for this function. You could say that every composition operator reads from the destination (maybe with the exception of OP_SOURCE). What we have here is the list of blend modes that cannot be expressed using glBlendFuncSeparate, so it's something that's very specific to CompositorOGL. I think we should move this function into CompositorOGL.h or similar.

Comment 15

[Markus Stange [:mstange]]

I just found bug 1239304, which makes me wonder if you'll get hit testing tree assertions on try with this patch.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8706833 [details] [diff] [review]
v3

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

::: gfx/layers/opengl/OGLShaderProgram.h
@@ +230,4 @@
>  
>    bool operator< (const ShaderConfigOGL& other) const {
> +    return mFeatures < other.mFeatures ||
> +           (int)mCompositionOp < (int)other.mCompositionOp;

This is a bad operator<. It doesn't define a total order and breaks the std::map that we use for caching shaders. This causes us to recompile shaders on every draw in some cases.
I'd use a lexicographic order like this:
    return mFeatures < other.mFeatures ||
           (mFeatures == other.mFeatures &&
            (int)mCompositionOp < (int)other.mCompositionOp);

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, v4

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, clip copy rect

Copying the full backdrop is super slow on my laptop's GPU, when using OGL on battery or D3D11. My test case with all mix-blend modes is slower to scroll than with basic layers. So we might as well try to minimize the damage now.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8708965 [details] [diff] [review]
part 2, clip copy rect

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

Mostly looks good, just worried about the offset adjustment.

I know you're looking into rendering bugs that are probably related, will wait to see what happens there.

::: gfx/layers/Compositor.cpp
@@ +366,5 @@
> +  gfx::Rect renderBound = mRenderBound;
> +
> +  // Compute the clip.
> +  renderBound.IntersectRect(renderBound, aClipRect);
> +  renderBound.MoveBy(offset);

I think we want to apply the negative of offset to the final result instead of adding it here.

If we know that drawing to a surface will only affect the rectangle (x,y,w,h) (using visible rects), then we allocate a surface of (w,h) and set the origin as (x,y) to save memory.

All of the aRect/aClipRect/aTransform calculations don't know about (or need to know about) this adjustment.

Right at the end we want to adjust our final result to be relative to (x,y) instead of (0,0), so we want to subtract the offset to get into the coordinate space of the actual RT instead of the coordinate space of the logical destination.

::: gfx/layers/Compositor.h
@@ +546,5 @@
>  
>    RefPtr<gfx::DrawTarget> mTarget;
>    gfx::IntRect mTargetBounds;
>  
> +  gfx::Rect mRenderBound;

mRenderBounds

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1112,5 @@
> +    // Create a transform from adjusted clip space to render target space,
> +    // translate it for the backdrop rect, then transform it into the backdrop's
> +    // uv-space.
> +    gfx::Matrix4x4 transform;
> +    transform.PreScale(mRenderBound.width, mRenderBound.height, 1.0);

Could make this PostScale for consistency

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +288,5 @@
>    }
>    vs << "  finalPosition.xy -= uRenderTargetOffset * finalPosition.w;" << endl;
>    vs << "  finalPosition = uMatrixProj * finalPosition;" << endl;
>    if (BlendOpIsMixBlendMode(blendOp)) {
> +    // Translate from clip space to (0..1), apply the backdrop transform, then

Mention that clip space is (-1,1)

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, clip copy rect

w/ comments addressed (that indeed was the rendering bug)

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8303030d9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/77ce3012d481

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b8303030d9c
https://hg.mozilla.org/mozilla-central/rev/77ce3012d481

Comment 23

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8708960 [details] [diff] [review]
part 1, v4

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: Severe performance regression in pages using CSS mix-blend modes.
[Describe test coverage new/current, TreeHerder]: Nightly for 4 weeks.
[Risks and why]: When using E10S and APZ on Firefox 46, certain websites (stripe.com, bhphotovideo.com) have severely degraded scrolling performance since we did not support asynchronous CSS mix-blending. This request is for the OpenGL (Mac) platform fix. Risk is low; bugs have been sussed out on Nightly and those fixes would be uplifted as well. Correctness issues are more likely than stability issues and we haven't seen indications of either.
[String/UUID change made/needed]:

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8709252 [details] [diff] [review]
part 2, clip copy rect

Approval Request Comment

See comment for previous patch.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

This is marked as fixed in 46 already. Is this really in 46 or did it land in 47 right on the edge of the merge and release? If you could double check for me, it would be a big help, thanks!

Comment 26

[David Anderson [:dvander] - inactive, e-mail if emergency]

This does appear to be on Aurora already, thanks for checking!

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

One more question - should this go into a release note? Can you suggest wording and a link for it?

Comment 28

[David Anderson [:dvander] - inactive, e-mail if emergency]

That might be a good idea. "Improved performance of CSS mix-blend compositing." works. What kind of link are you looking for?

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

I wondered if this needed dev docs, something on MDN. Maybe not if it's just about improved performance.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Though, when we make claims about improved performance it's nice to have a blog post or explanation or proof to point to for the specifics.

Comment 31

[David Anderson [:dvander] - inactive, e-mail if emergency]

Feel free to take it out. It's not worth doing performance measurements on this.