Bug 100608 – Canvas webkitBackingStorePixelRatio does not change on resize reset

• Component: Canvas
• Status: RESOLVED FIXED

[Timothy Hatcher]

The pixel ratio should update when a resize reset happens.

[Timothy Hatcher]

<rdar://problem/10822030>

[Radar WebKit Bug Importer]

<rdar://problem/12588913>

[Timothy Hatcher]

Created attachment 171133 [details]
Proposed Change

This will need new expected results for other platforms.

[WebKit Review Bot]

Comment on attachment 171133 [details]
Proposed Change

Attachment 171133 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14617834

New failing tests:
fast/canvas/canvas-resize-reset-pixelRatio.html
platform/chromium/virtual/gpu/fast/canvas/canvas-resize-reset-pixelRatio.html

[Darin Adler]

Comment on attachment 171133 [details]
Proposed Change

View in context: https://bugs.webkit.org/attachment.cgi?id=171133&action=review

> Source/WebCore/ChangeLog:6
> +        The backing store was not being recreated using the current page pixel ratio
> +        when a resize reset occurred.

What is a “resize reset”?

> Source/WebCore/html/HTMLCanvasElement.cpp:265
> +    IntSize newSize = IntSize(w, h);

Could just use construction syntax here instead of repeating IntSize.

> Source/WebCore/html/HTMLCanvasElement.cpp:308
> +    // NOTE: High-DPI canvas requires the platform-specific ImageBuffer implementation to respect
> +    // the resolutionScale parameter.

I do not understand this comment that you moved here. How does this comment help someone reading this function?

> Source/WebCore/html/HTMLCanvasElement.h:85
>      void setSize(const IntSize& newSize)
>      { 
> -        if (newSize == size())
> +        if (newSize == size() && targetDeviceScaleFactor() == m_deviceScaleFactor)

It seems really strange that if a canvas is in a window that’s moved from one screen to another, the device scale factor is updated only when the canvas’s size changes. Is that really the design?

I don’t like the design of this setSize function. Before the change, the function was like changing width and height with the DOM, but optimized in two ways:

1) If the size did not change, it skipped the reset. Setting width and height through the DOM does not have that kind of optimization.
2) If the size did change, then it did one reset rather than the two you’d get if you set both width and height through the DOM.

For me, (1) does not seem like a good thing, It’s strange that this setSize function has this optimization, but setting the width and height DOM attributes or content attributes does not. On the other hand, to me (2) seems purely good because it boosts performance but otherwise is not detectable.

What code calls this function? Are those callers really relying on behavior (1)? If they are not, then we could just remove the early return rather than adding the mysterious scale factor check here.

[Darin Adler]

Comment on attachment 171133 [details]
Proposed Change

View in context: https://bugs.webkit.org/attachment.cgi?id=171133&action=review

>> Source/WebCore/html/HTMLCanvasElement.h:85
>> +        if (newSize == size() && targetDeviceScaleFactor() == m_deviceScaleFactor)
> 
> It seems really strange that if a canvas is in a window that’s moved from one screen to another, the device scale factor is updated only when the canvas’s size changes. Is that really the design?
> 
> I don’t like the design of this setSize function. Before the change, the function was like changing width and height with the DOM, but optimized in two ways:
> 
> 1) If the size did not change, it skipped the reset. Setting width and height through the DOM does not have that kind of optimization.
> 2) If the size did change, then it did one reset rather than the two you’d get if you set both width and height through the DOM.
> 
> For me, (1) does not seem like a good thing, It’s strange that this setSize function has this optimization, but setting the width and height DOM attributes or content attributes does not. On the other hand, to me (2) seems purely good because it boosts performance but otherwise is not detectable.
> 
> What code calls this function? Are those callers really relying on behavior (1)? If they are not, then we could just remove the early return rather than adding the mysterious scale factor check here.

Note, the layout test does not test this setSize change.

[Timothy Hatcher]

Comment on attachment 171133 [details]
Proposed Change

View in context: https://bugs.webkit.org/attachment.cgi?id=171133&action=review

>> Source/WebCore/ChangeLog:6
>> +        when a resize reset occurred.
> 
> What is a “resize reset”?

The fact that a canvas is reset when the size changes.

>> Source/WebCore/html/HTMLCanvasElement.cpp:265
>> +    IntSize newSize = IntSize(w, h);
> 
> Could just use construction syntax here instead of repeating IntSize.

Will do.

>> Source/WebCore/html/HTMLCanvasElement.cpp:308
>> +    // the resolutionScale parameter.
> 
> I do not understand this comment that you moved here. How does this comment help someone reading this function?

It probably can be removed, or moved back to the constructor.

>>> Source/WebCore/html/HTMLCanvasElement.h:85
>>> +        if (newSize == size() && targetDeviceScaleFactor() == m_deviceScaleFactor)
>> 
>> It seems really strange that if a canvas is in a window that’s moved from one screen to another, the device scale factor is updated only when the canvas’s size changes. Is that really the design?
>> 
>> I don’t like the design of this setSize function. Before the change, the function was like changing width and height with the DOM, but optimized in two ways:
>> 
>> 1) If the size did not change, it skipped the reset. Setting width and height through the DOM does not have that kind of optimization.
>> 2) If the size did change, then it did one reset rather than the two you’d get if you set both width and height through the DOM.
>> 
>> For me, (1) does not seem like a good thing, It’s strange that this setSize function has this optimization, but setting the width and height DOM attributes or content attributes does not. On the other hand, to me (2) seems purely good because it boosts performance but otherwise is not detectable.
>> 
>> What code calls this function? Are those callers really relying on behavior (1)? If they are not, then we could just remove the early return rather than adding the mysterious scale factor check here.
> 
> Note, the layout test does not test this setSize change.

I had the same inclination as you (just remove it). However, this function is only used by Document::getCSSCanvasContext. That function takes a width and height and passes it to setSize. We don't want to reset the canvas each time getCSSCanvasContext is called from JavaScript. So that is why this function differs from the DOM path.

We can likely remove the optimization to only do one reset, because reset already has a fast path for consecutive calls. The fast path might not have existed when this optimization was added. Keeping it does no harm though.

The layout test does test the setSize change by calling getCSSCanvasContext before and after the pixel ratio change and passing the same size.

[Timothy Hatcher]

http://trac.webkit.org/changeset/134099