GNOME Bugzilla – Bug 723529
glimagesink: only draws on buffer push
Last modified: 2014-05-30 02:35:05 UTC
I'm using a recent gst-plugins-gl from git (2afcb176 - 2014-01-17) with gstreamer 1.2.2. I'm trying to get glimagesink to update the display in 2 scenarios: 1) The video frame rate is very slow (say 1 fps) and the window manager sends an expose or configure(resize) event between frames. 2) The pipeline is paused and the the window manager sends an expose or configure(resize) event. I managed to get the code to respond to X expose events when the pipeline is paused by removing the check in gst_gl_window_x11_handle_event() that the expose event has been sent by the plugin in - with this patch... --- gst-plugins-gl-git-2afcb176/gst-libs/gst/gl/x11/gstglwindow_x11.c.pre-expose-when-paused 2014-01-31 17:21:19.851703135 +0000 +++ gst-plugins-gl-git-2afcb176/gst-libs/gst/gl/x11/gstglwindow_x11.c 2014-01-31 17:22:02.597449630 +0000 @@ -651,12 +651,6 @@ break; } - /* just ignore request that does not come from us - * they are un-necessary and it overloads the drawer - */ - if (!event.xexpose.send_event) - break; - if (window->draw) { context = gst_gl_window_get_context (window); context_class = GST_GL_CONTEXT_GET_CLASS (context); However, I think by doing this I've violated the locking assumptions of the code, because now I can get a deadlock between the gst buffer push thread and the gl thread when resizing the gl window while the pipeline is running... When a buffer is pushed, gst_glimage_sink_render() tries to replace the stored_buffer texture while holding the glimage_sink lock. When the old buffer is unreferenced, a synchronous call is made to the gl thread using gst_gl_window_send_message() to delete the texture. This call can deadlock waiting for completion if an expose event arrives in the gl thread, because gst_glimage_sink_on_draw() also obtains the glimage_sink lock. Full stack trace of the deadlocked threads below. Presumably this wouldn't have happened if I'd left the send_event check in the X expose event handler, but what should I have done? So basically my question is whether it is reasonable to try and get glimagesink to render in the scenarios I mentioned at above, or do I need to come up with another solution? Chris. (gdb) thread 5 [Switching to thread 5 (Thread 0xed8ffb40 (LWP 10995))]
+ Trace 233117
I think it's the applications' job to call gst_video_overlay_expose() whenever necessary... which would then make sure things are redrawn. For the case when glimagesink creates the window itself, it should listen to the expose events itself.
Created attachment 268071 [details] [review] glimagesink: remove unused stored_buffer field
Created attachment 268072 [details] [review] x11: allow Expose events from the outside for our own windows (In reply to comment #1) > I think it's the applications' job to call gst_video_overlay_expose() whenever > necessary... which would then make sure things are redrawn. > > For the case when glimagesink creates the window itself, it should listen to > the expose events itself. We apparently didn't do that :)
You don't get expose events in the application, presumably because gst_gl_window_x11_handle_event() has handled the event for the internally created gl window and this window is completely covering the parent window - the one set with gst_video_overlay_set_window_handle(). I suppose glimagesink could synthesize an expose event for the parent window whenever it doesn't handle an expose on the internal window, but this strikes me as a weird hack. Mind you - it also seems a bit weird that glimagesink implements gst_video_overlay_expose() by sending itself an expose event... They are normally only sent by the X server. Resizes are not such a problem as both the internal and parent windows get the ConfigureNotify event. However, if you call gst_video_overlay_expose() from the application, then the resulting redraw happens at the previous window size, so the display tends to always be one resize behind where it should be. ximagesink handles exposes by default, but you can turn it off with the handle-expose property. ximagesink doesn't seem to handle configure events until the next buffer push.
(In reply to comment #4) > You don't get expose events in the application, presumably because > gst_gl_window_x11_handle_event() has handled the event for the internally > created gl window and this window is completely covering the parent window - > the one set with gst_video_overlay_set_window_handle(). The completely covered state fits my oservations as well. > I suppose glimagesink could synthesize an expose event for the parent window > whenever it doesn't handle an expose on the internal window, but this strikes > me as a weird hack. Mind you - it also seems a bit weird that glimagesink > implements gst_video_overlay_expose() by sending itself an expose event... They > are normally only sent by the X server. The whole parenting scenario is a little weird and ultimately, should not be necessary. > Resizes are not such a problem as both the internal and parent windows get the > ConfigureNotify event. However, if you call gst_video_overlay_expose() from the > application, then the resulting redraw happens at the previous window size, so > the display tends to always be one resize behind where it should be. Well, the following are my observations. 1. This all works perfectly fine for 'raw' X. 2. Using gtk+ I experienced this issue. I tried XGetWindowAttributes on both the parent win or our internal win, sending an extra expose to the parent win, removing the parenting logic and hooking up both display connections to the GMainLoop. All resulting in stale data which leads me to believe that something is different on the gtk+ side of things. > ximagesink handles exposes by default, but you can turn it off with the > handle-expose property. > > ximagesink doesn't seem to handle configure events until the next buffer push.
(In reply to comment #5) > 2. Using gtk+ I experienced this issue. I tried XGetWindowAttributes on both > the parent win or our internal win, sending an extra expose to the parent win, > removing the parenting logic and hooking up both display connections to the > GMainLoop. All resulting in stale data which leads me to believe that > something is different on the gtk+ side of things. I had a trace through in the debugger. When I resize the window gtk+ re-calculates the layout for the container widget hierarchy and eventually my video window (GtkDrawingArea) gets the new allocation... gtk_drawing_area_size_allocate() calls gdk_window_move_resize(), which results in the expected XMoveResizeWindow() call in _gdk_x11_window_move_resize_child(). gtk_drawing_area_size_allocate() then calls gtk_drawing_area_send_configure(), my application "configure_event" handler calls gst_video_overlay_expose(), which results in the XGetWindowAttributes() call in gst_gl_window_x11_draw(). It does indeed return the old size, not the one set in the proceeding XMoveResizeWindow() call. I tried adding an XSync() call before XGetWindowAttributes(), but it make no difference. I don't know what one is supposed to do about this. Normal applications use ConfigureEvent to detect window size changes, but gstglwindow can't do this because it is not in charge of receiving events for parent_win (presumably this is why internal_win exists at all). If you can't rely on XGetWindowAttributes() to detect the size changes of parent_win, then the whole strategy seems to be out of luck. Perhaps gst needs a gst_video_overlay_configure() call added to the overlay interface, so the application can inform the plugin of the configure events.
1- What is the status of attachment 268072 [details] [review] ? 2- Also about attachment 268071 [details] [review] I think it should be reverted. Instead, just unref the buffer after releasing the lock to avoid the deadlock you encountered. We cannot rely on the fact that GstBaseSink keeps a ref on the last gstbuffer. Also the user can set "enable-last-sample=0" 3- what is the status of the first remark #1 1) ? Some optimizations to avoid unneeded conversion and FBO have been commited and about the extra expose event maybe you will get more chance due to a recent commit too: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/gst-libs/gst/gl/x11?id=42a3bb7d6e84065ff056ad2ec736cef305dd091c
1) Right so I believe that that entire check should disappear as we don't go through the expose event to redraw anymore. 2) Agreed 3) Relies on 1)
commit 1f90323a4dbe01f24d46a7dfde4642842f1ba1c3 Author: Matthew Waters <ystreet00@gmail.com> Date: Fri May 30 11:51:01 2014 +1000 glwindow_x11: allow expose events to redraw our window otherwise we will not update the window contents on low framerate streams until the next buffer https://bugzilla.gnome.org/show_bug.cgi?id=723529 commit 5eb4934750feefdac44e7403de86338e5fb1efcb Author: Matthew Waters <ystreet00@gmail.com> Date: Fri May 30 11:46:00 2014 +1000 glimagesink: unref the old buffer outside the lock it could very well deadlock https://bugzilla.gnome.org/show_bug.cgi?id=723529 commit 1cb7e22b98cfb0fd2b0777a08493a974c5202ed3 Author: Matthew Waters <ystreet00@gmail.com> Date: Fri May 30 11:35:04 2014 +1000 Revert "[880/906] glimagesink: remove unused stored_buffer field" This reverts commit af3a68db7dc473fb6903c18966b39e4c3f1464d7. Conflicts: ext/gl/gstglimagesink.c https://bugzilla.gnome.org/show_bug.cgi?id=723529
Closing as all the major issues should be solved. Chris, if the resizing negotiation is still an issue, please open a new bug.