Bug 73011 – Change ImageData to reference Uint8ClampedArray rather than CanvasPixelArray

• Component: WebGL
• Status: RESOLVED FIXED

[Antranig Basman]

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

[Andrew J. Baker]

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/

[Charles Pritchard]

Patch seems to be available on bug #74455

[Kenneth Russell]

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.

[Kenneth Russell]

Changing synopsis to be clearer.

[Kenneth Russell]

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.

[Filip Pizlo]

(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!

[Kenneth Russell]

(In reply to comment #6)
> No objections from me, this sounds great!

Thanks; filed and took Bug 83655 for the cleanup.

[Kenneth Russell]

Created attachment 138106 [details]
Patch

[Oliver Hunt]

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?

[Philippe Normand]

Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12465245

[Early Warning System Bot]

Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12483207

[Early Warning System Bot]

Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12482216

[Build Bot]

Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12467262

[Gyuyoung Kim]

Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12479192

[Kenneth Russell]

(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.

[Build Bot]

Comment on attachment 138106 [details]
Patch

Attachment 138106 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12470258

[Oliver Hunt]

(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.

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 138212 [details]
Patch for EWS bots

[Kenneth Russell]

Created attachment 138380 [details]
Patch

[Kenneth Russell]

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().

[Oliver Hunt]

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!

[WebKit Review Bot]

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

[Kenneth Russell]

Created attachment 138450 [details]
Rebased Patch

[WebKit Review Bot]

Comment on attachment 138450 [details]
Rebased Patch

Clearing flags on attachment: 138450

Committed r114992: <http://trac.webkit.org/changeset/114992>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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?

[Kenneth Russell]

(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.

[Kenneth Russell]

(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 .