[FFmpeg-devel] [PATCH] Fix huge reported DTS on Ogg Theora files with bogus granulepos

[Brion Vibber]

On Sun, Jan 15, 2017 at 5:35 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> > --- a/libavformat/oggparsetheora.c
> > +++ b/libavformat/oggparsetheora.c
> > @@ -153,6 +153,10 @@ static uint64_t theora_gptopts(AVFormatContext
> *ctx, int idx, uint64_t gp,
> >      if (!thp)
> >          return AV_NOPTS_VALUE;
> >
> > +    // Fix for broken files; negative granulepos is invalid.
>
> Please add a reference to the spec and section that says this
>
> is this documented in some specification about ogg or theora ?
> I cannot find any definite statment that granulepos is a signed
> field, just that -1 as in twos compliment is a special value for it.
> and that its interpretation is left to the codec, but i knew that
> already.
>
> is something declaring the highest bit to be 0 for all frames of
> all codecs in ogg ?
>

Monty says negative values other than the special -1 are forbidden, but
we're still double-checking the spec to make sure we can source that
correctly... :)


In libogg's ogg_packet type, granulepos is stored and exposed as a signed
ogg_int64_t:
https://xiph.org/ogg/doc/libogg/ogg_packet.html

Ogg Vorbis encoding does explicitly exclude other negative values for
granulepos: https://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-132000A.2

The general bitstream documentation at
https://xiph.org/vorbis/doc/framing.html only mentions -1 as a special
value, but isn't clear on the meaning of the rest of the values...


Further general bitstream documentation at
https://www.xiph.org/ogg/doc/oggstream.html indicates that "Packets and
pages must be arranged in ascending granule-position and time order", in
which case a legitimate value of 0xffff'ffff'ffff'fffe (-2) would be wildly
out of place; affected parts of the input file look roughly like:

packet 2633: granulepos 2600<<6 + 33
packet 2634: granulepos -2
packet 2635: granulepos 2600<<6 + 35

Theora's granulepos combines a frame count for keyframe, bit-shifted, with
a count of frames since the keyframe. This can be decoded into a total
frame count, which increases by one on every frame, as well as the GP
value's always-positive change. (See https://www.theora.org/doc/Theora.pdf
section A.2.3)

So I've got a bad input file one way or another -- either the -2 value is
violating the spec by its existence, or it's violating the spec by not
updating the total frame count by one.


>
> orthogonal to this, timestamps half as large would still cause problems
> if software has problems with huge timestamps
> We should reject invalid granulepos, if this is invalid but i think
> this is not a sufficient fix if the problem is frame duplication
> from large timestamp differences
>

*nod* similar problems would occur with an out of place large positive
granulepos, which would still be invalid due to being out of place even if
it's a valid number. But I've only seen it with -1s (expected) and this
file with -2 (unexpected and seemingly invalid) so far.

One further possibility is to validate Theora granulepos values against the
previous frame's granulepos; we should always see the _decoded_ granulepos
advance by 1 from frame to frame unless we're seeking. (Theora handles
extra space between frames by encoding a special empty "duplicate" frame
packet to take up space in the frame sequence.)

-- brion



>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No snowflake in an avalanche ever feels responsible. -- Voltaire
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>