Bug 113493 – Enhance the restrictions on going through the faster hardware path for uploading Canvas2D to WebGL

• Component: WebGL
• Status: RESOLVED INVALID

[Jun Jiang]

Current restrictions on going through the faster hardware path for uploading Canvas2D to WebGL have two issues:
1. Although Level is assumed to be 0 and hard-coded in ImageBuffer::copyToPlatformTexture as follows:
 extensions->copyTextureCHROMIUM(GraphicsContext3D::TEXTURE_2D, sourceTexture, texture, 0, internalFormat);
 The level parameter is not checked in WebGLRenderingContext::texImage2D(..., canvas) and level with a value larger than 0 can still pass through.
2. The type restrictions "type == texture->getType(target, level) && type == GraphicsContext3D::UNSIGNED_BYTE" result in that the first WebGLRenderingContext::texImage2D(..., canvas) call will go through SW path and only the successive call will go through the faster hardware path. It is because for the first call, the texture is not defined before and texture->getType(target, level) returns 0.

[Jun Jiang]

Created attachment 195510 [details]
Patch

[WebKit Review Bot]

Comment on attachment 195510 [details]
Patch

Attachment 195510 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17311525

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/texture-active-bind.html

[WebKit Review Bot]

Created attachment 195520 [details]
Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Zhenyao Mo]

Comment on attachment 195510 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3877
> +        && (texture->getType(target, level) == GraphicsContext3D::UNSIGNED_BYTE || !texture->isValid(target, level))

Actually I don't understand why we care about target texture's previous type?  Looking at https://code.google.com/p/chromium/codesearch#chromium/src/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt

I don't see why previous type is relevant.  Probably you can just remove this condition.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3878
> +        && !level) {

Instead of limiting to level == 0 here, why don't we add an extra parameter level to copyToPlatformTexture()?

[Kenneth Russell]

Mo: thanks for taking this review.
Jun: please work with Mo on this review; I'll r+ when it's agreed upon.

[Jun Jiang]

Comment on attachment 195510 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3877
>> +        && (texture->getType(target, level) == GraphicsContext3D::UNSIGNED_BYTE || !texture->isValid(target, level))
> 
> Actually I don't understand why we care about target texture's previous type?  Looking at https://code.google.com/p/chromium/codesearch#chromium/src/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt
> 
> I don't see why previous type is relevant.  Probably you can just remove this condition.

Hi, Zhenyao. Thanks for your comments.
There are two kinds of check on "type" here. One is for the target type, shown as "type == GraphicsContext3D::UNSIGNED_BYTE"; it was used as our white-list together with restrictions on format(RGB/RGBA) since some format/types are not supported in gpu::gles2::CopyTextureCHROMIUMResourceManager::CopyTextureCHROMIUM(). The other is what you mentioned on the check for previous type; It was added here because in gpu::gles2::GLES2DecoderImpl::DoCopyTextureCHROMIUM(), the implementation is like this: 1) if the destination texture is defined, the type is determined by its previous value even the destination texture is redefined; 2). if the destination texture is not defined, it would be defined but the type is determined by that of its source texture.
That's why the check goes like this. I am not quite sure about several things at present, for example, what other types exactly will be supported besides UNSIGNED_INT in the future, which type will be the default type when the destination texture is defined or re-defined when multiple types are supported in DoCopyTextureCHROMIUM(), etc. It seems some discussion is already being happened at chromium side(see https://code.google.com/p/chromium/issues/detail?id=153925). Before that happens, we can make the white-list works better.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3878
>> +        && !level) {
> 
> Instead of limiting to level == 0 here, why don't we add an extra parameter level to copyToPlatformTexture()?

Yes, we can pass a level parameter to copyToPlatformTexture() to avoid hard-coding the magic number 0 as parameter. But the limitation on "level == 0" may be still needed. In fact, the limitation on that "level == 0" is because we do the texture -> (FBO-texture) copy which uses glFrameBufferTexture2D(..., level) in DoCopyTextureCHROMIUM() where the level must be 0.

[Jun Jiang]

Add Gregg Tavares in for discussion. Any comments are welcome.

[Zhenyao Mo]

Comment on attachment 195510 [details]
Patch

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

>>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3877
>>> +        && (texture->getType(target, level) == GraphicsContext3D::UNSIGNED_BYTE || !texture->isValid(target, level))
>> 
>> Actually I don't understand why we care about target texture's previous type?  Looking at https://code.google.com/p/chromium/codesearch#chromium/src/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt
>> 
>> I don't see why previous type is relevant.  Probably you can just remove this condition.
> 
> Hi, Zhenyao. Thanks for your comments.
> There are two kinds of check on "type" here. One is for the target type, shown as "type == GraphicsContext3D::UNSIGNED_BYTE"; it was used as our white-list together with restrictions on format(RGB/RGBA) since some format/types are not supported in gpu::gles2::CopyTextureCHROMIUMResourceManager::CopyTextureCHROMIUM(). The other is what you mentioned on the check for previous type; It was added here because in gpu::gles2::GLES2DecoderImpl::DoCopyTextureCHROMIUM(), the implementation is like this: 1) if the destination texture is defined, the type is determined by its previous value even the destination texture is redefined; 2). if the destination texture is not defined, it would be defined but the type is determined by that of its source texture.
> That's why the check goes like this. I am not quite sure about several things at present, for example, what other types exactly will be supported besides UNSIGNED_INT in the future, which type will be the default type when the destination texture is defined or re-defined when multiple types are supported in DoCopyTextureCHROMIUM(), etc. It seems some discussion is already being happened at chromium side(see https://code.google.com/p/chromium/issues/detail?id=153925). Before that happens, we can make the white-list works better.

