Summary: | Enhance the restrictions on going through the faster hardware path for uploading Canvas2D to WebGL | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jun Jiang <jun.a.jiang> | ||||||||
Component: | WebGL | Assignee: | Jun Jiang <jun.a.jiang> | ||||||||
Status: | RESOLVED INVALID | ||||||||||
Severity: | Normal | CC: | abarth, bajones, bfulgham, cc-bugs, dglazkov, dino, eric.carlson, esprehn+autocc, feature-media-reviews, fishd, gman, jamesr, jer.noble, junov, kbr, noam, ojan.autocc, senorblanco, tkent+wkapi, webkit.review.bot, zmo | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 111126 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Jun Jiang
2013-03-28 02:42:29 PDT
Created attachment 195510 [details]
Patch
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 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
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()? Mo: thanks for taking this review. Jun: please work with Mo on this review; I'll r+ when it's agreed upon. 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. Add Gregg Tavares in for discussion. Any comments are welcome. 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? 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 ? 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. 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. Created attachment 196469 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. 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 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 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. 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.
|