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

[mypopy at gmail.com]

On Sat, Jul 25, 2020 at 3:21 AM Alexander Strasser <eclipse7 at gmx.net> wrote:
>
> On 2020-07-24 19:56 +0800, zhilizhao wrote:
> >
> >
> > > On Jul 24, 2020, at 9:40 AM, mypopy at gmail.com wrote:
> > >
> > > On Fri, Jul 24, 2020 at 4:43 AM Alexander Strasser <eclipse7 at gmx.net <mailto: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
> >
> > The example has an issue but the point stands.
> >
> > With 4Kbps, 3Kbps, and so on as input, the output is 2.5Kbps.
> > With 4bps, 3bps as input, the output is 4.
> >
> > The precision is decrease as frame number increase.
>
> I have a rough time following your examples.
>
> For [4000b/s, 3000b/s, 2000b/s, 1000b/s] I always get 2500 as a result
> for all methods.
>
> For [4b/s, 3b/s, 2b/s, 1b/s] I get 1 with the current ffmpeg master
> and with my patch I attached before I get 1 too. So no difference to
> current state of things.
>
> If I use non-integer arithmetic I get 2.5b/s as the ideal result.
> If I use the method I proposed in my last comment:
>
> > >> 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 get 2b/s which is with 3b/s one of the two closest integer results
> for 2.5b/s.
>
> I just proposed the last patch to avoid possible overflow problems,
> it wasn't meant to significantly improve results.
>
> If you are aiming for improving accuracy, I guess going for
> implementing the sum of bitrates is the most straight forward
> way. It wouldn't need floating point arithmetic and overflow
> wouldn't be much of a concern if I'm not mistaken.
>
> Am I missing something?
>
>
After repeated thinking， I can accept this solution, thx