[FFmpeg-devel] [PATCH] Vorbis-in-Ogg: Do not set timebase to invalid values

[Justin Ruggles]

On 02/07/2011 08:51 AM, Michael Niedermayer wrote:

> On Fri, Feb 04, 2011 at 01:05:27PM -0500, Justin Ruggles wrote:
>> On 02/04/2011 11:04 AM, Michael Niedermayer wrote:
>>
>>> On Thu, Feb 03, 2011 at 06:14:16PM -0500, Justin Ruggles wrote:
>>>> On 01/29/2011 06:32 AM, Baptiste Coudurier wrote:
>>>>
>>>>> On 1/29/11 3:06 AM, Ronald S. Bultje wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Sat, Jan 29, 2011 at 5:52 AM, Reimar D?ffinger
>>>>>> <Reimar.Doeffinger at gmx.de> wrote:
>>>>>>> On Sat, Jan 29, 2011 at 01:46:31AM -0800, Jason Garrett-Glaser wrote:
>>>>>>>> On Fri, Jan 28, 2011 at 10:22 PM, Reimar D?ffinger
>>>>>>>> <Reimar.Doeffinger at gmx.de> wrote:
>>>>>>>>> On 29 Jan 2011, at 05:42, M?ns Rullg?rd <mans at mansr.com> wrote:
>>>>>>>>>> Janne Grunau <janne-ffmpeg at jannau.net> writes:
>>>>>>>>>>
>>>>>>>>>>> From: Reimar D?ffinger <Reimar.Doeffinger at gmx.de>
>>>>>>>>>>>
>>>>>>>>>>> cherry picked from git.videolan.org repo
>>>>>>>>>>>
>>>>>>>>>>> Janne
>>>>>>>>>>> ---8<---
>>>>>>>>>>> Avoids an assert when the sample rate is invalid and the timebase
>>>>>>>>>>> is thus set to e.g. 1/0.
>>>>>>>>>>> Sample file is http://samples.mplayerhq.hu/ogg/fuzzed-srate-crash.ogg
>>>>>>>>>>> ---
>>>>>>>>>>> libavformat/oggparsevorbis.c |   10 +++++++---
>>>>>>>>>>> 1 files changed, 7 insertions(+), 3 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
>>>>>>>>>>> index cdb0266..d743d25 100644
>>>>>>>>>>> --- a/libavformat/oggparsevorbis.c
>>>>>>>>>>> +++ b/libavformat/oggparsevorbis.c
>>>>>>>>>>> @@ -221,6 +221,7 @@ vorbis_header (AVFormatContext * s, int idx)
>>>>>>>>>>>     if (os->buf[os->pstart] == 1) {
>>>>>>>>>>>         const uint8_t *p = os->buf + os->pstart + 7; /* skip "\001vorbis" tag */
>>>>>>>>>>>         unsigned blocksize, bs0, bs1;
>>>>>>>>>>> +        int srate;
>>>>>>>>>>>
>>>>>>>>>>>         if (os->psize != 30)
>>>>>>>>>>>             return -1;
>>>>>>>>>>> @@ -229,7 +230,7 @@ vorbis_header (AVFormatContext * s, int idx)
>>>>>>>>>>>             return -1;
>>>>>>>>>>>
>>>>>>>>>>>         st->codec->channels = bytestream_get_byte(&p);
>>>>>>>>>>> -        st->codec->sample_rate = bytestream_get_le32(&p);
>>>>>>>>>>> +        srate = bytestream_get_le32(&p);
>>>>>>>>>>>         p += 4; // skip maximum bitrate
>>>>>>>>>>>         st->codec->bit_rate = bytestream_get_le32(&p); // nominal bitrate
>>>>>>>>>>>         p += 4; // skip minimum bitrate
>>>>>>>>>>> @@ -249,8 +250,11 @@ vorbis_header (AVFormatContext * s, int idx)
>>>>>>>>>>>         st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
>>>>>>>>>>>         st->codec->codec_id = CODEC_ID_VORBIS;
>>>>>>>>>>>
>>>>>>>>>>> -        st->time_base.num = 1;
>>>>>>>>>>> -        st->time_base.den = st->codec->sample_rate;
>>>>>>>>>>> +        if (srate > 0) {
>>>>>>>>>>> +            st->codec->sample_rate = srate;
>>>>>>>>>>> +            st->time_base.num = 1;
>>>>>>>>>>> +            st->time_base.den = srate;
>>>>>>>>>>> +        }
>>>>>>>>>>>     } else if (os->buf[os->pstart] == 3) {
>>>>>>>>>>>         if (os->psize > 8)
>>>>>>>>>>>             ff_vorbis_comment (s, &st->metadata, os->buf + os->pstart + 7, os->psize - 8);
>>>>>>>>>>> --
>>>>>>>>>>> 1.7.4.rc2
>>>>>>>>>>
>>>>>>>>>> I still want to know why common code doesn't catch this.  Replicating
>>>>>>>>>> this check in each and every demuxer is insane.
>>>>>>>>>
>>>>>>>>> I already explained it and nobody cared.
>>>>>>>>> I also gave a reason why, even if the common code would catch it this is of advantage.
>>>>>>>>> In addition relying on common code is risky, streams can be added at any time, demuxers might parse headers even if they are in the middle of the stream, unless data is validated immediately (i.e. in the demuxer) there is a significant risk that it will end up outside FFmpeg, in the application.
>>>>>>>>> However common code could set the timebase properly/better if the decoder figures out the sample rate, but I don't feel like investigating where to do this.
>>>>>>>>
>>>>>>>> Maybe it could be done in the wrapper functions, e.g. the utility
>>>>>>>> functions, not per-demuxer?  So it's still done in libav*, but not in
>>>>>>>> each codec repeatedly.
>>>>>>>
>>>>>>> I was going to say that there is no place to do this that does not include
>>>>>>> a lot of issues, but there actually is one:
>>>>>>> av_set_pts_info
>>>>>>> With the little issue that this code incorrectly bypasses that function.
>>>>>>> A lot of code does that, so a first patch is to make everyone actually
>>>>>>> use that.
>>>>>>> I probably missed an few.
>>>>>>
>>>>>> I really like this patch. Might want a few people to check and make
>>>>>> sure we didn't miss any, but this looks very right.
>>>>>
>>>>> Yes, that's the way to do it, but av_set_pts_info has a problem:
>>>>> It's using unsigned as parameters while AVRational is two int, and I'm
>>>>> not sure how av_reduce exactly behaves.
>>>>
>>>>
>>>> av_reduce() takes 2 int64_t and a max absolute output value, so because
>>>> av_set_pts_info() uses INT_MAX for the max value this situation is fine.
>>>>  The unsigned ints will be converted to int64_t without any issue and
>>>> the output will be limited INT_MAX, but might be inexact.
>>>>
>>>> I am in favor of this patch.  But yes av_set_pts_info() also needs to be
>>>> changed after this is done to ensure time_base.den is not set to 0.
>>>>
>>>> Regarding codec->time_base vs. st->time_base, for audio if
>>>> codec->time_base is not set to something other than 0/1 by the demuxer
>>>> it never gets set as far as I can tell.  av_find_stream_info() only
>>>> copies the st->time_base to codec->time_base for video and subtitle
>>>> streams.  Also, they can definitely be separate values (e.g. FLV streams
>>>> always have st->time_base of 1/1000, but codec->time_base should be
>>>> 1/sample_rate).
>>>>
>>>> So I have some questions.
>>>>
>>>
> 
>>>> 1) Why is anything in libavformat setting codec->time_base for decoders
>>>> when the documentation says that's supposed to be done by libavcodec
>>>> ("decoding: Set by libavcodec.")?
>>>
>>> Because the docs are buggy
> 
> here the docs should be fixed
> 
> 
>>>
>>> [...]
>>>> 3) It seems libavformat assumes that codec->time_base is not set for
>>>> audio and always uses codec->sample_rate instead.  Should the
>>>> AVCodecContext.time_base documentation be changed to say it's only valid
>>>> for video and subtitles? or should avcodec_open() set it to
>>>> 1/sample_rate for audio? or something else?
>>>
>>> I would leave the documentation and not add special cases
>>
>>
>> Why leave the documentation if it is buggy?
> 
> i see no bug related to audio in the timebase documentation. If you send a
> patch maybe we can figure out how we misunderstand each other


I just didn't understand what you were saying. "the docs are buggy" and
"leave the documentation" seems contradictory and didn't really answer
my questions as to the best way to move forward.  My best guess is:

1) change time_base documentation to say "decoding: Set by user if
known, overridden by libavcodec if known"

2) set time_base equal to 1/sample_rate for audio after init in
avcodec_open() if sample_rate > 0.

-Justin