Created attachment 341152 [details] [review]
gst-play: Support track change on playbin3

Thanks, this is an interesting addition. There's another question here which is whether gst-play shouldn't rather be moved to the GstPlayer API, but that is unlikely to happen in the near future anyway. And also whether we should add explicit switches to make gst-play use playbin or playbin3.

(In reply to Tim-Philipp Müller from comment #2)
> Thanks, this is an interesting addition. There's another question here which
> is whether gst-play shouldn't rather be moved to the GstPlayer API, but that
> is unlikely to happen in the near future anyway. 
Does GstPlayer API is meaning the one in plugins-bad? Actually, I tried to modify it first before this patch, but it was not simple to optimization for me, because existing API might be changed :) 
(There are lots of redundancy between "GstStreams, GstStreamCollection" and "GstPlayerStreamInfo, GstPlayerMediaInfo")

> add explicit switches to make gst-play use playbin or playbin3.
I will update new patch with addition of commend line option for explicit playbin3 selection :)

Created attachment 341158 [details] [review]
gst-play: Support track change on playbin3

gst-play: Support track change on playbin3

* playbin3 does not support {current,n}-{audio,video,text}
properties, and they were replaced by GstStreams API.
So, GstStreams API and select-stream event should be used
for track change in case of playbin3.
see also https://bugzilla.gnome.org/show_bug.cgi?id=769079

* By using commend line option "--use-playbin3", gst-play will
use playbin3 regardless of "USE_PLAYBIN" env variable.

Review of attachment 341158 [details] [review]:

::: tools/gst-play.c
@@ +76,3 @@
+  gboolean is_playbin3;
+  GstStreamCollection *collection;
+  GstStream *cur_audio;

It would be "safer" to just store the stream-id instead of the stream object.

The reason is that there can be different stream objects (i.e. updated in the meantime) for the same stream id.

@@ +518,3 @@
+        g_mutex_unlock (&play->selection_lock);
+      }
+      /* TODO: "stream-notify might be used " */

I think it's fine to ignore that for the gst-play use-case imho.

@@ +537,3 @@
+
+            if (type & GST_STREAM_TYPE_AUDIO) {
+              gst_object_replace ((GstObject **) & play->cur_audio,

Just store the stream-id instead.

@@ +1097,3 @@
+  guint nb_stream = 0;
+
+  g_mutex_lock (&play->selection_lock);

Instead of this just:

Iterate all streams of the collection:
 * Remember a total number of streams of each type (nb_audio, nb_video,...)
 * Remember the current collection index for the activated audio/video/text (and -1 if not present)

Then you just have to increment the correct stream index (% nb_<type>), and re-iterate the collection to pick the correct stream object/id/tag.

@@ +1146,3 @@
+  }
+
+  if (!play->collection) {

Maybe do this check at the very top instead of creating something that you then discard :)

@@ +1186,3 @@
+      if (cur_flags & flag) {
+        cur_flags &= ~flag;
+        g_object_set (play->playbin, "flags", cur_flags, NULL);

One shouldn't have to fiddle with flags with playbin3. They should end up being automatically configured.

@@ +1342,3 @@
       }
     case 'a':
+      if (play->is_playbin3)

Move the if (play->is_playbin3) check into play_cycle_track_selection() which then redirects to play_cycle_track_selection_playbin3(). Makes the code smaller/cleaner.

(In reply to Edward Hervey from comment #5)
> Review of attachment 341158 [details] [review] [review]:

Hello Edward Hervey
Thanks for your detailed review:)

> @@ +1186,3 @@
> +      if (cur_flags & flag) {
> +        cur_flags &= ~flag;
> +        g_object_set (play->playbin, "flags", cur_flags, NULL);
> 
> One shouldn't have to fiddle with flags with playbin3. They should end up
> being automatically configured.

To disable a track, we have two options,
One is setting playbin3 flags (like playbin) and the other is excluding stream-id from select-streams event 
e.g., to disable audio, select-streams event must have stream-id of only video (and plus text if exists).

The difference is that, "flags setting" causes just reconfiguration of playsink, but "select-streams" causes removal of corresponding decoder also.
Which one is suitable approach?

Anyway, the second option does not work now and causes crash. It seems bug of playbin3/decodebin3, though

(In reply to Seungha Yang from comment #6)
> To disable a track, we have two options,
> One is setting playbin3 flags (like playbin) and the other is excluding
> stream-id from select-streams event 
> e.g., to disable audio, select-streams event must have stream-id of only
> video (and plus text if exists).
> 
> The difference is that, "flags setting" causes just reconfiguration of
> playsink, but "select-streams" causes removal of corresponding decoder also.
> Which one is suitable approach?

  In the new pb3/db3/streams world, and since gst-play should also be an example of good API usage, I would much prefer the second option.
  The reason is for performance (less elements used) but also for simplicity (only one API to use for deciding what you want, and not some playbin specific API).

  Note that it doesn't mean that doing it the first way shouldn't work. Just not for gst-play.

> 
> Anyway, the second option does not work now and causes crash. It seems bug
> of playbin3/decodebin3, though

  Ah, let's open a bug report about this then :)

Created attachment 344691 [details] [review]
gst-play: Support track change on playbin3

- Used "stream-id" to store currently activated track rather than GstStream object
- Integrated play_cycle_track_selection_playbin3() into
  play_cycle_track_selection()

I made a track disabling related bug. See bug #778015

Created attachment 348091 [details] [review]
gst-play: Support track change on playbin3

Fix invalid memory access and invalid GstStream object unref

*** Bug 776894 has been marked as a duplicate of this bug. ***

Thanks ! I also added a check for not sending empty SELECT_STREAMS (could happen if for example you deactivated audio on a audio only stream).

commit a8e05cc9cc66bb55868d5950321d7e7f33312eca
Author: Seungha Yang <sh.yang@lge.com>
Date:   Thu Mar 16 17:52:04 2017 +0900

    gst-play: Support track change on playbin3
    
    * playbin3 does not support {current,n}-{audio,video,text}
    properties, and they were replaced by GstStreams API.
    So, GstStreams API and select-stream event should be used
    for track change in case of playbin3.
    see also https://bugzilla.gnome.org/show_bug.cgi?id=769079
    
    * By using commend line option "--use-playbin3", gst-play will
    use playbin3 regardless of "USE_PLAYBIN" env variable.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=775469