Closed Bug 863223 Opened 9 years ago Closed 9 years ago

[layers-refactoring] regressed async canvas updates

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: vlad, Assigned: snorp)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Ugh, what a pain. I guess I can look into this.
Nick, is there any reason we can't always use AddNoSwapPaint() in ShadowLayerForwarder::UpdateTexture()? Do we ever need a reply?
Flags: needinfo?(ncameron)
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
Attachment #739072 - Flags: review?(ncameron)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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
Flags: needinfo?(ncameron)
Attachment #739072 - Flags: review?(ncameron)
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.
Assignee: nobody → snorp
Attachment #739072 - Attachment is obsolete: true
Attachment #743808 - Flags: review?(ncameron)
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
Attachment #743808 - Flags: review?(ncameron) → review+
https://hg.mozilla.org/mozilla-central/rev/29aa0fd47707
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.
Depends on: 871390
This was just a typo on my part. I meant to do CanvasWebGL async, not 2D. Oops. I'm attaching a patch to 871390
You need to log in before you can comment on or make changes to this bug.