[Bug 793058] qtdemux: add support for edit lists for fragmented files in push mode

[GStreamer (GNOME Bugzilla)]

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

--- Comment #13 from Thiago Sousa Santos <thiagossantos at gmail.com> ---
(In reply to Alicia Boya García from comment #12)
> Review of attachment 369399 [details] [review]:
> 
> r-
> 
> I don't understand how this is supposed to work. I have added comments in
> the points that look the most suspicious to me.
> 
> ::: gst/isomp4/qtdemux.c
> @@ +129,3 @@
>  #define QTSAMPLE_DTS(stream,sample) (QTSTREAMTIME_TO_GSTTIME((stream),
> (sample)->timestamp))
>  /* timestamp + offset + cslg_shift is the outgoing PTS */
> +#define QTSAMPLE_PTS(stream,sample) (QTSTREAMTIME_TO_GSTTIME((stream),
> (sample)->timestamp + (stream)->cslg_shift + (stream)->elst_shift +
> (sample)->pts_offset))
> 
> QTSAMPLE_PTS() is used to set buffer PTS. By adding elst_shift you are
> introducing an inconsistency into qtdemux:
> 
> So far buffer PTS in qtdemux is always track presentation time. You need to
> calculate stream time to get movie presentation time, where edits are
> applied.
> 
> With this change, you would change that definition for push mode streams
> with basic edit lists. It would become track presentation time + media_time
> of the edit. It's not even movie presentation time, you would need to
> subtract elst_shift for that instead*.

AFAIK this code is for both push and pull mode to get correct PTS and DTS
alignment. Initial PTS should be 0 if DTS could be negative but it can't so we
shift PTS forward (together with segment.start) and keep initial DTS as 0 to
achieve the same effect.

elst_shift is only non-zero on the scenarios outlined on the spec:

"""
A non‐empty edit may insert a portion of the media timeline that is not present
in the initial movie, and
is present only in subsequent movie fragments. Particularly in an empty initial
movie of a fragmented
movie file (when there are no media samples yet present), the segment_duration
of this edit may be
zero, whereupon the edit provides the offset from media composition time to
movie presentation time,
for the movie and subsequent movie fragments. It is recommended that such an
edit be used to
establish a presentation time of 0 for the first presented sample, when
composition offsets are used.
"""

Do you have a sample file that meets this criteria but fails? I'd be happy to
look at it. The missing check is if the first segment is empty but that could
be added, maybe.

> 
> *and that would be problematic because an edit list can cause negative movie
> decode and presentation time... which cannot be handled in buffer time
> because GstBuffer.{pts,dts} are unsigned.
> 
> @@ +352,3 @@
> +  /* elst */
> +  guint64 elst_shift;
> +
> 
> The comment does not explain what that field is about. If I've understood
> correctly the code it would be something like this.
> 
> /* In push mode, when there is an edit list with a single, non-empty edit
> with duration=0, this field stores the media_start of that edit in trak
> timescale. In other cases, this field is zero. */
> 
> @@ +4793,3 @@
>    stream->segment.rate = rate;
>    stream->segment.start = start + QTSTREAMTIME_TO_GSTTIME (stream,
> +      stream->cslg_shift + stream->elst_shift);
> 
> Why do you shift buffer time above if you then shift the segment.start in
> the same direction? Stream time will reverse the elst_shift that you added,
> returning (unedited) track presentation time.

Because the goal here is not to offset the start time (and have emptiness on
the beginning) but to have PTS be shifted forwarded because we can't shift DTS
backwards because it is unsigned.


Maybe I should have left better comments around the whole elst_shift scenario.
I can improve the patch in that regard.

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