I wonder if there's already a bug for this, but maybe it only came up on irc

Problem is both in avidemux and baseparse.

The former messes up timestamping for VBR audio in push mode (essentially assuming each chunk to consist of 1 frame).  The latter fails in PTS interpolating/mirroring of DTS (since no PTS is provided by avidemux in push mode).

Fixed by following commits:

commit 314400d45af1a9718a7209a9c162c7a7ee9d95f7
Author: Mark Nauwelaerts <mnauw@users.sourceforge.net>
Date:   Sat Feb 9 12:32:02 2013 +0100

    baseparse: improve PTS interpolating
    
    ... and tracking of DTS.  Fixes cases where PTS is locked on to the
    DTS of an incoming buffer with no PTS with invalid data, leading to
    no outgoing PTS (since it is not allowed smaller than DTS).
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=691481

commit f0645b79c58a162619f2fc2d030971cce4acd514
Author: Mark Nauwelaerts <mnauw@users.sourceforge.net>
Date:   Fri Feb 8 21:28:02 2013 +0100

    avidemux: proper position reporting and push mode timestamping
    
    ... and align current_total semantics in push and pull mode,
    which tracks bytes for CBR and blocks for VBR.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=691481

Might the baseparse commit also help with bug #690152 (which is mis-titled IIRC) ?

Tim: the patch I posted in #690152 does fix the bug, it just needed some review (although it was pushed in master already).

This patch
http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=314400d45af1a9718a7209a9c162c7a7ee9d95f7
breaks audio timestamps. I have file (MKV with DTS audio) for which it generates non monotonous timestamps

Created attachment 236160 [details] [review]
Patch

This code
if (next_pts >= parse->priv->next_dts)
        parse->priv->next_pts = next_pts;
doesn't really set priv->next_pts to anything because few lines above 
priv->next_pts is set to GST_CLOCK_TIME_NONE

Any chance you could make that file available for us (for future testing/reference)?

Erm, yes, a few lines above priv->next_pts is set to _NONE, but AFAICS that particular one is not involved in the comparison mentioned (only next_pts and priv->next_dts), so I am not quite clear on the "because" part and the relation there ...

You're correct. I was staring at the code for too long. 
This is probably not the right fix. I'm not sure how to make test case for this, as it happens during seeking in a large MKV file. The bug causes the DCA parser to output non monotonous timestamps, sometimes with PTS < DTS.

The problem here seems to be that priv->next_dts is set to segment->start on seek, but PTS of first buffer is before segment->start.

so priv->next_pts is set from first buffer (after seek), priv->next_dts is set from segment start, which results in priv->next_pts < priv->next_pts.

That doesn't seem right. How is this supposed to work? Why is next_dts set to segment start?

It is set to segment start so we can start guestimating timestamps from that point in case buffers have no ts (usually used in the 0 case).

In any case, if incoming buffers have ts, then chain (via adapter) should be picking up those ...

The issue in my case seems to be that on seek, the DTS start from segment->start, while PTS start from segment->start - n (first buffer PTS).  The buffers only have PTS set.

If the incoming buffer has PTS (but not DTS), shouldn't the interpolated DTS start from that PTS (instead of segment->start, which is completely arbitrary, as the segment->start time doesn't have to be same as first  buffer?)

Created attachment 236248 [details] [review]
Patch

This patch is not really meant to be committed, it just demonstrates what is going wrong in my case and fixes it.

After seek, I get incoming buffers with PTS < segment->start and no DTS. In case like this, baseparse starts interpolating dts from segment->start, which means that the interpolated dts > buffer PTS.

Created attachment 236468 [details] [review]
baseparse: more inter-timestamp tracking

Given that receiving a segment event sets priv->prev_dts/_pts to _NONE, this patch should have the PTS picked up from the DTS of incoming buffers if no valid PTS has been seen on the latter.

If I read things right, this should fix the case described and is along the lines of what is suggested in Comment #12.

It could also simply be said here that matroskademux is wrong or incomplete in that it apparently still simply/only sets _TIMESTAMP rather than anything _DTS/_PTS, and the same might be so for quite some demuxers / baseparse upstream elements out there.

As such, the more general problem in baseparse is what to do with 2 sort-of-independent timelines (PTS/DTS) which may or may not be provided in all sorts of (in)valid combinations on the input.

This patch turns interpolate_pts almost into a "equalize_ts" option, and it might make sense to have such baseparse option (and use that in the 'if' of this patch).  That would probably (?) have all audio cases deal with (legacy) upstream input (equalizing pts / dts, ok for audio typically). OTOH, that would probably not go well for the video cases, which would have to run without that option enabled.  No idea though how to handle all sorts of incomplete input in that case then ...  Best there is to make sure that demuxers always at least set _DTS in stead of _TIMESTAMP, since that is likely the known ts as they provide data in decoding order.

I don't understand the very first paragraph; There is NO DTS on incoming buffers in my case, only PTS. The PTS doesn't need to be picked, we already get it from demuxer.

