Hello slomo

I'm deeply interested in gstplayer with playbin3. Are there any plan for this? Can I contribute something on this? :)

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 :)

Created attachment 348249 [details] [review]
player: New API to use playbin3

Add API gst_player_new_playbin3() to explicitly enable playbin3

Created attachment 348250 [details] [review]
player: Make use of GstStreams API

Handle stream-collection/streams-selected message for playbin3

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.

Created attachment 348252 [details] [review]
player: Support get current track API for playbin3

Created attachment 348253 [details] [review]
player: Use stream-notify signal

Instead of {video,audio,text}-tags-changed signal in case of playbin3

Created attachment 348254 [details] [review]
player: Add subtitle language handle for playbin3

playbin3 has no "current-text" property

Created attachment 348255 [details] [review]
player: Use stream-notify signal

(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.

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 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.

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

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

Review of attachment 348252 [details] [review]:

Looks good

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.

Review of attachment 348255 [details] [review]:

Looks good too

(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.

(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 :)

(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.

(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

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)

Created attachment 348469 [details] [review]
player: Support playbin3 track selection

Created attachment 348470 [details] [review]
player: Use stream-notify signal

(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  :)

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 :)

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

(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;

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);

No. g_getenv("GST_PLAYER_USE_PLAYBIN3") :)

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"

update merged version attachment 348480 [details] [review]

Attachment 348480 [details] pushed as e25da85 - player: Make use of GstStreams API with playbin3