[Bug 752603] qtdemux: Unable to play streaming MP4 (H264+AAC) file from VLC

[GStreamer (GNOME Bugzilla)]

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

Thibault Saunier <tsaunier at gnome.org> changed:

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

--- Comment #71 from Thibault Saunier <tsaunier at gnome.org> ---
Review of attachment 372659:
 --> (https://bugzilla.gnome.org/review?bug=752603&attachment=372659)

In overall this patch makes a lot of sense to me.

::: gst/isomp4/qtdemux.c
@@ -2369,3 @@
       /* map segment to internal qt segments and push on each stream */
       if (demux->n_streams) {
-        if (demux->fragmented) {

In the new code path this checks become `demux->upstream_format_is_time`
(inverted) - we agreed this is the right thing to do as per Bug #796480.

@@ +3955,3 @@
   gint64 base_offset, running_offset;
   guint32 frag_num;
+  GstClockTime min_decode_time_ns = GST_CLOCK_TIME_NONE;

Just call it `min_dts`?

@@ +4068,3 @@

+    if (stream) {
+      GstClockTime decode_time_ns =

Just `dts` or so - ClockTime already implies NS.

You can also just do min_dts = MIN(min_dts, QTSTREAMTIME_TO_GSTTIME (stream,
decode_time));

@@ +4132,2 @@
     pssh_node = qtdemux_tree_get_sibling_by_type (pssh_node, FOURCC_pssh);
   }

Just move L4070 right after that line removing the `if` I think.

@@ +4159,3 @@
+     * parsed... which is OK -- some apps (mostly tests) expect a segment to
+     * be emitted after a moov, and we can emit a second segment anyway for
+     * special cases like this. */

Agreed.

@@ -4532,3 @@
       gst_buffer_unref (moov);
       qtdemux->got_moov = TRUE;
-      if (!qtdemux->fragmented && !qtdemux->upstream_format_is_time) {

What makes it possible to remove that block? (Sorry, it is not clear to me).

@@ -6017,3 @@
   GList *iter;

-  gst_qtdemux_push_pending_newsegment (qtdemux);

Why can you remove that like this? It is totally unclear to me

@@ +13753,3 @@
+    GstClockTime duration;
+    /* set duration in the segment info */
+    gst_qtdemux_get_duration (qtdemux, &duration);

You need to check that `duration != 0` here as before I think.

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