After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775487 - player: Move to playbin3 and GstStreams API
player: Move to playbin3 and GstStreams API
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-12-01 19:01 UTC by Sebastian Dröge (slomo)
Modified: 2017-03-22 13:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
player: New API to use playbin3 (6.38 KB, patch)
2017-03-19 09:00 UTC, Seungha Yang
none Details | Review
player: Make use of GstStreams API (12.43 KB, patch)
2017-03-19 09:02 UTC, Seungha Yang
none Details | Review
player: Support playbin3 track selection (4.09 KB, patch)
2017-03-19 09:02 UTC, Seungha Yang
none Details | Review
player: Support get current track API for playbin3 (4.48 KB, patch)
2017-03-19 09:02 UTC, Seungha Yang
accepted-commit_now Details | Review
player: Use stream-notify signal (3.41 KB, patch)
2017-03-19 09:03 UTC, Seungha Yang
none Details | Review
player: Add subtitle language handle for playbin3 (1.46 KB, patch)
2017-03-19 09:03 UTC, Seungha Yang
accepted-commit_now Details | Review
player: Use stream-notify signal (3.42 KB, patch)
2017-03-19 09:16 UTC, Seungha Yang
none Details | Review
player: Make use of GstStreams API with playbin3 (13.54 KB, patch)
2017-03-22 11:16 UTC, Seungha Yang
needs-work Details | Review
player: Support playbin3 track selection (4.14 KB, patch)
2017-03-22 11:17 UTC, Seungha Yang
accepted-commit_now Details | Review
player: Use stream-notify signal (3.38 KB, patch)
2017-03-22 11:18 UTC, Seungha Yang
accepted-commit_now Details | Review
player: Make use of GstStreams API with playbin3 (22.60 KB, patch)
2017-03-22 12:53 UTC, Seungha Yang
committed Details | Review

Description Sebastian Dröge (slomo) 2016-12-01 19:01:52 UTC
See summary
Comment 1 Seungha Yang 2017-03-18 10:59:40 UTC
Hello slomo

I'm deeply interested in gstplayer with playbin3. Are there any plan for this? Can I contribute something on this? :)
Comment 2 Sebastian Dröge (slomo) 2017-03-18 11:20:52 UTC
No plans really. The idea would be to use playbin3 in there instead of playbin, and map all the API to the new playbin3 API (stream info via GstStreamCollection/GstStream, stream selection, etc).
Ideally without any changes in the public API, which should be possible. And ideally configurable for the time being which version of playbin to use.


Go ahead if you want to take this on :)
Comment 3 Seungha Yang 2017-03-19 09:00:19 UTC
Created attachment 348249 [details] [review]
player: New API to use playbin3

Add API gst_player_new_playbin3() to explicitly enable playbin3
Comment 4 Seungha Yang 2017-03-19 09:02:04 UTC
Created attachment 348250 [details] [review]
player: Make use of GstStreams API

Handle stream-collection/streams-selected message for playbin3
Comment 5 Seungha Yang 2017-03-19 09:02:32 UTC
Created attachment 348251 [details] [review]
player: Support playbin3 track selection

select-streams event should be used instead of
current-{audio,video,text} signals. Playbin3 does not support the signals.
Comment 6 Seungha Yang 2017-03-19 09:02:55 UTC
Created attachment 348252 [details] [review]
player: Support get current track API for playbin3
Comment 7 Seungha Yang 2017-03-19 09:03:20 UTC
Created attachment 348253 [details] [review]
player: Use stream-notify signal

Instead of {video,audio,text}-tags-changed signal in case of playbin3
Comment 8 Seungha Yang 2017-03-19 09:03:46 UTC
Created attachment 348254 [details] [review]
player: Add subtitle language handle for playbin3

playbin3 has no "current-text" property
Comment 9 Seungha Yang 2017-03-19 09:16:49 UTC
Created attachment 348255 [details] [review]
player: Use stream-notify signal
Comment 10 Seungha Yang 2017-03-19 09:18:12 UTC
(In reply to Sebastian Dröge (slomo) from comment #2)
> No plans really. The idea would be to use playbin3 in there instead of
> playbin, and map all the API to the new playbin3 API (stream info via
> GstStreamCollection/GstStream, stream selection, etc).
> Ideally without any changes in the public API, which should be possible. And
> ideally configurable for the time being which version of playbin to use.
> 
> 
> Go ahead if you want to take this on :)

