Closed Bug 947781 Opened 6 years ago Closed 6 years ago

Implement ReadbackLayer for OMTC

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: mattwoodrow, Assigned: bas.schouten)

References

Details

Attachments

(4 files)

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?
Flags: needinfo?(nical.bugzilla)
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.
Blocks: 899785
(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.
Flags: needinfo?(nical.bugzilla)
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.
Assignee: nobody → bas
Status: NEW → ASSIGNED
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?
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(matt.woodrow)
Attachment #8463366 - Attachment description: Refactor RotatedBuffer code to allow more broad reusal → Part 1: Refactor RotatedBuffer code to allow more broad reusal
Attachment #8463366 - Attachment description: Part 1: Refactor RotatedBuffer code to allow more broad reusal → Part 1: Refactor RotatedBuffer code to allow easier reuse
Attachment #8463406 - Flags: review?(nical.bugzilla) → review+
Attachment #8463366 - Flags: review?(matt.woodrow) → review+
Attachment #8464104 - Flags: review?(matt.woodrow)
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?
Attachment #8464104 - Flags: review?(matt.woodrow) → review+
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
Attachment #8464100 - Flags: review?(nical.bugzilla) → review+
Depends on: 1123400
Depends on: 1127459
Depends on: 1127558
Depends on: 1132432
You need to log in before you can comment on or make changes to this bug.