[webkit-reviews] review denied: [Bug 102161] WebGL: Avoid unnecessary memory copy or conversion in texImage2D and texSubImage2D for HTMLVideoElement : [Attachment 174103] 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 102161: WebGL: Avoid unnecessary memory copy or conversion in texImage2D
and texSubImage2D for HTMLVideoElement
https://bugs.webkit.org/show_bug.cgi?id=102161

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

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


I'm sorry, but I think that the approach in this patch is flawed. I think you
should add a new inner class GraphicsContext3D::ImageLocker, implemented per
platform. An instance of that object could be allocated on the stack at the
call site and a method call against it could replace the call to getImageData.
For the Skia platform its constructor could allocate an SkAutoLockPixels and
its destructor delete it. CC'ing a few other people who know Skia's internals
better than I do in case they want to veto my assertion about the access to the
pixel data outside the scope of SkAutoLockPixels, but regardless this patch
should be restructured to not add Skia-specific #ifdefs.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3738
> +PassRefPtr<Image> WebGLRenderingContext::videoFrameToImage(HTMLVideoElement*
video, bool backingStoreCopy, ExceptionCode& ec)

It's strongly discouraged to add bool arguments because it makes the call sites
difficult to read. Just pass the BackingStoreCopy enum.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3758
> +    return buf->copyImage(copyBackingStore);

Is it legal for all ports to use DontCopyBackingStore here?

> Source/WebCore/html/canvas/WebGLRenderingContext.h:375
> +    PassRefPtr<Image> videoFrameToImage(HTMLVideoElement*, bool
backingStoreCopy, ExceptionCode&);

Same comment as in the .cpp file about passing the BackingStoreCopy enum
directly.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:900
> +#if USE(SKIA)

Please don't add an #ifdef here. API changes made here should be done for all
ports.

>> Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:99
>> +	       return true;
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
[whitespace/indent] [3]

It is a really bad idea to assume that you can access the pixels outside the
scope of the SkAutoLockPixels.