GNOME Bugzilla – Bug 769079
playbin3: review signals
Last modified: 2018-11-03 11:48:09 UTC
Should document playbin3/decodebin3 to be an experimental technology preview and that the API might still change in future. This bug is about reviewing the exposed playbin3 API before releasing, in particular: - "{video,audio,text}-changed" signals - "{video,audio,text}-tags-changed" signals Emitted from streaming thread, which is awkward for apps. Apps should really use the new stream messages instead, which replace all of this much better. I would suggest to remove those entirely, or replace with a single streams-changed signal for discoverability if needed. - "get-{video,audio,text}-pad" action signals - "get-{video,audio,text}-tags" action signals Those are no longer needed, all the info is in the GstStreams now. Pads were only exposed to get caps for resolution etc, which is in GstStreams now. I would suggest to remove those entirely as well and make apps use the streams API, or alternatively provide a "get-streams" action signal which returns a streams collection if that makes sense (does it?). - "about-to-finish": remove for now until gapless playback is implemented?
Also the properties: - "n-{audio,video,text}" - "current-{audio,video,text}" I don't think there's value in trying to mimic backwards compatibility here. People will happily switch to new/better API as long as we tell them what to use instead.
My 2cents on the situation. The idea was to make it trivial to have any playbin-using app switch to playbin3 by using the USE_PLAYBIN3 environment variable. In order for that to fully work, playbin3 would need to have the same signal/properties available. That being said: * It adds extra maintenance (having to maintain that fallback/conversion API) * It adds false expectations (people using backward compatible API instead of the new one) After thinking about it a bit more I would just remove any signal/property which is not proxying signals of: * playsink * decodebin3 * urisourcebin "about-to-finish" should also go away until we implement the proper gapless handling. Note that this would also require adapting gst-validate accordingly (it currently uses the fallback properties/signals for checking playbin3 behaviour). Summary: Remove these properties and signals: * current-uri / suburi / current-suburi / about-to-finish : Re-add once gapless playback is properly implement * {current,n}-{audio,video,text} : Replaced by streams API * {audio,video,text}-[tags-]changed : Replaced by streams API * get-{audio,video,text}-{pad,tags} : Replaced by streams API
While we're doing API cleansing, the subtitle-encoding property in decodebin3/playbin3 should also go away and be replaced by a GstContext.
My bad, suburi actually works :)
(In reply to Edward Hervey from comment #4) > My bad, suburi actually works :) It does, although now it'd be trivial to support an array of auxillary URIs instead of just one, so we might want to change the API anyway
(In reply to Jan Schmidt from comment #5) > (In reply to Edward Hervey from comment #4) > > My bad, suburi actually works :) > > It does, although now it'd be trivial to support an array of auxillary URIs > instead of just one, so we might want to change the API anyway And that would help implementing bug #765305
In terms of API for this - it's been an often-requested feature to be able to add external subtitles after playback has already started (bug #589515), so perhaps the API should take this into account, e.g. one could make it an 'add-suburi' action signal rather than a property with G_TYPE_STRV or such.
(In reply to Tim-Philipp Müller from comment #7) > In terms of API for this - it's been an often-requested feature to be able > to add external subtitles after playback has already started (bug #589515), > so perhaps the API should take this into account, e.g. one could make it an > 'add-suburi' action signal rather than a property with G_TYPE_STRV or such. Yep, with a corresponding 'remove-suburi' one to satisfy the request in bug #765305.
Would it make sense to move all of the remove props/signals to a playbin3compat subclass? Or are we planning to maintain the playbin2 code the rest of the lifetime of the 1.x series?
I don't think it's an either/or decision, nor one we have to make right now. I also think it'd be fair enough to say that playbin2 is no longer fully maintained but still there for backwards compat for those who don't want to switch over. That's what we did with playbin1 after all. I don't think it's really a problem (wasn't then either).
(In reply to Edward Hervey from comment #2) > Note that this would also require adapting gst-validate accordingly (it > currently uses the fallback properties/signals for checking playbin3 > behaviour). I added GstStream support to validate so I think this is no longer an issue. Currently validate has to cache the current stream collection and selected streams. It would be nice to have this done in playbin3 as properties. Any objection? Which container do you prefer for the selected streams ? GList ? GPtrArray ?
Another thing I noticed is that we should also hook up the autoplug-* signals just like in old playbin/decodebin/uridecodebin. Otherwise we have regressions.
(In reply to Guillaume Desmottes from comment #11) > Currently validate has to cache the current stream collection and selected > streams. It would be nice to have this done in playbin3 as properties. > Any objection? Which container do you prefer for the selected streams ? > GList ? GPtrArray ? Would something like this be ok (2 top commits)? https://git.collabora.com/cgit/user/cassidy/gstreamer/gst-plugins-base.git/log/?h=playbin3-props
Instead of taking those signals verbatim from decodebin2, it would probably be good to rethink what they're good for and check if maybe a different API doesn't make more sense. Generally one thing that should probably be changed now is to include the GstStream object in the signal to provide more context / information.
As discussed at GUADEC with Edward and Tim, it might make sense to get rid of the signals completely for 1.10 instead of having a half-baked solution there that we will change again later. However the idea for the signals we came up with was (for parsebin and decodebin) a) only have a single signal instead of 5 (?) different ones. It would get all factories and produce the new, ordered factories plus returning if the stream should be directly exposed or autoplugging should continue. This should cover all use cases of the existing signals (note that autoplug-query is completely irrelevant in the context of decodebin3) b) pass the streams objects to that signal, and information about what kind of element is expected (parser/demuxer, decoder, ...). The latter would allow to distinguish parsebin and decodebin asking for things
*** Bug 769580 has been marked as a duplicate of this bug. ***
Created attachment 336248 [details] [review] playbin3: Remove fallback properties/signals These can all be used via the GstStream API
Still need to do something about the autoplug-* signals, but that's not a blocker as I see it.
Hi, I noticed that in 1.10 the "about-to-finish" signal is still advertised in the playbin3 documentation but it's never actually emitted. Apps switching from playbin to playbin3 can still connect it, but the associated callback is never called, and this can lead to some confusion. Should the signal be disabled until the functionality gets added back? Thanks, Antonio
(In reply to Edward Hervey from comment #2) <snip> > Summary: > Remove these properties and signals: > * current-uri / suburi / current-suburi / about-to-finish : Re-add once > gapless playback is properly implement > * {current,n}-{audio,video,text} : Replaced by streams API > * {audio,video,text}-[tags-]changed : Replaced by streams API > * get-{audio,video,text}-{pad,tags} : Replaced by streams API Bonus points all around if you can document how to port to playbin3 in the playbin2 docs that explain those properties and signals.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/278.