1202394 - Avoid read-back in TextureClient::UpdateFromSurface() when possible.

Status: NEW

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

I am trying to simplify TextureClient and I come accross a lot of code for UpdateFromSurface implemented for each TextureClient backend.

I noticed that:
- UpdateFromSurface is only used by ImageContainer
- implementations of UpdateFromSurface seem expect the surface to be a DataSourceSurface but the api take a SourceSurface.

How much do we win from having backend-specific upload logic implemented in UpadteFromSurface? Is it really intended to be used with data surfaces only?
If the method is intended to be a fast path for copying from cpu-side image data into TextureClient, can we at least change it into UpdateFromDataSurface(DataSourceSurface*) to make things more evident? Right now if you pass a D2D surface to a D3D11 TextureClient, you read-back and upload the texture again, which is way slower than just borrowing the DrawTarget from the TextureClient and doing CopySurface in a backend-independent way!

Comment 1

[Nicolas Silva [:nical]]

After digging a bit further, bug 1176363 indicates that the method was added in order to be able to update the texture client off the main thread where BorrowDrawTarget isn't safe to use.

In bug 1190950, the interface changed from taking a DataSourceSurface to a SourceSurface to cover more cases.

So in the end we do need this method, but it wasn't clear by looking at the code why (I was reading the doc comment "This function can be used to update the contents of the TextureClient off the main thread." as "overrides of this method must work off the main thread" rather than "We need this because the other way to update textures don't work off the main thread".

How about we rename this into UpdateOffThePaintingThread(gfx::SourceSurface*); so that it is clear that the thing is not some kind of helper to update from a surface that we'd use by default on the main thread, and so that we don't need to go through hg history to understand the point of having this code at all?

Comment 2

[Nicolas Silva [:nical]]

... or have UpdateFromSurface use BorrowDrawTarget if we are in the main thread and the surface is not a data surface, which would promote the method into being the default way to update a TextureClient from a SourceSurface.

Comment 3

[Matt Woodrow (:mattwoodrow)]

The X11 impl doesn't require readback into a data source surface, so that's why the parameter is SourceSurface, not DataSourceSurface.

Changing the naming to be more explicit seems fine with me, as does making it smarter about which way to do things based on current thread.

Comment 4

[Bas Schouten (:bas.schouten)]

(In reply to Nicolas Silva [:nical] from comment #1)
> After digging a bit further, bug 1176363 indicates that the method was added
> in order to be able to update the texture client off the main thread where
> BorrowDrawTarget isn't safe to use.
> 
> In bug 1190950, the interface changed from taking a DataSourceSurface to a
> SourceSurface to cover more cases.
> 
> So in the end we do need this method, but it wasn't clear by looking at the
> code why (I was reading the doc comment "This function can be used to update
> the contents of the TextureClient off the main thread." as "overrides of
> this method must work off the main thread" rather than "We need this because
> the other way to update textures don't work off the main thread".
> 
> How about we rename this into
> UpdateOffThePaintingThread(gfx::SourceSurface*); so that it is clear that
> the thing is not some kind of helper to update from a surface that we'd use
> by default on the main thread, and so that we don't need to go through hg
> history to understand the point of having this code at all?

The function is faster for some texture clients even when used -on- the painting thread. So we definitely -do- want to use this helper by default if we can on the main thread as well.