[FFmpeg-devel] [PATCH 1/2] lavc/aac_ac3_parser: fix the potential overflow

[mypopy at gmail.com]

On Fri, Jul 24, 2020 at 4:43 AM Alexander Strasser <eclipse7 at gmx.net> wrote:
>
> 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 <barryjzhao at tencent.com>
> > > >
> > > > Fix the potential overflow.
> > > >
> > > > Suggested-by: Alexander Strasser <eclipse7 at gmx.net>
> > > > Signed-off-by: Jun Zhao <barryjzhao at tencent.com>
> > > > ---
> > > >  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.
>
I see, my concern is

avctx->bit_rate += (s->bit_rate - avctx->bit_rate) / s->frame_number;

will lose precision in (s->bit_rate - avctx->bit_rate) /
s->frame_number, this is the reason I used the  double replace
uint64_t

I can give an example of you code, suppose we probe 4 ADTS AAC frames,
the s->bit_rate is 4Kbps 3Kbps 2Kbps 1Kbps respectively,

In this code, we will always get the bitrate 4Kbps, but in an ideal
situation, we want to get the average bitrate be close to  (4 + 3 + 2
+ 1) / 4 = 2.5Kbps after the probe