GNOME Bugzilla – Bug 751104
make Gtk sinks usable from gst-launch
Last modified: 2015-08-16 13:38:16 UTC
This command should popup a new window: gst-launch-1.0 videotestsrc ! glupload ! gtkglsink
Created attachment 305474 [details] [review] GstGtkGLSink: fix possible warning in finalize If the element is finalized before going in READY state the widget could still be NULL.
Created attachment 305475 [details] [review] GstGtkGLSink: Post error if widget gets destroyed
Created attachment 305476 [details] [review] GstGtkGLSink: Post error if widget gets destroyed
Created attachment 305477 [details] [review] GstGtkGLSink: Ensure widget has a toplevel parent Checking for a parent is not enough, it must have a toplevel one. If widget has no toplevel parent then add it in a GtkWindow, that make it usable from gst-launch-1.0.
Code should be duplicated in the non-gl sink as well, but let's first check if that's correct, maybe we could add a base class when we have everything in place. I've been reading this code and I think it's lacking a few things to be solid: 1) It shouldn't call gtk_widget_realize() in _get_gl_context() but rather wait for "realized" signal. But then what should happen if show_frame() is called and the widget is still not realized? 2) It should handle "unrealized" signal, it can happen if the user remove the widget from its parent, especially since we keep a strong ref so that won't trigger "destroyed" signal. 3) Wondering if widget_destroy_cb() should g_clear_object(>k_sink->widget); instead. If we keep a strong ref on a widget we are responsible to release that ref on destroy normally. The widget cannot be used any more for any GTK work.
I like this! (In reply to Xavier Claessens from comment #5) > Code should be duplicated in the non-gl sink as well, but let's first check > if that's correct, maybe we could add a base class when we have everything > in place. > > I've been reading this code and I think it's lacking a few things to be > solid: > > 1) It shouldn't call gtk_widget_realize() in _get_gl_context() but rather > wait for "realized" signal. But then what should happen if show_frame() is > called and the widget is still not realized? "realize" needs to happen before show_frame. In order to get the GL context, we need the widget realized in order for Gtk to set up it's GL context. In order to share the Gtk GL context with upstream elements, it's required that the Gtk GL context be created in before upstream wants it to share with. For what to do when not realized, I can think of a couple of use case that will require different behaviour so maybe a property is required to choose between them. An starting list of strategies for the property: {error, warn, ignore, wait? (probably not possible)}. > 2) It should handle "unrealized" signal, it can happen if the user remove > the widget from its parent, especially since we keep a strong ref so that > won't trigger "destroyed" signal. Agreed. > 3) Wondering if widget_destroy_cb() should > g_clear_object(>k_sink->widget); instead. If we keep a strong ref on a > widget we are responsible to release that ref on destroy normally. The > widget cannot be used any more for any GTK work. Agreed.
commit d95fd04960d53e7314b830135d8d04577e3d6f9c Author: Xavier Claessens <xavier.claessens@collabora.com> Date: Wed Jun 17 09:36:57 2015 -0400 GstGtkGLSink: Ensure widget has a toplevel parent Checking for a parent is not enough, it must have a toplevel one. If widget has no toplevel parent then add it in a GtkWindow, that make it usable from gst-launch-1.0. https://bugzilla.gnome.org/show_bug.cgi?id=751104 commit b71b7dc9e60bc37a5143310b1bf00658de9a7a8b Author: Xavier Claessens <xavier.claessens@collabora.com> Date: Wed Jun 17 09:36:40 2015 -0400 GstGtkGLSink: Post error if widget gets destroyed https://bugzilla.gnome.org/show_bug.cgi?id=751104 commit 72b48a39d809115df6c9465081f983c874fd4025 Author: Xavier Claessens <xavier.claessens@collabora.com> Date: Tue Jun 16 16:21:26 2015 -0400 GstGtkGLSink: fix possible warning in finalize If the element is finalized before going in READY state the widget could still be NULL. https://bugzilla.gnome.org/show_bug.cgi?id=751104
(In reply to Xavier Claessens from comment #5) > Code should be duplicated in the non-gl sink as well, but let's first check > if that's correct, maybe we could add a base class when we have everything > in place. I'm reopening as code as not been duplicated in gtksink yet (should not be too much work). Xavier, do you have time to provide patchs ?
Created attachment 307501 [details] [review] gtk: Fix race between queue_draw and destroy In GTK dispose can be called before the last ref is reached. This happens when you close the container window. The dispose will be explicitly called, and destroyed notify will be fired. This patch fixes this race by properly tracking the widget state. In the sink, we now set the widget pointer to NULL, so the widget will properly get created again if you set your pipeline to NULL state after the widget was destroy, and set it back to PLAYING.
Created attachment 307502 [details] [review] gtksink: Ensure the copy pasted code remains the same Move back the default property at the same place they are in the other sink. This helps when using a diff viewer to synchronized this unfortunate copy paste.
Created attachment 307503 [details] [review] gtksink: Create a window if the widget is unparented The same way as it's now done with the gtkglsink, create a top level window if the widget is not parented.
I also took the liberty to fix some possible race. Note that it is still recommended to set the pipeline to NULL before letting GTK destroy the widget. One of the big gain here, is that the widget will be recreated when you go back to PLAYING now. Before you would have to create a new sink. It's a convenience.
Review of attachment 307501 [details] [review]: ::: ext/gtk/gstgtkglsink.c @@ +441,3 @@ { GstGtkGLSink *gtk_sink; + GstFlowReturn ret = GST_FLOW_OK; You don't need this var anymore ::: ext/gtk/gtkgstglwidget.c @@ +613,3 @@ + g_main_context_invoke_full (main_context, G_PRIORITY_DEFAULT, + (GSourceFunc) _queue_draw, g_object_ref (widget), g_object_unref); Thinking again about this, it would be simpler to use g_idle_add() here, and it has the advantage of retuning a source ID that you can g_source_remove() in dispose. You can then even remove your destroyed boolean. And that also solves my concern about the g_object_unref as GDestroyNotify that could be the last unref of the widget and I'm not 100% sure it's guaranteed to happen in GTK thread (it probably is, I guess...).
There is a call to gtk_widget_queue_resize() that should be protected the same way as _queue_draw().
Created attachment 307510 [details] [review] gtk: Fix race between queue_draw and destroy In GTK dispose can be called before the last ref is reached. This happens when you close the container window. The dispose will be explicitly called, and destroyed notify will be fired. This patch fixes this race by properly tracking the widget state. In the sink, we now set the widget pointer to NULL, so the widget will properly get created again if you set your pipeline to NULL state after the widget was destroy, and set it back to PLAYING.
Created attachment 307511 [details] [review] gtksink: Ensure the copy pasted code remains the same Move back the default property at the same place they are in the other sink. This helps when using a diff viewer to synchronized this unfortunate copy paste.
Created attachment 307512 [details] [review] gtksink: Create a window if the widget is unparented The same way as it's now done with the gtkglsink, create a top level window if the widget is not parented.
This time with the idler. One of the benifit of the idler is not simplicity (it's pretty much the same) but it let me avoid flooding the main context if the display path is really slow. Notice that two small change slipped in the race fix, I removed a spurious gtk_widget_queue_resize (GTK_WIDGET (widget)); called from the wrong thread. And I added a missing gst_buffer_replace (&widget->priv->buffer, NULL); as done in the GL case. This prevent assertion on resize. While testing resize I notice that a back frame is displayed. That because resize is wrongly implemented (it's not asynchronous as it should). I could remove the redraw idler, and the black frame would go away, but that would systematically drop a buffer. I'll file another but for that, and I'll create base classes cause it's becoming ridiculous to copy paste this stuff.
Attachment 307510 [details] pushed as 0bc7e9a - gtk: Fix race between queue_draw and destroy Attachment 307511 [details] pushed as dae8008 - gtksink: Ensure the copy pasted code remains the same Attachment 307512 [details] pushed as 4be2229 - gtksink: Create a window if the widget is unparented