I reported this issue originally with Chromium and was told to report it here instead: http://code.google.com/p/chromium/issues/detail?id=101378 I can reproduce the error in all versions of Chrome I have tried since Chrome 14 - unfortunately I can't reproduce the issue in WebKit since nightlies won't start on my machine (Windows 7, AMD x64) for a reason I can't determine. The demonstration page referenced is working in Firefox 7 and 8. What steps will reproduce the problem? 1. Browse to the above URL 2. Type command "sph" in command interpreter at top left What is the expected result? A moving pattern of red, yellow and black bands should appear What happens instead? Receive a "TypeError" from Chrome - nothing is rendered Please provide any additional information below. Attach a screenshot if possible. The line responsible for the failure is this one (complex.js line 29) gl.readPixels(0, 0, config.width, config.height, gl.RGBA, gl.UNSIGNED_BYTE, imageData.data); According to the HTML5 spec, the "data" element of the ImageData structure should be a "Uint8ClampedArray" which one would expect is compatible with the array format read into by gl.readPixels (array of RGBA unsigned bytes). This is the case on recent Firefoxes and the call succeeds. However, on Chrome the type of imageData.data is shown as "CanvasPixelArray" which is not type compatible and the gl.readPixels call fails. http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-putimagedata
For potential performance improvements to the 2D context as well as 3D, please see my article here... http://hacks.mozilla.org/2011/12/faster-canvas-pixel-manipulation-with-typed-arrays/
Patch seems to be available on bug #74455
The patch on bug 74455 only adds the Uint8ClampedArray type; it doesn't wire it up to the Canvas implementation. This bug can depend on that one.
Changing synopsis to be clearer.
I've begun work on this. It turns out that because the representation of ImageData changes from WTF::ByteArray to Uint8ClampedArray, it's necessary to change some other methods (ImageBuffer's getUnmultipliedImageData, getPremultipliedImageData and putByteArray) from ByteArray to Uint8ClampedArray as well, in order to avoid introducing, even temporarily, some extremely inefficient data copies. The Filter Effects implementation consequently must change simultaneously from ByteArray to Uint8ClampedArray. After these changes, it's actually possible to delete ByteArray, JSByteArray and related code from JavaScriptCore! In order to keep the size of the initial patch manageable, I am planning to leave ByteArray and JSByteArray in the source tree at first, and once this patch lands, immediately follow up with another patch which removes them from the tree and all build systems. If anyone objects to the plan above due to temporarily incurring technical debt, please follow up.
(In reply to comment #5) > I've begun work on this. It turns out that because the representation of ImageData changes from WTF::ByteArray to Uint8ClampedArray, it's necessary to change some other methods (ImageBuffer's getUnmultipliedImageData, getPremultipliedImageData and putByteArray) from ByteArray to Uint8ClampedArray as well, in order to avoid introducing, even temporarily, some extremely inefficient data copies. > > The Filter Effects implementation consequently must change simultaneously from ByteArray to Uint8ClampedArray. > > After these changes, it's actually possible to delete ByteArray, JSByteArray and related code from JavaScriptCore! > > In order to keep the size of the initial patch manageable, I am planning to leave ByteArray and JSByteArray in the source tree at first, and once this patch lands, immediately follow up with another patch which removes them from the tree and all build systems. > > If anyone objects to the plan above due to temporarily incurring technical debt, please follow up. No objections from me, this sounds great!
(In reply to comment #6) > No objections from me, this sounds great! Thanks; filed and took Bug 83655 for the cleanup.
Created attachment 138106 [details] Patch
Comment on attachment 138106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138106&action=review > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:-131 > - if (rect.x() < 0 || rect.y() < 0 || endx > size.width() || endy > size.height()) > - memset(data, 0, result->length()); why is this removed? Does uint8array zero initialize? if so we probably want a non-zero initializing version so we can avoid the double fill here. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:343 > +void ImageBufferData::putData(Uint8ClampedArray*& source, const IntSize& sourceSize, const IntRect& sourceRect, const IntPoint& destPoint, const IntSize& size, bool accelerateRendering, bool unmultiplied, float resolutionScale) Errr, not a change in this patch per se, but why Uint8ClampedArray*&? why does source need to be a reference? > Source/WebCore/platform/graphics/filters/FEBlend.cpp:110 > + unsigned char alphaA = srcPixelArrayA->item(pixelOffset + 3); Just for my own sanity: is item() non-virtual? > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:-249 > - if (rect.x() < 0 > - || rect.y() < 0 > - || rect.maxX() > size.width() > - || rect.maxY() > size.height()) > - memset(data, 0, result->length()); > - and again, are we introducing an unnecessary clear and set?
Comment on attachment 138106 [details] Patch Attachment 138106 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12465245
Comment on attachment 138106 [details] Patch Attachment 138106 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12483207
Comment on attachment 138106 [details] Patch Attachment 138106 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12482216
Comment on attachment 138106 [details] Patch Attachment 138106 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12467262
Comment on attachment 138106 [details] Patch Attachment 138106 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12479192
(In reply to comment #9) > (From update of attachment 138106 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138106&action=review > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:-131 > > - if (rect.x() < 0 || rect.y() < 0 || endx > size.width() || endy > size.height()) > > - memset(data, 0, result->length()); > > why is this removed? Does uint8array zero initialize? if so we probably want a non-zero initializing version so we can avoid the double fill here. This is briefly mentioned in the ChangeLog. Uint8ClampedArray zero-initializes by virtue of ArrayBuffer always zero-initializing (via tryFastCalloc). I'm somewhat reluctant to introduce a non-zero-initializing variant, because it isn't clear that there'll be a performance win (allocation is relatively expensive anyway), and the calling code becomes simpler and more obviously correct when the allocations are zero-initialized. > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:343 > > +void ImageBufferData::putData(Uint8ClampedArray*& source, const IntSize& sourceSize, const IntRect& sourceRect, const IntPoint& destPoint, const IntSize& size, bool accelerateRendering, bool unmultiplied, float resolutionScale) > > Errr, not a change in this patch per se, but why Uint8ClampedArray*&? why does source need to be a reference? I'm not sure -- in this case I only made the syntactic change. Would you like me to investigate this further in a follow-up patch? > > Source/WebCore/platform/graphics/filters/FEBlend.cpp:110 > > + unsigned char alphaA = srcPixelArrayA->item(pixelOffset + 3); > > Just for my own sanity: is item() non-virtual? Yes, it's non-virtual. It's defined in all of the concrete typed array view classes, mostly via templates. > > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:-249 > > - if (rect.x() < 0 > > - || rect.y() < 0 > > - || rect.maxX() > size.width() > > - || rect.maxY() > size.height()) > > - memset(data, 0, result->length()); > > - > > and again, are we introducing an unnecessary clear and set? As above, Uint8ClampedArray allocations are zeroed, so this check becomes unnecessary. If any of these answers are unsatisfactory please adjust the review bit. I'll look into the various EWS failures later.
Comment on attachment 138106 [details] Patch Attachment 138106 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12470258
(In reply to comment #15) > (In reply to comment #9) > > (From update of attachment 138106 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=138106&action=review > > > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:-131 > > > - if (rect.x() < 0 || rect.y() < 0 || endx > size.width() || endy > size.height()) > > > - memset(data, 0, result->length()); > > > > why is this removed? Does uint8array zero initialize? if so we probably want a non-zero initializing version so we can avoid the double fill here. > > This is briefly mentioned in the ChangeLog. Uint8ClampedArray zero-initializes by virtue of ArrayBuffer always zero-initializing (via tryFastCalloc). I'm somewhat reluctant to introduce a non-zero-initializing variant, because it isn't clear that there'll be a performance win (allocation is relatively expensive anyway), and the calling code becomes simpler and more obviously correct when the allocations are zero-initialized. In the image data from canvas, the allocation time is very low compared to the fill cost. I'd like to see perf numbers comparing getImageData of an entire canvas a few thousand times before and after this patch.
It does look like the mandatory zeroing increases the time of getImageData() by about 10%. Timings for 1000 calls to getImageData against a 1024x1024 canvas: TOT Chromium: 10180 ms 10322 ms 9990 ms 9966 ms Average: 10114.5 My build: 10595 ms 10671 ms 11330 ms 11603 ms 11493 ms Average: 11138.4 I'll add a createUninitialized() factory method to Uint8ClampedArray and change this patch to use it. In the meantime, I'm submitting another patch just to see if it passes the EWS bots.
Created attachment 138212 [details] Patch for EWS bots
Created attachment 138380 [details] Patch
This revised patch adds Uint8ClampedArray::createUninitialized() and uses it throughout the code paths which allocated uninitialized ByteArrays. Oliver, as you suspected, this eliminates the performance regression of the previous patch. 1000 calls to getImageData() now take: 9527 ms 9441 ms 9723 ms 9687 ms 9556 ms Average: 9586.8 I don't know exactly why it's faster than the old code path, but suspect that some work that went into efficiently GC'ing typed arrays might not have been done for the old CanvasPixelArray path. The new patch also replaces a few manual calls to memset() with zeroFill(), which replaces ByteArray::clear().
Comment on attachment 138380 [details] Patch It's possibly related to some of the inanity JSC had wrt to providing "ByteArray" that claimed to be a "CanvasPixelArray", woo!
Comment on attachment 138380 [details] Patch Rejecting attachment 138380 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: LayoutTests/fast/dom/Window/script-tests/postmessage-clone.js patching file LayoutTests/fast/dom/Window/window-postmessage-clone-expected.txt patching file LayoutTests/platform/chromium/fast/dom/Window/window-postmessage-clone-expected.txt patching file LayoutTests/platform/gtk/fast/dom/Window/window-postmessage-clone-expected.txt Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Oliver Hunt']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12522045
Created attachment 138450 [details] Rebased Patch
Comment on attachment 138450 [details] Rebased Patch Clearing flags on attachment: 138450 Committed r114992: <http://trac.webkit.org/changeset/114992>
All reviewed patches have been landed. Closing bug.
After this change, a regression test that had not previously failed started failing consistently on the Mac build. The test failing is fast/dom/Window/window-postmessage-clone.html, and the failure is this: - CONSOLE MESSAGE: line 86: TypeError: No default value - CONSOLE MESSAGE: line 86: TypeError: No default value These two lines are part of the expected results and are not showing up any more. This patch is big and there are no per-file-change comments so I can’t guess what aspect of the patch created these new failures. For example, I don’t know the significance of the changes to SerializedScriptValue.cpp and whether these changes would be expected to affect this test. The test does covers serialization of canvas image data, so I guess it’s not a surprise that the results changed. Can someone with sufficient expertise update the expected results if this is a progression?
(In reply to comment #27) > After this change, a regression test that had not previously failed started failing consistently on the Mac build. The test failing is fast/dom/Window/window-postmessage-clone.html, and the failure is this: > > - CONSOLE MESSAGE: line 86: TypeError: No default value > - CONSOLE MESSAGE: line 86: TypeError: No default value > > These two lines are part of the expected results and are not showing up any more. This patch is big and there are no per-file-change comments so I can’t guess what aspect of the patch created these new failures. For example, I don’t know the significance of the changes to SerializedScriptValue.cpp and whether these changes would be expected to affect this test. > > The test does covers serialization of canvas image data, so I guess it’s not a surprise that the results changed. Can someone with sufficient expertise update the expected results if this is a progression? Sorry about that -- I tried to update all the expectations appropriately. The failure is almost certainly a progression, not a regression, since the TypeErrors were probably happening because CanvasPixelArray wasn't cloneable. I'll file a bug and fix it right away.
(In reply to comment #28) > (In reply to comment #27) > > After this change, a regression test that had not previously failed started failing consistently on the Mac build. The test failing is fast/dom/Window/window-postmessage-clone.html, and the failure is this: > > > > - CONSOLE MESSAGE: line 86: TypeError: No default value > > - CONSOLE MESSAGE: line 86: TypeError: No default value > > > > These two lines are part of the expected results and are not showing up any more. This patch is big and there are no per-file-change comments so I can’t guess what aspect of the patch created these new failures. For example, I don’t know the significance of the changes to SerializedScriptValue.cpp and whether these changes would be expected to affect this test. > > > > The test does covers serialization of canvas image data, so I guess it’s not a surprise that the results changed. Can someone with sufficient expertise update the expected results if this is a progression? > > Sorry about that -- I tried to update all the expectations appropriately. The failure is almost certainly a progression, not a regression, since the TypeErrors were probably happening because CanvasPixelArray wasn't cloneable. I'll file a bug and fix it right away. Layout test issue fixed in https://bugs.webkit.org/show_bug.cgi?id=84741 / http://trac.webkit.org/changeset/115088 .