Fix for bug 6100:...

Fix for bug 6100:
- Introduce a CanvasPixelArray to serve as the holder for the ImageData
  pixel array of a canvas element.

This is the V8-specific change.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8330

[iposva, 2009-01-17 00:43:39]

Renamed ScriptPixelBuffer -> CanvasPixelArray to match the spec naming.

-Ivan

[eroman, 2009-01-17 06:13:48]

please also include tests_fixable.txt in this CL, to remove the the 19 layout
test failures from bug 6100.

http://codereview.chromium.org/18180/diff/206/63
File webkit/port/bindings/v8/CanvasPixelArray.cpp (right):

http://codereview.chromium.org/18180/diff/206/63#newcode41
Line 41: init(0);
im hoping this ctor can be deleted alltogether.
Not a fan of malloc(0).

http://codereview.chromium.org/18180/diff/206/63#newcode51
Line 51: m_length = 0;
why set length to 0 in dtor?

http://codereview.chromium.org/18180/diff/206/63#newcode57
Line 57: ASSERT((length & 3) == 0);  // Make sure the length is set correctly.
eew! why not simply: (length % 4) == 0 ?

in case you lifted this from old JSC version, note that they do the check on the
_buffer_, not the length. presumably to make sure their placement new will use a
32-bit aligned block.

so i don't think this is actually necessary.

