1289816 - Simplify CopyableCanvasLayer::UpdateTarget

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

This code is quite complicated and is causing us to hold on to snapshots and trigger unnecessary copies. Let's sort it out.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Separate UpdateTarget and UpdateSurface

This mess has been nagging me for a while and I feel much better now. UpdateTarget is called in two places: one with no DrawTarget (basic canvas layer) and one with a DrawTarget (when copying into a TextureClient in CanvasClient2D). In the first case the goal of UpdateTarget is to get a Source Surface stored in mSurface for BasicCanvasLayer::Paint, while in the first case we jsut want to copy in aDestTarget without storing the surface. 

Having a separate method for each case simplifies things a great deal. mSurface is only used in BasicCanvasLayer::Paint, just after calling UpdateTarget so I removed it and UpdateSurface now returns the surface so that we can use it right away without storing it. This removes some confusion about what is a temporary state and lets us handle the BufferProvider case specifically in Paint to avoid that the snapshot triggers a copy of the BufferProvider's target under the hood.

Comment 2

[Jamie Nicol [:jnicol]]

Comment on attachment 8775548 [details] [diff] [review]
Separate UpdateTarget and UpdateSurface

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

Looks good

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d51327ec371e
Simplify CopyableCanvasLayer::UpdateTarget and remove unnecessary copies. r=jnicol

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2138a46d6c for canvas failures like https://treeherder.mozilla.org/logviewer.html#?job_id=32818862&repo=mozilla-inbound

Comment 5

[Nicolas Silva [:nical]]

Attachment: Separate UpdateTarget and UpdateSurface

My mistake was that if mGLContext is not null we should use the GLContext-specific code path even if mBufferProvider is not null (with skiagl both pointers are not null).

This fixes the reftest failures. The patch does mostly the same except that UpdateSurface prioritizes mGLContext's code path instead of mBufferProvider.
I also moved UpdateTarget from CopyableCanvasLayer to ClientCanvasLayer because it's the only canvas layer type that uses it. The only other change to this method is that I made it return false when the copy fails so that we don't bother sending the texture to the compositor if the copy failed.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5db12322fd3
Simplify CopyableCanvasLayer::UpdateTarget and remove unnecessary copies. r=jnicol

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e5db12322fd3