ref counting issue fix for Context in gst-libs/gst/gl/gstglcontext.c

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ref counting issue fix for Context in gst-libs/gst/gl/gstglcontext.c

fvanzile
There is a ref counting issue in gst-libs/gst/gl/gstglcontext.c.
_GstGLContextPrivate holds onto GThread's without proper ref counting.  
when a thread was released I would get exceptions.
see my previous email "heap corruption in gstreamer when IP camera with
an rtp stream disconnects (losses network)"



--- a/gst-libs/gst/gl/gstglcontext.c
+++ b/gst-libs/gst/gl/gstglcontext.c
@@ -371,7 +371,7 @@ gst_gl_context_new (GstGLDisplay * display)
   * @context_type: a #GstGLPlatform specifying the type of context in
@handle
   * @available_apis: a #GstGLAPI containing the available OpenGL apis
in @handle
   *
- * Wraps an existing OpenGL context into a #GstGLContext.
+ * Wraps an existing OpenGL context into a #GstGLContext.
   *
   * Note: The caller is responsible for ensuring that the OpenGL context
   * represented by @handle stays alive while the returned #GstGLContext is
@@ -668,19 +668,37 @@ gst_gl_context_finalize (GObject * object)
          g_cond_wait (&context->priv->destroy_cond,
&context->priv->render_lock);
        GST_INFO_OBJECT (context, "gl thread joined");

-      g_thread_unref (context->priv->gl_thread);
-      context->priv->gl_thread = NULL;
+      if (context->priv->gl_thread) {
+        g_thread_unref (context->priv->gl_thread);
+        context->priv->gl_thread = NULL;
+      }
      }
      g_mutex_unlock (&context->priv->render_lock);

      gst_gl_window_set_close_callback (context->window, NULL, NULL, NULL);
      gst_object_unref (context->window);
+    context->window = NULL;
    }

-  if (context->priv->sharegroup)
+  if (context->priv->active_thread) {
+      g_thread_unref (context->priv->active_thread);
+      context->priv->active_thread = NULL;
+  }
+
+  if (context->priv->gl_thread) {
+    g_thread_unref (context->priv->gl_thread);
+    context->priv->gl_thread = NULL;
+  }
+
+  if (context->priv->sharegroup) {
      _context_share_group_unref (context->priv->sharegroup);
+    context->priv->sharegroup = NULL;
+  }

