GNOME Bugzilla – Bug 787466
glimagesink: release buffers on GST_EVENT_FLUSH_START
Last modified: 2017-09-10 00:21:14 UTC
Created attachment 359427 [details] [review] Patch proposal According to GStreamer design documentation : Events, elements are expected to discard buffers on GST_EVENT_FLUSH_START. This helps upstream elements unblocked. Do almost same as GST_STATE_CHANGE_PAUSED_TO_READY. https://bugzilla.gnome.org/show_bug.cgi?id=787290 Note that "enable-last-sample" may be needed to be set to FALSE.
This is incorrect. On flush-start, element should only trigger threads to stop. It is on flush-stop that we wait for these threads to terminate an then release resources and reset the state (with stream locks held). I have not read your patch, but you should first fix your comment/explanation, and then fix the code accordingly.
Buffers, like the last-sample, are also released upon drain and allocation query (not yet implemented in GL elements like glimagesink). Note that display sink have no choice but to keep a frame (or a copy of it) for redraw purpose (glimagesink included). Meanwhile, explain the bug you are trying to fix please.
https://gstreamer.freedesktop.org/documentation/design/events.html "... discards any buffers it is holding ..." ----------------------------------------------- Flushing happens in two stages. a source element sends the FLUSH_START event to the downstream peer element. The downstream element starts rejecting buffers from the upstream elements. It sends the flush event further downstream and discards any buffers it is holding as well as return from the chain function as soon as possible. This makes sure that all upstream elements get unblocked. This event is not synchronized with the STREAM_LOCK and can be done in the application thread. ----------------------------------------------- Please read https://bugzilla.gnome.org/show_bug.cgi?id=787290 ----------------------------------------------- "gstglimagesink.c" In gst_glimage_sink_event(), add handler for GST_EVENT_FLUSH_START . (almost same as GST_STATE_CHANGE_PAUSED_TO_READY in gst_glimage_sink_change_state()) I think this prevents deadlock in gst_omx_video_dec_loop() by releasing output port. -----------------------------------------------
Review of attachment 359427 [details] [review]: That code is wrong, because if gst_video_overlay_expose() is called during a seek, you'll now get a visual glitch. ::: ext/gl/gstglimagesink.c @@ +1078,3 @@ + gl_sink->stored_sync = NULL; + + // GST_GLIMAGE_SINK_UNLOCK (gl_sink); C++ comment, useless line.
(In reply to Yuji Kuwabara from comment #3) > https://gstreamer.freedesktop.org/documentation/design/events.html > > "... discards any buffers it is holding ..." > > ----------------------------------------------- > Flushing happens in two stages. > > a source element sends the FLUSH_START event to the downstream peer element. > The downstream element starts rejecting buffers from the upstream elements. > It sends the flush event further downstream and discards any buffers it is > holding as well as return from the chain function as soon as possible. This > makes sure that all upstream elements get unblocked. This event is not > synchronized with the STREAM_LOCK and can be done in the application thread. > ----------------------------------------------- The documentation is maybe not clear, but the referred buffers are the in-flight buffer. E.g. a buffer being pushed will be discarded immediately. The other buffers are part of the state. Specifically to glimagesink, these buffer may be used through other interface, and you cannot just get rid of them without causing side effect. I think this bug is a pretty bad start, so I'll close it now. If you do encountered a bug, and if this patch fixes a bug for you, please file a new issue and describe the issue. We will guide you on how this can be fixed.