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

[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

[Matthew Waters]

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