Bug 102161 – WebGL: Avoid unnecessary memory copy or conversion in texImage2D and texSubImage2D for HTMLVideoElement

• Component: WebGL
• Status: RESOLVED FIXED

[Jun Jiang]

In texImage2D(..., HTMLVideoElement*, ...) and texSubImage2D(...,HTMLVideoElement*, ...), there are several memory copy or conversion involved. The data path involves following transforms:
1. Paint Video data(YUV format) to ImageBuffer(RGBA or BGRA format) in m_videoCache.
2. Copy Image from ImageBuffer in m_videoCache with BackingStore copied.
3. extract the data from Image returned by 2) to a vector. There are two steps involved:
   3.1. unpack the data from Image to an intermediate format.
   3.2  pack the data in intermediate format to the destination format according to the format and alphaOp.
4. pass the extracted data to GL through gltexImage2D(...) or gltexSubImage2D(...).

There are several cases in steps 2-4 that we can avoid memory copy or conversions by taking account of alpha value of Video and GL extension.

[Jun Jiang]

Created attachment 174103 [details]
Patch

[WebKit Review Bot]

Attachment 174103 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/GraphicsContext3D.cpp:177:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/GraphicsContext3D.cpp:178:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:98:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3630:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3868:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/GraphicsContext3D.h:577:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/GraphicsContext3D.h:578:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/GraphicsContext3D.h:902:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/GraphicsContext3D.h:903:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/GraphicsContext3D.h:904:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/GraphicsContext3D.h:905:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/GraphicsContext3D.h:906:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/GraphicsContext3D.h:907:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 18 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jun Jiang]

Changing the code as the script(check-webkit-style) suggested would be greatly inconsistent with the original coding style in the changed files. If it is a must, I will change the code accordingly.

[Kenneth Russell]

Comment on attachment 174103 [details]
Patch

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.

[Jun Jiang]

Kenneth,your comments and concern are quite reasonable. It is error prone to access the pixels outside the scope of the SkAutoLockPixels. And there is another way to solve this issue besides adding a new class GraphicsContext3D::ImageLocker as you had suggested. 
We can add a new function to ImageBuffer class. In this function, for skia port, SkAutoLockPixels would be called and would call through to glTexImage2D if no conversion is needed. If conversion is needed, it will return false and follow the traditional path. It is much like what had been done to texImage2D in WebGL for HTMLCanvasElement by introducing ImageBuffer::copyToPlatformTexture().

[Kenneth Russell]

