Closed Bug 1238496 Opened 4 years ago Closed 4 years ago

Add full mix-blend mode support to the D3D11 compositor

Categories

(Core :: Graphics: Layers, defect)

46 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 + fixed
firefox47 + fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

No description provided.
These were used for earlier mix-blend-mode support but didn't get removed when that got backed out.

Leaving new genshaders.sh output out to avoid a huge diff.
Attachment #8710511 - Flags: review?(bas)
Attached patch part 2, mix-blend support (obsolete) — Splinter Review
This patch introduces full mix-blend mode support for the D3D11 compositor. There are three new vertex shaders, one for each mask type, that use a common blend output struct.

There is also one new pixel shader that takes in four configuration values: the layer type (rgb, rgba, ycbcr, color), the mask type, the blend operation, and whether or not the backdrop is premultiplied. This seemed like the best solution given the difficulty of configuring HLSL at runtime.

Otherwise this is the same as the OpenGL patch in bug 1235995 - we copy the current render target where it intersects with the layer quad, and sample from this according to the mix-blend spec[1].

Unfortunately we use ps_4_0_level_9_3, so we're limited to Pixel Shader Model 2.x, which doesn't support much control flow. Most of the branching is flattened. The blend shader uses ~350 instruction slots, and I'm not sure whether that's likely or not to cause problems. (On the other hand, level_9_3 is only 1% of our users, so we could consider dropping it.)

[1] https://www.w3.org/TR/compositing-1/
Attachment #8710520 - Flags: review?(bas)
Attachment #8710511 - Flags: review?(bas) → review+
Comment on attachment 8710520 [details] [diff] [review]
part 2, mix-blend support

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +414,5 @@
>        RefPtr<CompositingRenderTarget> surface = nullptr;
>  
>        RefPtr<CompositingRenderTarget>& lastSurf = aContainer->mLastIntermediateSurface;
>        if (lastSurf && !aContainer->mChildrenChanged && lastSurf->GetRect().IsEqualEdges(surfaceRect)) {
> +        //surface = lastSurf;

testing?
Sorry, accidentally attached an older patch.
Attachment #8710520 - Attachment is obsolete: true
Attachment #8710520 - Flags: review?(bas)
Attachment #8710598 - Flags: review?(bas)
Comment on attachment 8710598 [details] [diff] [review]
part 2, mix-blend support

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

::: gfx/layers/d3d11/BlendingHelpers.hlslh
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Helper functions.
> +float hardlight(float dest, float src) {

Have you considered making some of these take float3 or float4 vector arguments? I'm not convinced the compiler will optimize this properly right now. But maybe I'm wrong, should be easy to check by looking at the generated shader code.

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +531,5 @@
>    }
>  
>    RefPtr<ID3D11Texture2D> texture;
>    HRESULT hr = mDevice->CreateTexture2D(&desc, nullptr, getter_AddRefs(texture));
> +

Is there a reason this assertion got removed?

@@ +954,5 @@
> +    // If the blend operation needs to read from the backdrop, copy the
> +    // current render target into a new texture and bind it now.
> +    if (BlendOpIsMixBlendMode(blendMode)) {
> +      gfx::Matrix4x4 backdropTransform;
> +      gfx::IntRect rect = ComputeBackdropCopyRect(aRect, aClipRect, aTransform, &backdropTransform);

In a lot of cases CopySubresourceRegion should suffice here. If we ever care about performance here we should probably optimize for that.
Attachment #8710598 - Flags: review?(bas) → review+
(In reply to Bas Schouten (:bas.schouten) from comment #5)
> ::: gfx/layers/d3d11/BlendingHelpers.hlslh
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +// Helper functions.
> > +float hardlight(float dest, float src) {
> 
> Have you considered making some of these take float3 or float4 vector
> arguments? I'm not convinced the compiler will optimize this properly right
> now. But maybe I'm wrong, should be easy to check by looking at the
> generated shader code.

I just tried this for a few functions and the generated code looked identical. Maybe it's smart enough to see the fourth channel is never used - the docs say float3 is a float4 internally with the last component ignored.

> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +531,5 @@
> >    }
> >  
> >    RefPtr<ID3D11Texture2D> texture;
> >    HRESULT hr = mDevice->CreateTexture2D(&desc, nullptr, getter_AddRefs(texture));
> > +
> 
> Is there a reason this assertion got removed?

Yeah, it looked subsumed by the gfxCriticalNote that follows a few lines down. (If that's not the case I can add it back.)

> @@ +954,5 @@
> > +    // If the blend operation needs to read from the backdrop, copy the
> > +    // current render target into a new texture and bind it now.
> > +    if (BlendOpIsMixBlendMode(blendMode)) {
> > +      gfx::Matrix4x4 backdropTransform;
> > +      gfx::IntRect rect = ComputeBackdropCopyRect(aRect, aClipRect, aTransform, &backdropTransform);
> 
> In a lot of cases CopySubresourceRegion should suffice here. If we ever care
> about performance here we should probably optimize for that.

It should be going into CopySubsourceRegion a few calls down. Definitely copying the entire texture was way too slow (janked more than basic layers).
https://hg.mozilla.org/mozilla-central/rev/7756089b8570
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1246388
Comment on attachment 8710598 [details] [diff] [review]
part 2, mix-blend support

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 Direct3D 11 (Windows Vista+) 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]:

(Note: this and part 1 were landed as one cset since it was just dead code removal.)
Attachment #8710598 - Flags: approval-mozilla-aurora?
Marking affected and tracking since this is a regression if we have e10s and apz enabled, and we plan some staged rollout during 46
Comment on attachment 8710598 [details] [diff] [review]
part 2, mix-blend support

Regression fixes for APZ, please uplift to aurora. Tested on nightly.
Attachment #8710598 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm hitting conflicts applying this to aurora.
Flags: needinfo?(dvander)
Version: unspecified → 46 Branch
You need to log in before you can comment on or make changes to this bug.