Bug 16954 – Support putImageData

• Component: DOM
• Status: RESOLVED FIXED

[Oliver Hunt]

We need to support the putImageData operation on canvas

[Oliver Hunt]

Created attachment 18574 [details]
Initial patch

Here's a first run at the patch, as well as a test case.

Currently it uses an ifdef PLATFORM(CG) in HTMLCanvasContext.cpp, but it should really be turned into a new method on the platform grpahics context.

[Mark Rowe (bdash)]

It might be worth using vImage for the premulitplication if it's available (and if testing shows it's actually a speedup in common cases).

[Eric Seidel (no email)]

Comment on attachment 18574 [details]
Initial patch

Not checking exceptions after each get will cause unexpected behavior.  Since you'll continue executing javascript even after you have an exception in the exec state.

[Eric Seidel (no email)]

Comment on attachment 18574 [details]
Initial patch

bah! no more #if PLATFORM!  We have to start moving to a platform abstraction, even if this is the first method added to a JSCanvasRenderingContext2DPlatform platform-implemented helper object.

[Mark Rowe (bdash)]

It's probably worth looking at how the Android fork abstracted out the canvas operations before inventing some new approach ourselves.

[Eric Seidel (no email)]

I also think that we should change all of the [custom] methods in CanvasRenderingContext2D.idl to actually document their args instead of using /* 3 */ etc.

[Alp Toker]

+ #include <JavaScriptCore/array_instance.h>

should be

#include <kjs/array_instance.h>

[Oliver Hunt]

Created attachment 18600 [details]
A much tidier version of the putImageData patch

Still needs (at minimum) stubs for gtk, qt, and wx

[Oliver Hunt]

Created attachment 18618 [details]
A hopefully "complete" patch

[Oliver Hunt]

