Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896553003:
Re-enable GPU-GPU copies of video textures to GL_RED (Closed)
|
Created:
3 years, 7 months ago by jiajia.qin Modified:
3 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, haraken, blink-reviews, yunchao Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-enable GPU-GPU copies of video textures to GL_RED
The CL https://codereview.chromium.org/2791813003/ introduced two bugs
710673 and 710874. So currently, for format=GL_RED/GL_RED_INTEGER or
type=GL_FLOAT, it will not go GPU-GPU path. But by investigating that
CL, we still can go GPU-GPU path using CopySubTextureCHROMIUM instead of
CopyTextureCHROMIUM like the earlier code did.
Using copySubTextureCHROMIUM can bypass the potential bug in
CopyTextureCHROMIUM in MacOS. But still need to figure out the reason.
BUG=710673
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/2896553003
Cr-Commit-Position: refs/heads/master@{#488014}
Committed: https://chromium.googlesource.com/chromium/src/+/ca29dd7baa80c00d30039af7dcbdfc1abca6864c
Patch Set 1 : rebased, remove the current texture check to avoid context lost #Patch Set 2 : rebased and clean up #
Messages
Total messages: 45 (30 generated)
Description was changed from ========== Reback to GPU-GPU copies of video textures to GL_RED The CL https://codereview.chromium.org/2791813003/ introduced two bugs 710673 and 710874. So currently, for format=GL_RED/GL_RED_INTEGER or type=GL_FLOAT, it will not go GPU-GPU path. But by investigating that CL, we still can go GPU-GPU path using CopySubTextureCHROMIUM instead of CopyTextureCHROMIUM like the earlier code did. Using copySubTextureCHROMIUM can bypass the potential bug in CopyTextureCHROMIUM in MacOS. But still need to figure out the reason. BUG=710673 ========== to ========== Reback to GPU-GPU copies of video textures to GL_RED The CL https://codereview.chromium.org/2791813003/ introduced two bugs 710673 and 710874. So currently, for format=GL_RED/GL_RED_INTEGER or type=GL_FLOAT, it will not go GPU-GPU path. But by investigating that CL, we still can go GPU-GPU path using CopySubTextureCHROMIUM instead of CopyTextureCHROMIUM like the earlier code did. Using copySubTextureCHROMIUM can bypass the potential bug in CopyTextureCHROMIUM in MacOS. But still need to figure out the reason. BUG=710673 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 ==========
The CQ bit was checked by jiajia.qin@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: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by jiajia.qin@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_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jiajia.qin@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by jiajia.qin@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
jiajia.qin@intel.com changed reviewers: + kbr@chromium.org
Hi Ken, please take a look. This change lets GL_RED reback to GPU-GPU path via copySubTextureCHROMIUM. Seems that all bots are green. I don't have an Android NVIDIA device. So I can't test whether crbug.com/710874 can reback to this path.
jiajia.qin@intel.com changed reviewers: + sandersd@chromium.org, zmo@chromium.org
Ping kbr@ Add zmo@ and sandersd@
Jiajia, thanks for this CL and the cleanup. I'm concerned about introducing performance regressions for rendering videos, because CopyTextureCHROMIUM can be more efficient than the combined TexImage2D + CopySubTextureCHROMIUM. What do you think about this? I agree that since your CL passes all the tests, it is otherwise safe to land. Second, there was another bug introduced by my original CL -- Issue 733172 -- and I'm concerned about making more changes to this code without understanding that one. Do you think you may be able to help investigate that as well? I think it may require changes to the specification of CopySubTextureCHROMIUM. Currently I am completely swamped and have a large bug backlog. Let me change your CL description to be a little more clear.
Description was changed from ========== Reback to GPU-GPU copies of video textures to GL_RED The CL https://codereview.chromium.org/2791813003/ introduced two bugs 710673 and 710874. So currently, for format=GL_RED/GL_RED_INTEGER or type=GL_FLOAT, it will not go GPU-GPU path. But by investigating that CL, we still can go GPU-GPU path using CopySubTextureCHROMIUM instead of CopyTextureCHROMIUM like the earlier code did. Using copySubTextureCHROMIUM can bypass the potential bug in CopyTextureCHROMIUM in MacOS. But still need to figure out the reason. BUG=710673 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 ========== Re-enable GPU-GPU copies of video textures to GL_RED The CL https://codereview.chromium.org/2791813003/ introduced two bugs 710673 and 710874. So currently, for format=GL_RED/GL_RED_INTEGER or type=GL_FLOAT, it will not go GPU-GPU path. But by investigating that CL, we still can go GPU-GPU path using CopySubTextureCHROMIUM instead of CopyTextureCHROMIUM like the earlier code did. Using copySubTextureCHROMIUM can bypass the potential bug in CopyTextureCHROMIUM in MacOS. But still need to figure out the reason. BUG=710673 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/07/13 02:00:02, Ken Russell (switch to Gerrit) wrote: > Jiajia, thanks for this CL and the cleanup. I'm concerned about introducing > performance regressions for rendering videos, because CopyTextureCHROMIUM can be > more efficient than the combined TexImage2D + CopySubTextureCHROMIUM. What do > you think about this? I think there may not be performance regression. CopyTextureCHROMIUM will do a hidden TexImage2D for dest texture if the dest texture is generated by GenTextures and bind to target but never use TexImage2D to allocate storage. CopySubTextureCHROMIUM copies the sub contents of source texture to dest texture without redefining dest texture. So I think performance regression won't happen unless the dest texture has called TexImage2D, and reaches here to call TexImage2D again. In this case, we reach here to optimize webgl api TexImage2D with HTMLVideoElement. The source texture is the video texture, the dest texture is only a texture id but haven't been allocated storage. So we don't need to consider TexImage2D called twice issue. > Second, there was another bug introduced by my original CL -- Issue 733172 -- > and I'm concerned about making more changes to this code without understanding > that one. Do you think you may be able to help investigate that as well? I think > it may require changes to the specification of CopySubTextureCHROMIUM. Currently > I am completely swamped and have a large bug backlog. Sure, I will dig into it. > Let me change your CL description to be a little more clear. Thanks.
On 2017/07/13 07:16:54, jiajia.qin wrote: > > Second, there was another bug introduced by my original CL -- Issue 733172 -- > > and I'm concerned about making more changes to this code without understanding > > that one. Do you think you may be able to help investigate that as well? I > think > > it may require changes to the specification of CopySubTextureCHROMIUM. > Currently > > I am completely swamped and have a large bug backlog. > > Sure, I will dig into it. > I roughly looked at that part of code. Currently, TexImage2D with video will go to CopyVideoTextureToPlatformTexture path which will use gpu-gpu copy (copySubTextureChromium). TexSubImage2D with video will go to WebGLRenderingContextBase::TexImageImpl, and then use GLES2Implementation::TexSubImage2D. I think bug 701060 and 672895 are both because when copying video to gl texture, we should use visible size not natual size or coded size. However, in WebGLRenderingContextBase.cpp, we always use natual size as the source video texture's width and height. So our fixing is to copy source video's visible size to dest texture. But the fixing is only for TexImage2D. TexSubImage2D seems still use natual size. So I assume that's why bug 733172 is there. I will continue to investigate it tomorrow. Note: test video in bug 733172 has nature size:(723, 542), visible size(720, 542), coded size(720, 542)
On 2017/07/13 09:50:21, jiajia.qin wrote: > On 2017/07/13 07:16:54, jiajia.qin wrote: > > > Second, there was another bug introduced by my original CL -- Issue 733172 > -- > > > and I'm concerned about making more changes to this code without > understanding > > > that one. Do you think you may be able to help investigate that as well? I > > think > > > it may require changes to the specification of CopySubTextureCHROMIUM. > > Currently > > > I am completely swamped and have a large bug backlog. > > > > Sure, I will dig into it. > > > > I roughly looked at that part of code. Currently, TexImage2D with video will go > to CopyVideoTextureToPlatformTexture path which will use gpu-gpu copy > (copySubTextureChromium). > TexSubImage2D with video will go to WebGLRenderingContextBase::TexImageImpl, > and then use GLES2Implementation::TexSubImage2D. I think bug 701060 and 672895 > are both because when copying video to gl texture, we should use visible size > not natual size or coded size. However, in WebGLRenderingContextBase.cpp, > we always use natual size as the source video texture's width and height. > So our fixing is to copy source video's visible size to dest texture. > But the fixing is only for TexImage2D. TexSubImage2D seems still use natual > size. So I assume that's why bug 733172 is there. I will continue to investigate > it tomorrow. > > Note: test video in bug 733172 has > nature size:(723, 542), visible size(720, 542), coded size(720, 542) Confirmed that if we use visible size as the video image size. TexSubImage can work well and bug 733172 is gone.
skcanvas_video_renderer looks correct to me. Be warned that when we last tested, Firefox does not correctly crop to the visible rect, but since there is no way to query the visible rect (form JS) we really do need to do it as part of the copy path. lgtm.
On 2017/07/14 08:59:42, jiajia.qin wrote: > On 2017/07/13 09:50:21, jiajia.qin wrote: > > On 2017/07/13 07:16:54, jiajia.qin wrote: > > > > Second, there was another bug introduced by my original CL -- Issue 733172 > > -- > > > > and I'm concerned about making more changes to this code without > > understanding > > > > that one. Do you think you may be able to help investigate that as well? I > > > think > > > > it may require changes to the specification of CopySubTextureCHROMIUM. > > > Currently > > > > I am completely swamped and have a large bug backlog. > > > > > > Sure, I will dig into it. > > > > > > > I roughly looked at that part of code. Currently, TexImage2D with video will > go > > to CopyVideoTextureToPlatformTexture path which will use gpu-gpu copy > > (copySubTextureChromium). > > TexSubImage2D with video will go to WebGLRenderingContextBase::TexImageImpl, > > and then use GLES2Implementation::TexSubImage2D. I think bug 701060 and 672895 > > are both because when copying video to gl texture, we should use visible size > > not natual size or coded size. However, in WebGLRenderingContextBase.cpp, > > we always use natual size as the source video texture's width and height. > > So our fixing is to copy source video's visible size to dest texture. > > But the fixing is only for TexImage2D. TexSubImage2D seems still use natual > > size. So I assume that's why bug 733172 is there. I will continue to > investigate > > it tomorrow. > > > > Note: test video in bug 733172 has > > nature size:(723, 542), visible size(720, 542), coded size(720, 542) > > Confirmed that if we use visible size as the video image size. TexSubImage can > work well and bug 733172 is gone. That's great to hear. Thanks Jiajia for fixing this bug. Once this fix lands, we can close Issue 733172 as a duplicate of Issue 710673. LGTM
The CQ bit was checked by jiajia.qin@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.
On 2017/07/18 19:21:38, Ken Russell (switch to Gerrit) wrote: > On 2017/07/14 08:59:42, jiajia.qin wrote: > > On 2017/07/13 09:50:21, jiajia.qin wrote: > > > On 2017/07/13 07:16:54, jiajia.qin wrote: > > > > > Second, there was another bug introduced by my original CL -- Issue > 733172 > > > -- > > > > > and I'm concerned about making more changes to this code without > > > understanding > > > > > that one. Do you think you may be able to help investigate that as well? > I > > > > think > > > > > it may require changes to the specification of CopySubTextureCHROMIUM. > > > > Currently > > > > > I am completely swamped and have a large bug backlog. > > > > > > > > Sure, I will dig into it. > > > > > > > > > > I roughly looked at that part of code. Currently, TexImage2D with video will > > go > > > to CopyVideoTextureToPlatformTexture path which will use gpu-gpu copy > > > (copySubTextureChromium). > > > TexSubImage2D with video will go to WebGLRenderingContextBase::TexImageImpl, > > > and then use GLES2Implementation::TexSubImage2D. I think bug 701060 and > 672895 > > > are both because when copying video to gl texture, we should use visible > size > > > not natual size or coded size. However, in WebGLRenderingContextBase.cpp, > > > we always use natual size as the source video texture's width and height. > > > So our fixing is to copy source video's visible size to dest texture. > > > But the fixing is only for TexImage2D. TexSubImage2D seems still use natual > > > size. So I assume that's why bug 733172 is there. I will continue to > > investigate > > > it tomorrow. > > > > > > Note: test video in bug 733172 has > > > nature size:(723, 542), visible size(720, 542), coded size(720, 542) > > > > Confirmed that if we use visible size as the video image size. TexSubImage can > > work well and bug 733172 is gone. > > That's great to hear. Thanks Jiajia for fixing this bug. Once this fix lands, we > can close Issue 733172 as a duplicate of Issue 710673. LGTM Hi Ken. Sorry for that my expression was not clear. The failure reason of 733172, 701060 and 672895 are similar. We should use video |visible_rect| not |natural_size|. But the fixed position is different since they go different path. https://chromium-review.googlesource.com/c/575365/ is the fixing for 733172. Bug 710673 is another issue. They have same natural_size, visible_rect and coded_size. But it still fails when using CopyTextureCHROMIUM. However, it works when using CopySubTextureCHROMIUM. So it seems that CopyTextureCHROMIUM has potential bug. This patch is only for that GL_RED can re-use gpu-gpu path by using CopySubTextureCHROMIUM. But why CopyTextureCHROMIUM not work needs more investigating. I rebased this CL and did a minor clean up. Please take another look before landing.
Thanks for cleaning up the complex code I added. Still LGTM.
The CQ bit was checked by jiajia.qin@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2896553003/#ps60001 (title: "rebased and clean up")
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": 60001, "attempt_start_ts": 1500503270258600,
"parent_rev": "010657779368509120a79b1b77e5ee56d99d3234", "commit_rev":
"ca29dd7baa80c00d30039af7dcbdfc1abca6864c"}
Message was sent while issue was closed.
Description was changed from ========== Re-enable GPU-GPU copies of video textures to GL_RED The CL https://codereview.chromium.org/2791813003/ introduced two bugs 710673 and 710874. So currently, for format=GL_RED/GL_RED_INTEGER or type=GL_FLOAT, it will not go GPU-GPU path. But by investigating that CL, we still can go GPU-GPU path using CopySubTextureCHROMIUM instead of CopyTextureCHROMIUM like the earlier code did. Using copySubTextureCHROMIUM can bypass the potential bug in CopyTextureCHROMIUM in MacOS. But still need to figure out the reason. BUG=710673 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 ========== Re-enable GPU-GPU copies of video textures to GL_RED The CL https://codereview.chromium.org/2791813003/ introduced two bugs 710673 and 710874. So currently, for format=GL_RED/GL_RED_INTEGER or type=GL_FLOAT, it will not go GPU-GPU path. But by investigating that CL, we still can go GPU-GPU path using CopySubTextureCHROMIUM instead of CopyTextureCHROMIUM like the earlier code did. Using copySubTextureCHROMIUM can bypass the potential bug in CopyTextureCHROMIUM in MacOS. But still need to figure out the reason. BUG=710673 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/2896553003 Cr-Commit-Position: refs/heads/master@{#488014} Committed: https://chromium.googlesource.com/chromium/src/+/ca29dd7baa80c00d30039af7dcbd... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ca29dd7baa80c00d30039af7dcbd...
Message was sent while issue was closed.
On 2017/07/19 22:35:03, commit-bot: I haz the power wrote: > Committed patchset #2 (id:60001) as > https://chromium.googlesource.com/chromium/src/+/ca29dd7baa80c00d30039af7dcbd... This appears to have broken R8UI tests on Linux AMD R7 240 See https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A...
Message was sent while issue was closed.
On 2017/07/21 15:09:55, sugoi1 wrote: > On 2017/07/19 22:35:03, commit-bot: I haz the power wrote: > > Committed patchset #2 (id:60001) as > > > https://chromium.googlesource.com/chromium/src/+/ca29dd7baa80c00d30039af7dcbd... > > This appears to have broken R8UI tests on Linux AMD R7 240 > See > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... Thanks for pointing that out. I'll disable it for R8UI again. The overall code simplifications are desired.
Message was sent while issue was closed.
On 2017/07/21 15:27:59, Ken Russell (switch to Gerrit) wrote: > On 2017/07/21 15:09:55, sugoi1 wrote: > > On 2017/07/19 22:35:03, commit-bot: I haz the power wrote: > > > Committed patchset #2 (id:60001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/ca29dd7baa80c00d30039af7dcbd... > > > > This appears to have broken R8UI tests on Linux AMD R7 240 > > See > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... > > Thanks for pointing that out. I'll disable it for R8UI again. The overall code > simplifications are desired. Sorry for bringing this regression. I agree to disable R8UI again. Thanks Ken. I will dig more deeper to check why copy(Sub)TexureCHROMIUM fails for GL_RED next week. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
