Bug 73011

Summary: Change ImageData to reference Uint8ClampedArray rather than CanvasPixelArray
Product: WebKit Reporter: Antranig Basman <amb26webkit>
Component: WebGLAssignee: Kenneth Russell <kbr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andrew_j_baker2, bashi, chuck, cmarrin, darin, dino, ericbidelman, fpizlo, gns, haraken, japhet, kbr, mdelaney7, ojan, oliver, pnormand, simon.fraser, webkit.review.bot, xan.lopez, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://ponder.org.uk/chromeTest/client.html
Bug Depends on: 74455, 76803    
Bug Blocks: 83655, 84741    
Attachments:
Description Flags
Patch
none
Patch for EWS bots
none
Patch
none
Rebased Patch none

Description Antranig Basman 2011-11-23 01:43:45 PST
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
Comment 1 Andrew J. Baker 2011-12-01 13:48:09 PST
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/
Comment 2 Charles Pritchard 2011-12-30 18:06:28 PST
Patch seems to be available on bug #74455
Comment 3 Kenneth Russell 2012-01-03 17:52:06 PST
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.
Comment 4 Kenneth Russell 2012-01-12 11:22:49 PST
Changing synopsis to be clearer.
Comment 5 Kenneth Russell 2012-04-10 18:53:48 PDT
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.
Comment 6 Filip Pizlo 2012-04-10 19:24:32 PDT
(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!
Comment 7 Kenneth Russell 2012-04-10 19:29:13 PDT
(In reply to comment #6)
> No objections from me, this sounds great!

Thanks; filed and took Bug 83655 for the cleanup.
Comment 8 Kenneth Russell 2012-04-20 09:14:13 PDT
Created attachment 138106 [details]
Patch
Comment 9 Oliver Hunt 2012-04-20 09:29:36 PDT
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 10 Philippe Normand 2012-04-20 09:29:59 PDT
Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12465245
Comment 11 Early Warning System Bot 2012-04-20 09:33:27 PDT
Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12483207
Comment 12 Early Warning System Bot 2012-04-20 09:36:53 PDT
Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12482216
Comment 13 Build Bot 2012-04-20 09:38:03 PDT
Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12467262
Comment 14 Gyuyoung Kim 2012-04-20 09:39:50 PDT
Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12479192
Comment 15 Kenneth Russell 2012-04-20 09:40:15 PDT
(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 16 Build Bot 2012-04-20 10:06:16 PDT
Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12470258
Comment 17 Oliver Hunt 2012-04-20 11:38:58 PDT
(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.
Comment 18 Kenneth Russell 2012-04-20 18:36:00 PDT
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.
Comment 19 Kenneth Russell 2012-04-20 18:40:59 PDT
Created attachment 138212 [details]
Patch for EWS bots
Comment 20 Kenneth Russell 2012-04-23 10:42:49 PDT
Created attachment 138380 [details]
Patch
Comment 21 Kenneth Russell 2012-04-23 10:45:28 PDT
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 22 Oliver Hunt 2012-04-23 10:54:29 PDT
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 23 WebKit Review Bot 2012-04-23 15:33:36 PDT
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
Comment 24 Kenneth Russell 2012-04-23 16:03:06 PDT
Created attachment 138450 [details]
Rebased Patch
Comment 25 WebKit Review Bot 2012-04-23 20:43:50 PDT
Comment on attachment 138450 [details]
Rebased Patch

Clearing flags on attachment: 138450

Committed r114992: <http://trac.webkit.org/changeset/114992>
Comment 26 WebKit Review Bot 2012-04-23 20:43:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Darin Adler 2012-04-24 10:24:24 PDT
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?
Comment 28 Kenneth Russell 2012-04-24 10:39:45 PDT
(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.
Comment 29 Kenneth Russell 2012-04-24 11:47:05 PDT
(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 .