GNOME Bugzilla – Bug 778434
applemedia: race condition in videotexturecache.m
Last modified: 2017-02-19 10:54:32 UTC
Context: Videotexturecache is a kind of wrapper on top of iosglmemory, used by avfvideosrc and vtdec. It creates a texture at the same time as creating the memory. Presumably it's a performance enhancement. Bug: Random EXC_BAD_ACCESS code=1 in GL code Cause: The cached texture needs to be freed at the right moment. The current technique is to pass the texture as user_data to gst_gl_memory_init and pass CFRelease as the destroy function. Unfortunately, the texture NEEDS to be destroyed in the gst_gl_context_thread or it will lead to a race condition where code running in the gst_gl_context_thread will reference a texture that is freed in a random gstreamer thread too soon.
Created attachment 345416 [details] [review] Have iosglmemory be aware of the texture lifetime of videotexturecache This solution treats the texture as a special object in iosglmemory and not as generic user_data to be destroyed by gstglmemory. The latter doesn't destroy the user_data in any particular thread. This solution may violate the encapsulation boundary between iosglmemory and videotexture cache; however, these objects are only used with one another and are tightly coupled anyway, since iosglmemory requires the use of a wrapped_texture. Ultimately, I think we should refactor this code so that videotexturecache is a subclass or a strategy object of iosglmemory, but I think this needs more thought before such a major change.
Created attachment 345418 [details] [review] manage the texture lifetime explicitly in iosglmemory This solution treats the texture as a special object in iosglmemory and not as generic user_data to be destroyed by gstglmemory. The latter doesn't destroy the user_data in any particular thread. This solution may violate the encapsulation boundary between iosglmemory and videotexture cache; however, these objects are only used with one another and are tightly coupled anyway, since iosglmemory requires the use of a wrapped_texture. Ultimately, I think we should refactor this code so that videotexturecache is a subclass or a strategy object of iosglmemory, but I think this needs more thought before such a major change.
Review of attachment 345418 [details] [review]: ::: sys/applemedia/iosglmemory.c @@ +43,3 @@ GstIOSGLMemory *mem = (GstIOSGLMemory *) gl_mem; + CFRelease (mem->texture); This can still be called from a random thread... basically whenever the memory loses its last reference. And the destroy notify is AFAIU called from exactly the same place, it would be called in the GST_GL_BASE_MEMORY_ALLOCATOR_CLASS(parent_class)->destroy() a couple of lines below.
The fix works because https://github.com/GStreamer/gst-plugins-bad/blob/95c842a860bef721390d9209411109fe6dff69cc/gst-libs/gst/gl/gstglbasememory.c#L445 gst_gl_context_thread_add (mem->context, (GstGLContextThreadFunc) _destroy_gl_objects, mem); whereas if (mem->notify) mem->notify (mem->user_data); An alternative fix is to run mem->notify on the context thread. But that assumes all user_data is gl-related which may or may not be the intent of the design.
(In reply to Nick Kallen from comment #4) > An alternative fix is to run mem->notify on the context thread. But that > assumes all user_data is gl-related which may or may not be the intent of > the design. I think that makes most sense
The intent is that notify not be called on the GL thread as it may require accessing resources (and locks) outside OpenGL which will deadlock when placed on the GL thread.
Author: Nick Kallen <nickkallen@me.com> Date: Fri Feb 10 11:32:23 2017 +0100 applemedia: free videotexturecache texture in gl thread The cached texture was treated as user_data passed to GstGLBaseMemory and freed with a GDestroyNotify function. However, this data must be treated specially: it must be destroyed in the GL thread. https://bugzilla.gnome.org/show_bug.cgi?id=778434
This breaks the macOS build: https://ci.gstreamer.net/job/cerbero-osx-x86-64-1010/5450/console
10:51:07 In file included from videotexturecache.m:30: 10:51:07 ./iosglmemory.h:46:3: error: unknown type name 'CVOpenGLESTextureRef'; did you mean 'CVOpenGLTextureRef'? 10:51:07 CVOpenGLESTextureRef texture; 10:51:07 ^~~~~~~~~~~~~~~~~~~~ 10:51:07 CVOpenGLTextureRef 10:51:07 /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CVOpenGLTexture.h:39:26: note: 'CVOpenGLTextureRef' declared here 10:51:07 typedef CVImageBufferRef CVOpenGLTextureRef; 10:51:07 ^ 10:51:07 In file included from videotexturecache.m:30: 10:51:07 ./iosglmemory.h:62:5: error: unknown type name 'CVOpenGLESTextureRef'; did you mean 'CVOpenGLTextureRef'? 10:51:07 CVOpenGLESTextureRef texture); 10:51:07 ^~~~~~~~~~~~~~~~~~~~ 10:51:07 CVOpenGLTextureRef 10:51:07 /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CVOpenGLTexture.h:39:26: note: 'CVOpenGLTextureRef' declared here 10:51:07 typedef CVImageBufferRef CVOpenGLTextureRef; 10:51:07 ^
Created attachment 345724 [details] [review] Fixes build issues on MacOS
commit 2f676d61a7e08076900e57b7b7ad56d5b296b66e Author: Nick Kallen <nickkallen@me.com> Date: Tue Feb 14 13:04:01 2017 +0100 Builds for MacOS https://bugzilla.gnome.org/show_bug.cgi?id=778434