GNOME Bugzilla – Bug 773912
fdsink: Returns flushing on PLAYING->PAUSED if it can block in ->render
Last modified: 2016-11-23 17:03:30 UTC
Created attachment 339068 [details] Example program Basesink has ->unlock() and ->unlock_stop() vfuncs, they are used to implement flushing and drop the buffer currently being rendered. In gst_base_sink_change_state(), when doing the PAUSED->PLAYING or PLAYING->PAUSED transition, it calls ->unlock(), which will cause the subclass to return GST_FLOW_FLUSHING and will stop streaming. I think we have to remove the unlock()/unlock_stop() from the state change, which also means that it can't take the preroll lock while blocking inside the ->render() vfunc. I'll try preparing patches for that to see how it works.
Created attachment 339084 [details] [review] NOMERGE: basesink: Don't call unlock() when going to PAUSED This is an example patch that seems to work, we need to figure out if we can just do it as-is or if we need ot add a flag so the subclasses can say if they support it. Instead of calling ->unlock() when going to PAUSED, just unlock the PREROLL lock before blocking in render and set it as prerolled. The only downside I can see is that if the subclass doesn't stop itself in the PAUSED state, it could result in a new buffer being delivered to the sink after the sink has claimed to have prerolled.
That seems like a breaking change, subclasses could assume that unlock() is called here. So that needs a flag at least, that is disabled by default. Why isn't the unlock() in your subclass making things work here?
It seems broken in every subclass I looked at that blocks inside render() . Nicolas mentioned that in some decoding sink a couple years ago, he had to had some hairy code in the unlock() to figure out if it was a pause or a real flush. I don't mind adding a flag of some kind, but I really don't like the code I'm adding to every subclass there, so I'm hoping we can do something nicer. Maybe have some way to make basesink know that it's already prerolled and release the lock before calling render() or something like that, the state machine there is horribly hairy and undocumented, so I'm really worried about modifying it.
Actually, it seems that this is handled correctly in appsink [1] and audiobasesink[2], that the solution is to not return from _render() but instead just call gst_base_sink_wait_preroll() and then continue afterwards... [1] https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/app/gstappsink.c#n838 [2] https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/audio/gstaudiobasesink.c#n2174
Created attachment 339150 [details] [review] fdsink: Block in preroll_wait on unlock The correct behaviour of anything stuck in the ->render() function between ->unlock() and ->unlock_stop() is to call gst_base_sink_wait_preroll() and only return an error if this returns an error, otherwise, it must continue where it left off!
Created attachment 339151 [details] [review] basesink: Document the interaction between unlock() and wait_preroll() This was totally non-obvious, the kind of big problem is that subclasses must be able to unblock their streaming thread and continue exactly where they left off on unpause!
If my interpretation is right, it means the following elements are also broken: gio*sink, multisocketsink, tcpclientsink, multiudpsink, dynudpsink, v4l2sink, shmsink, curl*sink, linsyssdisink, and dshowvideosink. The basesink element template comment on the unlock method is also misleading. Basically, every sink in GStremer that implement unlock/unlock_stop except appsink, shout2 and audiobasesink is currently misbehaving!
(In reply to Olivier Crête from comment #7) > If my interpretation is right, it means the following elements are also > broken: gio*sink, multisocketsink, tcpclientsink, multiudpsink, dynudpsink, > v4l2sink, shmsink, curl*sink, linsyssdisink, and dshowvideosink. The > basesink element template comment on the unlock method is also misleading. > > Basically, every sink in GStremer that implement unlock/unlock_stop except > appsink, shout2 and audiobasesink is currently misbehaving! It wouldn't be the first time that something is handled wrong in almost all of our elements. Let's just fix it then :) Please go ahead, or at least file bugs for all the broken ones with details.
Merged, I'll open new bugs for the other elements. commit e6febb5bc1a003b7b6d702675c6a83e9565e0ae6 Author: Olivier Crête <olivier.crete@collabora.com> Date: Fri Nov 4 18:54:10 2016 -0400 basesink: Document the interaction between unlock() and wait_preroll() This was totally non-obvious, the kind of big problem is that subclasses must be able to unblock their streaming thread and continue exactly where they left off on unpause! https://bugzilla.gnome.org/show_bug.cgi?id=773912 commit 5216322d39448ed61c86bb1b3dd9c8c5e6feccf3 Author: Olivier Crête <olivier.crete@collabora.com> Date: Fri Nov 4 18:46:45 2016 -0400 fdsink: Block in preroll_wait on unlock The correct behaviour of anything stuck in the ->render() function between ->unlock() and ->unlock_stop() is to call gst_base_sink_wait_preroll() and only return an error if this returns an error, otherwise, it must continue where it left off! https://bugzilla.gnome.org/show_bug.cgi?id=773912