OGG Vorbis timestamp generation broken

[Ulrik Mikaelsson]

Summary of the bug: When demuxing Ogg/Vorbis streams, the packet-duration and PTS is incorrectly set.

How to reproduce:

1. Create ogg/vorbis asset (have repro:ed using multiple different audio-sources). Here we are using am audio-extraction from Tears of Steel. (Attached to ticket)

   % ffmpeg -i ToS-4k-1920.mov -map a tos.ogg
   OR
   % oggenc tos.flac
   

2. First sign of problem

   % ffmpeg -i tos.ogg tos.out.ogg
   # Spews a big bunch of errors;
   # [libvorbis @ 0x71f3c918a840] Queue input is backward in time
   #     Last message repeated 8 times
   # [ogg @ 0x71f3c915e040] Non-monotonous DTS in output stream 0:0; previous: 1818176, current: 1817856; changing to 1818176. This may result in incorrect timestamps in the output file.
   

3. Determine parsing actually yields incorrect result;

For clean canonical audio, I would expect that for any frame N `pts[N+1] = pts[N] + nb_samples[N]. This can be easily checked using awk (or your weapon of choice)

% ffprobe tos.ogg -show_frames \
    | gawk -F= '/nb_samples=/ {predicted_pts += $2} /pts=/ {pts = $2; print predicted_pts"-"pts"="(predicted_pts-pts)}'
# prints a diff of 0 for _most_ packets, but a diff of `-448` for ~4% of packets (depending on input audio)

On the frames where unexpected PTS is seen, it is also seen that pkt_duration != nb_samples which seems to have part in causing the bug.

The problem has been confirmed on ffmpeg v6.0.1 built on Alpine Linux and v7.1 built on OSX. Analysis of the demuxer indicates the code involved has been around for some ~10 years.

Result of early analysis:
Looking into ogg_read_packet(), ogg_calc_pts() seems to lean heavily on ogg_stream.lastpts being set earlier in the chain, resetting it to AV_NOPTS_VALUE on each invocation.

This seems to happen in vorbis_packet(), but does not seem to work as intended. This function starts with a big if-statement;

/* first packet handling
 * here we parse the duration of each packet in the first page and compare
 * the total duration to the page granule to find the encoder delay and
 * set the first timestamp */
if ((!os->lastpts || os->lastpts == AV_NOPTS_VALUE) && !(os->flags & OGG_FLAG_EOS) && (int64_t)os->granule>=0) {
   // ...  calculates lastpts by looking at page granule - sum(remaining packet-durations in page)

Since os->lastpts is reset for every packet emitted, this comment is outright incorrect. This block is executed for _every single packet_ (except the first on each page, which uses granule from last page). Besides incorrect behavior, this also constitutes a lot of wasted cycles on almost all packets.

This block also calls av_vorbis_parse_reset for each packet, clearing out AVVorbisParseContext.previous_blocksize. Since this is cleared for each packet, parsing the _current_ packet (at os->buf + os->pstart) will fail to properly take previous block-size into account, causing a wrong packet-duration to be calculated, both in the first if-statement, and the one below;

    /* parse packet duration */
    if (os->psize > 0) {
        duration = av_vorbis_parse_frame_flags(priv->vp, os->buf + os->pstart, 1, &flags);
        if (duration < 0) {

I have no obvious idea on how to fix this. I might device something that works for Vorbis, but I don't know enough about how _other codecs_ are packaged in Ogg, to ensure no poor interaction on them.

My immediate instinct would probably be to _not_ clear out os->lastpts in ogg_calc_pts(), when os->pduration is set, instead incrementing pts and dts with this number. But I don't know if this might break any other codec?

[Ulrik Mikaelsson]

Tears of Steel audio, encoded using ffmpeg -i ToS-4k-1920.mov -map a tos.ogg