[Webkit-unassigned] [Bug 102161] WebGL: Avoid unnecessary memory copy or conversion in texImage2D and texSubImage2D for HTMLVideoElement

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=102161


Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #174103|review?                     |review-
               Flag|                            |




--- Comment #4 from Kenneth Russell <kbr at google.com>  2012-11-14 17:52:17 PST ---
(From update of attachment 174103)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.