It's been a while since I was reading matroska specs, but IIRC matroska doesn't store DTS, only PTS. At very least for video streams this is true, as the timestamps from matroska are not monotonous (so it's easy to confirm).

I don't really understand the point of interpolating the DTS, especially from segment start. Segment start does not guarantee that the incoming buffers are within the segment boundaries. Also, if decoder really relies on having proper DTS (i.e. for buffer under/overflow), the interpolated DTSs probably won't be very helpful.

Just to recap, the problem in my case:

* Incoming buffers only have PTS (no DTS), matroska doesn't store DTS
* PTS gets interpolated from the PTS value of first buffer
* DTS gets interpolated from value of segment->start
* (segment->start) > (PTS of first buffer) because the seek goes 
   to last keyframe before segment start, thus on output, I get buffers
   with DTS > PTS, which breaks number of things

Actually, the end of my previous comment was not exactly correct, what I really get is audio buffers with non monotonous timestamps, because the baseparse sometimes sets output buffer PTS to calculated DTS .

Btw, your patch doesn't really work for me, because for some reason parse->priv->prev_dts is always 0 (it's not set to GST_CLOCK_TIME_NONE), so the condition !GST_CLOCK_TIME_IS_VALID (parse->priv->prev_dts) is never true.
Without the condition it works.

Created attachment 236501 [details] [review]
baseparse: more inter-timestamp tracking

Hm, yes, first paragraph has PTS and DTS switched.

Updated version with a few tweaks (such as minding to set _NONE).

It may well be that matroska specs refer to _PTS, but the rest of the explanation comes down to saying that baseparse will stay in trouble with ts handling (on 2 timelines) unless we e.g. accept that PTS = DTS for audio and handle it that way.  That does not cover the other cases, but not sure how well those can be covered with limited baseparse semantic awareness in that regard.

Created attachment 236502 [details] [review]
baseparse: more inter-timestamp tracking

Erm, now really the updated version ...

Thanks Mark, the new patch seems to be working. I'm perfectly fine with assuming PTS = DTS for audio, I didn't realize you only meant that for audio.

Created attachment 237392 [details] [review]
baseparse: more inter-timestamp tracking

To try and wrap this up, attached patch is a slightly evolved version of the previous one, which makes the behaviour there (and in related patch before that) not dependent on the pts_interpolate setting, which seems not suited.

Rather, another setting, infer_ts is added (on by default), which allows baseparse to estimate PTS/DTS from each other, which is effectively being done here (and before as well).

Comments (such as on naming) welcome, and if no objections will be committing this patch to resolve this ...

Is this the right patch? It seems like the old one.

Created attachment 237467 [details] [review]
baseparse: more inter-timestamp tracking

Indeed, sigh, this time the intended one ...

Seems to be working fine for me.

The commited patch breaks backwards seek in mp3 files:

dts: 0:00:22.987754240, pts: 0:00:22.987754240, duration: 0:00:00.026122448
dts: 0:00:23.013876688, pts: 0:00:23.013876688, duration: 0:00:00.026122448                                                                                                                    
dts: 0:00:23.039999136, pts: 0:00:23.039999136, duration: 0:00:00.026122448
dts: 0:00:23.066121584, pts: 0:00:23.066121584, duration: 0:00:00.026122448

SEEK TO 0

dts: 0:00:00.000000000, pts: 0:00:23.118366480, duration: 0:00:00.026122448
dts: 0:00:00.026122448, pts: 0:00:23.144488928, duration: 0:00:00.026122448
dts: 0:00:00.052244896, pts: 0:00:23.170611376, duration: 0:00:00.026122448
dts: 0:00:00.078367344, pts: 0:00:23.196733824, duration: 0:00:00.026122448

Mark's patch (baseparse: more inter-timestamp tracking) does not help for mp3 btw

Indeed, that problem seems to have been elsewhere.  Following commit should take care of it:

commit 6ddbaaa95cf47ca353ae6d43feb2f75627d5849b
Author: Mark Nauwelaerts <mnauw@users.sourceforge.net>
Date:   Wed Mar 27 18:25:08 2013 +0100

    baseparse: reset next_pts upon SEGMENT event
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=691481

Since no objections so far, also:

commit 76acdee9087c4b569cc500057d22ab9a44892bea
Author: Mark Nauwelaerts <mnauw@users.sourceforge.net>
Date:   Tue Feb 26 19:58:49 2013 +0100

    baseparse: more inter-timestamp tracking
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=691481

This breaks A/V sync and smooth video playback in some files. A/V sync works again when not calling set_frame_rate() in the video parsers, smooth video playback is broken in any case.

commit 76acdee9087c4b569cc500057d22ab9a44892bea
Author: Mark Nauwelaerts <mnauw@users.sourceforge.net>
Date:   Tue Feb 26 19:58:49 2013 +0100

    baseparse: more inter-timestamp tracking

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

Note: it's the combination of that change and using the framerate to guess buffer durations that makes things fail

commit f27a3e12f6d35695ebbf2edc5aa4937c2e7c3843
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Tue Apr 23 11:47:54 2013 +0200

    baseparse: Only infer TS if PTS interpolation is enabled
    
    Otherwise this is breaking timestamps of formats that
    need reordering.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=597662


Also related to this:
commit b7d63d3fb1f71d8ebf822df50c516dda5967af7b
Author: Jan Schmidt <thaytan@noraisin.net>
Date:   Wed Sep 12 22:58:04 2012 -0700

    videoparsers: Disable PTS interpolation in the base parse class
    
    All these formats have re-ordered PTS which the base class gets
    wrong. It's better to leave them blank and let the decoder sort it
    out. Better yet would be to track and interpolate the timestamps
    in the subclasses (FIXME)