| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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.
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
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
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
> 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".
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
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
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
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".
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 --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