Closed Bug 1243071 Opened 4 years ago Closed 4 years ago

Add full mix-blend mode support to the D3D9 compositor

Categories

(Core :: Graphics: Layers, defect)

46 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox46 + fixed
firefox47 + verified

People

(Reporter: dvander, Assigned: dvander)

References

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

D3D9 is proving the most tricky since pixel shaders are strictly limited to 64 instruction slots. The combined mega-shader in the D3D11 patch uses around ~440, 53 just for determining how to get the source pixels.

I'll try to split it into a few smaller shaders.
Attached patch part 1, SetShaderMode cleanup (obsolete) — Splinter Review
This is a small cleanup to SetShaderMode to make it easier to configure the pipeline for mix-blending:

1. SetShaderMode now returns the number of base textures that'll be bound, so we can compute the mask and backdrop slots separately in DrawQuad().
2. I copied PixelShaderArray/VertexShaderArray from CompositorD3D11.
Attachment #8712276 - Flags: review?(bas)
Comment on attachment 8712276 [details] [diff] [review]
part 1, SetShaderMode cleanup

The approach of splitting up shaders into groups isn't working out well, they're still too big for PS Model 2.0. Going to take a different approach.
Attachment #8712276 - Flags: review?(bas)
Attached patch WIP v1 (obsolete) — Splinter Review
This approach is working better - instead of doing everything in one shader, we split it into four steps: copy backdrop, render effect chain, copy new backdrop as source, then blend. This only requires one new vertex shader and 15 new pixel shaders (one for each blend mode).

Unfortunately, there are still two shaders that overflow the ps_2_0 limit (hue and saturation). Since I'm not particularly concerned about D3D9 performance here I'll probably have a software path for those.
Attachment #8712276 - Attachment is obsolete: true
This patch implements mix-blending in D3D9 entirely in software. It's reasonably fast on the worst pages I've seen, and passes reftests. It requires five steps:

(1) Copy the backdrop into a new texture.
(2) Render the effect chain with OP_SOURCE as the blend op.
(3) Copy the backdrop again - that contains our new source pixels.
(4) Readback both textures and do the blend with Cairo.
(5) StretchRect the result back onto the rendertarget.

I have a follow-up patch to do shaders for all but two of the blend ops (which use too many instructions for ps_2_0 unfortunately), but it's not passing tests yet.
Attachment #8712532 - Attachment is obsolete: true
Attachment #8712902 - Flags: review?(bas)
Comment on attachment 8712902 [details] [diff] [review]
part 3, software support

Matt, does this ComputeBackdropCopyRect change look okay?
Attachment #8712902 - Flags: review?(matt.woodrow)
Comment on attachment 8712902 [details] [diff] [review]
part 3, software support

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

::: gfx/layers/Compositor.cpp
@@ +374,5 @@
>    gfx::Rect dest = aTransform.TransformAndClipBounds(aRect, renderBounds);
>    dest -= offset;
>  
> +  // Ensure we don't round out to -1, which trips up Direct3D.
> +  dest.IntersectRect(dest, mRenderBounds);

I don't think this is correct, and I doubt the usage of mRenderBounds above is either.

mRenderBounds is a constant for a given layer tree, and is the size of the final window we're compositing to.

For this function we only care about the size of the current destination, which could be an intermediate surface (created for a ContainerLayer), and could be different sized.

The intersection we want would be (0,0,GetCurrentRenderTarget()->GetSize().width,GetCurrentRenderTarget()->GetSize().height) I believe.

CompositorOGL::DrawQuad seems to use mRenderBounds too, and I'm confused as to how that generates correct results.
Comment on attachment 8712902 [details] [diff] [review]
part 3, software support

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

::: gfx/layers/Compositor.cpp
@@ +374,5 @@
>    gfx::Rect dest = aTransform.TransformAndClipBounds(aRect, renderBounds);
>    dest -= offset;
>  
> +  // Ensure we don't round out to -1, which trips up Direct3D.
> +  dest.IntersectRect(dest, mRenderBounds);

Sorry, I got confused with the mRenderBounds member on LayerManagerComposite.

This should be correct, but it'd be clearer (and simpler) to just use GetCurrentRenderTarget() as the source of the size, and get rid of mRenderBounds.
Separate patch to remove mRenderBounds.
Attachment #8712985 - Flags: review?(matt.woodrow)
Attachment #8712902 - Attachment description: part 1, software support → part 2, software support
Attachment #8712902 - Flags: review?(matt.woodrow)
Attachment #8712985 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8712902 [details] [diff] [review]
part 3, software support

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

::: gfx/layers/d3d9/CompositorD3D9.cpp
@@ +314,5 @@
> +    blendMode = blendEffect->mBlendMode;
> +
> +    // Pixel Shader Model 2.0 is too limited to perform blending in the same way
> +    // as Direct3D 11 - there are too many instructions, and we don't have
> +    // configurable shaders (as we do with OGL) that would avoid a huge shader

There's probably some things that could be done here.. but I really doubt it'd be worth it.

