Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738163002:
Enable CopyTextureCHROMIUM in Blink for Tex{Sub}Image2D with more cases (Closed)
|
Created:
3 years, 9 months ago by qiankun Modified:
3 years, 8 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, Stephen Chennney, dshwang, jam, jbroman, fmalita+watch_chromium.org, blink-reviews, Rik, darin-cc_chromium.org, Justin Novosad, pdr+graphicswatchlist_chromium.org, haraken, piman+watch_chromium.org, kinuko+watch, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis, Yang Gu, hubbe, aleksandar.stojiljkovic Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable CopyTextureCHROMIUM in Blink for Tex{Sub}Image2D with more cases
BUG=612542
TEST=webgl and webgl2 conformance tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2738163002
Cr-Commit-Position: refs/heads/master@{#463269}
Committed: https://chromium.googlesource.com/chromium/src/+/03b5cd7ff9b0c8457efe2578d18de91141be30b6
Patch Set 1 #
Total comments: 19
Patch Set 2 : fix comments#4 #
Total comments: 2
Patch Set 3 : rebase only #Patch Set 4 : fix Y16 video texture #Patch Set 5 : rebase only #Patch Set 6 : enable fallback path #
Total comments: 10
Patch Set 7 : remove passthrough suppressions #Patch Set 8 : add back passthrough suppressions #Patch Set 9 : rebase only #Patch Set 10 : rebase only--blink renaming and formatting #Messages
Total messages: 71 (44 generated)
Description was changed from
==========
Enable CopyTextureCHROMIUM in Blink for Tex{Sub}Image2D with more cases
BUG=612542
TEST=webgl and webgl2 conformance tests
==========
to
==========
Enable CopyTextureCHROMIUM in Blink for Tex{Sub}Image2D with more cases
BUG=612542
TEST=webgl and webgl2 conformance tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
qiankun.miao@intel.com changed reviewers: + kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org
After long time investigation, I think I figured out most of the issues for enabling the extension in Blink. I have two issues to discuss with you: 1. texSubImage2D requires format and type to match the value specified when defining the texture with texImage2D in WebGL 1.0. WebGL 2.0 also has similar requirement: combination of format/type and internalformat specified previously should be valid. But after removing texture info caching in Blink, it is impossible to get internalformat info in texSubImage2D. We also need internalformat to skip the GPU-GPU path due to some dirve bugs on that format. Without internalformat info for texSubImage2D, we can not do that skip. I added CTS expectations for such cases. 2. For SRGB format, current GPU-CPU-GPU path for uploading DOM elements to a texture in Blink WebGL doesn't do RGB-to-SRGB conversion. WebGL conformance tests also expect no conversion. But in glCopyTextureCHROMIUM, when copying a RGB texture to a SRGB texture, color space conversion will be done. See some discussions in https://github.com/KhronosGroup/WebGL/issues/2165. Do you have some suggestions how to solve the above issues? https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16432: } This code can be removed once the issue in https://codereview.chromium.org/2711903002/ is fixed.
https://codereview.chromium.org/2738163002/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/webgl_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/webgl_conformance_expectations.py:438: self.Flaky('conformance/textures/video/' + Are we able to remove these flaky entries? https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16432: } On 2017/03/09 08:53:02, qiankun wrote: > This code can be removed once the issue in > https://codereview.chromium.org/2711903002/ is fixed. Can you add it into a TODO in the code? Otherwise it might get lost https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:642: case GL_RGB5_A1: I am wondering if we can push this to the command buffer side and still go down the GPU path (a different path in Mac). https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:646: case GL_RGB9_E5: I am not sure about this. Again, I thought such color bit matching requirement is only for fixed point formats, not for floats. Otherwise, there won't a format that can match with RGB9_E5. https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:823: SkSurface::kDiscardWrite_TextureHandleAccess)) Can we cache this and reuse it for both textureId and textureTarget? https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4908: if (functionID == TexSubImage2D && extensionEnabled(EXTsRGBName)) This is a big fallback. Can we look into lifting this? https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: // texture to half float or float texture. Well, the same can be said for RGBA8 formats, so I am not sure if we want to put in this limit.
On 2017/03/09 08:53:02, qiankun wrote: > After long time investigation, I think I figured out most of the issues for > enabling the extension in Blink. I have two issues to discuss with you: > 1. texSubImage2D requires format and type to match the value specified when > defining the texture with texImage2D in WebGL 1.0. WebGL 2.0 also has similar > requirement: combination of format/type and internalformat specified previously > should be valid. But after removing texture info caching in Blink, it is > impossible to get internalformat info in texSubImage2D. We also need > internalformat to skip the GPU-GPU path due to some dirve bugs on that format. > Without internalformat info for texSubImage2D, we can not do that skip. I added > CTS expectations for such cases. Can we move this needed validation into command buffer side? In the GPU-GPU path, is there a way to inject this validation? > 2. For SRGB format, current GPU-CPU-GPU path for uploading DOM elements to a > texture in Blink WebGL doesn't do RGB-to-SRGB conversion. WebGL conformance > tests also expect no conversion. But in glCopyTextureCHROMIUM, when copying a > RGB texture to a SRGB texture, color space conversion will be done. See some > discussions in https://github.com/KhronosGroup/WebGL/issues/2165. > > Do you have some suggestions how to solve the above issues? It seems that we should change the conformance tests to allow color space conversion. Let's raise this issue to the next working group meeting before taking any action. > > https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_decoder.cc:16432: } > This code can be removed once the issue in > https://codereview.chromium.org/2711903002/ is fixed.
On 2017/03/10 00:14:05, Zhenyao Mo wrote: > On 2017/03/09 08:53:02, qiankun wrote: > > After long time investigation, I think I figured out most of the issues for > > enabling the extension in Blink. I have two issues to discuss with you: > > 1. texSubImage2D requires format and type to match the value specified when > > defining the texture with texImage2D in WebGL 1.0. WebGL 2.0 also has similar > > requirement: combination of format/type and internalformat specified > previously > > should be valid. But after removing texture info caching in Blink, it is > > impossible to get internalformat info in texSubImage2D. We also need > > internalformat to skip the GPU-GPU path due to some dirve bugs on that format. > > Without internalformat info for texSubImage2D, we can not do that skip. I > added > > CTS expectations for such cases. > > Can we move this needed validation into command buffer side? In the GPU-GPU > path, is there a way to inject this validation? That's possible by adding format and type to arguments of glCopySubTextureCHROMIUM. Maybe we also need to tell glCopySubTextureCHROMIUM that the call is from TexSubImage2D. It works but a little nasty. > > > 2. For SRGB format, current GPU-CPU-GPU path for uploading DOM elements to a > > texture in Blink WebGL doesn't do RGB-to-SRGB conversion. WebGL conformance > > tests also expect no conversion. But in glCopyTextureCHROMIUM, when copying a > > RGB texture to a SRGB texture, color space conversion will be done. See some > > discussions in https://github.com/KhronosGroup/WebGL/issues/2165. > > > > Do you have some suggestions how to solve the above issues? > > It seems that we should change the conformance tests to allow color space > conversion. Let's raise this issue to the next working group meeting before > taking any action. Ok. Let's wait the decision of working group. > > > > > > https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:16432: } > > This code can be removed once the issue in > > https://codereview.chromium.org/2711903002/ is fixed.
Thanks for reviewing. I updated the CL. PTAL. https://codereview.chromium.org/2738163002/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/webgl_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/webgl_conformance_expectations.py:438: self.Flaky('conformance/textures/video/' + On 2017/03/10 00:10:23, Zhenyao Mo wrote: > Are we able to remove these flaky entries? I am not sure about this. Maybe we can try it later. https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2738163002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16432: } On 2017/03/10 00:10:23, Zhenyao Mo wrote: > On 2017/03/09 08:53:02, qiankun wrote: > > This code can be removed once the issue in > > https://codereview.chromium.org/2711903002/ is fixed. > > Can you add it into a TODO in the code? Otherwise it might get lost Done. https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:642: case GL_RGB5_A1: On 2017/03/10 00:10:23, Zhenyao Mo wrote: > I am wondering if we can push this to the command buffer side and still go down > the GPU path (a different path in Mac). We can add a readback path for such formats, for example RGB5_A1 and RGB9_E5. This is a TODO for CopyTextureCHROMIUM in command buffer. https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:646: case GL_RGB9_E5: On 2017/03/10 00:10:23, Zhenyao Mo wrote: > I am not sure about this. Again, I thought such color bit matching requirement > is only for fixed point formats, not for floats. > > Otherwise, there won't a format that can match with RGB9_E5. RGB9_5 can also run the readback path. I think color bit matching rule doesn't only apply to fixed point formats, see following statements in ES 3.0.5 spec: " If internalformat is sized, the internal format of the new texel array is internalformat, and this is also the new texel array’s effective internal format. If the component sizes of internalformat do not exactly match the corresponding component sizes of the source buffer’s effective internal format, described below, an INVALID_OPERATION error is generated. " " Calling CopyTexSubImage3D, CopyTexImage2D, or CopyTexSubImage2D will result in an INVALID_OPERATION error if any of the following conditions is true: • internalformat of the texel array being (re)specified is RGB9_E5, " https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:823: SkSurface::kDiscardWrite_TextureHandleAccess)) On 2017/03/10 00:10:24, Zhenyao Mo wrote: > Can we cache this and reuse it for both textureId and textureTarget? Done. https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4908: if (functionID == TexSubImage2D && extensionEnabled(EXTsRGBName)) On 2017/03/10 00:10:24, Zhenyao Mo wrote: > This is a big fallback. Can we look into lifting this? When we get clear on how to handle sRGB texture, we can think about lifting this. Add a TODO here. https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: // texture to half float or float texture. On 2017/03/10 00:10:24, Zhenyao Mo wrote: > Well, the same can be said for RGBA8 formats, so I am not sure if we want to put > in this limit. Or we can increase the tolerance in the tests? I met this issue for depth_capture_tests where tolerance == 1.5/65535 : https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep....
https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: // texture to half float or float texture. On 2017/03/10 02:48:04, qiankun wrote: > On 2017/03/10 00:10:24, Zhenyao Mo wrote: > > Well, the same can be said for RGBA8 formats, so I am not sure if we want to > put > > in this limit. > > Or we can increase the tolerance in the tests? I met this issue for > depth_capture_tests where tolerance == 1.5/65535 : > https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep.... > What will be the tolerance to make it pass? I think slightly increase the tolerance is fine if the benefit is a much faster path.
https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: // texture to half float or float texture. On 2017/03/10 22:45:43, Zhenyao Mo wrote: > On 2017/03/10 02:48:04, qiankun wrote: > > On 2017/03/10 00:10:24, Zhenyao Mo wrote: > > > Well, the same can be said for RGBA8 formats, so I am not sure if we want to > > put > > > in this limit. > > > > Or we can increase the tolerance in the tests? I met this issue for > > depth_capture_tests where tolerance == 1.5/65535 : > > > https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep.... > > > > What will be the tolerance to make it pass? I think slightly increase the > tolerance is fine if the benefit is a much faster path. When calling glCopy{Sub}TextureCHROMIUM, source_internal_format of the source texture in this case is BGRA8 or RGBA8 on different OSes. So, we should set the tolerance to 1.0 / 255 to make the test pass.
On 2017/03/10 23:54:19, qiankun wrote: > https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > (right): > > https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: // > texture to half float or float texture. > On 2017/03/10 22:45:43, Zhenyao Mo wrote: > > On 2017/03/10 02:48:04, qiankun wrote: > > > On 2017/03/10 00:10:24, Zhenyao Mo wrote: > > > > Well, the same can be said for RGBA8 formats, so I am not sure if we want > to > > > put > > > > in this limit. > > > > > > Or we can increase the tolerance in the tests? I met this issue for > > > depth_capture_tests where tolerance == 1.5/65535 : > > > > > > https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep.... > > > > > > > What will be the tolerance to make it pass? I think slightly increase the > > tolerance is fine if the benefit is a much faster path. > > When calling glCopy{Sub}TextureCHROMIUM, source_internal_format of the source > texture in this case is BGRA8 or RGBA8 on different OSes. So, we should set the > tolerance to 1.0 / 255 to make the test pass. That (from 1.5 / 65535 to 255 / 65535) seems a big tolerance. You encode 8bit into a float, so you can't lose so much data. tolerance with the same scale as 1.5/65535 bur increased a little bit seems fine.
On 2017/03/11 00:46:11, Zhenyao Mo wrote: > On 2017/03/10 23:54:19, qiankun wrote: > > > https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > > (right): > > > > > https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: // > > texture to half float or float texture. > > On 2017/03/10 22:45:43, Zhenyao Mo wrote: > > > On 2017/03/10 02:48:04, qiankun wrote: > > > > On 2017/03/10 00:10:24, Zhenyao Mo wrote: > > > > > Well, the same can be said for RGBA8 formats, so I am not sure if we > want > > to > > > > put > > > > > in this limit. > > > > > > > > Or we can increase the tolerance in the tests? I met this issue for > > > > depth_capture_tests where tolerance == 1.5/65535 : > > > > > > > > > > https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep.... > > > > > > > > > > What will be the tolerance to make it pass? I think slightly increase the > > > tolerance is fine if the benefit is a much faster path. > > > > When calling glCopy{Sub}TextureCHROMIUM, source_internal_format of the source > > texture in this case is BGRA8 or RGBA8 on different OSes. So, we should set > the > > tolerance to 1.0 / 255 to make the test pass. > > That (from 1.5 / 65535 to 255 / 65535) seems a big tolerance. You encode 8bit > into a float, so you can't lose so much data. tolerance with the same scale as > 1.5/65535 bur increased a little bit seems fine. The precision lost doesn't take place at GPU-GPU copy from BGRA8 to float texture. It comes from painting the video texture in media::PIXEL_FORMAT_Y16 to an imagebuffer in BGRA8 format, see the code path below on Linux: #0 (anonymous namespace)::WebMediaPlayerMS::paint (this=0x2f4f7dd63a20, canvas=0x2f4f7ddc3220, rect=..., flags=...) at ../../content/renderer/media/webmediaplayer_ms.cc:463 #1 0x0000564b6b65ff00 in (anonymous namespace)::HTMLVideoElement::paintCurrentFrame (this=0x9a1b5bc32e8, canvas=0x2f4f7ddc3220, destRect=..., flags=0x0) at ../../third_party/WebKit/Sourc e/core/html/HTMLVideoElement.cpp:212 #2 0x0000564b6c99c88c in (anonymous namespace)::WebGLRenderingContextBase::texImageHelperHTMLVideoElement (this=0xda1a9d050e8, functionID=(anonymous namespace)::WebGLRenderingContextBase ::TexImage2D, target=3553, level=0, internalformat=6408, format=6408, type=5126, xoffset=0, yoffset=0, zoffset=0, video=0x9a1b5bc32e8, sourceImageRect=..., depth=1, unpackImageHeight=0, e xceptionState=...) at ../../third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5249 #3 0x0000564b6c99d06d in (anonymous namespace)::WebGLRenderingContextBase::texImage2D (this=0xda1a9d050e8, target=3553, level=0, internalformat=6408, format=6408, type=5126, video=0x9a1b 5bc32e8, exceptionState=...) at ../../third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5311 The CPU path does 16-bit integer to float conversion so the tolerance can be set to 1.5 / 65535, see the CPU path below: #0 (anonymous namespace)::(anonymous namespace)::FlipAndConvertY16 (video_frame=0x5dd1d343860, out=0x5dd1d639020 '\253' <repeats 200 times>..., format=6408, type=5126, flip_y=false, outp ut_row_bytes=1536) at ../../media/renderers/skcanvas_video_renderer.cc:548 #1 0x000055e136e6e6a8 in (anonymous namespace)::(anonymous namespace)::TexImageHelper (frame=0x5dd1d343860, format=6408, type=5126, flip_y=false, temp_buffer=0x7ffd38cfb2d0) at ../../med ia/renderers/skcanvas_video_renderer.cc:622 #2 0x000055e136e6e894 in (anonymous namespace)::SkCanvasVideoRenderer::TexSubImage2D (target=3553, gl=0x5dd1d1aed20, frame=0x5dd1d343860, level=0, format=6408, type=5126, xoffset=0, yoff set=0, flip_y=false, premultiply_alpha=false) at ../../media/renderers/skcanvas_video_renderer.cc:903 #3 0x000055e135b6ec0d in (anonymous namespace)::WebMediaPlayerMS::texImageImpl (this=0x5dd1cbb07a0, functionID=(anonymous namespace)::WebMediaPlayer::TexSubImage2D, target=3553, gl=0x5dd 1d1aed20, level=0, internalformat=0, format=6408, type=5126, xoffset=0, yoffset=0, zoffset=0, flip_y=false, premultiply_alpha=false) at ../../content/renderer/media/webmediaplayer_ms.cc:6 27 #4 0x000055e13422f0c8 in (anonymous namespace)::HTMLVideoElement::texImageImpl (this=0x1dd0fc5832e8, functionID=(anonymous namespace)::WebMediaPlayer::TexSubImage2D, target=3553, gl=0x5d d1d1aed20, level=0, internalformat=0, format=6408, type=5126, xoffset=0, yoffset=0, zoffset=0, flipY=false, premultiplyAlpha=false) at ../../third_party/WebKit/Source/core/html/HTMLVideoE lement.cpp:242 #5 0x000055e13556bc40 in (anonymous namespace)::WebGLRenderingContextBase::texImageHelperHTMLVideoElement (this=0x825023af338, functionID=(anonymous namespace)::WebGLRenderingContextBase ::TexSubImage2D, target=3553, level=0, internalformat=0, format=6408, type=5126, xoffset=0, yoffset=0, zoffset=0, video=0x1dd0fc5832e8, sourceImageRect=..., depth=1, unpackImageHeight=0, exceptionState=...) at ../../third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5272
Description was changed from
==========
Enable CopyTextureCHROMIUM in Blink for Tex{Sub}Image2D with more cases
BUG=612542
TEST=webgl and webgl2 conformance tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
to
==========
Enable CopyTextureCHROMIUM in Blink for Tex{Sub}Image2D with more cases
BUG=612542
TEST=webgl and webgl2 conformance tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
On 2017/03/13 09:32:46, qiankun wrote: > On 2017/03/11 00:46:11, Zhenyao Mo wrote: > > On 2017/03/10 23:54:19, qiankun wrote: > > > > > > https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... > > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... > > > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: > // > > > texture to half float or float texture. > > > On 2017/03/10 22:45:43, Zhenyao Mo wrote: > > > > On 2017/03/10 02:48:04, qiankun wrote: > > > > > On 2017/03/10 00:10:24, Zhenyao Mo wrote: > > > > > > Well, the same can be said for RGBA8 formats, so I am not sure if we > > want > > > to > > > > > put > > > > > > in this limit. > > > > > > > > > > Or we can increase the tolerance in the tests? I met this issue for > > > > > depth_capture_tests where tolerance == 1.5/65535 : > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep.... > > > > > > > > > > > > > What will be the tolerance to make it pass? I think slightly increase the > > > > tolerance is fine if the benefit is a much faster path. > > > > > > When calling glCopy{Sub}TextureCHROMIUM, source_internal_format of the > source > > > texture in this case is BGRA8 or RGBA8 on different OSes. So, we should set > > the > > > tolerance to 1.0 / 255 to make the test pass. > > > > That (from 1.5 / 65535 to 255 / 65535) seems a big tolerance. You encode 8bit > > into a float, so you can't lose so much data. tolerance with the same scale > as > > 1.5/65535 bur increased a little bit seems fine. > > The precision lost doesn't take place at GPU-GPU copy from BGRA8 to float > texture. It comes from painting the video texture in media::PIXEL_FORMAT_Y16 to > an imagebuffer in BGRA8 format, see the code path below on Linux: Thanks for tracking that down Qiankun. I've CC'd hubbe@ and aleksandar.stojiljkovic@ as they both worked on this code path, which is relatively new, and which allows images from depth cameras to be uploaded to WebGL textures. This tolerance regression is unfortunate, and will basically break this code path. Could you please try to avoid it? One idea would be to add another API to HTMLVideoElement which would return true if it's rendering a 16-bit video, and use it to skip the acceleration paths in WebGLRenderingContextBase::texImageHelperHTMLVideoElement, instead falling back to CPU-to-GPU upload? > #0 (anonymous namespace)::WebMediaPlayerMS::paint (this=0x2f4f7dd63a20, > canvas=0x2f4f7ddc3220, rect=..., flags=...) at > ../../content/renderer/media/webmediaplayer_ms.cc:463 > #1 0x0000564b6b65ff00 in (anonymous > namespace)::HTMLVideoElement::paintCurrentFrame (this=0x9a1b5bc32e8, > canvas=0x2f4f7ddc3220, destRect=..., flags=0x0) at > ../../third_party/WebKit/Sourc > e/core/html/HTMLVideoElement.cpp:212 > #2 0x0000564b6c99c88c in (anonymous > namespace)::WebGLRenderingContextBase::texImageHelperHTMLVideoElement > (this=0xda1a9d050e8, functionID=(anonymous namespace)::WebGLRenderingContextBase > ::TexImage2D, target=3553, level=0, internalformat=6408, format=6408, type=5126, > xoffset=0, yoffset=0, zoffset=0, video=0x9a1b5bc32e8, sourceImageRect=..., > depth=1, unpackImageHeight=0, e > xceptionState=...) at > ../../third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5249 > #3 0x0000564b6c99d06d in (anonymous > namespace)::WebGLRenderingContextBase::texImage2D (this=0xda1a9d050e8, > target=3553, level=0, internalformat=6408, format=6408, type=5126, video=0x9a1b > 5bc32e8, exceptionState=...) at > ../../third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5311 > > The CPU path does 16-bit integer to float conversion so the tolerance can be set > to 1.5 / 65535, see the CPU path below: > #0 (anonymous namespace)::(anonymous namespace)::FlipAndConvertY16 > (video_frame=0x5dd1d343860, out=0x5dd1d639020 '\253' <repeats 200 times>..., > format=6408, type=5126, flip_y=false, outp > ut_row_bytes=1536) at ../../media/renderers/skcanvas_video_renderer.cc:548 > #1 0x000055e136e6e6a8 in (anonymous namespace)::(anonymous > namespace)::TexImageHelper (frame=0x5dd1d343860, format=6408, type=5126, > flip_y=false, temp_buffer=0x7ffd38cfb2d0) at ../../med > ia/renderers/skcanvas_video_renderer.cc:622 > #2 0x000055e136e6e894 in (anonymous > namespace)::SkCanvasVideoRenderer::TexSubImage2D (target=3553, gl=0x5dd1d1aed20, > frame=0x5dd1d343860, level=0, format=6408, type=5126, xoffset=0, yoff > set=0, flip_y=false, premultiply_alpha=false) at > ../../media/renderers/skcanvas_video_renderer.cc:903 > #3 0x000055e135b6ec0d in (anonymous namespace)::WebMediaPlayerMS::texImageImpl > (this=0x5dd1cbb07a0, functionID=(anonymous > namespace)::WebMediaPlayer::TexSubImage2D, target=3553, gl=0x5dd > 1d1aed20, level=0, internalformat=0, format=6408, type=5126, xoffset=0, > yoffset=0, zoffset=0, flip_y=false, premultiply_alpha=false) at > ../../content/renderer/media/webmediaplayer_ms.cc:6 > 27 > #4 0x000055e13422f0c8 in (anonymous namespace)::HTMLVideoElement::texImageImpl > (this=0x1dd0fc5832e8, functionID=(anonymous > namespace)::WebMediaPlayer::TexSubImage2D, target=3553, gl=0x5d > d1d1aed20, level=0, internalformat=0, format=6408, type=5126, xoffset=0, > yoffset=0, zoffset=0, flipY=false, premultiplyAlpha=false) at > ../../third_party/WebKit/Source/core/html/HTMLVideoE > lement.cpp:242 > #5 0x000055e13556bc40 in (anonymous > namespace)::WebGLRenderingContextBase::texImageHelperHTMLVideoElement > (this=0x825023af338, functionID=(anonymous namespace)::WebGLRenderingContextBase > ::TexSubImage2D, target=3553, level=0, internalformat=0, format=6408, type=5126, > xoffset=0, yoffset=0, zoffset=0, video=0x1dd0fc5832e8, sourceImageRect=..., > depth=1, unpackImageHeight=0, > exceptionState=...) at > ../../third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5272
https://codereview.chromium.org/2738163002/diff/20001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:96: bug=612542) It's not a good idea to suppress this many test failures for a long period of time. Can you figure out a way to avoid needing to mark these tests as failing?
aleksandar.stojiljkovic@intel.com changed reviewers: + aleksandar.stojiljkovic@intel.com
https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: // texture to half float or float texture. On 2017/03/10 23:54:19, qiankun wrote: > On 2017/03/10 22:45:43, Zhenyao Mo wrote: > > On 2017/03/10 02:48:04, qiankun wrote: > > > On 2017/03/10 00:10:24, Zhenyao Mo wrote: > > > > Well, the same can be said for RGBA8 formats, so I am not sure if we want > to > > > put > > > > in this limit. > > > > > > Or we can increase the tolerance in the tests? I met this issue for > > > depth_capture_tests where tolerance == 1.5/65535 : > > > > > > https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep.... > > > > > > > What will be the tolerance to make it pass? I think slightly increase the > > tolerance is fine if the benefit is a much faster path. > > When calling glCopy{Sub}TextureCHROMIUM, source_internal_format of the source > texture in this case is BGRA8 or RGBA8 on different OSes. So, we should set the > tolerance to 1.0 / 255 to make the test pass. On 2017/03/14 02:33:31, Ken Russell wrote: > Thanks for tracking that down Qiankun. > > I've CC'd hubbe@ and aleksandar.stojiljkovic@ as they both worked on this code > path, which is relatively new, and which allows images from depth cameras to be > uploaded to WebGL textures. > > This tolerance regression is unfortunate, and will basically break this code > path. Could you please try to avoid it? One idea would be to add another API to > HTMLVideoElement which would return true if it's rendering a 16-bit video, and > use it to skip the acceleration paths in > WebGLRenderingContextBase::texImageHelperHTMLVideoElement, instead falling back > to CPU-to-GPU upload? Thanks. Yes, that would break the 16 bit video. Another work that could be affected is HDR video - hubbe@ has started work on HDR video decoding (https://crbug.com/682416) and I'm planning to work on support for GL_RGB10_A2. Could we try this as a fix (so that we also enable copy texture for isFloatType(type) case): [L.5213-L.5217] is guarding two things: 1. copyVideoTextureToPlatformTexture (no precision loss, I think) 2. accelerated image buffer (painting to RGBA is introducing cutting precision to 8 bits). So, would splitting 1. and 2. and inserting 16-bit video code on L.5269 between them fix this? I remember doing that [1] but changed my mind later to avoid introducing unnecessary change: [1] L.5095 https://codereview.chromium.org/2476693002/diff/100001/third_party/WebKit/Sou...
On 2017/03/14 13:43:47, aleksandar.stojiljkovic wrote: > https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > (right): > > https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: // > texture to half float or float texture. > On 2017/03/10 23:54:19, qiankun wrote: > > On 2017/03/10 22:45:43, Zhenyao Mo wrote: > > > On 2017/03/10 02:48:04, qiankun wrote: > > > > On 2017/03/10 00:10:24, Zhenyao Mo wrote: > > > > > Well, the same can be said for RGBA8 formats, so I am not sure if we > want > > to > > > > put > > > > > in this limit. > > > > > > > > Or we can increase the tolerance in the tests? I met this issue for > > > > depth_capture_tests where tolerance == 1.5/65535 : > > > > > > > > > > https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep.... > > > > > > > > > > What will be the tolerance to make it pass? I think slightly increase the > > > tolerance is fine if the benefit is a much faster path. > > > > When calling glCopy{Sub}TextureCHROMIUM, source_internal_format of the source > > texture in this case is BGRA8 or RGBA8 on different OSes. So, we should set > the > > tolerance to 1.0 / 255 to make the test pass. > > On 2017/03/14 02:33:31, Ken Russell wrote: > > Thanks for tracking that down Qiankun. > > > > I've CC'd hubbe@ and aleksandar.stojiljkovic@ as they both worked on this code > > path, which is relatively new, and which allows images from depth cameras to > be > > uploaded to WebGL textures. > > > > This tolerance regression is unfortunate, and will basically break this code > > path. Could you please try to avoid it? One idea would be to add another API > to > > HTMLVideoElement which would return true if it's rendering a 16-bit video, and > > use it to skip the acceleration paths in > > WebGLRenderingContextBase::texImageHelperHTMLVideoElement, instead falling > back > > to CPU-to-GPU upload? > > Thanks. > Yes, that would break the 16 bit video. Another work that could be affected is > HDR video - hubbe@ has started work on HDR video decoding > (https://crbug.com/682416) and I'm planning to work on support for GL_RGB10_A2. > > Could we try this as a fix (so that we also enable copy texture for > isFloatType(type) case): > [L.5213-L.5217] is guarding two things: > 1. copyVideoTextureToPlatformTexture (no precision loss, I think) > 2. accelerated image buffer (painting to RGBA is introducing cutting precision > to 8 bits). > So, would splitting 1. and 2. and inserting 16-bit video code on L.5269 between > them fix this? > I remember doing that [1] but changed my mind later to avoid introducing > unnecessary change: > [1] > L.5095 > https://codereview.chromium.org/2476693002/diff/100001/third_party/WebKit/Sou... Ken, Aleksandar, thank you very much. You comments are very useful to fix the precison lost issue. I will try the methods you suggested.
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Per today's working group discussion, we definitely need to do more work (spec wise) in the uploading to sRGB area. For now let's just do the simple thing and do no colorspace conversion.
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
In the past two weeks, I implemented a fallback path for glCopyTextureCHROMIUM in command buffer. It already landed at https://codereview.chromium.org/2760843002/. I updated this CL accordingly. https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5212: // texture to half float or float texture. On 2017/03/14 13:43:46, aleksandar.stojiljkovic wrote: > On 2017/03/10 23:54:19, qiankun wrote: > > On 2017/03/10 22:45:43, Zhenyao Mo wrote: > > > On 2017/03/10 02:48:04, qiankun wrote: > > > > On 2017/03/10 00:10:24, Zhenyao Mo wrote: > > > > > Well, the same can be said for RGBA8 formats, so I am not sure if we > want > > to > > > > put > > > > > in this limit. > > > > > > > > Or we can increase the tolerance in the tests? I met this issue for > > > > depth_capture_tests where tolerance == 1.5/65535 : > > > > > > > > > > https://cs.chromium.org/chromium/src/content/test/data/media/getusermedia-dep.... > > > > > > > > > > What will be the tolerance to make it pass? I think slightly increase the > > > tolerance is fine if the benefit is a much faster path. > > > > When calling glCopy{Sub}TextureCHROMIUM, source_internal_format of the source > > texture in this case is BGRA8 or RGBA8 on different OSes. So, we should set > the > > tolerance to 1.0 / 255 to make the test pass. > > On 2017/03/14 02:33:31, Ken Russell wrote: > > Thanks for tracking that down Qiankun. > > > > I've CC'd hubbe@ and aleksandar.stojiljkovic@ as they both worked on this code > > path, which is relatively new, and which allows images from depth cameras to > be > > uploaded to WebGL textures. > > > > This tolerance regression is unfortunate, and will basically break this code > > path. Could you please try to avoid it? One idea would be to add another API > to > > HTMLVideoElement which would return true if it's rendering a 16-bit video, and > > use it to skip the acceleration paths in > > WebGLRenderingContextBase::texImageHelperHTMLVideoElement, instead falling > back > > to CPU-to-GPU upload? > > Thanks. > Yes, that would break the 16 bit video. Another work that could be affected is > HDR video - hubbe@ has started work on HDR video decoding > (https://crbug.com/682416) and I'm planning to work on support for GL_RGB10_A2. > > Could we try this as a fix (so that we also enable copy texture for > isFloatType(type) case): > [L.5213-L.5217] is guarding two things: > 1. copyVideoTextureToPlatformTexture (no precision loss, I think) > 2. accelerated image buffer (painting to RGBA is introducing cutting precision > to 8 bits). > So, would splitting 1. and 2. and inserting 16-bit video code on L.5269 between > them fix this? > I remember doing that [1] but changed my mind later to avoid introducing > unnecessary change: > [1] > L.5095 > https://codereview.chromium.org/2476693002/diff/100001/third_party/WebKit/Sou... Done as aleksandar suggested. https://codereview.chromium.org/2738163002/diff/20001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:96: bug=612542) On 2017/03/14 02:33:38, Ken Russell wrote: > It's not a good idea to suppress this many test failures for a long period of > time. Can you figure out a way to avoid needing to mark these tests as failing? The fallback path is ready in command buffer. We don't need these suppressions with the fallback path now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great work. LGTM I have two questions, but you don't need to address them in this CL. https://codereview.chromium.org/2738163002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4896: // fallback path, but it doesn't know the type info. So, we still cannot do Maybe I miss something, but in command buffer side, we are able to get the type info for texSubImage calls because we can get the TextureRef object from target. https://codereview.chromium.org/2738163002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4906: if (type == GL_HALF_FLOAT_OES) Is this the right place for this? Because in most cases (all except ES2), HALF_FLOAT_OES is just emulated by HALF_FLOAT.
kbr@chromium.org changed reviewers: + geofflang@chromium.org
Qiankun, thank you for your hard dedicated work in getting to this point. It's awesome that CopyTextureCHROMIUM is able to handle all of these cases. The code changes look good, but a couple of questions about tests. CC'ing Geoff Lang, who's responsible for the pass-through command buffer, because I'm not sure we should skip so many WebGL tests in that configuration without knowing what we need to do to fix them. https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:55: bug=625738) What issues remain in getting this test to pass, both for WebGL 1.0 and 2.0? Is some more validation needed in Copy{Sub}TextureCHROMIUM? https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl_conformance_expectations.py:178: ['passthrough'], bug=1932) # angle bug ID These broad new skip entries are concerning, because we've been making a lot of progress on the pass-through command buffer to this point. What work needs to be done in the pass-through command buffer (or ANGLE) to make them pass?
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for reviewing, zmo and kbr. See my comments below. https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:55: bug=625738) On 2017/04/01 01:13:26, Ken Russell wrote: > What issues remain in getting this test to pass, both for WebGL 1.0 and 2.0? Is > some more validation needed in Copy{Sub}TextureCHROMIUM? We need to do TexSubImage validations in CopySubTextureCHROMIUM. The difficulty is CopySubTextureCHROMIUM doesn't have format/type arguments or know whether the caller is TexSubImage or not. https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl_conformance_expectations.py:178: ['passthrough'], bug=1932) # angle bug ID On 2017/04/01 01:13:26, Ken Russell wrote: > These broad new skip entries are concerning, because we've been making a lot of > progress on the pass-through command buffer to this point. What work needs to be > done in the pass-through command buffer (or ANGLE) to make them pass? I saw Geoff already landed two patches to update glCopyTextureCHROMIUM entry point in ANGLE and chromium passthrough path. But these tests are still failing with passthrough path, see try-bots in patch set#7. I saw at least two NOTIMPLEMENTED() crashes: Error GLES2DecoderPassthroughImpl::DoTraceBeginCHROMIUM and GLES2DecoderPassthroughImpl::DoFlushDriverCachesCHROMIUM. https://codereview.chromium.org/2738163002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2738163002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4896: // fallback path, but it doesn't know the type info. So, we still cannot do On 2017/03/31 21:12:59, Zhenyao Mo wrote: > Maybe I miss something, but in command buffer side, we are able to get the type > info for texSubImage calls because we can get the TextureRef object from target. We can detect the type info as you said. The fallback path doesn't work in ES2 context for WebGL 1. I can enhance the fallback path to support ES2 context in near future. Also I need to add type info into CopyTextureCHROMIUMResourceManager::DoCopyTexture and CopyTextureCHROMIUMResourceManager::DoCopySubTexture. I will investigate it next. https://codereview.chromium.org/2738163002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4906: if (type == GL_HALF_FLOAT_OES) On 2017/03/31 21:12:59, Zhenyao Mo wrote: > Is this the right place for this? Because in most cases (all except ES2), > HALF_FLOAT_OES is just emulated by HALF_FLOAT. This is used to skip gpu path for RGBA/RGBA/HALF_FLOAT_OES texture in WebGL 1. The dest type got at GLES2DecoderImpl::DoCopyTextureCHROMIUM and GLES2DecoderImpl::DoCopySubTextureCHROMIUM is still HALF_FLOAT_OES. I may use a fallback path for RGBA/RGBA/HAL_FLOAT_OES texture. I need to add type info into CopyTextureCHROMIUMResourceManager::DoCopyTexture and CopyTextureCHROMIUMResourceManager::DoCopySubTexture.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl_conformance_expectations.py:178: ['passthrough'], bug=1932) # angle bug ID So far I've only updated the bindings to the new function prototypes but haven't added support for any of the new features. The NOTIMPLEMENTED are expected. I think these failures are OK until we implement the new features in ANGLE's CHROMIUM_copy_texture
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2738163002/#ps140001 (title: "add back passthrough suppressions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/04/06 14:17:10, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Ken, this needs owner approval from you for blink side changes.
Thanks Qiankun and Geoff for your feedback. LGTM https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2738163002/diff/100001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:55: bug=625738) On 2017/04/05 08:43:14, qiankun wrote: > On 2017/04/01 01:13:26, Ken Russell wrote: > > What issues remain in getting this test to pass, both for WebGL 1.0 and 2.0? > Is > > some more validation needed in Copy{Sub}TextureCHROMIUM? > > We need to do TexSubImage validations in CopySubTextureCHROMIUM. The difficulty > is CopySubTextureCHROMIUM doesn't have format/type arguments or know whether the > caller is TexSubImage or not. OK. I assume we'll continue to work together to resolve this, either by adding more arguments to CopySubTextureCHROMIUM, or assuming that its caller is TexSubImage2D and ensuring that all callers using it for a TexImage2D operation get the format/type correct when defining the texture (if that will work).
The CQ bit was checked by zmo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:
While running git apply --index -3 -p1;
error: patch failed:
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5164
Falling back to three-way merge...
Applied patch to
'third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp' with
conflicts.
U third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
Patch:
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
Index: third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
diff --git
a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
index
3982dddc41d4fd2a2963f8324dfcbaff0c419407..15dcc4ab8f958eec31b3719969f27a3225700f56
100644
--- a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
@@ -819,13 +819,13 @@ sk_sp<SkImage>
WebGLRenderingContextBase::makeImageSnapshot(
SharedGpuContext::gr(), SkBudgeted::kYes, imageInfo, 0,
imageInfo.alphaType() == kOpaque_SkAlphaType ? nullptr
: &disableLCDProps);
- GLuint textureId = skia::GrBackendObjectToGrGLTextureInfo(
- surface->getTextureHandle(
- SkSurface::kDiscardWrite_TextureHandleAccess))
- ->fID;
+ const GrGLTextureInfo* textureInfo = skia::GrBackendObjectToGrGLTextureInfo(
+ surface->getTextureHandle(SkSurface::kDiscardWrite_TextureHandleAccess));
+ GLuint textureId = textureInfo->fID;
+ GLenum textureTarget = textureInfo->fTarget;
drawingBuffer()->copyToPlatformTexture(
- gl, textureId, GL_RGBA, GL_UNSIGNED_BYTE, 0, true, false, IntPoint(0, 0),
+ gl, textureTarget, textureId, true, false, IntPoint(0, 0),
IntRect(IntPoint(0, 0), drawingBuffer()->size()), BackBuffer);
return surface->makeImageSnapshot();
}
@@ -989,76 +989,6 @@ static const GLenum kSupportedTypesTexImageSourceES3[] = {
GL_HALF_FLOAT, GL_FLOAT, GL_UNSIGNED_INT_10F_11F_11F_REV,
};
-bool isUnsignedIntegerFormat(GLenum internalformat) {
- switch (internalformat) {
- case GL_R8UI:
- case GL_R16UI:
- case GL_R32UI:
- case GL_RG8UI:
- case GL_RG16UI:
- case GL_RG32UI:
- case GL_RGB8UI:
- case GL_RGB16UI:
- case GL_RGB32UI:
- case GL_RGBA8UI:
- case GL_RGB10_A2UI:
- case GL_RGBA16UI:
- case GL_RGBA32UI:
- return true;
- default:
- return false;
- }
-}
-
-bool isSignedIntegerFormat(GLenum internalformat) {
- switch (internalformat) {
- case GL_R8I:
- case GL_R16I:
- case GL_R32I:
- case GL_RG8I:
- case GL_RG16I:
- case GL_RG32I:
- case GL_RGB8I:
- case GL_RGB16I:
- case GL_RGB32I:
- case GL_RGBA8I:
- case GL_RGBA16I:
- case GL_RGBA32I:
- return true;
- default:
- return false;
- }
-}
-
-bool isIntegerFormat(GLenum internalformat) {
- return (isUnsignedIntegerFormat(internalformat) ||
- isSignedIntegerFormat(internalformat));
-}
-
-bool isFloatType(GLenum type) {
- switch (type) {
- case GL_FLOAT:
- case GL_HALF_FLOAT:
- case GL_HALF_FLOAT_OES:
- case GL_UNSIGNED_INT_10F_11F_11F_REV:
- return true;
- default:
- return false;
- }
-}
-
-bool isSRGBFormat(GLenum internalformat) {
- switch (internalformat) {
- case GL_SRGB_EXT:
- case GL_SRGB_ALPHA_EXT:
- case GL_SRGB8:
- case GL_SRGB8_ALPHA8:
- return true;
- default:
- return false;
- }
-}
-
} // namespace
WebGLRenderingContextBase::WebGLRenderingContextBase(
@@ -4959,21 +4889,23 @@ void WebGLRenderingContextBase::texImage2D(GLenum
target,
sentinelEmptyRect(), 1, 0, exceptionState);
}
-bool WebGLRenderingContextBase::canUseTexImageByGPU(
- TexImageFunctionID functionID,
- GLint internalformat,
- GLenum type) {
- if (functionID == TexImage2D &&
- (isFloatType(type) || isIntegerFormat(internalformat) ||
- isSRGBFormat(internalformat)))
+bool WebGLRenderingContextBase::canUseTexImageByGPU(GLenum type) {
+#if OS(MACOSX)
+ // RGB5_A1 is not color-renderable on NVIDIA Mac, see crbug.com/676209.
+ // Though, glCopyTextureCHROMIUM can handle RGB5_A1 internalformat by doing a
+ // fallback path, but it doesn't know the type info. So, we still cannot do
+ // the fallback path in glCopyTextureCHROMIUM for
+ // RGBA/RGBA/UNSIGNED_SHORT_5_5_5_1 format and type combination.
+ if (type == GL_UNSIGNED_SHORT_5_5_5_1)
return false;
- // TODO(crbug.com/622958): Implement GPU-to-GPU path for WebGL 2 and more
- // internal formats.
- if (functionID == TexSubImage2D &&
- (isWebGL2OrHigher() || extensionEnabled(OESTextureFloatName) ||
- extensionEnabled(OESTextureHalfFloatName) ||
- extensionEnabled(EXTsRGBName)))
+#endif
+ // OES_texture_half_float doesn't support HALF_FLOAT_OES type for
+ // CopyTexImage/CopyTexSubImage. And OES_texture_half_float doesn't require
+ // HALF_FLOAT_OES type texture to be renderable. So, HALF_FLOAT_OES type
+ // texture cannot be copied to or drawn to by glCopyTextureCHROMIUM.
+ if (type == GL_HALF_FLOAT_OES)
return false;
+
return true;
}
@@ -4996,10 +4928,8 @@ SnapshotReason
WebGLRenderingContextBase::functionIDToSnapshotReason(
void WebGLRenderingContextBase::texImageCanvasByGPU(
TexImageFunctionID functionID,
HTMLCanvasElement* canvas,
+ GLenum target,
GLuint targetTexture,
- GLenum targetInternalformat,
- GLenum targetType,
- GLint targetLevel,
GLint xoffset,
GLint yoffset,
const IntRect& sourceSubRectangle) {
@@ -5007,10 +4937,9 @@ void WebGLRenderingContextBase::texImageCanvasByGPU(
ImageBuffer* buffer = canvas->buffer();
if (buffer &&
!buffer->copyToPlatformTexture(
- functionIDToSnapshotReason(functionID), contextGL(), targetTexture,
- targetInternalformat, targetType, targetLevel,
- m_unpackPremultiplyAlpha, m_unpackFlipY, IntPoint(xoffset,
yoffset),
- sourceSubRectangle)) {
+ functionIDToSnapshotReason(functionID), contextGL(), target,
+ targetTexture, m_unpackPremultiplyAlpha, m_unpackFlipY,
+ IntPoint(xoffset, yoffset), sourceSubRectangle)) {
NOTREACHED();
}
} else {
@@ -5018,9 +4947,9 @@ void WebGLRenderingContextBase::texImageCanvasByGPU(
toWebGLRenderingContextBase(canvas->renderingContext());
ScopedTexture2DRestorer restorer(gl);
if (!gl->drawingBuffer()->copyToPlatformTexture(
- contextGL(), targetTexture, targetInternalformat, targetType,
- targetLevel, m_unpackPremultiplyAlpha, !m_unpackFlipY,
- IntPoint(xoffset, yoffset), sourceSubRectangle, BackBuffer)) {
+ contextGL(), target, targetTexture, m_unpackPremultiplyAlpha,
+ !m_unpackFlipY, IntPoint(xoffset, yoffset), sourceSubRectangle,
+ BackBuffer)) {
NOTREACHED();
}
}
@@ -5031,8 +4960,6 @@ void WebGLRenderingContextBase::texImageByGPU(
WebGLTexture* texture,
GLenum target,
GLint level,
- GLint internalformat,
- GLenum type,
GLint xoffset,
GLint yoffset,
GLint zoffset,
@@ -5045,24 +4972,18 @@ void WebGLRenderingContextBase::texImageByGPU(
ScopedTexture2DRestorer restorer(this);
GLuint targetTexture = texture->object();
- GLenum targetType = type;
- GLenum targetInternalformat = internalformat;
- GLint targetLevel = level;
bool possibleDirectCopy = false;
- if (functionID == TexImage2D) {
- possibleDirectCopy = Extensions3DUtil::canUseCopyTextureCHROMIUM(
- target, internalformat, type, level);
+ if (functionID == TexImage2D || functionID == TexSubImage2D) {
+ possibleDirectCopy = Extensions3DUtil::canUseCopyTextureCHROMIUM(target);
}
GLint copyXOffset = xoffset;
GLint copyYOffset = yoffset;
+ GLenum copyTarget = target;
// if direct copy is not possible, create a temporary texture and then copy
// from canvas to temporary texture to target texture.
if (!possibleDirectCopy) {
- targetLevel = 0;
- targetInternalformat = GL_RGBA;
- targetType = GL_UNSIGNED_BYTE;
contextGL()->GenTextures(1, &targetTexture);
contextGL()->BindTexture(GL_TEXTURE_2D, targetTexture);
contextGL()->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
@@ -5073,21 +4994,26 @@ void WebGLRenderingContextBase::texImageByGPU(
GL_CLAMP_TO_EDGE);
contextGL()->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T,
GL_CLAMP_TO_EDGE);
- contextGL()->TexImage2D(GL_TEXTURE_2D, 0, targetInternalformat, width,
- height, 0, GL_RGBA, targetType, 0);
+ contextGL()->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width, height, 0,
+ GL_RGBA, GL_UNSIGNED_BYTE, 0);
copyXOffset = 0;
copyYOffset = 0;
+ copyTarget = GL_TEXTURE_2D;
}
- if (image->isCanvasElement()) {
- texImageCanvasByGPU(functionID, static_cast<HTMLCanvasElement*>(image),
- targetTexture, targetInternalformat, targetType,
- targetLevel, copyXOffset, copyYOffset,
- sourceSubRectangle);
- } else {
- texImageBitmapByGPU(static_cast<ImageBitmap*>(image), targetTexture,
- targetInternalformat, targetType, targetLevel,
- !m_unpackFlipY);
+ {
+ // glCopyTextureCHROMIUM has a DRAW_AND_READBACK path which will call
+ // texImage2D. So, reset unpack buffer parameters before that.
+ ScopedUnpackParametersResetRestore temporaryResetUnpack(this);
+ if (image->isCanvasElement()) {
+ texImageCanvasByGPU(functionID, static_cast<HTMLCanvasElement*>(image),
+ …
(message too large)
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2738163002/#ps180001 (title: "rebase only--blink renaming and formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491835451670320,
"parent_rev": "0338b35867900b81de5e6960eec0572f152974b7", "commit_rev":
"03b5cd7ff9b0c8457efe2578d18de91141be30b6"}
Message was sent while issue was closed.
Description was changed from
==========
Enable CopyTextureCHROMIUM in Blink for Tex{Sub}Image2D with more cases
BUG=612542
TEST=webgl and webgl2 conformance tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
to
==========
Enable CopyTextureCHROMIUM in Blink for Tex{Sub}Image2D with more cases
BUG=612542
TEST=webgl and webgl2 conformance tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2738163002
Cr-Commit-Position: refs/heads/master@{#463269}
Committed:
https://chromium.googlesource.com/chromium/src/+/03b5cd7ff9b0c8457efe2578d18d...
==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/03b5cd7ff9b0c8457efe2578d18d... |