(In reply to comment #5)
> Kenneth,your comments and concern are quite reasonable. It is error prone to access the pixels outside the scope of the SkAutoLockPixels. And there is another way to solve this issue besides adding a new class GraphicsContext3D::ImageLocker as you had suggested. 
> We can add a new function to ImageBuffer class. In this function, for skia port, SkAutoLockPixels would be called and would call through to glTexImage2D if no conversion is needed. If conversion is needed, it will return false and follow the traditional path. It is much like what had been done to texImage2D in WebGL for HTMLCanvasElement by introducing ImageBuffer::copyToPlatformTexture().

That's possible but I would prefer to introduce an ImageLocker class for two reasons. First, putting more GraphicsContext3D-related calls into the ImageBuffer implementation more tightly binds together conceptually unrelated areas of the code.

Second, and possibly more importantly, the pixel packing and unpacking code in GraphicsContext3D has been improved by Mozilla Corporation, and it's desired to merge back their changes. However, the code structure is different, and relies on being able to get a stable pointer to the source data. Having a port-independent ImageLocker class would enable this as well.

Overall, it would be a really nice contribution if you'd restructure this patch to add and use an ImageLocker abstraction that worked for all ports (or do that in a separate bug and then build this one on it).

[Jun Jiang]

The related pack/unpacking code in Firefox did make much improvement. 
1. All the texImage2D(...) functions for ArrayBuffer, ImageData, Image, Canvas and Video are unified to use a single texImage2D_base() function which accepts a pointer to the source data as parameter.
2. For the conversion part, it introduces a WebGLImageConverter object to do the unpack,pack and flipY operations. The flipY operation is carried out in the pack operation instead of being done separately. 
I agree that WebKit may need to merge back some changes done in Firefox. As what item 1 did, introducing the ImageLocker class can help us to abstract all the texImage2D functions to a general texImage2D_base function to use the pointer to the source data as parameter. And then we can implement the texImage2D_base in a clean and clear way.
By checking the code of Graphics ports, it seems only Skia provides Lock/Unlock operation to the pixel data while other ports like cairo, cg and qt didn't provide that. I will work on the ImageLocker class first.

[Kenneth Russell]

(In reply to comment #7)
> The related pack/unpacking code in Firefox did make much improvement. 
> 1. All the texImage2D(...) functions for ArrayBuffer, ImageData, Image, Canvas and Video are unified to use a single texImage2D_base() function which accepts a pointer to the source data as parameter.
> 2. For the conversion part, it introduces a WebGLImageConverter object to do the unpack,pack and flipY operations. The flipY operation is carried out in the pack operation instead of being done separately. 
> I agree that WebKit may need to merge back some changes done in Firefox. As what item 1 did, introducing the ImageLocker class can help us to abstract all the texImage2D functions to a general texImage2D_base function to use the pointer to the source data as parameter. And then we can implement the texImage2D_base in a clean and clear way.
> By checking the code of Graphics ports, it seems only Skia provides Lock/Unlock operation to the pixel data while other ports like cairo, cg and qt didn't provide that. I will work on the ImageLocker class first.

Thank you in advance for doing this work. Please let me know if you need any help or encounter any problems.

[Brian Salomon]

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

[Jun Jiang]

Kenneth, I have finished the code for ImageLocker for all platform and get started to test it. Currently, I have tested Skia port via chromium and will test Cairo and Qt port under ubuntu. But it seems I would have some difficulty in testing Cg port. It is a little difficult to set up an environemnt for cg port. Can you give some suggestions on that? Thanks.

[Kenneth Russell]

(In reply to comment #10)
> Kenneth, I have finished the code for ImageLocker for all platform and get started to test it. Currently, I have tested Skia port via chromium and will test Cairo and Qt port under ubuntu. But it seems I would have some difficulty in testing Cg port. It is a little difficult to set up an environemnt for cg port. Can you give some suggestions on that? Thanks.

Jian, that's great news. I can help you test the Cg port if you would like to upload an intermediate patch here.

[Jun Jiang]

Created attachment 175430 [details]
Patch

[Jun Jiang]

Kenneth, the intermediate patch is attached. Any comments are welcome.
The idea here is to implement the ImageLocker class to hold all the states of the Image and therefore the address of the source can be used steadily during the lifetime of the object. Most of work is to change the procedure in GraphicsContext3D::getImageData() to the object-oriented ImageLocker class. The lifetime of the states were originally only in GraphicsContext3D::getImageData(), now it is determined by the lifetime of the object of ImageLocker.

[Jun Jiang]

Created attachment 175737 [details]
Patch

[Early Warning System Bot]

Comment on attachment 175737 [details]
Patch

Attachment 175737 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14960591

[Early Warning System Bot]

Comment on attachment 175737 [details]
Patch

Attachment 175737 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14937611

[Build Bot]

Comment on attachment 175737 [details]
Patch

Attachment 175737 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14970360

[Jun Jiang]

Created attachment 175751 [details]
Patch

[Build Bot]

Comment on attachment 175751 [details]
Patch

Attachment 175751 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14970421

New failing tests:
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html

[Jun Jiang]

Created attachment 176183 [details]
Patch

[Jun Jiang]

fix the issue for Mac OS X.

[Kenneth Russell]

Comment on attachment 176183 [details]
Patch

Per offline discussion, the ImageLocker changes have been split off into another bug. Marking this patch r-; expecting this to be rebased on top of the other work and for this patch to be significantly smaller.

[Jun Jiang]

As Kenneth had suggested, the patch was a little big. To be more focused, it was splited  into 3 bugs respectively, the first one is bug 103606, the second one is  bug 103885 and the third one is 102161. Specially, in bug 102161, it will deal with issues specific to HTMLVideoElement related only.

[Jun Jiang]

Created attachment 177448 [details]
Patch

[Jun Jiang]

regenerated the patch based on ImageExtractor and evaluated its performance gain under Linux and Windows in Chromium.  
In a simple test, decode a 720p ogv file, upload the video to a 3D sphere and the sphere requests animation repeatedly. In Linux (CPU: 3.4Ghz), the fps is both 60 FPS before and after the optimization but the cpu utilization for the renderer process is decreased from ~%97 to ~62% after applying the patch. For Win7(CPU: 2.6GHz), the FPS is increased from ~32FPS to ~54FPS after applying the patch.

[Kenneth Russell]

Comment on attachment 177448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177448&action=review

Nice results but the code needs more work.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3634
> +            || (alphaOp == GraphicsContext3D::AlphaDoPremultiply && alphaValue == 0xFF))

It isn't legal to make this determination based on the alpha value of the upper-left pixel of the video. Please make the code correct in all cases.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3781
> +    // if the graphics port provides support to the DontCopyBackingStore semantics. SKIA, CG, CAIRO and QT ports already support this DontCopyBackingStore behavior. 

Where is it specified that DontCopyBackingStore is supported by these ports?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3782
> +#if USE(SKIA) || USE(CG) || USE(CAIRO) || PLATFORM(QT)

How is this #if going to be kept up-to-date as more ports begin to support the optimization? An API which each port implements is needed rather than an #ifdef here.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3894
> +            alphaOp = GraphicsContext3D::AlphaDoNothing;

Same comments as above.

[Jun Jiang]

Hi, Kenneth. Thanks for your comments. Following is some of my considerations: 
1). The AlphaOp optimizations for HTMLVideoElement.
In general, the Image is converted from YUV format to RGBA or BGRA format. During this conversion, the alpha value is always set to 0xFF and no PremultiplyAlpha operation is applied. 
However, in some ports, if the alpha value if not zero and no PremultiplyAlpha is required, AlphaOp will be set to UnmultiplyAlpha and a successive UnmultiplyAlpha operation would be carried out during unpack/pack. In fact, the UnmultiplyAlpha operation is not needed due to current implementation for YUV->BGRA conversion. Moreover, if in the future, an alpha value other than 0xFF is set during YUV->BGRA conversion, the alpha value is uniform for each pixel of the Image. If the top-left pixel has an alpha of 0xFF, all other will have an alpha of 0xFF too.
That is why the patch goes that way. I am still considering if a more suitable method can be found and applied.
2). Backing-storing copy.  
First, following is the commit that introduces the DontCopyBackingStore semantics.
SKIA:
 commit d469b6e559bb78260a40dd43ebdcb9b0a2de7ac5
 https://bugs.webkit.org/show_bug.cgi?id=73949
    * platform/graphics/skia/ImageBufferSkia.cpp:
    (WebCore::ImageBuffer::copyImage):
    Implement ImageBuffer::copyImage() with DontCopyBackingStore semantics.
