[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

--- Comment #74 from Alicia Boya García <aboya at igalia.com> ---
Review of attachment 372659:
 --> (https://bugzilla.gnome.org/review?bug=752603&attachment=372659)

::: gst/isomp4/qtdemux.c
@@ +4068,3 @@

+    if (stream) {
+      GstClockTime decode_time_ns =

The name is for comparison with another variable, `decode_time` which stores
the starting decode time of a fragment in track timescale. This is the same
value, converted to nanoseconds. Therefore I think the name makes sense, but I
have no strong feelings for or against.

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

In the new code `need_segment` is only relevant for push mode and this is a
pull-mode function, so it's not necessary.

Later in this review is explained where this assignment had an effect.

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

-  gst_qtdemux_push_pending_newsegment (qtdemux);

After my patch there is no longer a gst_qtdemux_push_pending_newsegment()
function. There is gst_qtdemux_check_send_pending_segment(), but it's only
intended to be used in push mode. I just have not found any use for it in the
new code... and not a good one in the old code either.

This function (like other _loop variants) is called in pull mode only.

As far as I know, before edit lists in fragmented files were supported in
qtdemux, the intent of this function call was to ensure the default segment was
propagated downstream here instead.

gst_qtdemux_push_pending_newsegment() only did something if `need_segment` was
TRUE. [Of course, that condition is pretty recent, but back then
qtdemux->pending_newsegment would serve the same purpose.]

On non fragmented files `need_segment` would be FALSE because of the old `if
(!qtdemux->fragmented && !qtdemux->upstream_format_is_time)` you saw before and
it would do nothing. 

Currently this call is unnecessary. The way segments work in pull mode is as
follows:

First, note upstream_format_is_time is always FALSE in pull-mode. Since qtdemux
is driving the pipeline and calling getrange() with file offsets it makes no
sense for the element upstream to send a TIME segment.

Therefore, there is no "direct segment propagation" case in pull mode, edit
lists are always mapped to GstSegment's (if the file does not contain an `edts`
atom this is not a problem, as a fallback one is created automatically).

stream->segment_index tracks what edit list segment we are currently in. It's
initialized to -1 (no segment).

stream->time_position tracks what is the current movie time (i.e. time after
applying edit lists) we are on. It's initialized to zero and grows as frames
are demuxed.

When gst_qtdemux_prepare_current_sample() finds segment_index == -1, it calls
gst_qtdemux_find_segment() to find the segment containing stream->time_position
and activates it (updates segment_index and pushes a new GstSegment
downstream).

Later segment_index is resetted to -1 in gst_qtdemux_advance_sample() when the
end of an edit list segment is found.

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

gst_qtdemux_get_duration() never returns 0. If qtdemux->duration == 0 it
returns GST_CLOCK_TIME_NONE.

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