@@ +886,5 @@
> + public:
> +  AutoSurfaceLock(IDirect3DSurface9* aSurface, DWORD aFlags = 0) {
> +    HRESULT hr = aSurface->LockRect(&mRect, nullptr, aFlags);
> +    if (FAILED(hr)) {
> +      gfxCriticalNote << "Failed to lock surface rect " << hexa(hr);

Important: Insert an mSurface = nullptr; here -and- a mRect.bits = nullptr; to prevent release builds from being allowed to read/write from/to random memory.

@@ +1007,5 @@
> +    aBackdropRect.y,
> +    aBackdropRect.XMost(),
> +    aBackdropRect.YMost()
> +  };
> +  HRESULT hr = d3d9Device->StretchRect(

nit: I'd prefer declaring hr once at the top of this function, little more vertical whitespace in this bit would make it more readable as well IMHO.
Attachment #8712902 - Flags: review?(bas) → review+
Comment on attachment 8712902 [details] [diff] [review]
part 3, software support

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

::: gfx/layers/d3d9/CompositorD3D9.cpp
@@ +886,5 @@
> + public:
> +  AutoSurfaceLock(IDirect3DSurface9* aSurface, DWORD aFlags = 0) {
> +    HRESULT hr = aSurface->LockRect(&mRect, nullptr, aFlags);
> +    if (FAILED(hr)) {
> +      gfxCriticalNote << "Failed to lock surface rect " << hexa(hr);

Never mind, I noticed you set mSurface after this. Still please make sure you null out mRect.pBits just in case.
Since D3D9 is placing the final blended pixels with StretchRect, it's very sensitive to rounding issues on the transformed layer quad. I'll do another patch to address that, but just using more precision in ComputeBackdropCopyRect fixes a lot of the reftest failures as well.
Attachment #8713411 - Flags: review?(matt.woodrow)
Attachment #8712902 - Attachment description: part 2, software support → part 3, software support
The current patch takes the backdrop rect and blends the entire thing. This isn't necessarily correct if it has been rounded out and the actual transformed layer quad is not pixel-aligned.

This patch fixes that by returning the transformed destination in addition to the backdrop rect, so we can blend the correct region.
Attachment #8713417 - Flags: review?(matt.woodrow)
Attachment #8713411 - Flags: review?(matt.woodrow) → review+
This patch adds hardware support for all but OP_HUE and OP_SATURATION. Unfortunately it fails about six blending tests, most are failing because we're blending an extra column of pixels to the left of the source texture. It doesn't seem that important to pursue, so I'm just leaving it here for posterity.
Attachment #8713417 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/975654b9b3d8
https://hg.mozilla.org/mozilla-central/rev/53ddcd8fee26
https://hg.mozilla.org/mozilla-central/rev/94d382292260
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8712985 [details] [diff] [review]
part 1, remove mRenderBounds

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 9 (Windows XP) 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]:
Attachment #8712985 - Flags: approval-mozilla-aurora?
Comment on attachment 8712902 [details] [diff] [review]
part 3, software support

Approval Request Comment

See previous comment.
Attachment #8712902 - Flags: approval-mozilla-aurora?
Comment on attachment 8713411 [details] [diff] [review]
part 2, use more precision

Approval Request Comment

See previous comment.
Attachment #8713411 - Flags: approval-mozilla-aurora?
Comment on attachment 8713417 [details] [diff] [review]
part 3.1, handle subpixel offsets

Approval Request Comment

See previous comment.
Attachment #8713417 - Flags: approval-mozilla-aurora?
Tracking, marking affected for 46 since it's an apz related regression.
Comment on attachment 8712902 [details] [diff] [review]
part 3, software support

This is a lot to uplift, but I'd like to try it in aurora rather than deal with this in mid-beta to support APZ and e10s staged rollouts.  David can you keep an eye out for regressions? How can we best test this fix?
Attachment #8712902 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Attachment #8712985 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8713411 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8713417 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm hitting conflicts applying this to aurora.
Flags: needinfo?(dvander)
David can you try rebasing this for beta? Do we want this for the apz experiment in beta 46 (which hasn't started yet)
Kats do you think we need this? 
Milan do you have an opinion here? This never did land in aurora.
Flags: needinfo?(milan)
Flags: needinfo?(bugmail.mozilla)
I didn't reproduce the "severe scrolling performance" on two Win XP 32-bit machines using Nightly 47.0a1 2016-01-26. What I could see is some trembling that appears when scrolling slowly. The trembling is still visible on Firefox 46 beta 9 and latest Nightly 48.0a1.

Were specific XP machines affected by this? Or is it something else I need to modify in order to reproduce this? 

Graphics
Adapter Description	Intel(R) HD Graphics
Adapter Drivers	igxprd32
Adapter RAM	Unknown
Asynchronous Pan/Zoom	none
Device ID	0x0152
Direct2D Enabled	Blocked for your graphics driver version.
DirectWrite Enabled	false (0.0.0.0)
Driver Date	2-4-2013
Driver Version	6.14.10.5437
GPU #2 Active	false
GPU Accelerated Windows	1/1 Direct3D 9 (OMTC)
What websites did you try? They have to be sites with mix-blend in their CSS. Like halfway down stripe.com/relay or bhphotovideo.com.
I used stripe.com and bhphotovideo.com (as seen in comment 16).
Any thoughts?
Flags: needinfo?(dvander)
(In reply to Petruta Rasa [QA] [:petruta] from comment #28)
> Any thoughts?

Not really unfortunately - if the GPU or computer specs are quite good that might be enough to mask the problem. As a last resort you could try scrolling the bottom half of this page (with all the pictures of bees): http://codepen.io/atelierbram/pen/weKoi

That's the stress test I used for this patch.
Flags: needinfo?(dvander)
Tried again with Firefox 47 beta 8 under Win XP and the scrolling looks better with the test page http://codepen.io/atelierbram/pen/weKoi. Trembling is still present on this site and the other ones used (stripe.com and bhphotovideo.com), but I can see the same issue on Google Chrome and other apps, so I'm assuming it's unrelated to this.

Marking as verified based on that.
Status: RESOLVED → VERIFIED
Version: unspecified → 46 Branch
You need to log in before you can comment on or make changes to this bug.