GNOME Bugzilla – Bug 770331
player: Implement a playlist API and next/previous commands
Last modified: 2018-11-03 13:54:47 UTC
See attached patch, and also following patch for gtk-play to make use of this. Please review the new API and let me know if this makes any sense or could be simplified somehow.
Created attachment 334074 [details] [review] player: Implement a playlist API and next/previous commands
Created attachment 334075 [details] [review] player: Implement a playlist API and next/previous commands
Created attachment 334076 [details] [review] gtk: Use new GstPlayer playlist API for next/previous
Created attachment 334077 [details] [review] gtk: Use new GstPlayer playlist API for next/previous
Created attachment 334078 [details] [review] player: Implement a playlist API and next/previous commands
Would be nice to have a user_data+destroy_notify in the playlist-queue "simple" implementation. I'm also not sure I understand what the auxilliary URIs are for? Only external subtitles? Anything else? How do you know which is which if you have more than one? Also adding Bastien in CC in case he has an opinion on this?
(In reply to Olivier Crête from comment #6) > Would be nice to have a user_data+destroy_notify in the playlist-queue > "simple" implementation. It already has that, see the arguments of the new() function. I would've preferred to also be able to use signals here btw, but multiple return values (or out parameters) don't work with that. > I'm also not sure I understand what the auxilliary > URIs are for? Only external subtitles? Anything else? How do you know which > is which if you have more than one? That's basically preparation for playbin3 :) Right now we can only have a single suburi, with playbin3 you can have multiple subtitles, external audio tracks, etc. With the streams API you would be able to know what to do with them.
(In reply to Sebastian Dröge (slomo) from comment #7) > (In reply to Olivier Crête from comment #6) > > Would be nice to have a user_data+destroy_notify in the playlist-queue > > "simple" implementation. > > It already has that, see the arguments of the new() function. I would've > preferred to also be able to use signals here btw, but multiple return > values (or out parameters) don't work with that. I meant for gst_player_playlist_queue_push(). I assume many simple apps will just want to use that to keep their playlist and be able to attach a small struct to each time (or maybe just a user-visible string). > > I'm also not sure I understand what the auxilliary > > URIs are for? Only external subtitles? Anything else? How do you know which > > is which if you have more than one? > > That's basically preparation for playbin3 :) Right now we can only have a > single suburi, with playbin3 you can have multiple subtitles, external audio > tracks, etc. With the streams API you would be able to know what to do with > them. How can playbin3 know which URI is what? For example, if you have multiple srt files, you'd need to be able to say which language it is, same for external audio. So this API seems to be missing something.
(In reply to Olivier Crête from comment #8) > (In reply to Sebastian Dröge (slomo) from comment #7) > > (In reply to Olivier Crête from comment #6) > > > Would be nice to have a user_data+destroy_notify in the playlist-queue > > > "simple" implementation. > > > > It already has that, see the arguments of the new() function. I would've > > preferred to also be able to use signals here btw, but multiple return > > values (or out parameters) don't work with that. > > I meant for gst_player_playlist_queue_push(). I assume many simple apps will > just want to use that to keep their playlist and be able to attach a small > struct to each time (or maybe just a user-visible string). Indeed, changing that to have user_data and a destroy_notify for each item. Good idea, thanks!
Created attachment 334116 [details] [review] player: Implement a playlist API and next/previous commands
Created attachment 334117 [details] [review] gtk: Use new GstPlayer playlist API for next/previous
(In reply to Olivier Crête from comment #8) > > > > I'm also not sure I understand what the auxilliary > > > URIs are for? Only external subtitles? Anything else? How do you know which > > > is which if you have more than one? > > > > That's basically preparation for playbin3 :) Right now we can only have a > > single suburi, with playbin3 you can have multiple subtitles, external audio > > tracks, etc. With the streams API you would be able to know what to do with > > them. > > How can playbin3 know which URI is what? For example, if you have multiple > srt files, you'd need to be able to say which language it is, same for > external audio. So this API seems to be missing something. Very good point. We might need (at the playbin3 and urisourcebin level) to have a way to provide extra meta-data for those additional URI (like language, audio mapping, 3D positioning, ...). As for knowing which URI is what (i.e. correlate them in the resulting GstStreams), maybe provide a specific stream-id ?
(In reply to Edward Hervey from comment #12) > Very good point. We might need (at the playbin3 and urisourcebin level) to > have a way to provide extra meta-data for those additional URI (like > language, audio mapping, 3D positioning, ...). And on the GstPlayerPlaylist level we would then need this too. A "struct { char *uri; GstStructure *metadata; }" maybe. Or what were you thinking of at the playbin level? Should discuss that part in a different bug though, specific to playbin, and make it a blocker of this one.
If the get_previous/current/next callbacks don't set auxiliary_uris Bad Things Happen. Suggest this is documented, or aux_uris is set to NULL before calling gst_player_playlist_get_current/next/previous. aux_uris and self->uri could leak if the callback fills in the data, but returns false (which is a pretty weird thing for the callback to do).
(In reply to Jonathan Miles from comment #14) > If the get_previous/current/next callbacks don't set auxiliary_uris Bad > Things Happen. > > Suggest this is documented, or aux_uris is set to NULL before calling > gst_player_playlist_get_current/next/previous. I'll change it so that all arguments are initialized to NULL before calling the callbacks. Thanks! > aux_uris and self->uri could leak if the callback fills in the data, but > returns false (which is a pretty weird thing for the callback to do). And that one, it's indeed weird but weird things happen :)
Created attachment 334926 [details] [review] player: Implement a playlist API and next/previous commands
(In reply to Sebastian Dröge (slomo) from comment #15) > (In reply to Jonathan Miles from comment #14) > > If the get_previous/current/next callbacks don't set auxiliary_uris Bad > > Things Happen. > > > > Suggest this is documented, or aux_uris is set to NULL before calling > > gst_player_playlist_get_current/next/previous. > > I'll change it so that all arguments are initialized to NULL before calling > the callbacks. Thanks! This one is done, they're all set to NULL first. > > aux_uris and self->uri could leak if the callback fills in the data, but > > returns false (which is a pretty weird thing for the callback to do). > > And that one, it's indeed weird but weird things happen :) This one not, the callback is not supposed to set them to anything... and if it still does we can't know if we're supposed to be free those or not.
Created attachment 336106 [details] [review] player: Implement a playlist API and next/previous commands
Created attachment 336107 [details] [review] gtk: Use new GstPlayer playlist API for next/previous
*** Bug 765304 has been marked as a duplicate of this bug. ***
Created attachment 351446 [details] [review] player: Implement a playlist API and next/previous commands
Created attachment 351447 [details] [review] player: Implement a uridecodebin ! concat ! playsink based audio-only backend This implements gapless real gapless playback.
Created attachment 351448 [details] [review] player: only output media-info changed events with the first buffer When switching streams, on a stream-start event, we were creating the media info before the new stream had sent its tag events resulting in the use of the previous stream's tag structure. Change that by delaying the creation of the media info until the first buffer arrives on the srcpad of the input (the same place EOS is detected) and resetting the media info on every stream-start event after concat.
Created attachment 351890 [details] [review] player: Reset inputs before stop
ping?
(In reply to Jan Schmidt from comment #25) > ping? pong! This needs some further design work and deciding where in GStreamer we want to have this :)
Should this bug be merged into Bug #776613?
Possibly. This one actually has patches though :)
I'm not sure if those should be linked. I think providing some kind of minimal playlist API for player API users would be good, independent of everything else. For playlist support in #777613 the first step would be to detect playlists and simply post them for the application to parse (e.g. with totem-plparser).
(In reply to Tim-Philipp Müller from comment #29) > I'm not sure if those should be linked. I think providing some kind of > minimal playlist API for player API users would be good, independent of > everything else. Indeed, playlist handling != playlist parsing. eg. https://git.gnome.org//browse/totem/tree/src/totem-playlist.h != totem-pl-parser The idea here would be for GstPlayer itself to have access to URLs and metadata extracted from playlists. This would include URLs, title, start time, subtitle URL (for external subs), current video/audio/subtitle tracks, etc. and GstPlayer automatically handling repeat and loop modes, and gapless transitions when possible. > For playlist support in #777613 the first step would be to detect playlists > and simply post them for the application to parse (e.g. with totem-plparser). I'd be happy to work on that, though it would really be a role-reversal (we currently pass playlists to GStreamer when totem-pl-parser doesn't handle them, rather than the opposite), but something similar to this would be the way to populate the playlist support requested in this bug.
I must be missing some coffee intake. Repeat and loop are the same thing ;) This is the list of metadata that totem keeps about each entry in the playlist: https://git.gnome.org//browse/totem/tree/src/totem-playlist.c#n104 PLAYING_COL: whether this entry is playing FILENAME_COL: the filename FILENAME_ESCAPED_COL: the filename escaped for display URI_COL: the URI TITLE_CUSTOM_COL: the title either from the playlist, or the source (eg. DVD title and chapter numbers) SUBTITLE_URI_COL: the URI for the external subtitle FILE_MONITOR_COL and MOUNT_COL: remove the item if the file is deleted, or the mount is going away MIME_TYPE_COL: used to populate GtkRecent files
We could say that "repeat" is repeating one track, and "loop" is looping the whole playlist :) So I guess there are two different functions to implement here for sure.
-- 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-bad/issues/419.