QT:
 commit fa77d736d7c759241d445e2d24513dbb3749184d
 [Qt] Implement ImageBuffer::copyImage(ImageBuffer::DontCopyBackingStore)
 https://bugs.webkit.org/show_bug.cgi?id=77689
CAIRO:
 commit e82a551c1ad08c2eeeb7620b5fe55ea1ed1e167e
 [Cairo] Implement ImageBuffer::copyImage for BackingStoreCopy == DontCopyBackingStore
 https://bugs.webkit.org/show_bug.cgi?id=85728
CG:
    commit 0e72db9bf51becaf5b25944a9ca6d2314af884e7
    https://bugs.webkit.org/show_bug.cgi?id=64535
    * platform/graphics/ImageBuffer.h:
    1) Added BackingStoreCopy enum for choosing to copy backing store or not in copyImage().
Second, I agree with you that an API may be needed to reflect this status for each port without having to add USE(XX) in the #if statement when it was newly supported.

[Kenneth Russell]

(In reply to comment #27)
> Hi, Kenneth. Thanks for your comments. Following is some of my considerations: 
> 1). The AlphaOp optimizations for HTMLVideoElement.
> In general, the Image is converted from YUV format to RGBA or BGRA format. During this conversion, the alpha value is always set to 0xFF and no PremultiplyAlpha operation is applied. 
> However, in some ports, if the alpha value if not zero and no PremultiplyAlpha is required, AlphaOp will be set to UnmultiplyAlpha and a successive UnmultiplyAlpha operation would be carried out during unpack/pack. In fact, the UnmultiplyAlpha operation is not needed due to current implementation for YUV->BGRA conversion. Moreover, if in the future, an alpha value other than 0xFF is set during YUV->BGRA conversion, the alpha value is uniform for each pixel of the Image. If the top-left pixel has an alpha of 0xFF, all other will have an alpha of 0xFF too.
> That is why the patch goes that way. I am still considering if a more suitable method can be found and applied.

