1251257 - CanvasClient updates are racy

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Attachment: canvas-synchro.html

CanvasClient copies the canvas's content into the same TextureCLient every frame and sends it over to the compositor in async transactions. Since there is no synchronization involved, copying frame N+1 can happen while the compositor is uploading it to a texture which is bad. The attached testcase shows a canvas changing color rapidly along with the page's background color. On Linux you can definitely see that the canvas and the content are out of sync. I haven't tested on other platforms bit i expect the result to be the same.

Comment 1

[Nicolas Silva [:nical]]

Hm. I thought that adding the flag IMMEDIATE_UPLOAD to the textures created by the CanvasClient would fix the issue if the texture has an intermediate buffer (should make the transaction synchronous and upload in the transaction) but it didn't do the job.

By the way, to reproduce the issue with the testcase, you should try with a release build, not a debug build.

Comment 2

[Sotaro Ikeda [:sotaro]]

I saw the problem on windows and linux. I did not see the problem on android and b2g.

Comment 3

[Matt Woodrow (:mattwoodrow)]

(In reply to Nicolas Silva [:nical] (PTO until March 7th) from comment #1)
> Hm. I thought that adding the flag IMMEDIATE_UPLOAD to the textures created
> by the CanvasClient would fix the issue if the texture has an intermediate
> buffer (should make the transaction synchronous and upload in the
> transaction) but it didn't do the job.
> 
> By the way, to reproduce the issue with the testcase, you should try with a
> release build, not a debug build.

I don't think IMMEDIATE_UPLOAD forces the transaction to be sync, it only makes us upload during the transaction callback (instead of at composite time).

That may be a bug, I'm not sure if IMMEDIATE_UPLOAD is particularly useful without that property. It seems valuable to decide what behaviour we want and make sure that actually happens.

We really want to avoid sync transactions though, so maybe we should fix this another way?

Comment 4

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> (In reply to Nicolas Silva [:nical] (PTO until March 7th) from comment #1)
> > Hm. I thought that adding the flag IMMEDIATE_UPLOAD to the textures created
> > by the CanvasClient would fix the issue if the texture has an intermediate
> > buffer (should make the transaction synchronous and upload in the
> > transaction) but it didn't do the job.
> > 
> > By the way, to reproduce the issue with the testcase, you should try with a
> > release build, not a debug build.
> 
> I don't think IMMEDIATE_UPLOAD forces the transaction to be sync, it only
> makes us upload during the transaction callback (instead of at composite
> time).

It tries to, at least, here: https://dxr.mozilla.org/mozilla-central/rev/be593a64d7c6a826260514fe758ef32a6ee580f7/gfx/layers/ipc/ShadowLayers.cpp#417


> We really want to avoid sync transactions though, so maybe we should fix
> this another way?

The better way to fix this would be reviving bug 1167235, but it'd be good to have a short-term solution in the mean time.

Comment 5

[Nicolas Silva [:nical]]

This is fixed now (either by bug 1167235, or some earlier change).