Discussion:
[Bug 778434] New: applemedia: race condition in videotexturecache.m
Add Reply
"GStreamer" (GNOME Bugzilla)
2017-02-10 09:21:32 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Bug ID: 778434
Summary: applemedia: race condition in videotexturecache.m
Classification: Platform
Product: GStreamer
Version: 1.11.1
OS: Mac OS
Status: NEW
Severity: normal
Priority: Normal
Component: gst-plugins-bad
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@gmail.com
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-10 10:40:12 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

--- Comment #1 from Nick Kallen <***@gmail.com> ---
Created attachment 345416
--> https://bugzilla.gnome.org/attachment.cgi?id=345416&action=edit
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-10 10:48:14 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Nick Kallen <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345416|0 |1
is obsolete| |

--- Comment #2 from Nick Kallen <***@gmail.com> ---
Created attachment 345418
--> https://bugzilla.gnome.org/attachment.cgi?id=345418&action=edit
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-10 10:56:39 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Nick Kallen <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-10 10:58:11 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345418|none |reviewed
status| |

--- Comment #3 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 345418:
--> (https://bugzilla.gnome.org/review?bug=778434&attachment=345418)

::: 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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-10 11:08:22 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net,
| |***@gmail.com
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-10 11:10:00 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

--- Comment #4 from Nick Kallen <***@gmail.com> ---
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-10 11:13:43 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

--- Comment #5 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to Nick Kallen from comment #4)
Post by "GStreamer" (GNOME Bugzilla)
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
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 00:31:43 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

--- Comment #6 from Matthew Waters (ystreet00) <***@gmail.com> ---
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 00:31:59 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345418|reviewed |accepted-commit_now
status| |
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 10:26:13 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

--- Comment #7 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Author: Nick Kallen <***@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
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 10:26:17 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #345418|accepted-commit_now |committed
status| |
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 10:26:26 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|git master |1.11.2
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 10:53:04 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |REOPENED
Resolution|FIXED |---

--- Comment #8 from Sebastian Dröge (slomo) <***@coaxion.net> ---
This breaks the macOS build:
https://ci.gstreamer.net/job/cerbero-osx-x86-64-1010/5450/console
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 10:53:15 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

--- Comment #9 from Sebastian Dröge (slomo) <***@coaxion.net> ---
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 ^
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 12:05:22 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

--- Comment #10 from Nick Kallen <***@gmail.com> ---
Created attachment 345724
--> https://bugzilla.gnome.org/attachment.cgi?id=345724&action=edit
Fixes build issues on MacOS
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 12:07:56 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Nick Kallen <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@noraisin.net
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-14 13:51:20 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Edward Hervey <***@bilboed.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|REOPENED |RESOLVED
CC| |***@bilboed.com
Resolution|--- |FIXED

--- Comment #11 from Edward Hervey <***@bilboed.com> ---
commit 2f676d61a7e08076900e57b7b7ad56d5b296b66e
Author: Nick Kallen <***@me.com>
Date: Tue Feb 14 13:04:01 2017 +0100

Builds for MacOS

https://bugzilla.gnome.org/show_bug.cgi?id=778434
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2017-02-19 10:54:32 UTC
Reply
Permalink
Raw Message
https://bugzilla.gnome.org/show_bug.cgi?id=778434

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|1.11.2 |1.10.4
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
Loading...