^ To enable the URL catcher above: commit 717265ebfbbb4ef477d7a0922f3322cb5ac21600 is the fix to be backported.

> 717265ebfbbb4ef477d7a0922f3322cb5ac21600:
> flvmux: Make sure best_time is initialized

Not sure why you refer to this patch if you are having issue with MOV. Have you tried reverting, to make sure it's a regression ?

I was just unable to compile -good without that patch, that's all :)

So here we go. The first CTTS is always 0, hence CTTS goes negative. This is totally not what the code expects. It also an exploit of spec ambiguity. Thanks for the same, I was looking for one.

The problem we'll have is that I implemented CTTS version 0, as the new version contradict itself. In version 1, CTTS can be negative, but that invalidate a bunch of other paragraph that where not updated as the DTS stored in the file is no longer a DTS. It's the DTS + a certain offset so the first CTTS is 0.

Also, some file (possibly generated from GST) lies about implementing CTTS 1, while actually implementing CTTS 0. That's going to be fun mess to fix. We'll probably need some quirk. If some more sample with B-Frames could be provided. I expect that one implementing CTTS 0 to work.

(In reply to Vivia Nikolaidou from comment #3)
> I was just unable to compile -good without that patch, that's all :)

You'll have to clarify, I really don't follow why you refer to patches against FLV muxer, in a bug about qtdemuxer.

vivia@kleio /u/l/s/g/h/gst-plugins-good ((37e3ca1...))> 
git checkout 135e5167304ced786d48f2187b3f0322edf7f0c0
Previous HEAD position was 37e3ca1... rtpbin: Rename some variables and debug output to make more sense
HEAD is now at 135e516... qtdemux: Adjust segment according to ctts offset
vivia@kleio /u/l/s/g/h/gst-plugins-good ((135e516...))> ./autogen.sh 
[...]
Now type 'make' to compile gst-plugins-good.
vivia@kleio /u/l/s/g/h/gst-plugins-good ((135e516...))> make -j10
[...]
gstflvmux.c: In function ‘gst_flv_mux_handle_buffer’:
gstflvmux.c:1543:7: error: ‘best_time’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
       && best_time / GST_MSECOND > G_MAXINT32) {
       ^
  CCLD     libgsticydemux.la
  CC       tvtime/libgstdeinterlace_la-scalerbob.lo
cc1: all warnings being treated as errors
Makefile:678: recipe for target 'libgstflv_la-gstflvmux.lo' failed
make[3]: *** [libgstflv_la-gstflvmux.lo] Error 1
make[3]: Leaving directory '/usr/local/src/gstreamer/head/gst-plugins-good/gst/flv'
Makefile:850: recipe for target 'flv' failed
make[2]: *** [flv] Error 2


That's why I backported that patch. Otherwise I wasn't able to compile -good at all, and therefore I wasn't able to properly bisect.

Ok, here's a video (picked from an Olympus camera) that implements CTTS version 0. Which works as expected (did I mention I implemented CTTS 0 ? ;-P).

https://people.collabora.com/~nicolas/dance.mov

Ok, so you have a custom branch of GStreamer for testing, I'd recommend to keep going forward with top of the tree for the following work on this bug.

So, the version cannot be trusted, also I realize that the spec didn't bump the Atom version even though they have change the semantic by making this field signed.

Created attachment 305500 [details] [review]
ctts_dump: Fix signess issues

It didn't bug, but use correct signess in traces. The number of
entries is unsigned while the offset can be signed according to
recent spec.

Created attachment 305501 [details] [review]
cslg: Add Composition Shift Least Greatest Atom

This simply add fourcc and dump function for the cslg Atom.

These two patches are just utilities that can help implementing negative CTTS support.

Attachment 305500 [details] pushed as 8a406c9 - ctts_dump: Fix signess issues
Attachment 305501 [details] pushed as d6e1e74 - cslg: Add Composition Shift Least Greatest Atom

Created attachment 305629 [details] [review]
qtdemux: Add cslg support

The cslg atom provide information about the DTS shift. This is
needed in recent version of ctts atom where the offset can be
negative.

Thiago, please have a look, but this fixes both videos for me. I'm missing a patch to comment about reading signed CTTS as a workaround to some muxers not complying to ISO.

I just notice, but the CTTS offset was already read as signed since it was introduce in 2009 by Robert Swain.

Review of attachment 305629 [details] [review]:

This is very close to a solution but there are still some things to solve.

::: gst/isomp4/qtdemux.c
@@ +7185,3 @@
+
+    stream->cslg_shift = ABS (least_soffset);
+  }

Also, keep in mind that parse_samples will be called incrementally, parsing only the first N samples so the observation of least_offset might not be correct at the beginning, this means that the initial segment.start could be wrong and we could end up with some desync if we find a smaller cslg_shift at some point.

Also, we should reset this value when a new fragment starts as it is only valid for the movie it is present and not for subsequent ones in a fragmented stream.

(In reply to Thiago Sousa Santos from comment #17)
> Review of attachment 305629 [details] [review] [review]:
> 
> This is very close to a solution but there are still some things to solve.
> 
> ::: gst/isomp4/qtdemux.c
> @@ +7185,3 @@
> +
> +    stream->cslg_shift = ABS (least_soffset);
> +  }
> 
> Also, keep in mind that parse_samples will be called incrementally, parsing
> only the first N samples so the observation of least_offset might not be
> correct at the beginning, this means that the initial segment.start could be
> wrong and we could end up with some desync if we find a smaller cslg_shift
> at some point.

We can try and improve the fallback, what do you propose ? Do we parse incrementally to distribute the load, or will the data be found later in the stream ?

> 
> Also, we should reset this value when a new fragment starts as it is only
> valid for the movie it is present and not for subsequent ones in a
> fragmented stream.

This is not a new bug, the CTTS has never been reset on streams since day one of CTTS support.

Created attachment 305723 [details] [review]
qtdemux: Add cslg support

The cslg atom provide information about the DTS shift. This is
needed in recent version of ctts atom where the offset can be
negative. When cslg is missing, we parse the CTTS table as proposed
in the spec to calculate these values.

In this implementation, we only need to know the shift. As GStreamer
cannot transport negative timestamps, we shift the timestamps forward
using that value and adapt the segment to compensate. This patch also
removes bogus offset of ctts_soffset, this offset shall be included
in the edit list.

Created attachment 305724 [details] [review]
qtmux: Set edit list to compensate DTS shift

We shift DTS forward to avoid negative timestamps which cannot be
represented with version 0 of the CTTS table. To stick with that
version (backward compatibility), the spec recommend using an
edit list entry to move back the presentation time to where it
should be.

Created attachment 305725 [details] [review]
qtmux: Use PTS to figure-out presence of gaps

We need to look at the presentation timestamp in order to conclude if
there is a gap at the start of a stream.

Created attachment 305726 [details] [review]
qtmux: Test gaps at start of stream

Oops, the other patches should be in another bug, sorry.

Attachment 305723 [details] pushed as dd72267 - qtdemux: Add cslg support