[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





--- Comment #9 from Brian Salomon <bsalomon at google.com>  2012-11-19 06:01:10 PST ---
(In reply to comment #4)
> (From update of attachment 174103 [details])
> 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.

It's correct that getPixels() should be performed while the pixels are locked (during the lifetime of a SkAutoLockPixels object or between matched lockPixels()/unlockPixels() calls).

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