diff mbox series

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

Message ID 1593436991-20076-1-git-send-email-mypopydev@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] lavc/aac_ac3_parser: fix the potential overflow
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jun Zhao June 29, 2020, 1:23 p.m. UTC
From: Jun Zhao <barryjzhao@tencent.com>

Fix the potential overflow.

Suggested-by: Alexander Strasser <eclipse7@gmx.net>
Signed-off-by: Jun Zhao <barryjzhao@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(-)

Comments

Anton Khirnov July 1, 2020, 2:23 p.m. UTC | #1
Quoting Jun Zhao (2020-06-29 15:23:10)
> From: Jun Zhao <barryjzhao@tencent.com>
> 
> Fix the potential overflow.
> 
> Suggested-by: Alexander Strasser <eclipse7@gmx.net>
> Signed-off-by: Jun Zhao <barryjzhao@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.
Alexander Strasser July 1, 2020, 7:05 p.m. UTC | #2
Hi Anton,
hi Jun!

On 2020-07-01 16:23 +0200, Anton Khirnov wrote:
> Quoting Jun Zhao (2020-06-29 15:23:10)
> > From: Jun Zhao <barryjzhao@tencent.com>
> >
> > Fix the potential overflow.
> >
> > Suggested-by: Alexander Strasser <eclipse7@gmx.net>
> > Signed-off-by: Jun Zhao <barryjzhao@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.


Best regards,
  Alexander
Alexander Strasser July 23, 2020, 8:43 p.m. UTC | #3
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@tencent.com>
> > >
> > > Fix the potential overflow.
> > >
> > > Suggested-by: Alexander Strasser <eclipse7@gmx.net>
> > > Signed-off-by: Jun Zhao <barryjzhao@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.


  Alexander
mypopy@gmail.com July 24, 2020, 1:40 a.m. UTC | #4
On Fri, Jul 24, 2020 at 4:43 AM Alexander Strasser <eclipse7@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@tencent.com>
> > > >
> > > > Fix the potential overflow.
> > > >
> > > > Suggested-by: Alexander Strasser <eclipse7@gmx.net>
> > > > Signed-off-by: Jun Zhao <barryjzhao@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
Zhao Zhili July 24, 2020, 11:56 a.m. UTC | #5
> On Jul 24, 2020, at 9:40 AM, mypopy@gmail.com wrote:
> 
> On Fri, Jul 24, 2020 at 4:43 AM Alexander Strasser <eclipse7@gmx.net <mailto:eclipse7@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@tencent.com>
>>>>> 
>>>>> Fix the potential overflow.
>>>>> 
>>>>> Suggested-by: Alexander Strasser <eclipse7@gmx.net>
>>>>> Signed-off-by: Jun Zhao <barryjzhao@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.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
Alexander Strasser July 24, 2020, 7:21 p.m. UTC | #6
On 2020-07-24 19:56 +0800, zhilizhao wrote:
>
>
> > On Jul 24, 2020, at 9:40 AM, mypopy@gmail.com wrote:
> >
> > On Fri, Jul 24, 2020 at 4:43 AM Alexander Strasser <eclipse7@gmx.net <mailto:eclipse7@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@tencent.com>
> >>>>>
> >>>>> Fix the potential overflow.
> >>>>>
> >>>>> Suggested-by: Alexander Strasser <eclipse7@gmx.net>
> >>>>> Signed-off-by: Jun Zhao <barryjzhao@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?


  Alexander
mypopy@gmail.com July 30, 2020, 1:18 p.m. UTC | #7
On Sat, Jul 25, 2020 at 3:21 AM Alexander Strasser <eclipse7@gmx.net> wrote:
>
> On 2020-07-24 19:56 +0800, zhilizhao wrote:
> >
> >
> > > On Jul 24, 2020, at 9:40 AM, mypopy@gmail.com wrote:
> > >
> > > On Fri, Jul 24, 2020 at 4:43 AM Alexander Strasser <eclipse7@gmx.net <mailto:eclipse7@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@tencent.com>
> > >>>>>
> > >>>>> Fix the potential overflow.
> > >>>>>
> > >>>>> Suggested-by: Alexander Strasser <eclipse7@gmx.net>
> > >>>>> Signed-off-by: Jun Zhao <barryjzhao@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
Alexander Strasser July 30, 2020, 7:47 p.m. UTC | #8
On 2020-07-30 21:18 +0800, mypopy@gmail.com wrote:
>
> After repeated thinking, I can accept this solution, thx

Just to avoid confusion. You accept this

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

or this

> > 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.

solution?


Thanks,
  Alexander
mypopy@gmail.com July 31, 2020, 1:45 a.m. UTC | #9
On Fri, Jul 31, 2020 at 3:47 AM Alexander Strasser <eclipse7@gmx.net> wrote:
>
> On 2020-07-30 21:18 +0800, mypopy@gmail.com wrote:
> >
> > After repeated thinking, I can accept this solution, thx
>
> Just to avoid confusion. You accept this
>
> > > > > avctx->bit_rate += (s->bit_rate - avctx->bit_rate) / s->frame_number;
>
This part  for potential overflow :)
> or this
>
> > > 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.
>
> solution?
>
>
> Thanks,
>   Alexander
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Alexander Strasser Aug. 12, 2020, 3:42 p.m. UTC | #10
On 2020-07-31 09:45 +0800, mypopy@gmail.com wrote:
> On Fri, Jul 31, 2020 at 3:47 AM Alexander Strasser <eclipse7@gmx.net> wrote:
> >
> > On 2020-07-30 21:18 +0800, mypopy@gmail.com wrote:
> > >
> > > After repeated thinking, I can accept this solution, thx
> >
> > Just to avoid confusion. You accept this
> >
> > > > > > avctx->bit_rate += (s->bit_rate - avctx->bit_rate) / s->frame_number;
> >
> This part  for potential overflow :)

Finally pushed. Thx!

[...]

  Alexander
diff mbox series

Patch

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;
     enum AVCodecID codec_id;
 } AACAC3ParseContext;
 
diff --git a/tests/ref/fate/adtstoasc_ticket3715 b/tests/ref/fate/adtstoasc_ticket3715
index 3b473ee..63e577a 100644
--- a/tests/ref/fate/adtstoasc_ticket3715
+++ b/tests/ref/fate/adtstoasc_ticket3715
@@ -1,4 +1,4 @@ 
-3e63cbb6bb6ec756d79fab2632fef305 *tests/data/fate/adtstoasc_ticket3715.mov
+feb8a43e4b7df2a4763bd9d55153c900 *tests/data/fate/adtstoasc_ticket3715.mov
 33324 tests/data/fate/adtstoasc_ticket3715.mov
 #extradata 0:        2, 0x00340022
 #tb 0: 1/44100