Bug 100611 – CSS canvas renders blurry at 2x pixel ratio

• Component: Canvas
• Status: RESOLVED FIXED

[Timothy Hatcher]

If you create a 2x pixel ratio CSS canvas it will render at 1x.

<rdar://problem/12574189>

[Radar WebKit Bug Importer]

<rdar://problem/12588963>

[Timothy Hatcher]

Created attachment 171143 [details]
Proposed Change

[WebKit Review Bot]

Comment on attachment 171143 [details]
Proposed Change

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

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-as-image-hidpi.html
fast/canvas/canvas-as-image-hidpi.html

[Tom Hudson]

I think you're going to have to update ImageBufferSkia to pay attention to your new parameter; it's already got a member variable tracking its scale.

Could we come up with a better name for the parameter and the enum values? I really can't tell reading the declaration or call sites what the new code is intended to do, or why you need to override the default parameter value.

[Timothy Hatcher]

(In reply to comment #4)
> I think you're going to have to update ImageBufferSkia to pay attention to your new parameter; it's already got a member variable tracking its scale.
> 
> Could we come up with a better name for the parameter and the enum values? I really can't tell reading the declaration or call sites what the new code is intended to do, or why you need to override the default parameter value.

It has a member tracking the resolution, but it never scaled like ImageBufferCG. So it isn't handling HiDPI the same as CG. I can add a FIXME and file a bug for Skia. But I don't know Skia/Chromium to be able to fix it.

[Timothy Hatcher]

(In reply to comment #4)
> Could we come up with a better name for the parameter and the enum values? I really can't tell reading the declaration or call sites what the new code is intended to do, or why you need to override the default parameter value.

As far as the name goes I am open to ideas. But this was the best I could think of while keeping it terse.

[Dean Jackson]

Comment on attachment 171143 [details]
Proposed Change

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

> LayoutTests/ChangeLog:3
> +        Test if -webkit-canvas in CSS uses the full backing store instead of alway 1x when rendering.

Typo: always

> LayoutTests/fast/canvas/canvas-as-image-hidpi.html:19
> +        ctx.fillStyle = "rgb(200, 0, 0)";
> +        ctx.fillRect(10, 10, 50, 50);
> +
> +        ctx.fillStyle = "rgba(0, 0, 200, 0.5)";
> +        ctx.fillRect(25, 25, 90, 90);

I'm not sure this will actually test the change. This particular code drawn into 100x100 and then stretched would be indistinguishable from the 200x200 image. Or am I missing it?

I can't think of a good way to test otherwise though.

> Source/WebCore/ChangeLog:3
> +        Make -webkit-canvas in CSS use the full backing store instead of alway 1x when rendering.

Typo: always

> Source/WebCore/platform/graphics/ImageBuffer.h:81
> +    enum Scale {
> +        Scaled,
> +        Unscaled
> +    };

Should this be called ScaleBehavior or something? Scale is a slightly ambiguous word.

[Timothy Hatcher]

Comment on attachment 171143 [details]
Proposed Change

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

>> LayoutTests/fast/canvas/canvas-as-image-hidpi.html:19
>> +        ctx.fillRect(25, 25, 90, 90);
> 
> I'm not sure this will actually test the change. This particular code drawn into 100x100 and then stretched would be indistinguishable from the 200x200 image. Or am I missing it?
> 
> I can't think of a good way to test otherwise though.

I did run the test before and after the change and the edges were blurry before.

>> Source/WebCore/platform/graphics/ImageBuffer.h:81
>> +    };
> 
> Should this be called ScaleBehavior or something? Scale is a slightly ambiguous word.

I can call it ScaleBehavior.

[Timothy Hatcher]

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

[Nico Weber]

(In reply to comment #5)
> (In reply to comment #4)
> > I think you're going to have to update ImageBufferSkia to pay attention to your new parameter; it's already got a member variable tracking its scale.
> > 
> > Could we come up with a better name for the parameter and the enum values? I really can't tell reading the declaration or call sites what the new code is intended to do, or why you need to override the default parameter value.
> 
> It has a member tracking the resolution, but it never scaled like ImageBufferCG. So it isn't handling HiDPI the same as CG. I can add a FIXME and file a bug for Skia. But I don't know Skia/Chromium to be able to fix it.

That's intentional, chrome is matching the safari/ios model, not the safari/mac one: http://www.html5rocks.com/en/tutorials/canvas/hidpi/