Bug 100611 - CSS canvas renders blurry at 2x pixel ratio
Summary: CSS canvas renders blurry at 2x pixel ratio
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-10-28 10:12 PDT by Timothy Hatcher
Modified: 2013-01-29 10:11 PST (History)
16 users (show)

See Also:


Attachments
Proposed Change (14.32 KB, patch)
2012-10-28 14:01 PDT, Timothy Hatcher
dino: review+
timothy: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2012-10-28 10:12:04 PDT
If you create a 2x pixel ratio CSS canvas it will render at 1x.

<rdar://problem/12574189>
Comment 1 Radar WebKit Bug Importer 2012-10-28 10:12:22 PDT
<rdar://problem/12588963>
Comment 2 Timothy Hatcher 2012-10-28 14:01:59 PDT
Created attachment 171143 [details]
Proposed Change
Comment 3 WebKit Review Bot 2012-10-29 00:01:44 PDT
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
Comment 4 Tom Hudson 2012-11-02 04:15:14 PDT
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.
Comment 5 Timothy Hatcher 2012-11-02 07:21:16 PDT
(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.
Comment 6 Timothy Hatcher 2012-11-02 07:22:39 PDT
(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.
Comment 7 Dean Jackson 2012-11-08 21:23:44 PST
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.
Comment 8 Timothy Hatcher 2012-11-09 07:11:46 PST
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.
Comment 9 Timothy Hatcher 2012-11-09 12:41:05 PST
http://trac.webkit.org/changeset/134100
Comment 10 Nico Weber 2013-01-29 10:11:48 PST
(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/