GNOME Bugzilla – Bug 579926
[directshowvideosink] Doesn't update the last frame after a seek with the pipeline in PAUSED state.
Last modified: 2010-08-14 13:11:39 UTC
On linux, using any of the availables videosink and with the pipeline in PAUSED state, when I send a seek event, for example to the next frame, the new frame is drawn by the video sink. Instead on Windows, using the directshowvideosink, I need to set the pipeline into PLAYING state before sending the seek event and then reset the pipeline to PAUSED state to get the next frame drawn, which doesn't always work as some times the seek is done before returning to the paused state. I guess I could be a bug on the directshowvideosink. Looking at the code, the Direct Show graph is paused when the pipeline goes into PAUSED state, and it's set into playing state only when the pipeline returns into playing state, and that should be why it doesn't draw the new frame when the pipeline is paused. We should be able to update the graph when a new frame comes into the video sink.
Has this been fixed?
Created attachment 153776 [details] [review] 0001-dshowvideosink-Use-the-GstVideoSink-base-class
Created attachment 153777 [details] [review] 0002-dshowvideosink-Prepare-the-implementation-of-preroll
Created attachment 153778 [details] [review] 0003-dshowvideosink-Implement-preroll-virtual-method
The previous patches implements the preroll() virtual method and fixes the bug. There is still an issue I haven't been able to fix, which is rendering the first preroll buffer. The current implementation prepare the window and connect the graph in a PAUSED_TO_READY state change, whilst it should be done in set_caps(), this way the window will ready and the graph connected when the first preroll buffer is received. I have tried to call gst_dshowvideosink_prepare_window() in set_caps() function, which calls gst_x_overlay_prepare_xwindow_id (GST_X_OVERLAY (sink)). The problem is that in the application side GST_X_OVERLAY(sink) is NULL. I don't understand why calling this funtion in the PAUSED_TO_PLAY state works but it doesn't in the READY_TO_PAUSED :/
If you move the code over to GstVideoSink, it would be best if you only implemented the ::show_frame() vfunc instead of preroll/render, so that the "show-preroll-frame" property works.
Created attachment 153788 [details] [review] 0003-dshowvideosink-Implement-preroll-virtual-method I fixed a mistake in the previous patch and used show_frame().
Review of attachment 153788 [details] [review]: ::: sys/dshowvideosink/dshowvideosink.cpp @@ +178,3 @@ GST_DEBUG_FUNCPTR (gst_dshowvideosink_unlock_stop); + gstbasesink_class->render = GST_DEBUG_FUNCPTR (gst_dshowvideosink_show_frame); + gstbasesink_class->preroll = GST_DEBUG_FUNCPTR (gst_dshowvideosink_show_frame); I think this should be gstvideosink_class->show_frame = GST_DEBUG_FUNCPTR(gst_dshowvideosink_show_frame);
Created attachment 153796 [details] [review] 0002-dshowvideosink-Implement-show_frame-virtual-method.patch Thanks for the review :) I used ::show_frame() from GstVideoSink instead of ::preroll() and ::render(), and deleted the second commit since it doesn't make to much sense.
I also want to implement the ::expose() function. Should I use gst_base_sink_get_last_buffer() or rather use a reference of the last rendered buffer? (I'm not sure if both are equivalent)
Regarding the issues in Comment 5, it was related to a bug in my application. I'll attach the patch tomorrow.
Created attachment 153858 [details] [review] 0003-dshowvideosink-Prepare-window-and-link-graph-on-set_caps() Prepare the window and connect the graph on set_caps(), this way everything is ready to render the first preroll buffer.
Created attachment 153859 [details] [review] 0004-dshowvideosink-Implement-XOverlay-expose.patch Implement XOverlay expose() function
Created attachment 153860 [details] [review] 0003-dshowvideosink-Prepare-window-and-link-graph-on-set_caps() Fixed typo in previous patch
Based on comments on IRC I think we should hold off on these changes and put htem in after the release so they have more chance to get a bit of testing before being released. Some context (poorly transcribed): "it's plausible that patch set wouldn't make things any worse, though my attempts to implement preroll properly in there were very much more invasive to make it reliable, so I'd be a bit worried that it doesn't handle any of the edge cases. The biggest problem with it is that it uses COM, and the COM initialisation semantics work really poorly with GStreamer: you're supposed to init COM on any thread that uses COM, but you must uninit COM on the same thread - and GStreamer doesn't really give you any way to do that. So it's flaky."
That sounds good. I will include the patched version in the next WinBuilds release to get as much feedback as possible.
Was squashed in: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=a51d318759b07eca60811e3d47d8e81b6d12cb8c