Simplify CopyableCanvasLayer::UpdateTarget

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

(Blocks 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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)
Blocks: 1290072
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 nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d51327ec371e
Simplify CopyableCanvasLayer::UpdateTarget and remove unnecessary copies. r=jnicol
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.
Attachment #8775548 - Attachment is obsolete: true
Flags: needinfo?(nical.bugzilla)
Attachment #8775951 - Flags: review?(jnicol)
Attachment #8775951 - Flags: review?(jnicol) → review+
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5db12322fd3
Simplify CopyableCanvasLayer::UpdateTarget and remove unnecessary copies. r=jnicol
https://hg.mozilla.org/mozilla-central/rev/e5db12322fd3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1291190
You need to log in before you can comment on or make changes to this bug.