Maybe Gregg has a better understanding, but to me texImage2D() should not be affected by the texture's previous state, like width/height/type/format. If it is, then that sounds like a wrong semantics to me.

>>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3878
>>> +        && !level) {
>> 
>> Instead of limiting to level == 0 here, why don't we add an extra parameter level to copyToPlatformTexture()?
> 
> Yes, we can pass a level parameter to copyToPlatformTexture() to avoid hard-coding the magic number 0 as parameter. But the limitation on "level == 0" may be still needed. In fact, the limitation on that "level == 0" is because we do the texture -> (FBO-texture) copy which uses glFrameBufferTexture2D(..., level) in DoCopyTextureCHROMIUM() where the level must be 0.

OK, again maybe Gregg could shed some light, but why don't we bind the source texture (from video) to a fbo, and glCopyTexImage2D to the target texture, then we don't have this level == 0 limitation? Am I totally off key in my understanding of the APIs here?

[Jun Jiang]

Hi, Zhenyao. Thanks for your comments. I agree with your ideas and I realized that more effort is needed to make the code clean. I need do some work at chromium side as well.
Gregg, could you share your ideas on this to ensure I am in the right direction ?

[Gregg Tavares]

The reason we don't use copyTexImage2D is it only handles 1 chase and not even well. The Problem with copyTexImage2D are:

No support for UNPACK_FLIP_Y_WEBGL            
No support for UNPACK_PREMULTIPLY_ALPHA_WEBGL 
No support for choosing the destination type

I'm not sure what's trying to be fixed here. My plan was to try to make CopyTextureCHROMIUM have a some corresponding function like IsCopyTextureFormatSupportedCHROMIUM(
    source_level, dest_level, dest_format, dest_type)

So that knowing if a format/type/level combo can use CopyTextureCHROMIUM would be encapsulated but I haven't gotten around to it.

[Jun Jiang]

Hi, Gregg. Thanks for your comments.
Here I am going to resolve some undocumented details or limitation about CopyTextureCHROMIUM(...) and improve the whitelist for CopyTextureCHROMIUM(...), which is similar to what you mentioned as IsCopyTextureFormatSupportedCHROMIUM(...). The code in WebKit is expected to be cleaned to be more concise and simpler with proper comments. 
Since the whitelist is dependent on the implementation of CopyTextureCHROMIUM(...), I need your help to confirm and make sure my change won't break your ideas. The first limitation in my mind is that level must be equal to 0. The second limitation is that some combination of format/type may not be supported; The guaranteed combination is (RGB/RGBA)/UNSIGNED_BYTE while others such as RGBA/UNSIGNED_SHORT_5551 may not be supported by some underlying drivers. The third limitation is that  in current implementation for CopyTextureCHROMIUM(...), the format type of destination texture is either chosen from the previous state of the texture or chosen from that of source texture which may seems strange from the semantics of texImage2D(...).
My first patch here is meant to respect all the limitations but make the code and comments more simpler and cleaner. A similar IsCopyTextureFormatSupportedCHROMIUM(source_format, source_type, dest_level, dest_format, dest_type) may be added in webkit and chromium. All the combination checking will be tested by gl_copy_texture_CHROMIUM_unittest.cc first. The first and second limitation above can be solved easily by parameters checking and the third one can be solved by choosing the type from the passed dest_type parameter. 
The second potential patch may add some complements. For example, when no flipY operation, no preMultiplyAlpha or unMultiplyAlpha operation is needed but level is not 0, we can consider to use glCopyTexImage2D().
I'd like to work on the first patch first since the code at present is a little loose.

[Jun Jiang]

Created attachment 196469 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 196469 [details]
Patch

Attachment 196469 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17432027

[WebKit Review Bot]

Comment on attachment 196469 [details]
Patch

Attachment 196469 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17519018

[Jun Jiang]

The buildbot fails in chromium because this patch depends on the API change at chromium side(https://codereview.chromium.org/13613006).

There are basically three changes here. First, a new parameter dest_type is added to the GL_CHROMIUM_copy_texture extension(https://codereview.chromium.org/13613006/). Second, a new function canUseCopyTextureCHROMIUM(format, type, level) function is added to check if copyTextureCHROMIUM() could be used. Third, ImageBuffer::copyToPlatformTexture() adds another two parameters: type and level; WebMediaPlayer::copyVideoTextureToPlatformTexture() adds another parameter dest_type.
One thing that may still needs discussion is that whether canUseCopyTextureCHROMIUM(format, type, level) should be put in chromium side or in Webkit side. This patch simply put it at WebKit side and make it work as a whitelist.

[Brent Fulgham]

Comment on attachment 196469 [details]
Patch

I think this is a Chromium-specific change, so I'm closing the bug. Please re-open if I'm mistaken and this has relevance to EFL or Gtk.