Please review the patch set. For easy review, I split them into several parts.

- Enable playbin3
I could not find the way to enable playbin3 without new API (except for env setting). So gst_player_new_playbin3() was added.

- Integrate GstStreamCollection/GstStream with GstPlayer{Media,Stream}Info
We might integrate them deeply (i.e., replace some member variables of them with
GstStream object itself), but I didn't since exposing them to application seems
to be unsafe for me. So, I just make functions for mapping them using stream-id.
Comment 11 Sebastian Dröge (slomo) 2017-03-20 09:59:06 UTC
Review of attachment 348249 [details] [review]:

I was more thinking of something in the GstPlayer config, i.e. gst_player_set/get_config(). Or is that too tricky because the pipeline is already created at initialization and deeply entangled with the thread?

::: gst-libs/gst/player/gstplayer.c
@@ +2770,3 @@
+    GstPlayerSignalDispatcher * signal_dispatcher)
+{
+  static GOnce once = G_ONCE_INIT;

You have to share the GOnce with the other one, otherwise you'll call it twice :)
Comment 12 Sebastian Dröge (slomo) 2017-03-20 09:59:42 UTC
Comment on attachment 348249 [details] [review]
player: New API to use playbin3

If that is not possible with the config easily, I would prefer an environment variable btw.
Comment 13 Sebastian Dröge (slomo) 2017-03-20 10:08:30 UTC
Review of attachment 348250 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +1890,3 @@
+  gst_message_parse_streams_selected (msg, &collection);
+
+  if (collection) {

if (!collection)
  return;

Keep indentation small :) Also elsewhere the same

@@ +1908,3 @@
+      GstStreamType stream_type;
+      const gchar *stream_id;
+      stream = gst_message_streams_selected_get_stream (msg, i);

Transfer full according to docs, so need to unref the stream.

Also generally, you probably want to connect to the signals of all the streams to know about tags/caps/etc changes

@@ +1912,3 @@
+      stream_id = gst_stream_get_stream_id (stream);
+      if (stream_type & GST_STREAM_TYPE_AUDIO)
+        self->audio_sid = g_strdup (stream_id);

If multiple audios are selected, you would assign them all here and leak memory
Comment 14 Sebastian Dröge (slomo) 2017-03-20 10:10:04 UTC
Review of attachment 348251 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +3717,3 @@
+    ret = gst_element_send_event (self->playbin,
+        gst_event_new_select_streams (stream_list));
+    g_list_free (stream_list);

In theory, after the unlock() the audio_sid/etc could've been changed from another thread and freed... so you need to make a copy of it for the list here
Comment 15 Sebastian Dröge (slomo) 2017-03-20 10:11:02 UTC
Review of attachment 348252 [details] [review]:

Looks good
Comment 16 Sebastian Dröge (slomo) 2017-03-20 10:12:11 UTC
Review of attachment 348254 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +2082,3 @@
+        if (self->use_playbin3) {
+          if (g_str_equal (self->subtitle_sid, stream_info->stream_id))
+            info->language = g_path_get_basename (suburi);

Hm why do we set the language to the basename of the URI? It should be set to whatever language tag is produced by that. Weird!

But you just keep behaviour here so that's fine.
Comment 17 Sebastian Dröge (slomo) 2017-03-20 10:14:12 UTC
Review of attachment 348255 [details] [review]:

Looks good too
Comment 18 Seungha Yang 2017-03-20 10:38:45 UTC
(In reply to Sebastian Dröge (slomo) from comment #11)
> Review of attachment 348249 [details] [review] [review]:
> 
> I was more thinking of something in the GstPlayer config, i.e.
> gst_player_set/get_config(). Or is that too tricky because the pipeline is
> already created at initialization and deeply entangled with the thread?

Yes, it's not clear as current flow. As you know, GstPlayer object creates playbin in its main thread at the beginning.

Firstly, I also tried to find a way to use config structure because I also thought that gst_player_new_playbin3() seems not cool... 
But allowing playbin3 only by env variable also seems too unkind API in my opinion.
What about an API like gst_player_new_with_config(.., GstStructure * config) ??

> ::: gst-libs/gst/player/gstplayer.c
> @@ +2770,3 @@
> +    GstPlayerSignalDispatcher * signal_dispatcher)
> +{
> +  static GOnce once = G_ONCE_INIT;
> 
> You have to share the GOnce with the other one, otherwise you'll call it
> twice :)
Oh, I missed it.
Comment 19 Sebastian Dröge (slomo) 2017-03-20 11:54:22 UTC
(In reply to Seungha Yang from comment #18)

> But allowing playbin3 only by env variable also seems too unkind API in my
> opinion.

It's only until playbin3 is considered stable, then only playbin3 will be possible. This is all just transient API :)
Comment 20 Seungha Yang 2017-03-20 13:15:19 UTC
(In reply to Sebastian Dröge (slomo) from comment #13)
> Review of attachment 348250 [details] [review] [review]:
> 
> ::: gst-libs/gst/player/gstplayer.c
> @@ +1890,3 @@
> +  gst_message_parse_streams_selected (msg, &collection);
> +
> +  if (collection) {
> 
> if (!collection)
>   return;
> 
> Keep indentation small :) Also elsewhere the same

Got it :)

> 
> @@ +1908,3 @@
> +      GstStreamType stream_type;
> +      const gchar *stream_id;
> +      stream = gst_message_streams_selected_get_stream (msg, i);
> 
> Transfer full according to docs, so need to unref the stream.
> 
> Also generally, you probably want to connect to the signals of all the
> streams to know about tags/caps/etc changes

If my understanding is correct, I attached stream-notify signal to collection not for each stream. So we can know the change of every stream now by attachment 348255 [details] [review]

> @@ +1912,3 @@
> +      stream_id = gst_stream_get_stream_id (stream);
> +      if (stream_type & GST_STREAM_TYPE_AUDIO)
> +        self->audio_sid = g_strdup (stream_id);
> 
> If multiple audios are selected, you would assign them all here and leak
> memory

Humm... As far as I know (although I've never use inputselector with playbin3), multiple audios in streams-selected msg is only possible by using custom combiner like inputselector. In that case, I guess decodebin3 will add all audio tracks' stream-ids in selected-streams message. But, it's weird behaviour, because player never know activated one by using selected-streams message.

About that, we should do 2 more works now in my opinion.
- Support custom-combiner in GstPlayer (use of playbin/playbin3's "{video,audio,text}-stream-combiner" properties).
- Filter non-activated stream out from selected-streams message (posted by decodebin3) at playbin3, in order for selected-streams to have only one stream-id per stream-type.
Comment 21 Sebastian Dröge (slomo) 2017-03-21 11:38:56 UTC
(In reply to Seungha Yang from comment #20)

> > @@ +1908,3 @@
> > +      GstStreamType stream_type;
> > +      const gchar *stream_id;
> > +      stream = gst_message_streams_selected_get_stream (msg, i);
> > 
> > Transfer full according to docs, so need to unref the stream.
> > 
> > Also generally, you probably want to connect to the signals of all the
> > streams to know about tags/caps/etc changes
> 
> If my understanding is correct, I attached stream-notify signal to
> collection not for each stream. So we can know the change of every stream
> now by attachment 348255 [details] [review] [review]

Ack

> > @@ +1912,3 @@
> > +      stream_id = gst_stream_get_stream_id (stream);
> > +      if (stream_type & GST_STREAM_TYPE_AUDIO)
> > +        self->audio_sid = g_strdup (stream_id);
> > 
> > If multiple audios are selected, you would assign them all here and leak
> > memory
> 
> Humm... As far as I know (although I've never use inputselector with
> playbin3), multiple audios in streams-selected msg is only possible by using
> custom combiner like inputselector. In that case, I guess decodebin3 will
> add all audio tracks' stream-ids in selected-streams message. But, it's
> weird behaviour, because player never know activated one by using
> selected-streams message.

This might be done automatically be playbin3 in the future. Consider a stream where you have two audio streams that should be played together for example, which some containers can specify but we don't support currently.

It would be good to at least catch this case here and do something meaningful instead of having an unhappy surprise later when playbin3 is extended :)

> About that, we should do 2 more works now in my opinion.
> - Support custom-combiner in GstPlayer (use of playbin/playbin3's
> "{video,audio,text}-stream-combiner" properties).
> - Filter non-activated stream out from selected-streams message (posted by
> decodebin3) at playbin3, in order for selected-streams to have only one
> stream-id per stream-type.

For later, just handle it in a meaningful way in GstPlayer for now without too much complications.



For the configuration of using playbin3, please go with an environment variable for now
Comment 22 Seungha Yang 2017-03-22 11:16:18 UTC
Created attachment 348468 [details] [review]
player: Make use of GstStreams API with playbin3

Detect the environment variable "USE_PLAYBIN3" to use playbin3
specific APIs (e.g., handle stream-collection/streams-selected/etc)
Comment 23 Seungha Yang 2017-03-22 11:17:37 UTC
Created attachment 348469 [details] [review]
player: Support playbin3 track selection
Comment 24 Seungha Yang 2017-03-22 11:18:19 UTC
Created attachment 348470 [details] [review]
player: Use stream-notify signal
Comment 25 Seungha Yang 2017-03-22 11:21:54 UTC
(In reply to Sebastian Dröge (slomo) from comment #21)
> (In reply to Seungha Yang from comment #20)
> For later, just handle it in a meaningful way in GstPlayer for now without
> too much complications.
OK, I just make player print debug message. Currently we are not sure how to handle multi-selected track.

> For the configuration of using playbin3, please go with an environment
> variable for now
It more looks good to me also  :)
Comment 26 Sebastian Dröge (slomo) 2017-03-22 11:42:59 UTC
Review of attachment 348468 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +2781,3 @@
+  env = g_getenv ("USE_PLAYBIN3");
+  if (env && g_str_has_prefix (env, "1"))
+    self->use_playbin3 = TRUE;

Call it GST_PLAYER_USE_PLAYBIN3, otherwise good :)
Comment 27 Sebastian Dröge (slomo) 2017-03-22 11:44:33 UTC
Thanks, looks all good now. Can you merge them all into a single patch after renaming the environment variable? I'll merge it into GIT then
Comment 28 Seungha Yang 2017-03-22 12:05:07 UTC
(In reply to Sebastian Dröge (slomo) from comment #26)
> Review of attachment 348468 [details] [review] [review]:
> 
> ::: gst-libs/gst/player/gstplayer.c
> @@ +2781,3 @@
> +  env = g_getenv ("USE_PLAYBIN3");
> +  if (env && g_str_has_prefix (env, "1"))
> +    self->use_playbin3 = TRUE;
> 
> Call it GST_PLAYER_USE_PLAYBIN3, otherwise good :)

Humm... like this?
#define GST_PLAYER_USE_PLAYBIN3 \
  (g_getenv ("USE_PLAYBIN3") && g_str_has_prefix (g_getenv ("USE_PLAYBIN3"), "1")
...
self->use_playbin3 = GST_PLAYER_USE_PLAYBIN3;
Comment 29 Seungha Yang 2017-03-22 12:25:50 UTC
Or does this look better?

#define GST_PLAYER_USE_PLAYBIN3(self) G_STMT_START {  \
  const gchar *__env = g_getenv ("USE_PLAYBIN3");     \
  if (__env && g_str_has_prefix (__env, "1"))         \
    self->use_playbin3 = TRUE;                        \
} G_STMT_END

...

GST_PLAYER_USE_PLAYBIN3 (self);
Comment 30 Sebastian Dröge (slomo) 2017-03-22 12:33:46 UTC
No. g_getenv("GST_PLAYER_USE_PLAYBIN3") :)
Comment 31 Seungha Yang 2017-03-22 12:53:16 UTC
Created attachment 348480 [details] [review]
player: Make use of GstStreams API with playbin3

Allow use of playbin3 and GstStreams API by setting
the environment variable "GST_PLAYER_USE_PLAYBIN3"
Comment 32 Seungha Yang 2017-03-22 12:53:43 UTC
update merged version attachment 348480 [details] [review]
Comment 33 Sebastian Dröge (slomo) 2017-03-22 13:18:39 UTC
Attachment 348480 [details] pushed as e25da85 - player: Make use of GstStreams API with playbin3