http://codereview.chromium.org/18180/diff/206/63#newcode81
Line 81: if (!(value > 0)) {
Can you add a comment that this funkiness is specifically to clamp NaN to 0 ?

http://codereview.chromium.org/18180/diff/206/63#newcode86
Line 86: m_data[index] = static_cast<unsigned char>(value + 0.5);
I see that JSByteArray.h does a similar rounding, but I also see a this line in
Interpreter.cpp which makes me think the net effect will be truncation:

2908:
jsByteArray->setIndex(i, JSImmediate::getTruncatedInt32(jsValue));

http://codereview.chromium.org/18180/diff/206/62
File webkit/port/bindings/v8/CanvasPixelArray.h (right):

http://codereview.chromium.org/18180/diff/206/62#newcode41
Line 41: class CanvasPixelArray : public RefCounted<CanvasPixelArray> {
this should probably be named "ByteArray" to match the JSC analog.

also this name is expected by html/ImageData.cpp which contains a JSC::ByteArray

http://codereview.chromium.org/18180/diff/206/62#newcode47
Line 47: CanvasPixelArray();
is this default constructor necessary?

http://codereview.chromium.org/18180/diff/206/62#newcode48
Line 48: CanvasPixelArray(size_t length);
can this be private?

http://codereview.chromium.org/18180/diff/206/64
File webkit/port/bindings/v8/V8CanvasPixelArrayCustom.cpp (right):

http://codereview.chromium.org/18180/diff/206/64#newcode51
Line 51: if ((index < 0) || (index > pixelBuffer->length())) {
I believe you want a gt_eq here, as in:
"index >= pixelBuffer->length()"

http://codereview.chromium.org/18180/diff/206/67
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/18180/diff/206/67#newcode1663
Line 1663: desc->InstanceTemplate()->SetIndexedPropertyHandler(
Feng had pointed out that the image data is really just a glorified js array, so
it would be faster to have v8 manage the array's memory / accesses directly,
rather than using custom property handlers.

I didn't have enough v8-savvy to do this when adding the original
canvaspixelarray bindings, but maybe you do :-). Anyhow, figured I would mention
it.

see: http://code.google.com/p/chromium/issues/detail?id=3473

[Dean McNamee, 2009-01-18 16:55:35]

You need to update the CL description to say CanvasPixelArray.

http://codereview.chromium.org/18180/diff/206/72
File webkit/build/port/SConscript (right):

http://codereview.chromium.org/18180/diff/206/72#newcode154
Line 154: 'bindings/v8/ScriptPixelBuffer.idl',
I thought this was CanvasPixelArray

http://codereview.chromium.org/18180/diff/206/63
File webkit/port/bindings/v8/CanvasPixelArray.cpp (right):

http://codereview.chromium.org/18180/diff/206/63#newcode47
Line 47: }
I don't know what the webkit rules are, but if I was writing this in Chrome, I
would have inlined the constructor in the header file.

http://codereview.chromium.org/18180/diff/206/63#newcode57
Line 57: ASSERT((length & 3) == 0);  // Make sure the length is set correctly.
I don't see a problem with the assert.  I tend to use & a lot when mod power of
2, so this doesn't seem so strange to me.  I think a better comment would be

// Make sure the length is a multiple of 4.

On 2009/01/17 06:13:48, eroman wrote:
> eew! why not simply: (length % 4) == 0 ?
> 
> in case you lifted this from old JSC version, note that they do the check on
the
> _buffer_, not the length. presumably to make sure their placement new will use
a
> 32-bit aligned block.
> 
> so i don't think this is actually necessary.

http://codereview.chromium.org/18180/diff/206/63#newcode59
Line 59: m_data = static_cast<unsigned char*>(malloc(m_length));
There is some recent WebKit activity going on about webkit allocations, the
ability to replace the allocation functions, etc.  Basically if you want to be
using malloc, you should be using fastMalloc() and fastFree() in WebKit code.

http://codereview.chromium.org/18180/diff/206/63#newcode89
Line 89: } // namespace WebCore
two spaces before comments?

http://codereview.chromium.org/18180/diff/206/62
File webkit/port/bindings/v8/CanvasPixelArray.h (right):

http://codereview.chromium.org/18180/diff/206/62#newcode41
Line 41: class CanvasPixelArray : public RefCounted<CanvasPixelArray> {
We'll probably have to fork ImageData.cpp, or work somehow upstream.  I don't
like the name JSC::ByteArray, it's really misleading, as Ivan pointed out to me
it clamps values, etc.  It's is not an array of bytes.  It is meant to be an
array of pixels, and I think CanvasPixelArray is a much better name (and matches
the spec).

On 2009/01/17 06:13:48, eroman wrote:
> this should probably be named "ByteArray" to match the JSC analog.
> 
> also this name is expected by html/ImageData.cpp which contains a
JSC::ByteArray
> 
>

http://codereview.chromium.org/18180/diff/206/67
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/18180/diff/206/67#newcode1663
Line 1663: desc->InstanceTemplate()->SetIndexedPropertyHandler(
On 2009/01/17 06:13:48, eroman wrote:
> Feng had pointed out that the image data is really just a glorified js array,
so
> it would be faster to have v8 manage the array's memory / accesses directly,
> rather than using custom property handlers.
> 
> I didn't have enough v8-savvy to do this when adding the original
> canvaspixelarray bindings, but maybe you do :-). Anyhow, figured I would
mention
> it.

This is, after all, why WebKit moved from CanvasPixelArray to JSC::ByteArray,
right?

> 
> see: http://code.google.com/p/chromium/issues/detail?id=3473

[iposva, 2009-01-20 01:28:07]

http://codereview.chromium.org/18180/diff/206/72
File webkit/build/port/SConscript (right):

http://codereview.chromium.org/18180/diff/206/72#newcode154
Line 154: 'bindings/v8/ScriptPixelBuffer.idl',
I figured I let the renaming settle before fixing all the different project
files multiple times.

On 2009/01/18 16:55:36, Dean McNamee wrote:
> I thought this was CanvasPixelArray

http://codereview.chromium.org/18180/diff/206/63
File webkit/port/bindings/v8/CanvasPixelArray.cpp (right):

http://codereview.chromium.org/18180/diff/206/63#newcode41
Line 41: init(0);
On 2009/01/17 06:13:48, eroman wrote:
> im hoping this ctor can be deleted alltogether.
> Not a fan of malloc(0).

Done.

http://codereview.chromium.org/18180/diff/206/63#newcode57
Line 57: ASSERT((length & 3) == 0);  // Make sure the length is set correctly.
On 2009/01/17 06:13:48, eroman wrote:
> eew! why not simply: (length % 4) == 0 ?
> 
> in case you lifted this from old JSC version, note that they do the check on
the
> _buffer_, not the length. presumably to make sure their placement new will use
a
> 32-bit aligned block.
> 
> so i don't think this is actually necessary.

Since this is an array of bytes I do not understand why the placement would be
important. In my case I do intend to make sure that the length is a multiple of
4 as is mandated by the spec.

http://codereview.chromium.org/18180/diff/206/63#newcode57
Line 57: ASSERT((length & 3) == 0);  // Make sure the length is set correctly.
On 2009/01/18 16:55:36, Dean McNamee wrote:
> I don't see a problem with the assert.  I tend to use & a lot when mod power
of
> 2, so this doesn't seem so strange to me.  I think a better comment would be
> 
> // Make sure the length is a multiple of 4.
> 
> On 2009/01/17 06:13:48, eroman wrote:
> > eew! why not simply: (length % 4) == 0 ?
> > 
> > in case you lifted this from old JSC version, note that they do the check on
> the
> > _buffer_, not the length. presumably to make sure their placement new will
use
> a
> > 32-bit aligned block.
> > 
> > so i don't think this is actually necessary.
> 
> 

Done (fixed the comment).

http://codereview.chromium.org/18180/diff/206/63#newcode59
Line 59: m_data = static_cast<unsigned char*>(malloc(m_length));
On 2009/01/18 16:55:36, Dean McNamee wrote:
> There is some recent WebKit activity going on about webkit allocations, the
> ability to replace the allocation functions, etc.  Basically if you want to be
> using malloc, you should be using fastMalloc() and fastFree() in WebKit code.

Done.

http://codereview.chromium.org/18180/diff/206/63#newcode81
Line 81: if (!(value > 0)) {
On 2009/01/17 06:13:48, eroman wrote:
> Can you add a comment that this funkiness is specifically to clamp NaN to 0 ?

Done.

http://codereview.chromium.org/18180/diff/206/63#newcode86
Line 86: m_data[index] = static_cast<unsigned char>(value + 0.5);
On 2009/01/17 06:13:48, eroman wrote:
> I see that JSByteArray.h does a similar rounding, but I also see a this line
in
> Interpreter.cpp which makes me think the net effect will be truncation:
> 
> 2908:
> jsByteArray->setIndex(i, JSImmediate::getTruncatedInt32(jsValue)); 
> 

Since none of this code goes through Interpreter.cpp or JSByteArray.h there is
no double addition which would lead to rounding up.

http://codereview.chromium.org/18180/diff/206/63#newcode89
Line 89: } // namespace WebCore
On 2009/01/18 16:55:36, Dean McNamee wrote:
> two spaces before comments?

Done.

http://codereview.chromium.org/18180/diff/206/62
File webkit/port/bindings/v8/CanvasPixelArray.h (right):

http://codereview.chromium.org/18180/diff/206/62#newcode41
Line 41: class CanvasPixelArray : public RefCounted<CanvasPixelArray> {
As Dean says this is not a byte array. It has certain properties not normally
associated with bytes: clamping to 0 (if NaN or less than 0) and to 255 (if
greater than 255).

Darin talked to Oliver at Apple and they should be OK with this name.

On 2009/01/17 06:13:48, eroman wrote:
> this should probably be named "ByteArray" to match the JSC analog.
> 
> also this name is expected by html/ImageData.cpp which contains a
JSC::ByteArray
> 
>

http://codereview.chromium.org/18180/diff/206/62#newcode47
Line 47: CanvasPixelArray();
On 2009/01/17 06:13:48, eroman wrote:
> is this default constructor necessary?

Removed.

http://codereview.chromium.org/18180/diff/206/62#newcode48
Line 48: CanvasPixelArray(size_t length);
On 2009/01/17 06:13:48, eroman wrote:
> can this be private? 

Done.

http://codereview.chromium.org/18180/diff/206/64
File webkit/port/bindings/v8/V8CanvasPixelArrayCustom.cpp (right):

http://codereview.chromium.org/18180/diff/206/64#newcode51
Line 51: if ((index < 0) || (index > pixelBuffer->length())) {
On 2009/01/17 06:13:48, eroman wrote:
> I believe you want a gt_eq here, as in:
> "index >= pixelBuffer->length()"

Good catch. Thanks!

http://codereview.chromium.org/18180/diff/206/67
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/18180/diff/206/67#newcode1663
Line 1663: desc->InstanceTemplate()->SetIndexedPropertyHandler(
On 2009/01/18 16:55:36, Dean McNamee wrote:
> On 2009/01/17 06:13:48, eroman wrote:
> > Feng had pointed out that the image data is really just a glorified js
array,
> so
> > it would be faster to have v8 manage the array's memory / accesses directly,
> > rather than using custom property handlers.
> > 
> > I didn't have enough v8-savvy to do this when adding the original
> > canvaspixelarray bindings, but maybe you do :-). Anyhow, figured I would
> mention
> > it.
> 
> This is, after all, why WebKit moved from CanvasPixelArray to JSC::ByteArray,
> right?
> 
> > 
> > see: http://code.google.com/p/chromium/issues/detail?id=3473
> 
> 

When I talked with Darin his concern was the support of this feature, butnot the
execution speed since this feature is not in widely supported yet. I understand
how it can be made significantly faster by moving this into the V8 runtime, but
that would be premature optimization at the moment.

[iposva, 2009-01-20 17:29:15]

New version uploaded.

[Feng Qian, 2009-01-20 18:01:08]

lgtm

we should file a feature request to make it fast.

http://codereview.chromium.org/18180/diff/82/216
File webkit/port/bindings/v8/CanvasPixelArray.cpp (right):

http://codereview.chromium.org/18180/diff/82/216#newcode52
Line 52: m_data = reinterpret_cast<unsigned char*>(-1);
I am curious why it is -1? not a usual null pointer value?

[eroman, 2009-01-20 19:21:04]

LGTM, thanks!