We need to support the putImageData operation on canvas
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.
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).
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.
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.
It's probably worth looking at how the Android fork abstracted out the canvas operations before inventing some new approach ourselves.
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.
+ #include <JavaScriptCore/array_instance.h> should be #include <kjs/array_instance.h>
Created attachment 18600 [details] A much tidier version of the putImageData patch Still needs (at minimum) stubs for gtk, qt, and wx
Created attachment 18618 [details] A hopefully "complete" patch
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;
+++ 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!
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.
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.
Created attachment 19312 [details] First pass at new implementation
Created attachment 19316 [details] Here we go!
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.
Created attachment 19321 [details] Hopefully address everyones concerns
Comment on attachment 19321 [details] Hopefully address everyones concerns About to push a better test case
Created attachment 19329 [details] Now with betterer testcases!!!
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.
Created attachment 19344 [details] now with more tests and logic tidying
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
Landed r30700