[FFmpeg-devel] [FFmpeg-cvslog] lavc/aac_ac3_parser: improve the raw AAC file bit rate calculation

[Alexander Strasser]

On 2020-06-28 21:10 +0800, mypopy at gmail.com wrote:
> On Sun, Jun 28, 2020 at 5:30 AM Alexander Strasser <eclipse7 at gmx.net> wrote:
> >
> > On 2020-06-26 01:56 +0000, Jun Zhao wrote:
> > > ffmpeg | branch: master | Jun Zhao <barryjzhao at tencent.com> | Sun May 17 12:10:05 2020 +0800| [60d79b1df9d4c6030010ccb0c134ede9e33158c2] | committer: Jun Zhao
> > >
[...]
> > >
> > > diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
> > > index 54e459844f..0746798dab 100644
> > > --- a/libavcodec/aac_ac3_parser.c
> > > +++ b/libavcodec/aac_ac3_parser.c
> > > @@ -97,8 +97,13 @@ get_next:
> > >              avctx->audio_service_type = s->service_type;
> > >          }
> > >
> > > -        if (avctx->codec_id != AV_CODEC_ID_EAC3)
> > > -            avctx->bit_rate = s->bit_rate;
> > > +        /* 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;
> > > +        }
> > >      }
> >
> > Wouldn't it be better to sum up the individual bit_rate values in
> > a private context variable and divide by number of frames?
> >
> I can't found a way in private context, so I change the AAC/AC3 parser

My wording probably wasn't perfect, I just meant in AACAC3ParseContext.

> > This way or the other, it might be useful to think about maximum
> > values of the total bits? I suspect the current int calculation
> > might overflow.
> >
> Will fix the potential overflow for int calculation

Please pardon me in advance. This is a bit of a rushed comment, but
as you are already working on this, I prefer to comment now.

One way to avoid the overflow and the multiplication could look like
this (not tested, may contain typos/errors etc.):

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

This should yield almost identical results to the algorithm you
implemented. I say almost because when the increment is negative
the rounding is different.

Anyway it would also share the same flaw, that it would overvalue
the first frames and later frames will have diminishing impact.

On a further note, I can't believe this is the only place we have
this kind of problem. I think it would be good to investigate, how
it is solved elsewhere in the code base.


  Alexander

> > >      return i;
> > > diff --git a/libavcodec/aac_ac3_parser.h b/libavcodec/aac_ac3_parser.h
> > > index c2506a5bfd..b04041f69d 100644
> > > --- a/libavcodec/aac_ac3_parser.h
> > > +++ b/libavcodec/aac_ac3_parser.h
> > > @@ -55,6 +55,8 @@ typedef struct AACAC3ParseContext {
> > >      uint64_t state;
> > >
> > >      int need_next_header;
> > > +    int frame_number;
> > > +    int last_bit_rate;
> > >      enum AVCodecID codec_id;
> > >  } AACAC3ParseContext;