947781 - Implement ReadbackLayer for OMTC

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

I think we'll need this before shipping OMTC on windows to get good enough performance for windowless transparent plugins.

I think we need to add new API to TextureClient (or one of the helper interfaces) that is similar to:

typedef void (* LockCallback)(gfxASurface* aContents, void *aClosure);
void AsyncLock(LockCallback aCallback, void *aClosure);

We can do a default implementation that just calls the callback synchronously using GetAsSurface().

TextureClients using d2d can be implemented like D3D10 does (in gfx/layers/d3d10/ReadbackManagerD3D10.cpp) to do a proper asynchronous callback once the drawing has completed.

We can then implement a LockCallback that uses PluginBackgroundSink to copy the TextureClient's data into the plugin's background surface.

Does that seem sane?

Comment 1

[Matt Woodrow (:mattwoodrow)]

The plugin background API is still using thebes, and that's our own use case for this API, so I'm planning on sticking with that for now.

Comment 2

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> I think we need to add new API to TextureClient (or one of the helper
> interfaces) that is similar to:
> 
> typedef void (* LockCallback)(gfxASurface* aContents, void *aClosure);
> void AsyncLock(LockCallback aCallback, void *aClosure);
> 
> We can do a default implementation that just calls the callback
> synchronously using GetAsSurface().
> 
> TextureClients using d2d can be implemented like D3D10 does (in
> gfx/layers/d3d10/ReadbackManagerD3D10.cpp) to do a proper asynchronous
> callback once the drawing has completed.
> 
> We can then implement a LockCallback that uses PluginBackgroundSink to copy
> the TextureClient's data into the plugin's background surface.
> 
> Does that seem sane?

I am not familiar at all with readback layers so I don't really understand what this implies. So from very very far away this doesn't look insane (too bad that we need to add thebes stuff but if that's only going to be used for the backround API I suppose that will do). You should be extra careful about lifetimes whenever you want something to interact with TextureClient/Host.

Comment 3

[Bas Schouten (:bas.schouten)]

I don't think we need this to turn it on on nightly, we need it before we go to aurora though. I will do this work.

Comment 4

[Bas Schouten (:bas.schouten)]

I'm not convinced anymore that we need this to ride the trains? I'll write the patch and see if we can uplift it though. Do you actually still think we need this Matt?

Comment 5

[Matt Woodrow (:mattwoodrow)]

Given the lack of complaints, maybe we don't. It would still be really good to have though, I'm sure some people care about plugin performance.

Comment 6

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Refactor RotatedBuffer code to allow easier reuse

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Do not transmit layer types that are unshadowed over the IPDL protocol.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Add readback functionality to TextureClients

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Part 4: Implement ReadbackLayers for OMTC.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8464104 [details] [diff] [review]
Part 4: Implement ReadbackLayers for OMTC.

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

::: gfx/layers/client/ContentClient.cpp
@@ +168,5 @@
> +  {
> +    // Manual AddRef to keep these layers alive! Very important these get killed
> +    // again in the destructor!
> +    for (uint32_t i = 0; i < mReadbackUpdates.Length(); ++i) {
> +      mReadbackUpdates[i].mLayer->AddRef();

This is a bit sad, couldn't we make ReadbackProcessor::Update always hold refs using nsRefPtr?

Comment 11

[Nicolas Silva [:nical]]

Comment on attachment 8464100 [details] [diff] [review]
Part 2: Add readback functionality to TextureClients

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

Looks good, what will call SetReadbackSink on the textures?

::: gfx/layers/d3d11/ReadbackManagerD3D11.h
@@ +43,5 @@
> +
> +  void ProcessTasks();
> +
> +  // The invariant maintained by |mTaskSemaphore| is that the readback thread
> +  // will awaken from WaitForMultipleObjects() at least once per readback 

nit: trailing space

Comment 12

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bc1a62eb443
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4ecea1452e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/c378df360469
https://hg.mozilla.org/integration/mozilla-inbound/rev/96379f178485

Comment 13

[Ed Morley [:emorley]]

Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=44891771&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=44891768&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df0091e830db
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ba00ca45db46
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/90d1352dbd19
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1528900bd8cf

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/436cc8abfda3
https://hg.mozilla.org/mozilla-central/rev/6346e96ceac0
https://hg.mozilla.org/mozilla-central/rev/881f358bc39e
https://hg.mozilla.org/mozilla-central/rev/9b44cf686aba