[Bug 775469] gst-play: Support track change on playbin3

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=775469

Edward Hervey <bilboed at bilboed.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #341158|none                        |needs-work
             status|                            |

--- Comment #5 from Edward Hervey <bilboed at bilboed.com> ---
Review of attachment 341158:
 --> (https://bugzilla.gnome.org/review?bug=775469&attachment=341158)

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

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.