-  gst_object_unref (context->display);
+  if (context->display) {
+    gst_object_unref (context->display);
+    context->display = NULL;
+  }

    if (context->gl_vtable) {
      g_slice_free (GstGLFuncs, context->gl_vtable);
@@ -729,10 +747,19 @@ gst_gl_context_activate (GstGLContext * context,
gboolean activate)
    result = context_class->activate (context, activate);

    if (result && activate) {
-    context->priv->active_thread = g_thread_self ();
+    GThread *saveOldThread = context->priv->active_thread; //save the
old thread
+    context->priv->active_thread = g_thread_self ();  // ref count is
1, does not increment count
+    g_thread_ref (context->priv->active_thread); //add ref count
+    if (saveOldThread) {
+        g_thread_unref (saveOldThread); // release old thread
+    }
+
      g_private_set (&current_context_key, context);
    } else {
-    context->priv->active_thread = NULL;
+    if (context->priv->active_thread) {
+      g_thread_unref (context->priv->active_thread);
+      context->priv->active_thread = NULL;
+    }
      g_private_set (&current_context_key, NULL);
    }
    GST_OBJECT_UNLOCK (context);
@@ -1002,7 +1029,7 @@ gst_gl_context_create (GstGLContext * context,
            _context_share_group_ref (other_context->priv->sharegroup);

      context->priv->gl_thread = g_thread_new ("gstglcontext",
-        (GThreadFunc) gst_gl_context_create_thread, context);
+        (GThreadFunc) gst_gl_context_create_thread, context); // ref
count 2

      while (!context->priv->created)
        g_cond_wait (&context->priv->create_cond,
&context->priv->render_lock);
@@ -1728,10 +1755,19 @@ gst_gl_wrapped_context_get_gl_platform
(GstGLContext * context)
  static gboolean
  gst_gl_wrapped_context_activate (GstGLContext * context, gboolean
activate)
  {
-  if (activate)
-    context->priv->gl_thread = g_thread_self ();
-  else
-    context->priv->gl_thread = NULL;
+  if (activate) {
+    GThread *saveOldThread = context->priv->gl_thread; // save old thread
+    context->priv->gl_thread = g_thread_self (); // ref count is one
+    g_thread_ref (context->priv->gl_thread); // add ref count
+    if (saveOldThread) {
+      g_thread_unref (saveOldThread); // release old thread
+    }
+  } else {
+    if (context->priv->gl_thread) {
+      g_thread_unref (context->priv->gl_thread);
+      context->priv->gl_thread = NULL;
+    }
+  }

    return TRUE;
  }


_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

gstglcontext.c (66K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: ref counting issue fix for Context in gst-libs/gst/gl/gstglcontext.c

Sebastian Dröge-3
On Wed, 2017-02-22 at 22:42 -0800, Frank VanZile wrote:
> There is a ref counting issue in gst-libs/gst/gl/gstglcontext.c.
> _GstGLContextPrivate holds onto GThread's without proper ref counting.  
> when a thread was released I would get exceptions.
> see my previous email "heap corruption in gstreamer when IP camera with 
> an rtp stream disconnects (losses network)"

Thanks for your patch. We use Bugzilla for patch review, can you create
a bug there and attach the patch in "git format-patch" format? Thanks!

https://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer&component=gst-plugins-bad

--
Sebastian Dröge, Centricular Ltd · http://www.centricular.com
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: ref counting issue fix for Context in gst-libs/gst/gl/gstglcontext.c

Matthew Waters
In reply to this post by fvanzile
So essentially this takes and keeps an extra ref while the context is
active on that thread.  That makes sense.

Can you provide a git format-patch against master and attach it to a new
bug in bugzilla
https://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer&component=gst-plugins-bad,
we can attribute your work and review the change properly :)

Cheers
-Matt

On 23/02/17 17:42, Frank VanZile wrote:

> There is a ref counting issue in gst-libs/gst/gl/gstglcontext.c.
> _GstGLContextPrivate holds onto GThread's without proper ref
> counting.  when a thread was released I would get exceptions.
> see my previous email "heap corruption in gstreamer when IP camera
> with an rtp stream disconnects (losses network)"
>
>
>
> --- a/gst-libs/gst/gl/gstglcontext.c
> +++ b/gst-libs/gst/gl/gstglcontext.c
> @@ -371,7 +371,7 @@ gst_gl_context_new (GstGLDisplay * display)
>   * @context_type: a #GstGLPlatform specifying the type of context in
> @handle
>   * @available_apis: a #GstGLAPI containing the available OpenGL apis
> in @handle
>   *
> - * Wraps an existing OpenGL context into a #GstGLContext.
> + * Wraps an existing OpenGL context into a #GstGLContext.
>   *
>   * Note: The caller is responsible for ensuring that the OpenGL context
>   * represented by @handle stays alive while the returned
> #GstGLContext is
> @@ -668,19 +668,37 @@ gst_gl_context_finalize (GObject * object)
>          g_cond_wait (&context->priv->destroy_cond,
> &context->priv->render_lock);
>        GST_INFO_OBJECT (context, "gl thread joined");
>
> -      g_thread_unref (context->priv->gl_thread);
> -      context->priv->gl_thread = NULL;
> +      if (context->priv->gl_thread) {
> +        g_thread_unref (context->priv->gl_thread);
> +        context->priv->gl_thread = NULL;
> +      }
>      }
>      g_mutex_unlock (&context->priv->render_lock);
>
>      gst_gl_window_set_close_callback (context->window, NULL, NULL,
> NULL);
>      gst_object_unref (context->window);
> +    context->window = NULL;
>    }
>
> -  if (context->priv->sharegroup)
> +  if (context->priv->active_thread) {
> +      g_thread_unref (context->priv->active_thread);
> +      context->priv->active_thread = NULL;
> +  }
> +
> +  if (context->priv->gl_thread) {
> +    g_thread_unref (context->priv->gl_thread);
> +    context->priv->gl_thread = NULL;
> +  }
> +
> +  if (context->priv->sharegroup) {
>      _context_share_group_unref (context->priv->sharegroup);
> +    context->priv->sharegroup = NULL;
> +  }
>
> -  gst_object_unref (context->display);
> +  if (context->display) {
> +    gst_object_unref (context->display);
> +    context->display = NULL;
> +  }
>
>    if (context->gl_vtable) {
>      g_slice_free (GstGLFuncs, context->gl_vtable);
> @@ -729,10 +747,19 @@ gst_gl_context_activate (GstGLContext * context,
> gboolean activate)
>    result = context_class->activate (context, activate);
>
>    if (result && activate) {
> -    context->priv->active_thread = g_thread_self ();
> +    GThread *saveOldThread = context->priv->active_thread; //save the
> old thread
> +    context->priv->active_thread = g_thread_self ();  // ref count is
> 1, does not increment count
> +    g_thread_ref (context->priv->active_thread); //add ref count
> +    if (saveOldThread) {
> +        g_thread_unref (saveOldThread); // release old thread
> +    }
> +
>      g_private_set (&current_context_key, context);
>    } else {
> -    context->priv->active_thread = NULL;
> +    if (context->priv->active_thread) {
> +      g_thread_unref (context->priv->active_thread);
> +      context->priv->active_thread = NULL;
> +    }
>      g_private_set (&current_context_key, NULL);
>    }
>    GST_OBJECT_UNLOCK (context);
> @@ -1002,7 +1029,7 @@ gst_gl_context_create (GstGLContext * context,
>            _context_share_group_ref (other_context->priv->sharegroup);
>
>      context->priv->gl_thread = g_thread_new ("gstglcontext",
> -        (GThreadFunc) gst_gl_context_create_thread, context);
> +        (GThreadFunc) gst_gl_context_create_thread, context); // ref
> count 2
>
>      while (!context->priv->created)
>        g_cond_wait (&context->priv->create_cond,
> &context->priv->render_lock);
> @@ -1728,10 +1755,19 @@ gst_gl_wrapped_context_get_gl_platform
> (GstGLContext * context)
>  static gboolean
>  gst_gl_wrapped_context_activate (GstGLContext * context, gboolean
> activate)
>  {
> -  if (activate)
> -    context->priv->gl_thread = g_thread_self ();
> -  else
> -    context->priv->gl_thread = NULL;
> +  if (activate) {
> +    GThread *saveOldThread = context->priv->gl_thread; // save old
> thread
> +    context->priv->gl_thread = g_thread_self (); // ref count is one
> +    g_thread_ref (context->priv->gl_thread); // add ref count
> +    if (saveOldThread) {
> +      g_thread_unref (saveOldThread); // release old thread
> +    }
> +  } else {
> +    if (context->priv->gl_thread) {
> +      g_thread_unref (context->priv->gl_thread);
> +      context->priv->gl_thread = NULL;
> +    }
> +  }
>
>    return TRUE;
>  }
>


_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

signature.asc (527 bytes) Download Attachment
Loading...