1303897 - Use TextureForwarder instead of CompositableForwarder when we can

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

We have a lot of code that uses CompositableForwarder when it really only deals with textures.

VideoBridge won't implement CompositableForwarder, so we need to use TextureForwarder instead.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Use TextureForwarder for Image::GetTextureClient

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Use TextureForwarder for TextureClientRecycleAllocator

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Remove unnecessary param to InitIPDLActor

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8792700 [details] [diff] [review]
Part 1: Use TextureForwarder for Image::GetTextureClient

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

Looks good, I just want some details about the DISALLOW_BIGIMAGE thingy.

::: gfx/layers/ipc/TextureForwarder.h
@@ +52,5 @@
> +   * Returns the type of backend that is used off the main thread.
> +   * We only don't allow changing the backend type at runtime so this value can
> +   * be queried once and will not change until Gecko is restarted.
> +   */
> +  LayersBackend GetCompositorBackendType() const

I thought we could have different compositor backends per LayerTransactionChild under a given CompositorBridgeChild. Looking at the code it seems that I was wrong (maybe we could at some point and not anymore). Having the compositor backend at the TextureForwarder level is a lot more practical so yay.

::: layout/base/FrameLayerBuilder.cpp
@@ +1590,5 @@
>      if (mLayerManager->GetBackendType() == LayersBackend::LAYERS_BASIC &&
>          mDrawTarget) {
>        RefPtr<SourceSurface> surface = mDrawTarget->Snapshot();
>        RefPtr<SourceSurfaceImage> image = new SourceSurfaceImage(mSize, surface);
> +      image->SetTextureFlags(TextureFlags::DISALLOW_BIGIMAGE);

Why do we need to set this flag?

I can see why we would not want to split a mask into several images, but I think the whole big-image thing is meant to support image layers that are bigger than what the gpu lets you fit in a texture (on mobile). I am not a FrameLayerbuilder expert but it looks like this would disable it for any type of ImageLayer.
If you think we don't need the BigImage fallback anymore, you should convince me and jeff why and file a bug to remove it from the code base entirely!

Comment 5

[Matt Woodrow (:mattwoodrow)]

(In reply to Nicolas Silva [:nical] from comment #4) 
> ::: gfx/layers/ipc/TextureForwarder.h
> @@ +52,5 @@
> > +   * Returns the type of backend that is used off the main thread.
> > +   * We only don't allow changing the backend type at runtime so this value can
> > +   * be queried once and will not change until Gecko is restarted.
> > +   */
> > +  LayersBackend GetCompositorBackendType() const
> 
> I thought we could have different compositor backends per
> LayerTransactionChild under a given CompositorBridgeChild. Looking at the
> code it seems that I was wrong (maybe we could at some point and not
> anymore). Having the compositor backend at the TextureForwarder level is a
> lot more practical so yay.

We still can have a backend per LayerTransactionChild I think. This changes it so we have a TextureFactoryIdentifier for both CompositorBridgeChild and LayerTransactionChild. That's still 'wrong', but I think less wrong, and close enough for VideoBridge to work.

I wrote up a plan for a full fix in bug 1281456, I'm working on doing that refactoring too.

> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1590,5 @@
> >      if (mLayerManager->GetBackendType() == LayersBackend::LAYERS_BASIC &&
> >          mDrawTarget) {
> >        RefPtr<SourceSurface> surface = mDrawTarget->Snapshot();
> >        RefPtr<SourceSurfaceImage> image = new SourceSurfaceImage(mSize, surface);
> > +      image->SetTextureFlags(TextureFlags::DISALLOW_BIGIMAGE);
> 
> Why do we need to set this flag?
> 
> I can see why we would not want to split a mask into several images, but I
> think the whole big-image thing is meant to support image layers that are
> bigger than what the gpu lets you fit in a texture (on mobile). I am not a
> FrameLayerbuilder expert but it looks like this would disable it for any
> type of ImageLayer.
> If you think we don't need the BigImage fallback anymore, you should
> convince me and jeff why and file a bug to remove it from the code base
> entirely!

This should just be setting the flag for the mask layer ImageLayer's layers::Image. It's basically the same as what we had before, except we set the flag on the Layer/Compositable previous and texture creation picked it up from there, now we set it on the Image. I'll double check I didn't make a mistake in the morning.

Comment 6

[Nicolas Silva [:nical]]

I have a counter-proposal based on using TextureClient/Host without IPDL, I'm hacking something up quickly to see how it works out and I'll come back to you with a proto asap.

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to Nicolas Silva [:nical] from comment #6)
> I have a counter-proposal based on using TextureClient/Host without IPDL,
> I'm hacking something up quickly to see how it works out and I'll come back
> to you with a proto asap.

Do you mean for bug 1302918?

I thought about doing this, but it seemed complex and I was worried about adding more code paths that need testing. IPDL is probably overkill, but it's (relatively) well tested now.

Comment 8

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Do you mean for bug 1302918?

Oh right, wrong bug, oops.

> I thought about doing this, but it seemed complex and I was worried about
> adding more code paths that need testing. IPDL is probably overkill, but
> it's (relatively) well tested now.

I have a prototype that works well, it's a large patch but most of it is moving code around. We can do the IPDL thing now and I'll come back to this later if need be. Making TextureClient work without IPDL is something I've been itching to do for a long while, it has a lot of small advantages none of which was big enough for me to go ahead and implement it. One of these advantages is actually being able to properly test the texture classes more easily. Anyway, I'll r+ the IPDL stuff to unblock you.

Comment 9

[Nicolas Silva [:nical]]

Comment on attachment 8792700 [details] [diff] [review]
Part 1: Use TextureForwarder for Image::GetTextureClient

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

R=me with a comment explaining that the BigImage thing you added only affects mask layers.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11dff62f0ef5
Part 1: Use TextureForwarder for Image::GetTextureClient. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/0af32711ba65
Part 2: Use TextureForwarder for TextureClientRecycleAllocator. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb3267d9e339
Part 3: Remove unnecessary param to InitIPDLActor. r=nical

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11dff62f0ef5
https://hg.mozilla.org/mozilla-central/rev/0af32711ba65
https://hg.mozilla.org/mozilla-central/rev/cb3267d9e339

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11dff62f0ef5
https://hg.mozilla.org/mozilla-central/rev/0af32711ba65
https://hg.mozilla.org/mozilla-central/rev/cb3267d9e339

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11dff62f0ef5
https://hg.mozilla.org/mozilla-central/rev/0af32711ba65
https://hg.mozilla.org/mozilla-central/rev/cb3267d9e339