Pixel access canvas APIs do not operate at backing store resolution

• Component: Canvas
• Status: RESOLVED FIXED

Pixel access canvas APIs do not operate at backing store resolution

<rdar://problem/10912680>

<http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-March/035112.html> proposes to solve this using {get,put}ImageDataHD functions.

Created attachment 137001 [details]
Add webkitGetImageDataHD and webkitPutImageDataHD to CanvasRenderingContext2D

[Adam Barth]

Comment on attachment 137001 [details]
Add webkitGetImageDataHD and webkitPutImageDataHD to CanvasRenderingContext2D

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:210
> +        void webkitPutImageDataHD(in ImageData imagedata, in float dx, in float dy)
> +            raises(DOMException);
> +        void webkitPutImageDataHD(in ImageData imagedata, in float dx, in float dy, in float dirtyX, in float dirtyY, in float dirtyWidth, in float dirtyHeight)
> +            raises(DOMException);

Is it worth announcing this new feature on webkit-dev?

[Adam Barth]

Would you also be willing to update https://trac.webkit.org/wiki/PrefixedAPIs with this new prefixed API and a reference to the specification?

(In reply to comment #3)
> (From update of attachment 137001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137001&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:210
> > +        void webkitPutImageDataHD(in ImageData imagedata, in float dx, in float dy)
> > +            raises(DOMException);
> > +        void webkitPutImageDataHD(in ImageData imagedata, in float dx, in float dy, in float dirtyX, in float dirtyY, in float dirtyWidth, in float dirtyHeight)
> > +            raises(DOMException);
> 
> Is it worth announcing this new feature on webkit-dev?

I’m not sure, since this is merely an enhancement to existing API, but I am going to send out a message.

(In reply to comment #4)
> Would you also be willing to update https://trac.webkit.org/wiki/PrefixedAPIs with this new prefixed API and a reference to the specification?

Yes.

[Adam Barth]

Thanks!

[WebKit Review Bot]

Comment on attachment 137001 [details]
Add webkitGetImageDataHD and webkitPutImageDataHD to CanvasRenderingContext2D

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

New failing tests:
fast/canvas/2d.imageDataHD.html
platform/chromium/virtual/gpu/fast/canvas/2d.imageDataHD.html

[WebKit Review Bot]

Created attachment 137035 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #7)
> (From update of attachment 137001 [details])
> Attachment 137001 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12393673
> 
> New failing tests:
> fast/canvas/2d.imageDataHD.html
> platform/chromium/virtual/gpu/fast/canvas/2d.imageDataHD.html

I am going to make the test exit early if the canvas doesn’t have a high-DPI backing store, and include expected results for that case.

[Darin Adler]

Comment on attachment 137001 [details]
Add webkitGetImageDataHD and webkitPutImageDataHD to CanvasRenderingContext2D

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1871
> +void CanvasRenderingContext2D::webkitPutImageDataHD(ImageData* data, float dx, float dy, float dirtyX, float dirtyY,
> +                                            float dirtyWidth, float dirtyHeight, ExceptionCode& ec)

I guess your editor tried to line up subsequent lines with the opening parenthesis on the first line, but later you renamed the function.

This is why I prefer to use very long lines, or some other way of indenting other than lining up each line with the next.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:230
> +        ImageData webkitGetImageDataHD(in [Optional=DefaultIsUndefined] float sx, in [Optional=DefaultIsUndefined] float sy,
> +                                       in [Optional=DefaultIsUndefined] float sw, in [Optional=DefaultIsUndefined] float sh)

What behavior would we get if we just said [Optional] instead of [Optional=DefaultIsUndefined]?

A side note: For a numeric argument the name of that modifier seems wrong. I think the only thing that needs to be specified in IDL is whether we want a 0 or a NaN for missing arguments.

> Source/WebCore/platform/graphics/ImageBuffer.h:99
> +        enum CoordinateSystem { LogicalCoordinateSystem, BackingStoreCoordinateSystem };

It almost seems that if we named this right it could be a WebCore-namespace-level type instead of a class-internal type. It’s really awkward outside the class when we have to spell it out.

> Source/WebCore/platform/graphics/ImageBuffer.h:104
> +        void putByteArray(Multiply multiplied, ByteArray*, const IntSize& sourceSize, const IntRect& sourceRect, const IntPoint& destPoint, CoordinateSystem = LogicalCoordinateSystem);

I think some day we should come back here and rename the type "Multiply" since that word is a verb and makes a confusing type name. The argument name "multiplied" is similarly unclear for no really good reason.

Comment on attachment 137001 [details]
Add webkitGetImageDataHD and webkitPutImageDataHD to CanvasRenderingContext2D

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

Thanks for the review!

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1871
>> +                                            float dirtyWidth, float dirtyHeight, ExceptionCode& ec)
> 
> I guess your editor tried to line up subsequent lines with the opening parenthesis on the first line, but later you renamed the function.
> 
> This is why I prefer to use very long lines, or some other way of indenting other than lining up each line with the next.

Indeed, I renamed after copying from purImageData() and didn’t notice the weird alignment. Fixed.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:230
>> +                                       in [Optional=DefaultIsUndefined] float sw, in [Optional=DefaultIsUndefined] float sh)
> 
> What behavior would we get if we just said [Optional] instead of [Optional=DefaultIsUndefined]?
> 
> A side note: For a numeric argument the name of that modifier seems wrong. I think the only thing that needs to be specified in IDL is whether we want a 0 or a NaN for missing arguments.

I don’t know about this. Admittedly, I just copied the getImageData declaration.

>> Source/WebCore/platform/graphics/ImageBuffer.h:99
>> +        enum CoordinateSystem { LogicalCoordinateSystem, BackingStoreCoordinateSystem };
> 
> It almost seems that if we named this right it could be a WebCore-namespace-level type instead of a class-internal type. It’s really awkward outside the class when we have to spell it out.

I agree, but I don’t know what the right names would be. I don’t think it’s too awkward the way I did it, though.

Fixed in <http://trac.webkit.org/r114150>.

(In reply to comment #4)
> Would you also be willing to update https://trac.webkit.org/wiki/PrefixedAPIs with this new prefixed API and a reference to the specification?

In <https://trac.webkit.org/wiki/PrefixedAPIs?action=diff&version=26>.