After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 778434 - applemedia: race condition in videotexturecache.m
applemedia: race condition in videotexturecache.m
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.11.1
Other Mac OS
: Normal normal
: 1.10.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-02-10 09:21 UTC by Nick Kallen
Modified: 2017-02-19 10:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Have iosglmemory be aware of the texture lifetime of videotexturecache (1.46 KB, patch)
2017-02-10 10:40 UTC, Nick Kallen
none Details | Review
manage the texture lifetime explicitly in iosglmemory (3.98 KB, patch)
2017-02-10 10:48 UTC, Nick Kallen
committed Details | Review
Fixes build issues on MacOS (3.75 KB, patch)
2017-02-14 12:05 UTC, Nick Kallen
none Details | Review

Description Nick Kallen 2017-02-10 09:21: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.
Comment 1 Nick Kallen 2017-02-10 10:40:12 UTC
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.
Comment 2 Nick Kallen 2017-02-10 10:48:14 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2017-02-10 10:58:11 UTC
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.
Comment 4 Nick Kallen 2017-02-10 11:10:00 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2017-02-10 11:13:43 UTC
(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
Comment 6 Matthew Waters (ystreet00) 2017-02-14 00:31:43 UTC
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.
Comment 7 Sebastian Dröge (slomo) 2017-02-14 10:26:13 UTC
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
Comment 8 Sebastian Dröge (slomo) 2017-02-14 10:53:04 UTC
This breaks the macOS build: https://ci.gstreamer.net/job/cerbero-osx-x86-64-1010/5450/console
Comment 9 Sebastian Dröge (slomo) 2017-02-14 10:53:15 UTC
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                          ^
Comment 10 Nick Kallen 2017-02-14 12:05:22 UTC
Created attachment 345724 [details] [review]
Fixes build issues on MacOS
Comment 11 Edward Hervey 2017-02-14 13:51:20 UTC
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