This code is quite complicated and is causing us to hold on to snapshots and trigger unnecessary copies. Let's sort it out.
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.
Attachment #8775548 - Flags: review?(jnicol)
Comment on attachment 8775548 [details] [diff] [review] Separate UpdateTarget and UpdateSurface Review of attachment 8775548 [details] [diff] [review]: ----------------------------------------------------------------- Looks good
Attachment #8775548 - Flags: review?(jnicol) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/d51327ec371e Simplify CopyableCanvasLayer::UpdateTarget and remove unnecessary copies. r=jnicol
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
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.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5db12322fd3 Simplify CopyableCanvasLayer::UpdateTarget and remove unnecessary copies. r=jnicol
You need to log in before you can comment on or make changes to this bug.