diff mbox series

[FFmpeg-devel,v1] avcodec/utils: use av_rescale()

Message ID 20200420110055.5134-1-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,v1] avcodec/utils: use av_rescale()
Related show

Checks

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

Commit Message

Limin Wang April 20, 2020, 11 a.m. UTC
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(-)

Comments

Michael Niedermayer April 21, 2020, 9:05 p.m. UTC | #1
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

[...]
Limin Wang April 22, 2020, 1:12 a.m. UTC | #2
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".
Limin Wang May 3, 2020, 12:58 p.m. UTC | #3
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.
Andreas Rheinhardt May 3, 2020, 1:10 p.m. UTC | #4
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
Limin Wang May 3, 2020, 1:19 p.m. UTC | #5
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 mbox series

Patch

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;
 }