863223 - [layers-refactoring] regressed async canvas updates

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It looks like the layers refactor regressed bug 829747, where we do async canvas updates.  CanvasClient::Updated() calls mForwarder->UpdateTexture, and ShadowLayerForwarder::UpdateTexture calls AddPaint not AddNoSwapPaint.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ugh, what a pain. I guess I can look into this.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Nick, is there any reason we can't always use AddNoSwapPaint() in ShadowLayerForwarder::UpdateTexture()? Do we ever need a reply?

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Make canvas updates asynchronous once again

Not sure if this is really correct for 2d canvas, but should be ok for 3d. Started a try run here: https://tbpl.mozilla.org/?tree=Try&rev=aa301dee400e

Comment 4

[Nick Cameron [:nrc]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> Nick, is there any reason we can't always use AddNoSwapPaint() in
> ShadowLayerForwarder::UpdateTexture()? Do we ever need a reply?

Yes, there is a reason - I think double buffered image layers need to get their surface descriptors back from the compositor, probably single buffered ones too. See BasicShadowLayerManager::ForwardTransaction

Comment 5

[Nick Cameron [:nrc]]

I think we probably need either an UpdateTextureNoSwap method, or UpdateTexture to take a no swap flag. Perhaps better would be if that flag could be attached to aCompositable's flags, rather than being passed explicitly.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Make canvas updates asynchronous once again

Comment 7

[Nick Cameron [:nrc]]

Comment on attachment 743808 [details] [diff] [review]
Make canvas updates asynchronous once again

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

r=me with the nits

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +103,5 @@
> +      aDescriptor->type() != SurfaceDescriptor::Tnull_t) {
> +    MOZ_ASSERT(aCompositable);
> +    MOZ_ASSERT(aCompositable->GetIPDLActor());
> +    mTxn->AddNoSwapEdit(OpPaintTexture(nullptr, aCompositable->GetIPDLActor(), 1,
> +                        SurfaceDescriptor(*aDescriptor)));

nit: indent (aligned with wrong opening bracket)

::: gfx/layers/ipc/ShadowLayers.h
@@ +276,5 @@
>    /**
> +   * Same as above, but performs an asynchronous layer transaction
> +   */
> +  virtual void UpdateTextureNoSwap(CompositableClient* aCompositable,
> +                             TextureIdentifier aTextureId,

nit: indent

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29aa0fd47707

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29aa0fd47707

Comment 10

[Matt Woodrow (:mattwoodrow)]

Unfortunately this causes us to leak shared memory with Canvas2D.

When we send the transaction, we add the Shmem to the changeset and clear our local reference to it.

On the compositor side, we give the Shmem to the ImageHost, which uploads to the texture from it and then creates a reply to send it back.

Since this is a no-reply transaction, we don't bother to send the reply and we leak instead.

What was the intended behaviour here? To create a new shmem every frame (and have the compositor free it), or to keep re-using it (and require that the compositor finish copying from it during the transaction).

We should also probably send this intention over IPC so that the compositor knows that it shouldn't be writing a reply. And assert that no reply changesets are generated.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

This was just a typo on my part. I meant to do CanvasWebGL async, not 2D. Oops. I'm attaching a patch to 871390