[FFmpeg-devel] [PATCH] Fix possible SIGFPEs with bad mov timings. Based on chromium patch 35_mov_bad_timings.patch

[Albert J. Wong (王重傑)]

On Thu, Nov 19, 2009 at 11:33 AM, Baptiste Coudurier <
baptiste.coudurier at gmail.com> wrote:

> On 11/19/2009 10:38 AM, Albert J. Wong (???) wrote:
>
>> On Thu, Nov 19, 2009 at 9:12 AM, Ronald S.
>> Bultje<rsbultje at gmail.com>wrote:
>>
>>  Hi,
>>>
>>> On Thu, Nov 19, 2009 at 1:45 AM, Baptiste Coudurier
>>> <baptiste.coudurier at gmail.com>  wrote:
>>>
>>>> I don't like the<  0 check, but I guess we don't have much choice
>>>> here.
>>>>
>>>
>>>
>> Yeah...also, the time_scale value is just a plain int itself, which
>> means that negatives are at least "valid" programmatically regardless
>> of spec. And having it be negative is pretty unexpected downstream.
>> Combine that with the ease with which you can get signed-unsigned
>> promotions accidentally in C, and this just becomes an annoying
>> risk.
>>
>> Er...
>>
>>>
>>> A) AVURational (bikeshed!!!!111) B) uint32_t num = READ_BE32(..),
>>> den = READ_BE32(..); if (num&  0x80000000 || den&  0x80000000) {
>>> den>>= 1; num>>= 1 }
>>>
>>>
>> Good point. Having a timescale in the 4-billion range is not likely
>> going to be very interesting.  I personally prefer the erroring out
>> scenario approach you outlined below rather than trying to "correct"
>> the timescale from one value w/o a great experience into another
>> value w/o a great experience. I've attached another patch to that
>> effect.  Let me know what you guys think.
>>
>
> Specs are using a 32 unsigned integer.
> Either we support it completely, or we choose an acceptable workaround.
>
>  35_mov_bad_timings_upstream.patch
>>
>>
>> diff --git libavformat/mov.c libavformat/mov.c index 6a8c149..09a72f9
>>
>> 100644 --- libavformat/mov.c +++ libavformat/mov.c @@ -1624,11
>> +1624,12 @@ static int mov_read_trak(MOVContext *c, ByteIOContext
>>
>> *pb, MOVAtom atom) return 0; }
>>
>> -    if (!sc->time_scale) { -        av_log(c->fc, AV_LOG_WARNING,
>> "stream %d, timescale not set\n", st->index); +    if
>> (c->time_scale<= 0) +        return -AVERROR_INVALIDDATA; +    if
>>
>> (sc->time_scale<= 0) { +        av_log(c->fc, AV_LOG_WARNING, "stream
>> %d, timescale %d invalid\n", st->index, +
>> sc->time_scale); sc->time_scale = c->time_scale; -        if
>>
>> (!sc->time_scale) -            sc->time_scale = 1; }
>>
>> av_set_pts_info(st, 64, 1, sc->time_scale);
>>
>
> Like I said I'm against erroring out. The file could be still demuxed to
> raw, completely ignoring timestamps.


How about something like

if (c->time_scale < 0)
  return AVERROR_INVALIDDATA;
else if (!c->time_scale)
  c->time_scale = 1;
if (sc->time_scale <=0) {
   // the log message whatever that was...
  sc_time_scape = c->time_scale;
}

?

That lets us handle the raw case, and chooses to ignore the potentially
negative values.  Is there's a better middle ground that I'm missing?

-Albert