[Bug 750310] GL: allow an application to provide an external backend

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=750310

--- Comment #16 from Matthew Waters <ystreet00 at gmail.com> ---
Review of attachment 305565:
 --> (https://bugzilla.gnome.org/review?bug=750310&attachment=305565)

Some comments.

::: gst-libs/gst/gl/gstgldisplay.c
@@ +113,3 @@
+   * @object: the #GstGLDisplay
+   *
+   * Allow to provide a @GstGLContext one-to-one with the display.

It's not one-to-one (whatever that means) it's overriding the context creation
mechanism.  Should also mention this could be called from any thread.  Also
that it's emitted with display's object lock held.

@@ +120,3 @@
+      g_signal_new ("create-context", G_TYPE_FROM_CLASS (klass),
+      G_SIGNAL_RUN_LAST, 0, NULL, NULL, g_cclosure_marshal_generic,
+      GST_GL_TYPE_CONTEXT, 0);

Should also pass the other context to the signal.

@@ +371,3 @@
+ * @error: resulting #GError
+ *
+ * Returns: Whether @pcontext contains the new created context.

Perhaps "whether a new context could be created"

Also mention that it requires the display's object lock to be held.

@@ +384,3 @@
+  g_return_val_if_fail (display != NULL, FALSE);
+  g_return_val_if_fail (p_context != NULL, FALSE);
+  g_return_val_if_fail (*p_context == NULL, FALSE);

Not sure you want to assert that what the p_context points to is NULL

@@ +388,3 @@
+  g_return_val_if_fail (*error == NULL, FALSE);
+
+  g_signal_emit (display, gst_gl_display_signals[CREATE_CONTEXT], 0,
&context);

Should also pass other_context to the signal.

@@ +405,3 @@
+    gst_object_ref (other_context);
+  else
+    other_context = gst_gl_display_get_gl_context_for_thread (display, NULL);

This is not going to work for non-glimagesink elements. Better to just use the
other_context directly and make glimagesink do this instead.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.