[webkit-reviews] review denied: [Bug 105821] Optimize the texture packing for texImage2D() and texSubImage2D() in WebGL : [Attachment 181738] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Jun Jiang <jun.a.jiang at intel.com>'s
request for review:
Bug 105821: Optimize the texture packing for texImage2D() and texSubImage2D()
in WebGL
https://bugs.webkit.org/show_bug.cgi?id=105821

Attachment 181738: Patch
https://bugs.webkit.org/attachment.cgi?id=181738&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=181738&action=review


This is nice work that gets closer to unifying the pixel packing/unpacking code
again between Firefox and WebKit. Some relatively small issues, plus one
higher-level one. Per offline discussion it sounds like a bug was caught during
testing of this patch, so r-'ing this one, but please post a revision when
ready.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:320
> +// Some code are merged back from Mozilla code in
content/canvas/src/WebGLTexelConversions.h

A high-level point: it would be better if the code that is expected to be
shared between Firefox and WebKit were clearly marked at its beginning and end,
as it is in Firefox's
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLTexelConv
ersions.h .

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1237
> +	   || format == GraphicsContext3D::DataFormatA16Little

Since the 16-bit per channel big and little endian formats are only used by the
Core Graphics port, and their use is rare, I wonder whether they could be
handled differently to reduce the code size on all platforms.

Specifically, perhaps data of this format could be converted to one of the
8-bit formats inside GraphicsContext3DCG.cpp, and the cross-platform code
wouldn't need to know about these formats.

If you agree, it isn't necessary to make that change in this patch -- it's
large enough as it is -- but please file another bug and make it depend on this
one to track that work.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1350
> +ALWAYS_INLINE unsigned TexelBytesForFormat(int format)

Can this take "GraphicsContext3D::DataFormat" rather than int?

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1408
> +	   unpackedIntermediateSrcData = malloc(mWidth * MaxNumberOfComponents
*MaxBytesPerComponent);

Use unpackedIntermediateSrcData = adoptArrayPtr(new uint8_t[mWidth *
MaxNumberOfComponents * MaxBytesPerComponent]).

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1414
> +	       free(unpackedIntermediateSrcData);

With OwnArrayPtr the call to free() is unnecessary.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1430
> +    const unsigned mWidth, mHeight;

Use WebKit naming convention here and throughout: m_ prefix.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1435
> +    void* unpackedIntermediateSrcData;

Avoid use of raw pointers -- use OwnArrayPtr<uint8_t> instead. Also, fix naming
convention.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1549
> +    const bool CanSrcFormatComeFromDOMElementOrImageData =
GraphicsContext3D::srcFormatComeFromDOMElementOrImageData(SrcFormat);

Capitalization: canSrcFormat...

Grammar: srcFormatComesFrom...

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1571
> +    const bool trivialPack = (DstFormat ==
GraphicsContext3D::DataFormatRGBA8 || DstFormat ==
GraphicsContext3D::DataFormatRGBA32F) && alphaOp ==
GraphicsContext3D::AlphaDoNothing && mDstStride > 0;

This method looks similar to Firefox's WebGLImageConverter::run. Why does this
version need to compute these two flags?

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1577
> +	   if (!trivialUnpack && trivialPack) {

Have you measured the cost of having these three if/else branches inside the
loop? The alternative would be to clone the loop.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1591
> +bool webGLPackPixels(const uint8_t* sourceData,
GraphicsContext3D::DataFormat sourceFormat, unsigned width, unsigned height,
unsigned sourceUnpackAlignment, unsigned destinationFormat, unsigned
destinationType, GraphicsContext3D::AlphaOp alphaOp, void* destinationData,
bool flipY)

Avoid referring to "WebGL" inside GraphicsContext3D. This separate function
doesn't appear to be needed at all; just put its body into
GraphicsContext3D::packPixels.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1594
> +    int reminder = sourceUnpackAlignment ? (validSrc %
sourceUnpackAlignment) : 0;

Typo: reminder -> remainder

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1600
> +	   destinationData = (uint8_t *)destinationData + dstStride*(height -
1);

Use C++-style rather than C-style casts in WebKit code:
static_cast<uint8_t*>(...). Also, spaces around "*" operator.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1610
> +	   uint8_t* dst = (uint8_t*)destinationData;

Fix C-style cast.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:645
> +    // The formats from DOM elements vary with Graphics ports. It can only
be RGBA8 or BGRA8 for non-CG port while much more for CGi port.

Typo: CGi -> CG