Have made additional changes
diff --git a/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp b/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp
index 07fae93..2ab45e9 100644
--- a/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp
+++ b/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp
@@ -265,7 +265,7 @@ JSValue* JSCanvasRenderingContext2D::drawImageFromRect(ExecState* exec, const Li
 JSValue* JSCanvasRenderingContext2D::putImageData(ExecState* exec, const List& args)
 {
     JSValue* value = args[0];
-    if (!value->isObject() || args.size() != 3)
+    if (args.size() != 3 || !value->isObject())
         return throwError(exec, TypeError);
 
     int xPos = args[1]->toInt32(exec);   
@@ -304,7 +304,7 @@ JSValue* JSCanvasRenderingContext2D::putImageData(ExecState* exec, const List& a
     int height;
     
     if (!jsWidth->getTruncatedInt32(width) || !jsHeight->getTruncatedInt32(height) || width <= 0 || height <= 0
-        || !data->isObject() || !static_cast<JSObject *>(data)->inherits(&ArrayInstance::info))
+        || !data->isObject() || !static_cast<JSObject*>(data)->inherits(&ArrayInstance::info))
         return throwError(exec, TypeError);
     
     unsigned bufferSize = width * height;

[Alp Toker]

+++ b/WebCore/html/CanvasRenderingContext2DGtk.cpp

This should be CanvasRenderingContext2DCairo.cpp

Also, the default license for Cairo stuff in WebKit is generally LGPL (as long as I'm writing it).

Cairo implementation coming up in a jiffy, nice work!

[Eric Seidel (no email)]

Comment on attachment 18618 [details]
A hopefully "complete" patch

I think this looks fine.

I would suggest replacing:
// Accursed flipped y axis
with a more useful comment, perhaps:
// CG flips the y-axis, so we unflip it here

 [Custom] void putImageData(/* 3 */);
should have real argument names, even if the other functions in that section don't. :p

These could be re-written as simpler statements using std:min:
1040    int endX = dest.x() + data.m_width;
1041    if (endX > m_canvas->width())
1042        endX = m_canvas->width();

We really should have a test case which demonstrates the "convert any exception to a TypeError exception" behavior.

Otherwise it's fine.

[Oliver Hunt]

Comment on attachment 18618 [details]
A hopefully "complete" patch

*sigh*

Need a way to convert null, numbers, and strings that contain numbers to ints

Note "true" cannot be treat as 1 if we are to match other browsers.

[Oliver Hunt]

Created attachment 19312 [details]
First pass at new implementation

[Oliver Hunt]

Created attachment 19316 [details]
Here we go!

[Darin Adler]

Comment on attachment 19316 [details]
Here we go!

I have a lot of comments, so review- for now.

 29 function dataToArray(data) {

You should put those function braces on new lines.

 43 function pixelShouldBe(ctx, x, y, colour) {

The shouldBe function from the JavaScript testing script can handle comparing arrays and it colors the PASS and FAIL strings too. I wish you would use that.

 351     if (args.size() == 3)

Is size() really the right thing here? We normally try to base things on whether the arguments are defined or not and never actually look at the size. And we normally allow too few or too many parameters.

 350     ExceptionCode ec;

If you're going to call setDOMException with the result from ec, then you need to initialize the ExceptionCode to 0. The function you're calling has no obligation to set the code to 0 if the function succeeds.

 1214     if (!buffer || !isfinite(dx) || !isfinite(dy))
 1215         return;

Seems a little strange to check dx and dy to see if they are finite *after* checking the buffer. No big deal either way.

 1221     ec = 0;

No need to set ec to 0. If there was a need to do so, we'd want to do that before the first return. But you should just remove it.

The negative number stuff looks really crazy to me. Is that really the correct logic? Are there test cases that cover all these?

 1243     if (dirtyX + dirtyWidth > data->width())
 1244         dirtyWidth = data->width() - dirtyX;

Could this be done more readably with the min function?

What prevents dirtyX from being > data->width()? Test case?

What prevents dirtyY from being > data->height()? Test case?

 1250     if (!dirtyRect.width() || !dirtyRect.height())
 1251         return;

There's an isEmptyFunction you can use for this.

I don't see any tests for all the different ways you do and don't get exceptions with this function. I think we need those.

I don't see any tests for all the special cases for negative numbers. We need those.

 73         void putImageData(ImageData*, const IntPoint& targetPoint, const IntRect& dirtyRect);

I don't think that "targetPoint" and "dirtyRect" are clear enough. Where exactly does the image go? Is targetPoint the top/left of something?

 167     int destx = targetPoint.x() + dirtyRect.x();

This can overflow, with undefined results. We need to check for that and make sure we get well-defined results.

 171         originx -= destx;

Maybe this can overflow too?

 174     if (originx > dirtyRect.x() + dirtyRect.width())

The value x() + width() is called right(). But of course it can overflow.

 176     int endx = targetPoint.x() + dirtyRect.x() + dirtyRect.width();

This can definitely overflow.

 199     // -originy to handle the accursed flipped y axis

This comment is not followed by any code that contains -originy. Why the comment then? What does the comment mean?

 211                 reinterpret_cast<uint32_t*>(destRows + x * 4)[0] = reinterpret_cast<uint32_t*>(srcRows + x * 4)[0];

Do we have a guarantee that the buffer is suitably aligned? If not, then we need to consider portability to processors that aren't able to do this operation.

[Oliver Hunt]

Created attachment 19321 [details]
Hopefully address everyones concerns

[Oliver Hunt]

Comment on attachment 19321 [details]
Hopefully address everyones concerns

About to push a better test case

[Oliver Hunt]

Created attachment 19329 [details]
Now with betterer testcases!!!

[Darin Adler]

Comment on attachment 19329 [details]
Now with betterer testcases!!!

Looks pretty good, but there are still some things I'd like to see fixed.

 351     if (args.size() == 7)

I think this should be ">= 7". There are no test cases passing "undefined". You should include those.

 1237     IntRect sourceRect(static_cast<int>(clipRect.x()) + destX, static_cast<int>(clipRect.y()) + destY, 
 1238                        static_cast<int>(clipRect.width()), static_cast<int>(clipRect.height()));

Why exactly does this use casts to int rather than the enclosingIntRect function or ceil or floor?

 1235     int destX = static_cast<int>(dx);
 1236     int destY = static_cast<int>(dy);

The code would come out better if this was an IntSize.

 1237     IntRect sourceRect(static_cast<int>(clipRect.x()) + destX, static_cast<int>(clipRect.y()) + destY, 
 1238                        static_cast<int>(clipRect.width()), static_cast<int>(clipRect.height()));

If destX/Y was an IntSize, this could be an assignment followed by a IntRect::move call.

 1234     clipRect.intersect(IntRect(0, 0, data->width(), data->height()));
 1239     sourceRect.intersect(IntRect(0, 0, buffer->size().width(), buffer->size().height()));

I think there's a nicer way to write these using the IntPoint and IntSize version of the constructor. We might even want to add a constructor that takes only an IntSize.

 1243     sourceRect.setX(sourceRect.x() - destX);
 1244     sourceRect.setY(sourceRect.y() - destY);

IntRect has a "move" function for making this kind of change. It could be sourceRect.move(-destOffset).

I don't think the test cases with negative values for "dirty width" and "dirty height" are good enough. They seem to test only cases that are entirely clipped out; I'm still confused by the meanings of negative numbers for these given the code implementing it and I'd really like to see test cases to clarify what we're doing is right.

I think we are using "float" way too much in CanvasRenderingContext2D. We should use double instead of float as much as possible, since JavaScript always uses doubles. The conversion to float should be inside the GraphicsContext class. When we get to 64-bit, it's going to be double even in there.

 161     ASSERT(sourceRect.width() > 0);
 162     ASSERT(sourceRect.height() > 0);

Should be ASSERT(!sourceRect.isEmpty()).

 171     int endx = destPoint.x() + sourceRect.x() + sourceRect.width();

Should use sourceRect.right().

 181     ASSERT(originy <= sourceRect.x() + sourceRect.height());

x + height is obviously wrong. This should use bottom().

 183     int endy = destPoint.y() + sourceRect.y() + sourceRect.height();

This should use bottom().

 190     // -originy to handle the accursed flipped y axis

We still have a comment that says "-originy", but no code that mentions it. Why is the Y axis flipped? Don't we always do everything to make it not be flipped?

 202                 reinterpret_cast<uint32_t*>(destRows + x * 4)[0] = reinterpret_cast<uint32_t*>(srcRows + x * 4)[0];

You didn't respond to my comments about alignment.

[Oliver Hunt]

Created attachment 19344 [details]
now with more tests and logic tidying

[Sam Weinig]

Comment on attachment 19344 [details]
now with more tests and logic tidying

The TYPE_MISMATCH_ERR exception can be thrown in the implementation of putImageData instead of in the binding.

r=me

[Oliver Hunt]

Landed r30700