On 2020-07-01 21:05 +0200, Alexander Strasser wrote:
> On 2020-07-01 16:23 +0200, Anton Khirnov wrote:
> > Quoting Jun Zhao (2020-06-29 15:23:10)
> > > From: Jun Zhao <[email protected]>
> > >
> > > Fix the potential overflow.
> > >
> > > Suggested-by: Alexander Strasser <[email protected]>
> > > Signed-off-by: Jun Zhao <[email protected]>
> > > ---
> > > libavcodec/aac_ac3_parser.c | 9 +++++----
> > > libavcodec/aac_ac3_parser.h | 4 ++--
> > > tests/ref/fate/adtstoasc_ticket3715 | 2 +-
> > > 3 files changed, 8 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
> > > index 0746798..b26790d 100644
> > > --- a/libavcodec/aac_ac3_parser.c
> > > +++ b/libavcodec/aac_ac3_parser.c
> > > @@ -98,11 +98,12 @@ get_next:
> > > }
> > >
> > > /* Calculate the average bit rate */
> > > - s->frame_number++;
> > > if (avctx->codec_id != AV_CODEC_ID_EAC3) {
> > > - avctx->bit_rate =
> > > - (s->last_bit_rate * (s->frame_number -1) +
> > > s->bit_rate)/s->frame_number;
> > > - s->last_bit_rate = avctx->bit_rate;
> > > + if (s->frame_number < UINT64_MAX) {
> > > + s->frame_number++;
> > > + s->last_bit_rate += (s->bit_rate -
> > > s->last_bit_rate)/s->frame_number;
> > > + avctx->bit_rate = (int64_t)llround(s->last_bit_rate);
> > > + }
> > > }
> > > }
> > >
> > > diff --git a/libavcodec/aac_ac3_parser.h b/libavcodec/aac_ac3_parser.h
> > > index b04041f..c53d16f 100644
> > > --- a/libavcodec/aac_ac3_parser.h
> > > +++ b/libavcodec/aac_ac3_parser.h
> > > @@ -55,8 +55,8 @@ typedef struct AACAC3ParseContext {
> > > uint64_t state;
> > >
> > > int need_next_header;
> > > - int frame_number;
> > > - int last_bit_rate;
> > > + uint64_t frame_number;
> > > + double last_bit_rate;
> >
> > This won't give the same result on all platforms anymore.
>
> It's also a bit different from what I had in mind.
>
> I was thinking more in the line of how it's implemented in
> libavcodec/mpegaudio_parser.c .
>
> There is a bit of noise there because of data that doesn't contain audio
> and also for the CBR case I think. Wouldn't be needed here AFAICT.
>
> I may well be missing something. If so understanding more would help me
> and we could fix both places. Otherwise if it's OK like it was done in
> mpegaudio_parser, we could maybe use the same strategy here too.
>
>
> Thanks for sending the patch and sorry for the delayed response.
I meant like this:
avctx->bit_rate += (s->bit_rate - avctx->bit_rate) / s->frame_number;
Patch attached. What do you think?
Would probably be even better to sum up in an uint64_t and divide
that sum to update the bit_rate field in AVCodecContext. Could be
implemented later for both parsers if it's considered worthwhile.
Alexander
From d25595dfa825ee0f18b7ac43a198478d2f4469f6 Mon Sep 17 00:00:00 2001
From: Alexander Strasser <[email protected]>
Date: Fri, 17 Jul 2020 20:51:43 +0200
Subject: [PATCH] lavc/aac_ac3_parser: fix potential overflow when averaging
bitrate
The new code is analog to how it's done in our mpegaudio parser.
---
libavcodec/aac_ac3_parser.c | 5 ++---
libavcodec/aac_ac3_parser.h | 1 -
tests/ref/fate/adtstoasc_ticket3715 | 2 +-
3 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
index 0746798dab..e5d4b53339 100644
--- a/libavcodec/aac_ac3_parser.c
+++ b/libavcodec/aac_ac3_parser.c
@@ -100,9 +100,8 @@ get_next:
/* Calculate the average bit rate */
s->frame_number++;
if (avctx->codec_id != AV_CODEC_ID_EAC3) {
- avctx->bit_rate =
- (s->last_bit_rate * (s->frame_number -1) + s->bit_rate)/s->frame_number;
- s->last_bit_rate = avctx->bit_rate;
+ avctx->bit_rate +=
+ (s->bit_rate - avctx->bit_rate) / s->frame_number;
}
}
diff --git a/libavcodec/aac_ac3_parser.h b/libavcodec/aac_ac3_parser.h
index b04041f69d..8b93cbf84f 100644
--- a/libavcodec/aac_ac3_parser.h
+++ b/libavcodec/aac_ac3_parser.h
@@ -56,7 +56,6 @@ typedef struct AACAC3ParseContext {
int need_next_header;
int frame_number;
- int last_bit_rate;
enum AVCodecID codec_id;
} AACAC3ParseContext;
diff --git a/tests/ref/fate/adtstoasc_ticket3715 b/tests/ref/fate/adtstoasc_ticket3715
index 3b473ee99e..fbdeeb0582 100644
--- a/tests/ref/fate/adtstoasc_ticket3715
+++ b/tests/ref/fate/adtstoasc_ticket3715
@@ -1,4 +1,4 @@
-3e63cbb6bb6ec756d79fab2632fef305 *tests/data/fate/adtstoasc_ticket3715.mov
+29ef0632a8eb5c336bf45a1d5076626e *tests/data/fate/adtstoasc_ticket3715.mov
33324 tests/data/fate/adtstoasc_ticket3715.mov
#extradata 0: 2, 0x00340022
#tb 0: 1/44100
--
2.17.1
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
