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 775045 - gtkglsink: re parenting the video widget into a different toplevel does not work
gtkglsink: re parenting the video widget into a different toplevel does not work
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-24 20:03 UTC by Juan Pablo Ugarte
Modified: 2018-11-03 15:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Simple test case (3.01 KB, text/x-csrc)
2016-11-24 20:03 UTC, Juan Pablo Ugarte
  Details
Test case using videotestsrc (3.03 KB, text/x-csrc)
2016-11-25 14:41 UTC, Juan Pablo Ugarte
  Details
Small patch to use g_clear_pointer() macros (3.75 KB, patch)
2016-11-25 15:13 UTC, Juan Pablo Ugarte
none Details | Review
WIP try to initialize GL context on realize event (6.81 KB, patch)
2016-11-25 15:17 UTC, Juan Pablo Ugarte
none Details | Review
WIP: initialize GL context on realize event (8.19 KB, patch)
2018-05-09 18:59 UTC, Juan Pablo Ugarte
none Details | Review
WIP: initialize GL context in create_context method (12.88 KB, patch)
2018-05-10 21:47 UTC, Juan Pablo Ugarte
none Details | Review
WIP: GstGLDisplay add context-removed signal and remove_context() (3.33 KB, patch)
2018-05-10 21:48 UTC, Juan Pablo Ugarte
none Details | Review

Description Juan Pablo Ugarte 2016-11-24 20:03:10 UTC
Created attachment 340711 [details]
Simple test case

Reparenting the video widget from gtkglsink to a different toplevel does not work because the GdkGLContext is different and GtkGstGLWidget uses the old one.

This is needed to implement fullscreen for example.

Ideally GtkGstGLWidget should get the context in the realize event and free it on unrealize if possible.

Attached a simple test case that toggles fullscreen mode on key release
Comment 1 Juan Pablo Ugarte 2016-11-25 14:41:13 UTC
Created attachment 340760 [details]
Test case using videotestsrc
Comment 2 Juan Pablo Ugarte 2016-11-25 15:13:31 UTC
Created attachment 340764 [details] [review]
Small patch to use g_clear_pointer() macros
Comment 3 Juan Pablo Ugarte 2016-11-25 15:17:07 UTC
Created attachment 340765 [details] [review]
WIP try to initialize GL context on realize event


After the second realize I get a bunch of

(a.out:13651): Gtk-WARNING **: fb setup not supported

bacause glCheckFramebufferStatusEXT() in gtk_gl_area_draw() does not return GL_FRAMEBUFFER_COMPLETE_EXT
Comment 4 Juan Pablo Ugarte 2018-05-09 18:59:23 UTC
Created attachment 371869 [details] [review]
WIP: initialize GL context on realize event

This patch makes going back to windowed mode work
Comment 5 Matthew Waters (ystreet00) 2018-05-10 01:47:19 UTC
Review of attachment 371869 [details] [review]:

The other part: changing the display/context used by the pipeline requires some thinking/design work as the GstContext machinery does not have support for that.

The easiest thing I can come up with is to add a 'resource-destroyed'-like signal on both GstGLDisplay and GstGLContext where on emission each user attempts to retrieve a new GstGLDisplay/GstGLContext.

::: ext/gtk/gtkgstglwidget.c
@@ +377,3 @@
+  if (!priv->other_context) {
+    GTK_GST_BASE_WIDGET_UNLOCK (gst_widget);
+    gst_gtk_invoke_on_main ((GThreadFunc) _get_gl_context, gst_widget);

Aren't we always called on the main thread already from GtkWidget?

@@ +388,3 @@
+
+  if (priv->context)
+    retval = gst_gl_display_add_context (priv->display, priv->context);

retval isn't declared or used.
Comment 6 Juan Pablo Ugarte 2018-05-10 13:34:52 UTC
(In reply to Matthew Waters (ystreet00) from comment #5)
> Review of attachment 371869 [details] [review] [review]:
> 
> The other part: changing the display/context used by the pipeline requires
> some thinking/design work as the GstContext machinery does not have support
> for that.
> 
> The easiest thing I can come up with is to add a 'resource-destroyed'-like
> signal on both GstGLDisplay and GstGLContext where on emission each user
> attempts to retrieve a new GstGLDisplay/GstGLContext.

I havent looked at the code at all, but from a quick look at the API
I would add 'context-removed' signal and gst_gl_display_remove_context () function to GstGLDisplay.
For GstGLContext maybe we can get away with a weak reference or we can add GtkWidget::destroy like signal if needed

> ::: ext/gtk/gtkgstglwidget.c
> @@ +377,3 @@
> +  if (!priv->other_context) {
> +    GTK_GST_BASE_WIDGET_UNLOCK (gst_widget);
> +    gst_gtk_invoke_on_main ((GThreadFunc) _get_gl_context, gst_widget);
> 
> Aren't we always called on the main thread already from GtkWidget?

yeah! copy/paste left over

> @@ +388,3 @@
> +
> +  if (priv->context)
> +    retval = gst_gl_display_add_context (priv->display, priv->context);
> 
> retval isn't declared or used.

Ops, I tried to remove all the debug things but missed that one
Comment 7 Juan Pablo Ugarte 2018-05-10 21:47:00 UTC
Created attachment 371913 [details] [review]
WIP: initialize GL context in create_context method

GtkGLArea create-context signal looks like a good place to initialize all the GL wrappers objects.
This still does not fix all issues, but I just want to post it here to get some feedback
Comment 8 Juan Pablo Ugarte 2018-05-10 21:48:06 UTC
Created attachment 371914 [details] [review]
WIP: GstGLDisplay add context-removed signal and remove_context()
Comment 9 GStreamer system administrator 2018-11-03 15:14:53 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/330.