Created attachment 350966 [details] [review]
qtdemux: Fix seeking on streams with frame reordering

The samples table is sorted by DTS, not PTS. As such we can only get the
correct result when using a binary search on it, if we search for the
DTS.
Also if we only ever search for the frame, where the following frame is
the first one with a PTS after the search position, we will generally
stop searching too early if frames are reordered.

In forwards playback this is not really a problem (after the decoder
reordered the frames, clipping is happening), in reverse playback
it means that we can output one or more frames too few as we stop too
early and the decoder would never receive it.

This assumes that DTS <= PTS. We search for the frame that has as DTS the target PTS (i.e. its PTS is >= the target PTS), and from there on search backwards to get closer to the target PTS.

Ah, so same stop condition you mentioned on IRC, but while going backward.

Review of attachment 350966 [details] [review]:

Looks good to me. I only have nitpick kind of comments.

::: gst/isomp4/qtdemux.c
@@ +1175,3 @@
+  /* sample->timestamp is now <= media_time, need to find the corresponding
+   * PTS now by looking backwards */
+  while (index > 0 && sample->timestamp + sample->pts_offset > mov_time) {

Maybe some parenthesis for the reader ?

@@ +1180,3 @@
   }
+
+

Maybe just one empty line will do ?

@@ +4931,2 @@
     goto eos;
+  }

Maybe you can remove that part, it does not change anything.

Review of attachment 350966 [details] [review]:

::: gst/isomp4/qtdemux.c
@@ +1163,3 @@
+    while (index < str->n_samples - 1) {
+      if (!qtdemux_parse_samples (qtdemux, str, index + 1))
+        goto parse_failed;

Actually, there is a possible corner case. If the position is passed that last DTS, which happens when you seek to the last sample, it will reach parse_failed. While it should pick the last sample as if it was searching backward.

DTS: 0 1 2
PTS: 1 3 2

The algo works for seeking at 1 and 2, but not 3.

Indeed, thanks. Will update :)

Created attachment 369325 [details] [review]
qtdemux: Fix seeking on streams with frame reordering

The samples table is sorted by DTS, not PTS. As such we can only get the
correct result when using a binary search on it, if we search for the
DTS.
Also if we only ever search for the frame, where the following frame is
the first one with a PTS after the search position, we will generally
stop searching too early if frames are reordered.

In forwards playback this is not really a problem (after the decoder
reordered the frames, clipping is happening), in reverse playback
it means that we can output one or more frames too few as we stop too
early and the decoder would never receive it.

(In reply to Nicolas Dufresne (stormer) from comment #5)
> Review of attachment 350966 [details] [review] [review]:
> 
> ::: gst/isomp4/qtdemux.c
> @@ +1163,3 @@
> +    while (index < str->n_samples - 1) {
> +      if (!qtdemux_parse_samples (qtdemux, str, index + 1))
> +        goto parse_failed;
> 
> Actually, there is a possible corner case. If the position is passed that
> last DTS, which happens when you seek to the last sample, it will reach
> parse_failed. While it should pick the last sample as if it was searching
> backward.
> 
> DTS: 0 1 2
> PTS: 1 3 2
> 
> The algo works for seeking at 1 and 2, but not 3.

This is actually wrong. If we seek to 3, what will happen is that all samples up to n_samples (3) - 2 + 1 == 2 are parsed (i.e. we parsed 0, 1, 2) and then the while loop finishes and we start looking backwards from the last sample (2).

This loop would only ever run into parse_failed if the parsing itself of the sample fails. It will never ever try to call qtdemux_parse_samples() with index==n_samples.


There was a different minor bug though, which is fixed now. And your other review comments too. I'll merge this right after 1.14 unless someone finds another problem :)

Attachment 369325 [details] pushed as 850e678 - qtdemux: Fix seeking on streams with frame reordering