[FFmpeg-trac] #11450(undetermined:new): OGG Vorbis timestamp generation broken

[FFmpeg]

#11450: OGG Vorbis timestamp generation broken
-------------------------------------+-------------------------------------
             Reporter:  Ulrik        |                     Type:  defect
  Mikaelsson                         |
               Status:  new          |                 Priority:  normal
            Component:               |                  Version:
  undetermined                       |  unspecified
             Keywords:               |               Blocked By:
             Blocking:               |  Reproduced by developer:  0
Analyzed by developer:  0            |
-------------------------------------+-------------------------------------
 **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?
-- 
Ticket URL: <https://trac.ffmpeg.org/ticket/11450>
FFmpeg <https://ffmpeg.org>
FFmpeg issue tracker