[FFmpeg-devel] [PATCH 1/6] avformat/avidec: support huge durations
Michael Niedermayer
michael at niedermayer.cc
Tue Mar 26 00:50:11 EET 2024
On Tue, Oct 03, 2023 at 08:53:33PM +0200, Michael Niedermayer wrote:
> On Tue, Oct 03, 2023 at 11:10:20AM +0200, Tomas Härdin wrote:
> > mån 2023-10-02 klockan 21:03 +0200 skrev Michael Niedermayer:
> > > On Mon, Oct 02, 2023 at 11:07:47AM +0200, Anton Khirnov wrote:
> > > > Quoting Michael Niedermayer (2023-10-01 00:28:56)
> > > > > On Sat, Sep 30, 2023 at 10:18:38PM +0200, Anton Khirnov wrote:
> > > > > > Quoting Michael Niedermayer (2023-09-30 16:31:43)
> > > > > > > On Sat, Sep 30, 2023 at 04:04:03PM +0200, Michael Niedermayer
> > > > > > > wrote:
> > > > > > > > On Sat, Sep 30, 2023 at 11:35:19AM +0200, Marton Balint
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Sat, 30 Sep 2023, Michael Niedermayer wrote:
> > > > > > > > >
> > > > > > > > > > Fixes: signed integer overflow: 109817402400 *
> > > > > > > > > > 301990077 cannot be represented in type 'long long'
> > > > > > > > > > Fixes: 51896/clusterfuzz-testcase-minimized-
> > > > > > > > > > ffmpeg_dem_AVI_fuzzer-6706191715139584
> > > > > > > > > >
> > > > > > > > > > Found-by: continuous fuzzing process
> > > > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > > > > > Signed-off-by: Michael Niedermayer
> > > > > > > > > > <michael at niedermayer.cc>
> > > > > > > > > > ---
> > > > > > > > > > libavformat/avidec.c | 12 ++++++++++--
> > > > > > > > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/libavformat/avidec.c
> > > > > > > > > > b/libavformat/avidec.c
> > > > > > > > > > index 00bd7a98a9d..cfc693842b7 100644
> > > > > > > > > > --- a/libavformat/avidec.c
> > > > > > > > > > +++ b/libavformat/avidec.c
> > > > > > > > > > @@ -27,6 +27,7 @@
> > > > > > > > > > #include "libavutil/avstring.h"
> > > > > > > > > > #include "libavutil/opt.h"
> > > > > > > > > > #include "libavutil/dict.h"
> > > > > > > > > > +#include "libavutil/integer.h"
> > > > > > > > > > #include "libavutil/internal.h"
> > > > > > > > > > #include "libavutil/intreadwrite.h"
> > > > > > > > > > #include "libavutil/mathematics.h"
> > > > > > > > > > @@ -476,7 +477,7 @@ static int
> > > > > > > > > > calculate_bitrate(AVFormatContext *s)
> > > > > > > > > > AVStream *st = s->streams[i];
> > > > > > > > > > FFStream *const sti = ffstream(st);
> > > > > > > > > > int64_t duration;
> > > > > > > > > > - int64_t bitrate;
> > > > > > > > > > + int64_t bitrate = 0;
> > > > > > > > > >
> > > > > > > > > > for (j = 0; j < sti->nb_index_entries; j++)
> > > > > > > > > > len += sti->index_entries[j].size;
> > > > > > > > > > @@ -484,7 +485,14 @@ static int
> > > > > > > > > > calculate_bitrate(AVFormatContext *s)
> > > > > > > > > > if (sti->nb_index_entries < 2 || st->codecpar-
> > > > > > > > > > >bit_rate > 0)
> > > > > > > > > > continue;
> > > > > > > > > > duration = sti->index_entries[j-1].timestamp -
> > > > > > > > > > sti->index_entries[0].timestamp;
> > > > > > > > > > - bitrate = av_rescale(8*len, st->time_base.den,
> > > > > > > > > > duration * st->time_base.num);
> > > > > > > > > > + if (INT64_MAX / duration >= st->time_base.num)
> > > > > > > > > > {
> > > > > > > > > > + bitrate = av_rescale(8*len, st-
> > > > > > > > > > >time_base.den, duration * st->time_base.num);
> > > > > > > > >
> > > > > > > > > Why not always use the AVInteger version? This is not
> > > > > > > > > performance sensitive
> > > > > > > > > as far as I see.
> > > > > > > >
> > > > > > > > We can, i will have to fix the rounding though so it
> > > > > > > > matches av_rescale()
> > > > > > >
> > > > > > > will apply this with just AVInteger and fixed rounding with
> > > > > > > my next push probably
> > > > > >
> > > > > > This seems MUCH less readable to me.
> > > > >
> > > > > we can add a av_rescale_2den()
> > > >
> > > > Won't av_rescale_q(len, (AVRational){8, duration}, st->time_base)
> > > > achieve the same effect?
> > >
> > > duration is 64bit AVRational is 32/32 bit, so i would expect that to
> > > not
> > > work.
> > > If duration was fitting in 32bit then duration * st->time_base.num
> > > would not
> > > have overflowed
> >
> > Maybe we need a 64/64-bit version of AVRational..
>
> Iam not convinced at this point that we need that.
> But its not hard to do, our AVInteger code will do any size integers by just
> changing a single number.
>
> The real annoyance is if you have 32/32 and 64/64 rationals then suddenly
> you need many functions to handle both. Sofar only a fuzzed file exceeded
> our 64 * 64 / 64 == 64 * 32/32 * 32/32 rescale in one place
> adding a 64 * 64 / (64 * 64) rescale would fix that one which seems more
> limited in spreading complexity through the codebase
Ill apply this below, because its undefined behavior and needs to be fixed.
If someone has a better solution please replace my code with it.
@@ -476,7 +477,7 @@ static int calculate_bitrate(AVFormatContext *s)
AVStream *st = s->streams[i];
FFStream *const sti = ffstream(st);
int64_t duration;
- int64_t bitrate;
+ AVInteger bitrate_i, den_i, num_i;
for (j = 0; j < sti->nb_index_entries; j++)
len += sti->index_entries[j].size;
@@ -484,9 +485,14 @@ static int calculate_bitrate(AVFormatContext *s)
if (sti->nb_index_entries < 2 || st->codecpar->bit_rate > 0)
continue;
duration = sti->index_entries[j-1].timestamp - sti->index_entries[0].timestamp;
- bitrate = av_rescale(8*len, st->time_base.den, duration * st->time_base.num);
- if (bitrate > 0) {
- st->codecpar->bit_rate = bitrate;
+ den_i = av_mul_i(av_int2i(duration), av_int2i(st->time_base.num));
+ num_i = av_add_i(av_mul_i(av_int2i(8*len), av_int2i(st->time_base.den)), av_shr_i(den_i, 1));
+ bitrate_i = av_div_i(num_i, den_i);
+ if (av_cmp_i(bitrate_i, av_int2i(INT64_MAX)) <= 0) {
+ int64_t bitrate = av_i2int(bitrate_i);
+ if (bitrate > 0) {
+ st->codecpar->bit_rate = bitrate;
+ }
}
}
return 1;
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240325/8cfed31a/attachment.sig>
More information about the ffmpeg-devel
mailing list