Do not reset layer when setting the same dimension to canvas

RESOLVED FIXED in Firefox 51

Status

()

Core
Canvas: 2D
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
We always reset layer when doing setDimension. I think we can reuse the layer and buffer if the dimension is the same. Then the layer doesn't need to realloate buffer.
(Assignee)

Comment 1

2 years ago
:nical, do you think we should do it?
Flags: needinfo?(nical.bugzilla)
Per spec, touching the canvas size (even without changing it) resets the canvas drawing state. If we want to reuse the buffer, it's fine but we need to be very careful about not keeping around any additional drawing state like transforms, clips or whatnot.

I don't feel very strongly for or against keeping the canvas buffer in that case as long as it doesn't complicate the code too much, although I would rather encourage web authors to not use SetDimension as a way to clear the canvas since it is going to be slower in any case.

Do you have an idea of how often web authors touch the dimensions of a canvas without changing the actual values?
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 3

2 years ago
(In reply to Nicolas Silva [:nical] from comment #2)
> Per spec, touching the canvas size (even without changing it) resets the
> canvas drawing state. If we want to reuse the buffer, it's fine but we need
> to be very careful about not keeping around any additional drawing state
> like transforms, clips or whatnot.
> 
> I don't feel very strongly for or against keeping the canvas buffer in that
> case as long as it doesn't complicate the code too much, although I would
> rather encourage web authors to not use SetDimension as a way to clear the
> canvas since it is going to be slower in any case.
> 
> Do you have an idea of how often web authors touch the dimensions of a
> canvas without changing the actual values?

I don't know how often the web authors set the same dimension. But I tried this case on chrome and safari and I believe they reused the buffer. From my test, if a page keeps setting the same dimension to a large canvas, firefox will use a lot of CPU resource and will have memory fragment problem in the end, then crash.
So yes, if the code doesn't complicate, I think it's good to have the reusing buffer mechanism.
(Assignee)

Comment 4

2 years ago
Created attachment 8776896 [details] [diff] [review]
reuse buffer.

Keep the buffer provider while the dimension is the same.
(Assignee)

Comment 5

2 years ago
Created attachment 8777213 [details]
testcase

This html keeps setting the same dimension for each frame.
(Assignee)

Comment 6

2 years ago
Created attachment 8777215 [details] [diff] [review]
reuse buffer

In this patch, I check the dimension before ClearTarget. If the dimension is the same, then keep the BufferProvider and clear the buffer data.
I tried the testcase attachment 8777213 [details] With BufferProviderShared, the FPS increased to 24 from 18 on windows.
Attachment #8776896 - Attachment is obsolete: true
Attachment #8777215 - Flags: review?(nical.bugzilla)

Updated

2 years ago
Attachment #8777215 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 8

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3758ab7ee78
Reuse canvas buffer when setting the same dimension. r=nical
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3758ab7ee78
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

a year ago
Depends on: 1341521
You need to log in before you can comment on or make changes to this bug.