902330 - Fix shmem texture clients for D3D9 and 11 compositors

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Nick Cameron [:nrc]]

Attachment: fix SupportsAzureContent

SupportsAzureContent is a total nightmare, it is rarely safe to use. This patch adds a safer alternative and fixes up a bunch of its uses

Comment 2

[Nick Cameron [:nrc]]

Attachment: Clear the non-opaque surfaces on creation for shmem texture clients and other minor fixups

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 786762 [details] [diff] [review]
fix SupportsAzureContent

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

::: gfx/layers/client/CompositableClient.cpp
@@ +127,1 @@
>      }

Don't comment this out.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 786763 [details] [diff] [review]
Clear the non-opaque surfaces on creation for shmem texture clients and other minor fixups

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

::: gfx/layers/client/TextureClient.cpp
@@ +327,5 @@
> +      nsRefPtr<gfxContext> context = new gfxContext(GetSurface());
> +      context->SetColor(gfxRGBA(0, 0, 0, 0));
> +      context->SetOperator(gfxContext::OPERATOR_SOURCE);
> +      context->Paint();
> +    }

Why do we need to do this? Normally we rely on the ContentClient's BeginPaint function to do any clearing that might be required.

Comment 5

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 786763 [details] [diff] [review]
> Clear the non-opaque surfaces on creation for shmem texture clients and
> other minor fixups
> 
> Review of attachment 786763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/TextureClient.cpp
> @@ +327,5 @@
> > +      nsRefPtr<gfxContext> context = new gfxContext(GetSurface());
> > +      context->SetColor(gfxRGBA(0, 0, 0, 0));
> > +      context->SetOperator(gfxContext::OPERATOR_SOURCE);
> > +      context->Paint();
> > +    }
> 
> Why do we need to do this? Normally we rely on the ContentClient's
> BeginPaint function to do any clearing that might be required.

Do you mean ThebesLayerBuffer::BeingPaint? ContentClient does nothing to clear. TLB::BeginPaint only clears buffers we've used before, not new buffers. If we have a new buffer then it assumes we don't need to clear it. This patch clears new buffers to make them actually zero, rather than uninitialised data.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Changed my mind.

We manage to get away without doing this on some platforms, right? B2G, android should be hitting this?

We should try figure out what conditions give us actual clear surfaces, and what ones don't, rather than just always clearing.

Comment 7

[David Zbarsky (:dzbarsky)]

I think we may also be hitting this with BasicCompositor.  I noticed that some tests end up rendering some blocks that should be a solid color but end up being random garbage.

Comment 8

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> Changed my mind.
> 
> We manage to get away without doing this on some platforms, right? B2G,
> android should be hitting this?
> 
> We should try figure out what conditions give us actual clear surfaces, and
> what ones don't, rather than just always clearing.

My theory is that we just don't notice on these platforms. The only reason I noticed on windows is that there is a great big chunk of transparent thebes layer over the title bar. I never saw this with web content. I guess it is pretty hard to come up with a case to repro this, its not covered by our tests and no-one has noticed on mobile/mac.

Comment 9

[Nick Cameron [:nrc]]

I'll try to come up with a test case.

Comment 10

[Nick Cameron [:nrc]]

OK, I made a test case and it is broken on Android and on Windows only with OMTC. Could you test on mac too please? (Coming soon...)

Comment 11

[Nick Cameron [:nrc]]

Attachment: test case

I'll make this into a ref test somehow later - gradients are really bad for ref tests.

Anyway, if this passes, then you should see a very pale pink square with a subtle gradient (that is it should be 50% opaque and you should see the gradient coming through). If it fails you should see an opaque or nearly opaque (on windows, alpha = 0xcd, different on Android) white or grey square. On Android the test becomes correct when you zoom in. I haven't checked if these patches fix android or not.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Passes just fine on mac, because we always clear in ContentClientIncremental.

You should get rid of that clear if you're going to add this one :)

Comment 13

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=66dadb3e49f9

Comment 14

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> Passes just fine on mac, because we always clear in ContentClientIncremental.
> 
> You should get rid of that clear if you're going to add this one :)

Filed bug 903129 for that because it might get a bit big if we have to clear in other texture clients.

Comment 15

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0428fd3a0d3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3ab8b0a16c

(ref test still to come)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0428fd3a0d3a
https://hg.mozilla.org/mozilla-central/rev/cc3ab8b0a16c

Comment 17

[Nick Cameron [:nrc]]

Attachment: reftest

Comment 18

[Nick Cameron [:nrc]]

Attachment: reftest

apparently canvas and content do integer division differently :-p

Comment 19

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c39b55de4aa9

Comment 20

[Nick Cameron [:nrc]]

Attachment: reftest

Reftest with fuzz, because no-one can seem to agree on how to divide by two. Sigh.

Carrying r=mattwoodrow (since it was he suggested fuzzing on irc).

Comment 21

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f387aacdee38

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f387aacdee38