On Sat, Jul 25, 2020 at 3:21 AM Alexander Strasser <eclip...@gmx.net> wrote: > > On 2020-07-24 19:56 +0800, zhilizhao wrote: > > > > > > > On Jul 24, 2020, at 9:40 AM, myp...@gmail.com wrote: > > > > > > On Fri, Jul 24, 2020 at 4:43 AM Alexander Strasser <eclip...@gmx.net > > > <mailto:eclip...@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 <barryjz...@tencent.com> > > >>>>> > > >>>>> Fix the potential overflow. > > >>>>> > > >>>>> Suggested-by: Alexander Strasser <eclip...@gmx.net> > > >>>>> Signed-off-by: Jun Zhao <barryjz...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
