Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1862183003:
Mac h264: Work around CGLContext+VTDecompresionSession deadlock (Closed)
DescriptionMac h264: Work around CGLContext+VTDecompresionSession deadlock
When we converted h264 to be decoded as 4:2:0 instead of 4:2:2, we
noticed a spike in GPU crash rates, in particular, in CGLDestroyContext.
Many of these crashes had other threads in VideoToolbox in what appears
to be a deadlock.
Part of the change from 4:2:2 to 4:2:0 is adding a path where we will
do manual YUV->RGB conversion if we cannot display directly with
CoreAnimation. In the process of this conversion, we bind the planes
of the IOSurface allocated by VideoToolbox to OpenGL textures. We leave
these bound until the GLImage is destroyed.
My guess here is that the destruction of these resources in the driver
is being deferred until CGLDestroyContext (we have seen that IOSurfaces
can be kept around in the OpenGL driver long after they are unbound
in crbug.com/158469), whereupon they are triggering a deadlock with
VideoToolbox, which may be re-using them.
This patch forces us to destroy the OpenGL texture objects that
reference the IOSurfaces immediately after use (and while their owning
CVPixelBuffer is still retained), which will hopefully avoid the
conflict with VideoToolbox.
Note that this is a speculative fix.
BUG=598388
Committed: https://crrev.com/f838f5f4880b9f4fe54e6e10812708c1c89b23d7
Cr-Commit-Position: refs/heads/master@{#385690}
Patch Set 1 #Patch Set 2 : Add me as ui owner for Mac #
Total comments: 10
Patch Set 3 : Reduce scope #
Total comments: 1
Patch Set 4 : Switch back to BindBlock #Patch Set 5 : format #
Messages
Total messages: 20 (7 generated)
ccameron@chromium.org changed reviewers: + avi@chromium.org, dcastagna@chromium.org
Adding dcastagna@ for YUV->RGB change. I tried to get the diffs to be minimal, but CR was only-so-successful. Adding avi@ at ui/OWNER -- also, adding myself as ui/ Mac owner, if that seems reasonable to you.
lgtm Owner stamp; I don't know this GL stuff to say. https://codereview.chromium.org/1862183003/diff/20001/ui/OWNERS File ui/OWNERS (right): https://codereview.chromium.org/1862183003/diff/20001/ui/OWNERS#newcode7 ui/OWNERS:7: ccameron@chromium.org not ui/gl? I'm OK either way, but this comes at the cost of having to do reviews... :)
CCing reveman@ since he suggested to avoid adding an explicit struct for the converter in crrev.com/1419733005 https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.mm (right): https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.mm:12: #include "base/mac/foundation_util.h" Is foundation_util.h included twice? https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.mm:196: class GLImageIOSurface::RGBConverter { In my original patch I had a similar struct to keep together all the converter state. Reveman suggested to leave it in GLImageIOSurface. CCing him. https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.mm:378: glGenFramebuffersEXT(1, &framebuffer); Do we need to regen this one all the time too? https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.mm:379: base::ScopedClosureRunner destroy_resources_runner(base::BindBlock(^{ nit: I'm assuming the caret has some special meaning in objective c. Could we use lambda syntax instead?
Actually ccing reveman@
reveman@chromium.org changed reviewers: + reveman@chromium.org
As a speculative fix, I'd much rather have a minimal patch than something that also includes a lot of refactoring. Is all this really needed to evaluate if deleting the textures help reduce the crash rate?
Description was changed from ========== Mac h264: Work around CGLContext+VTDecompresionSession deadlock When we converted h264 to be decoded as 4:2:0 instead of 4:2:2, we noticed a spike in GPU crash rates, in particular, in CGLDestroyContext. Many of these crashes had other threads in VideoToolbox in what appears to be a deadlock. Part of the change from 4:2:2 to 4:2:0 is adding a path where we will do manual YUV->RGB conversion if we cannot display directly with CoreAnimation. In the process of this conversion, we bind the planes of the IOSurface allocated by VideoToolbox to OpenGL textures. We leave these bound until the GLImage is destroyed. My guess here is that the destruction of these resources in the driver is being deferred until CGLDestroyContext (we have seen that IOSurfaces can be kept around in the OpenGL driver long after they are unbound in crbug.com/158469), whereupon they are triggering a deadlock with VideoToolbox, which may be re-using them. This patch forces us to destroy the OpenGL texture objects that reference the IOSurfaces immediately after use (and while their owning CVPixelBuffer is still retained), which will hopefully avoid the conflict with VideoToolbox. Note that this is a speculative fix. Even if this doesn't fix the bug, though, this is a step towards sharing the RGBConverter objects between all GLImages, so that we don't re-create the same GL shader for every frame. The RGBConverter functions in the patch are orderer in a way to minimize diffs for review. They will be re-ordered in a follow-on patch. BUG=598388 ========== to ========== Mac h264: Work around CGLContext+VTDecompresionSession deadlock When we converted h264 to be decoded as 4:2:0 instead of 4:2:2, we noticed a spike in GPU crash rates, in particular, in CGLDestroyContext. Many of these crashes had other threads in VideoToolbox in what appears to be a deadlock. Part of the change from 4:2:2 to 4:2:0 is adding a path where we will do manual YUV->RGB conversion if we cannot display directly with CoreAnimation. In the process of this conversion, we bind the planes of the IOSurface allocated by VideoToolbox to OpenGL textures. We leave these bound until the GLImage is destroyed. My guess here is that the destruction of these resources in the driver is being deferred until CGLDestroyContext (we have seen that IOSurfaces can be kept around in the OpenGL driver long after they are unbound in crbug.com/158469), whereupon they are triggering a deadlock with VideoToolbox, which may be re-using them. This patch forces us to destroy the OpenGL texture objects that reference the IOSurfaces immediately after use (and while their owning CVPixelBuffer is still retained), which will hopefully avoid the conflict with VideoToolbox. Note that this is a speculative fix. BUG=598388 ==========
I'd much prefer to change this to have 1 GL shader per GLContextObj -- but I can do that in a separate patch. I've changed this to just use a lambda to delete the textures at exit of the function. https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.mm (right): https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.mm:12: #include "base/mac/foundation_util.h" On 2016/04/06 20:36:58, Daniele Castagna wrote: > Is foundation_util.h included twice? Oops. Removed. https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.mm:378: glGenFramebuffersEXT(1, &framebuffer); On 2016/04/06 20:36:58, Daniele Castagna wrote: > Do we need to regen this one all the time too? It made the RGBConverter not capture any state of the particular caller, so it seemed natural. Now that I've removed RGBConverter, it seems less necessary. https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.mm:379: base::ScopedClosureRunner destroy_resources_runner(base::BindBlock(^{ On 2016/04/06 20:36:58, Daniele Castagna wrote: > nit: I'm assuming the caret has some special meaning in objective c. Could we > use lambda syntax instead? Yeah, it's for a block, which is the ObjC version of a lambda. Lambdas are a bit nastier in that I need to bind the values of the variables -- I can't actually capture them and then put them in a base::ScopedClosureRunner. I've switch to the lambda version ... lmk which you prefer.
lgtm, thanks! https://codereview.chromium.org/1862183003/diff/40001/ui/OWNERS File ui/OWNERS (right): https://codereview.chromium.org/1862183003/diff/40001/ui/OWNERS#newcode7 ui/OWNERS:7: ccameron@chromium.org sgtm but please keep me in the loop for all glimage related changes
LGTM https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.mm (right): https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.mm:379: base::ScopedClosureRunner destroy_resources_runner(base::BindBlock(^{ On 2016/04/06 at 22:35:21, ccameron wrote: > On 2016/04/06 20:36:58, Daniele Castagna wrote: > > nit: I'm assuming the caret has some special meaning in objective c. Could we > > use lambda syntax instead? > > Yeah, it's for a block, which is the ObjC version of a lambda. > > Lambdas are a bit nastier in that I need to bind the values of the variables -- I can't actually capture them and then put them in a base::ScopedClosureRunner. I've switch to the lambda version ... lmk which you prefer. Can't you just add {y,uv}_texture in the closure list? I'm afraid we're not supposed to bind lambdas though according to https://chromium-cpp.appspot.com/. I don't have a strong preference here, I made that comment since I'm not familiar with objective C and I never saw a caret before a block.
Thanks! Updated back to the BindBlock bit. I'll keep reveman@ in the loop for the GLImage changes. Hopefully this will get me a high enough review volume to get my latency down (no really!). https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.mm (right): https://codereview.chromium.org/1862183003/diff/20001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.mm:379: base::ScopedClosureRunner destroy_resources_runner(base::BindBlock(^{ On 2016/04/07 01:35:07, Daniele Castagna wrote: > On 2016/04/06 at 22:35:21, ccameron wrote: > > On 2016/04/06 20:36:58, Daniele Castagna wrote: > > > nit: I'm assuming the caret has some special meaning in objective c. Could > we > > > use lambda syntax instead? > > > > Yeah, it's for a block, which is the ObjC version of a lambda. > > > > Lambdas are a bit nastier in that I need to bind the values of the variables > -- I can't actually capture them and then put them in a > base::ScopedClosureRunner. I've switch to the lambda version ... lmk which you > prefer. > > Can't you just add {y,uv}_texture in the closure list? > > I'm afraid we're not supposed to bind lambdas though according to > https://chromium-cpp.appspot.com/. If I include y/uv in the closure list, then I can't bind to create a base::Closure (I checked with danakj and vmpstr on this). > I don't have a strong preference here, I made that comment since I'm not > familiar with objective C and I never saw a caret before a block. > > I think for now BindBlock is better, so I went back to it. It would be nice to have a base::ScopedLambdaRunner, but that's a bikeshed for another day.
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, dcastagna@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1862183003/#ps80001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862183003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862183003/80001
Message was sent while issue was closed.
Description was changed from ========== Mac h264: Work around CGLContext+VTDecompresionSession deadlock When we converted h264 to be decoded as 4:2:0 instead of 4:2:2, we noticed a spike in GPU crash rates, in particular, in CGLDestroyContext. Many of these crashes had other threads in VideoToolbox in what appears to be a deadlock. Part of the change from 4:2:2 to 4:2:0 is adding a path where we will do manual YUV->RGB conversion if we cannot display directly with CoreAnimation. In the process of this conversion, we bind the planes of the IOSurface allocated by VideoToolbox to OpenGL textures. We leave these bound until the GLImage is destroyed. My guess here is that the destruction of these resources in the driver is being deferred until CGLDestroyContext (we have seen that IOSurfaces can be kept around in the OpenGL driver long after they are unbound in crbug.com/158469), whereupon they are triggering a deadlock with VideoToolbox, which may be re-using them. This patch forces us to destroy the OpenGL texture objects that reference the IOSurfaces immediately after use (and while their owning CVPixelBuffer is still retained), which will hopefully avoid the conflict with VideoToolbox. Note that this is a speculative fix. BUG=598388 ========== to ========== Mac h264: Work around CGLContext+VTDecompresionSession deadlock When we converted h264 to be decoded as 4:2:0 instead of 4:2:2, we noticed a spike in GPU crash rates, in particular, in CGLDestroyContext. Many of these crashes had other threads in VideoToolbox in what appears to be a deadlock. Part of the change from 4:2:2 to 4:2:0 is adding a path where we will do manual YUV->RGB conversion if we cannot display directly with CoreAnimation. In the process of this conversion, we bind the planes of the IOSurface allocated by VideoToolbox to OpenGL textures. We leave these bound until the GLImage is destroyed. My guess here is that the destruction of these resources in the driver is being deferred until CGLDestroyContext (we have seen that IOSurfaces can be kept around in the OpenGL driver long after they are unbound in crbug.com/158469), whereupon they are triggering a deadlock with VideoToolbox, which may be re-using them. This patch forces us to destroy the OpenGL texture objects that reference the IOSurfaces immediately after use (and while their owning CVPixelBuffer is still retained), which will hopefully avoid the conflict with VideoToolbox. Note that this is a speculative fix. BUG=598388 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Mac h264: Work around CGLContext+VTDecompresionSession deadlock When we converted h264 to be decoded as 4:2:0 instead of 4:2:2, we noticed a spike in GPU crash rates, in particular, in CGLDestroyContext. Many of these crashes had other threads in VideoToolbox in what appears to be a deadlock. Part of the change from 4:2:2 to 4:2:0 is adding a path where we will do manual YUV->RGB conversion if we cannot display directly with CoreAnimation. In the process of this conversion, we bind the planes of the IOSurface allocated by VideoToolbox to OpenGL textures. We leave these bound until the GLImage is destroyed. My guess here is that the destruction of these resources in the driver is being deferred until CGLDestroyContext (we have seen that IOSurfaces can be kept around in the OpenGL driver long after they are unbound in crbug.com/158469), whereupon they are triggering a deadlock with VideoToolbox, which may be re-using them. This patch forces us to destroy the OpenGL texture objects that reference the IOSurfaces immediately after use (and while their owning CVPixelBuffer is still retained), which will hopefully avoid the conflict with VideoToolbox. Note that this is a speculative fix. BUG=598388 ========== to ========== Mac h264: Work around CGLContext+VTDecompresionSession deadlock When we converted h264 to be decoded as 4:2:0 instead of 4:2:2, we noticed a spike in GPU crash rates, in particular, in CGLDestroyContext. Many of these crashes had other threads in VideoToolbox in what appears to be a deadlock. Part of the change from 4:2:2 to 4:2:0 is adding a path where we will do manual YUV->RGB conversion if we cannot display directly with CoreAnimation. In the process of this conversion, we bind the planes of the IOSurface allocated by VideoToolbox to OpenGL textures. We leave these bound until the GLImage is destroyed. My guess here is that the destruction of these resources in the driver is being deferred until CGLDestroyContext (we have seen that IOSurfaces can be kept around in the OpenGL driver long after they are unbound in crbug.com/158469), whereupon they are triggering a deadlock with VideoToolbox, which may be re-using them. This patch forces us to destroy the OpenGL texture objects that reference the IOSurfaces immediately after use (and while their owning CVPixelBuffer is still retained), which will hopefully avoid the conflict with VideoToolbox. Note that this is a speculative fix. BUG=598388 Committed: https://crrev.com/f838f5f4880b9f4fe54e6e10812708c1c89b23d7 Cr-Commit-Position: refs/heads/master@{#385690} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f838f5f4880b9f4fe54e6e10812708c1c89b23d7 Cr-Commit-Position: refs/heads/master@{#385690}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1874013002/ by ccameron@chromium.org. The reason for reverting is: This didn't fix the issue. The issue appears to be a GL program leak.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
