| Message ID | 20200420110055.5134-1-lance.lmwang@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v1] avcodec/utils: use av_rescale() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Mon, Apr 20, 2020 at 07:00:55PM +0800, lance.lmwang@gmail.com wrote: > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavcodec/utils.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index 26c038dfd9..005d596dfd 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -2229,8 +2229,8 @@ int64_t ff_guess_coded_bitrate(AVCodecContext *avctx) > const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); > bits_per_coded_sample = av_get_bits_per_pixel(desc); > } > - bitrate = (int64_t)bits_per_coded_sample * avctx->width * avctx->height * > - framerate.num / framerate.den; > + bitrate = av_rescale(avctx->width * avctx->height, > + bits_per_coded_sample * framerate.num, framerate.den); why this change ? also bits_per_coded_sample * framerate.num could possibly overflow after this i think thx [...]
On Tue, Apr 21, 2020 at 11:05:59PM +0200, Michael Niedermayer wrote: > On Mon, Apr 20, 2020 at 07:00:55PM +0800, lance.lmwang@gmail.com wrote: > > From: Limin Wang <lance.lmwang@gmail.com> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > --- > > libavcodec/utils.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > > index 26c038dfd9..005d596dfd 100644 > > --- a/libavcodec/utils.c > > +++ b/libavcodec/utils.c > > @@ -2229,8 +2229,8 @@ int64_t ff_guess_coded_bitrate(AVCodecContext *avctx) > > const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); > > bits_per_coded_sample = av_get_bits_per_pixel(desc); > > } > > - bitrate = (int64_t)bits_per_coded_sample * avctx->width * avctx->height * > > - framerate.num / framerate.den; > > + bitrate = av_rescale(avctx->width * avctx->height, > > + bits_per_coded_sample * framerate.num, framerate.den); > > why this change ? I recall mp3dec.c, avidec.c etc always use av_scale to get the bitrate, so I think it's more general to use av_rescale. If I'm misunderstanding, please ignore the patch. > > also > bits_per_coded_sample * framerate.num > could possibly overflow after this i think It seems that the int64_t is necessary still. Then I prefer to: + bitrate = av_rescale((int64_t)bits_per_coded_sample * avctx->width * avctx->height, + framerate.num, framerate.den); > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Nations do behave wisely once they have exhausted all other alternatives. > -- Abba Eban > _______________________________________________ > 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 Mon, Apr 20, 2020 at 07:00:55PM +0800, lance.lmwang@gmail.com wrote: > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavcodec/utils.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index 26c038dfd9..005d596dfd 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -2229,8 +2229,8 @@ int64_t ff_guess_coded_bitrate(AVCodecContext *avctx) > const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); > bits_per_coded_sample = av_get_bits_per_pixel(desc); > } > - bitrate = (int64_t)bits_per_coded_sample * avctx->width * avctx->height * > - framerate.num / framerate.den; > + bitrate = av_rescale(avctx->width * avctx->height, > + bits_per_coded_sample * framerate.num, framerate.den); > > return bitrate; > } > -- > 2.21.0 > ping the patchset.
lance.lmwang@gmail.com: > On Mon, Apr 20, 2020 at 07:00:55PM +0800, lance.lmwang@gmail.com wrote: >> From: Limin Wang <lance.lmwang@gmail.com> >> >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com> >> --- >> libavcodec/utils.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >> index 26c038dfd9..005d596dfd 100644 >> --- a/libavcodec/utils.c >> +++ b/libavcodec/utils.c >> @@ -2229,8 +2229,8 @@ int64_t ff_guess_coded_bitrate(AVCodecContext *avctx) >> const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); >> bits_per_coded_sample = av_get_bits_per_pixel(desc); >> } >> - bitrate = (int64_t)bits_per_coded_sample * avctx->width * avctx->height * >> - framerate.num / framerate.den; >> + bitrate = av_rescale(avctx->width * avctx->height, >> + bits_per_coded_sample * framerate.num, framerate.den); Is this supposed to be a cosmetic patch or do you have a testcase where it leads to better results? Anyway, bits_per_coded_sample * framerate.num might overflow (the function parameter is 64bit, but the calculation will nevertheless be performed as int * int). (I am not saying that the overflow checks for the code as-is are perfect, as I don't know what checks have already been performed to make sure that the calculation doesn't overflow an int64_t; but you would add a new possibility for overflow.) And your indentation is off: the second line (i.e. bits_per_coded_sample) should be aligned with the first function parameter in the first line (i.e. avctx->width). - Andreas
On Sun, May 03, 2020 at 03:10:09PM +0200, Andreas Rheinhardt wrote: > lance.lmwang@gmail.com: > > On Mon, Apr 20, 2020 at 07:00:55PM +0800, lance.lmwang@gmail.com wrote: > >> From: Limin Wang <lance.lmwang@gmail.com> > >> > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > >> --- > >> libavcodec/utils.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c > >> index 26c038dfd9..005d596dfd 100644 > >> --- a/libavcodec/utils.c > >> +++ b/libavcodec/utils.c > >> @@ -2229,8 +2229,8 @@ int64_t ff_guess_coded_bitrate(AVCodecContext *avctx) > >> const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); > >> bits_per_coded_sample = av_get_bits_per_pixel(desc); > >> } > >> - bitrate = (int64_t)bits_per_coded_sample * avctx->width * avctx->height * > >> - framerate.num / framerate.den; > >> + bitrate = av_rescale(avctx->width * avctx->height, > >> + bits_per_coded_sample * framerate.num, framerate.den); > > Is this supposed to be a cosmetic patch or do you have a testcase where > it leads to better results? Anyway, bits_per_coded_sample * > framerate.num might overflow (the function parameter is 64bit, but the > calculation will nevertheless be performed as int * int). > (I am not saying that the overflow checks for the code as-is are > perfect, as I don't know what checks have already been performed to make > sure that the calculation doesn't overflow an int64_t; but you would add > a new possibility for overflow.) > > And your indentation is off: the second line (i.e. > bits_per_coded_sample) should be aligned with the first function > parameter in the first line (i.e. avctx->width). Sorry, my mistake, I ping another patch in fact. please ignore the patch. > > - Andreas > _______________________________________________ > 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".
diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 26c038dfd9..005d596dfd 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2229,8 +2229,8 @@ int64_t ff_guess_coded_bitrate(AVCodecContext *avctx) const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); bits_per_coded_sample = av_get_bits_per_pixel(desc); } - bitrate = (int64_t)bits_per_coded_sample * avctx->width * avctx->height * - framerate.num / framerate.den; + bitrate = av_rescale(avctx->width * avctx->height, + bits_per_coded_sample * framerate.num, framerate.den); return bitrate; }