Please find another way to encapsulate this logic that is more maintainable -- perhaps closer to the implementation of the video elements. Or drop this part of the patch altogether. The current rules are incomprehensible and will be unmaintainable for anybody else looking at this code in the future -- myself included.

> 2). Backing-storing copy.  
> First, following is the commit that introduces the DontCopyBackingStore semantics.
> SKIA:
>  commit d469b6e559bb78260a40dd43ebdcb9b0a2de7ac5
>  https://bugs.webkit.org/show_bug.cgi?id=73949
>     * platform/graphics/skia/ImageBufferSkia.cpp:
>     (WebCore::ImageBuffer::copyImage):
>     Implement ImageBuffer::copyImage() with DontCopyBackingStore semantics.
> QT:
>  commit fa77d736d7c759241d445e2d24513dbb3749184d
>  [Qt] Implement ImageBuffer::copyImage(ImageBuffer::DontCopyBackingStore)
>  https://bugs.webkit.org/show_bug.cgi?id=77689
> CAIRO:
>  commit e82a551c1ad08c2eeeb7620b5fe55ea1ed1e167e
>  [Cairo] Implement ImageBuffer::copyImage for BackingStoreCopy == DontCopyBackingStore
>  https://bugs.webkit.org/show_bug.cgi?id=85728
> CG:
>     commit 0e72db9bf51becaf5b25944a9ca6d2314af884e7
>     https://bugs.webkit.org/show_bug.cgi?id=64535
>     * platform/graphics/ImageBuffer.h:
>     1) Added BackingStoreCopy enum for choosing to copy backing store or not in copyImage().
> Second, I agree with you that an API may be needed to reflect this status for each port without having to add USE(XX) in the #if statement when it was newly supported.

The current change to WebGLRenderingContext is unmaintainable so I'm afraid you will need to find a better way to do it.

[Jun Jiang]

Created attachment 178225 [details]
Patch

[Kenneth Russell]

Comment on attachment 178225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178225&action=review

Thanks for making these revisions. This looks almost perfect, but needs a couple more small changes to the logic. Along with that, please clean up a few comments.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:933
> +        ImageHtmlDomSource m_imageHtmlDomSource;

This should be declared next to m_image to match the initialization order in the constructor.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:193
> +        // which is true at present and may be changed in the future and needs adjustment accordingly

See comment below in the Skia port about complete sentences.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:196
> +        if (!premultiplyAlpha && m_imageHtmlDomSource == HtmlDomCanvas)

See comment below in the Skia port about how to write this test.

> Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:83
> +        // which is true at present and may be changed in the future and needs adjustment accordingly

Please add "." at the end of comments to make them complete sentences.

> Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp:86
> +        if (m_imageHtmlDomSource == HtmlDomCanvas)

This isn't the minimal change to the logic. Please write this instead as "if (m_imageHtmlDomSource != HtmlDomVideo)".

[Jun Jiang]

Created attachment 178413 [details]
Patch

[Jun Jiang]

Met "HTTP Error 500: Internal Server Error" in try-bot for win. Will upload the patch again.

[Jun Jiang]

Created attachment 178680 [details]
Patch

[Kenneth Russell]

Comment on attachment 178680 [details]
Patch

Looks good. Thanks for persisting with this patch. r=me

[WebKit Review Bot]

Comment on attachment 178680 [details]
Patch

Clearing flags on attachment: 178680

Committed r137397: <http://trac.webkit.org/changeset/137397>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.