Mac: YouTube videos flashing, and not represented as overlays |
|||||||||||||
Issue descriptionVersion: r393351 OS: Mac What steps will reproduce the problem? (1) Watch a youtube video (something with a white background is better) (2) Move the mouse around (3) Observe black patches flashing See attached video. Of note is that we're seeing the video frames come to the compositor as non-overlay-candidate IOSurfaceDrawQuads -- it usually comes in as a YUV draw quad. The bug may be two things then: 1. We're sending YouTube video down the IOSurfaceDrawQuads path 2. Rendering of IOSurfaceDrawQuads may be broken in general (I think we may be near or at the point where we can kill them off).
,
May 12 2016
You are probably looking for a change made after 393106 (known good), but no later than 393118 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/8d0f39db6e6aa60e17c38e2ed7ace717baceef86..bcca0faaf544ca2c414629e11614daed74b2a50f
,
May 12 2016
Suspecting: r393116: Add media::VideoFrame::WrapNativeTextures() Local revert fixes the problem. This is easy to reproduce, so I can look into finding what's going wrong instead of going straight for a revert, if you'd like.
,
May 12 2016
Yeah, I'd appreciate it if you could figure out why it's using the wrong format. Maybe I messed up my change to media/video/gpu_memory_buffer_video_frame_pool.c?
,
May 16 2016
There are two issues here. The black flickering has been around for a long time, and needs to be fixed in M51-beta. The overlay issue is a simple typo in the above patch (just needs to set overlay bit for NV12 surfaces unconditionally, I think).
,
May 16 2016
The black flickering bisects back to https://chromium.googlesource.com/chromium/src/+log/ef5d078caf070abc857a90463435499896c93a83..3960b8b282030a4f91059cf25d6b01c8beab57b8 Which suggests the issue is with the YUV<->RGB conversion.
,
May 16 2016
This is a symptom of an underlying bug that we're aware of but somehow hadn't been causing problems ... yet (though I'd guess that issue 543324 was this). The bug is that GLRendererLayerTree is sloppy about how it re-uses buffers. It will keep an IOSurface on screen as a CALayer's contents while the GLRenderer is drawing to it. So far this has "just worked" for us, but it is really living dangerously. The issue was introduced in r385690, but only by coincidence. If you just put a glFlush anywhere in the middle of the compositor's drawing code, this issue will be triggered. That patch deletes a texture, which triggers a glFlush. I have an idea for a workaround. Going to try to get it working ASAP. Moving this to M51, since it happens there.
,
May 16 2016
The easiest workaround for this is to move the ownership of the y_texture and uv_texture from the GLImageIOSurface over to the YUVToRGBConverter. This will remove the implicit glFlush and make the issue go away. This is probably the only patch which is safe enough to merge to M51. The long term fix is to remove partial swap support. That might be an option once we have canvas, flash, and WebGL all rendering to overlays (hopefully M53).
,
May 17 2016
M51 Stable is launching very soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged ASAP. All changes MUST be merged into the release branch by 5pm on May 20 to make into the desktop Stable final build cut. Thank you!
,
May 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ded54a2ab9cf36b048b1d9e26f97397aa54dc46f commit ded54a2ab9cf36b048b1d9e26f97397aa54dc46f Author: ccameron <ccameron@chromium.org> Date: Wed May 18 00:36:40 2016 Workaround for Mac video flickering The partial swap support on Mac (GLRendererLayerTree) is brittle because it allows drawing to an IOSurface while that IOSurface is being used as a CALayer's contents. This happens to not cause problems because the compositor usually draws the entire page in one chunk. If we introduce a glFlush to the middle of this, then we can end up with partially drawn content appearing on-screen (in the case of the issue that the bug was filed on, black tiles behind a YouTube video). The texture deletion in GLImageIOSurface happens in the middle of the compositor frame being drawn, and causes an implicit glFlush. Avoid this deletion and implicit glFlush by scoping the textures used by GLImageIOSurface to the YUVToRGBConverter structure. The long-term solution is to have the GLRendererLayerTree not support partial swap, and make BufferQueue not allow re-use of a backbuffer until IOSurfaceIsInUse is no longer true. BUG= 611551 Review-Url: https://codereview.chromium.org/1980183002 Cr-Commit-Position: refs/heads/master@{#394290} [modify] https://crrev.com/ded54a2ab9cf36b048b1d9e26f97397aa54dc46f/ui/gl/gl_image_io_surface.mm [modify] https://crrev.com/ded54a2ab9cf36b048b1d9e26f97397aa54dc46f/ui/gl/yuv_to_rgb_converter.cc [modify] https://crrev.com/ded54a2ab9cf36b048b1d9e26f97397aa54dc46f/ui/gl/yuv_to_rgb_converter.h
,
May 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9c1004fad352061d1e2e4b410174b9eee2bccf0b commit 9c1004fad352061d1e2e4b410174b9eee2bccf0b Author: ccameron <ccameron@chromium.org> Date: Wed May 18 05:51:56 2016 Fix overlay computation in GpuMemoryBufferVideoFramePool In https://codereview.chromium.org/1960563002, we changed the computation of the ALLOW_OVERLAY flag, which disabled overlays. This changes the logic back to its previous behavior. While we're in the neighborhood, use regular TextureDrawQuads to draw video frames. We originally used IOSurfaceDrawQuad because only it had support for rectangle textures. TextureDrawQuad has had support for rectangle textures for a while now. BUG= 611551 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Review-Url: https://codereview.chromium.org/1993463002 Cr-Commit-Position: refs/heads/master@{#394338} [modify] https://crrev.com/9c1004fad352061d1e2e4b410174b9eee2bccf0b/cc/layers/video_layer_impl.cc [modify] https://crrev.com/9c1004fad352061d1e2e4b410174b9eee2bccf0b/cc/resources/video_resource_updater.cc [modify] https://crrev.com/9c1004fad352061d1e2e4b410174b9eee2bccf0b/cc/resources/video_resource_updater.h [modify] https://crrev.com/9c1004fad352061d1e2e4b410174b9eee2bccf0b/media/video/gpu_memory_buffer_video_frame_pool.cc
,
May 19 2016
Cls listed at comment #10 and #11 are baked/verified in canary and safe to merge? If so, please request a merge to M51 by applying "Merge-Request-51" label. Please note that M51 Stable is launching very soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged ASAP. All changes MUST be merged into the release branch by 5pm on May 20 to make into the desktop Stable final build cut.
,
May 19 2016
Adding merge request!
,
May 19 2016
Your change meets the bar and is auto-approved for M51 (branch: 2704)
,
May 19 2016
Please try to merge your change to M51 branch 2704 asap. Thank you.
,
May 20 2016
Working on the merge -- lots of conflicts, it's taking a little while.
,
May 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cd0c6bd64c989e9103c33f55ddb42d6721ab4f2a commit cd0c6bd64c989e9103c33f55ddb42d6721ab4f2a Author: Christopher Cameron <ccameron@chromium.org> Date: Fri May 20 18:20:22 2016 Mac 420 video: Add core profile shaders BUG= 611551 BUG= 535214 Review URL: https://codereview.chromium.org/1887843003 Cr-Commit-Position: refs/heads/master@{#387192} (cherry picked from commit 048fcf6d17a1f9404774b278386dc4e98482baf4) Review URL: https://codereview.chromium.org/1998313002 . Cr-Commit-Position: refs/branch-heads/2704@{#618} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/cd0c6bd64c989e9103c33f55ddb42d6721ab4f2a/ui/gl/gl_image_io_surface.mm
,
May 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c605e03f431ec403cb34578df279856065d9c7f commit 8c605e03f431ec403cb34578df279856065d9c7f Author: Christopher Cameron <ccameron@chromium.org> Date: Fri May 20 19:00:08 2016 Attach YUV420 to RGB converter to GLContext This is less hacky than the global lookup table. Leave the IOSurface-specific parts of the conversion inside GLImageIOSurface. If this converter is needed on other platforms, this will make it easier to reuse. BUG= 611551 BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Review URL: https://codereview.chromium.org/1882953006 Cr-Commit-Position: refs/heads/master@{#387986} (cherry picked from commit dde9e95fb0cd656c952906a630052d560d8a6753) Review URL: https://codereview.chromium.org/2005463002 . Cr-Commit-Position: refs/branch-heads/2704@{#619} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/gpu/command_buffer/service/gl_context_virtual.cc [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/gpu/command_buffer/service/gl_context_virtual.h [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/BUILD.gn [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/gl.gyp [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/gl_context.cc [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/gl_context.h [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/gl_context_cgl.cc [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/gl_context_cgl.h [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/gl_image_io_surface.h [modify] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/gl_image_io_surface.mm [add] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/yuv_to_rgb_converter.cc [add] https://crrev.com/8c605e03f431ec403cb34578df279856065d9c7f/ui/gl/yuv_to_rgb_converter.h
,
May 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/efc4b4f2155ce94b83bb773bc2e6e8f9ff69a4a1 commit efc4b4f2155ce94b83bb773bc2e6e8f9ff69a4a1 Author: Christopher Cameron <ccameron@chromium.org> Date: Fri May 20 19:08:25 2016 Workaround for Mac video flickering The partial swap support on Mac (GLRendererLayerTree) is brittle because it allows drawing to an IOSurface while that IOSurface is being used as a CALayer's contents. This happens to not cause problems because the compositor usually draws the entire page in one chunk. If we introduce a glFlush to the middle of this, then we can end up with partially drawn content appearing on-screen (in the case of the issue that the bug was filed on, black tiles behind a YouTube video). The texture deletion in GLImageIOSurface happens in the middle of the compositor frame being drawn, and causes an implicit glFlush. Avoid this deletion and implicit glFlush by scoping the textures used by GLImageIOSurface to the YUVToRGBConverter structure. The long-term solution is to have the GLRendererLayerTree not support partial swap, and make BufferQueue not allow re-use of a backbuffer until IOSurfaceIsInUse is no longer true. BUG= 611551 Review-Url: https://codereview.chromium.org/1980183002 Cr-Commit-Position: refs/heads/master@{#394290} (cherry picked from commit ded54a2ab9cf36b048b1d9e26f97397aa54dc46f) Review URL: https://codereview.chromium.org/2001753002 . Cr-Commit-Position: refs/branch-heads/2704@{#621} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/efc4b4f2155ce94b83bb773bc2e6e8f9ff69a4a1/ui/gl/gl_image_io_surface.mm [modify] https://crrev.com/efc4b4f2155ce94b83bb773bc2e6e8f9ff69a4a1/ui/gl/yuv_to_rgb_converter.cc [modify] https://crrev.com/efc4b4f2155ce94b83bb773bc2e6e8f9ff69a4a1/ui/gl/yuv_to_rgb_converter.h
,
May 20 2016
All merges are in. Hopefully none of them broke the build.
,
May 25 2016
Tested the issue on Mac 10.11.5 using chrome version 51.0.2704.63 with the below steps 1.Go to URL https://www.youtube.com/watch?v=2Fao21JPxow 2.Move the mouse around the video 3.Not observed any black patches. Please find the attached screen cast for the same.Adding TE-Verified labels. Thanks,
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by ccameron@chromium.org
, May 12 20166.7 MB
6.7 MB Download