[FFmpeg-devel] avcodec/omx: fix xFramerate calculation

Submitted by Aman Gupta on Aug. 30, 2019, 12:30 a.m.

Details

Message ID 20190830003025.70136-1-ffmpeg@tmm1.net
State New
Headers show

Commit Message

Aman Gupta Aug. 30, 2019, 12:30 a.m.
From: Aman Gupta <aman@tmm1.net>

Integer overflow in the Q16 framerate calculation was sending
invalid values to the OMX encoder.

On the RPI4, this manifested as bitrate controls being ignored
on video streams with 60000/1001 framerates. Video streams with
30000/1001 framerates were not affected.

Signed-off-by: Aman Gupta <aman@tmm1.net>
---
 libavcodec/omx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

lance.lmwang@gmail.com Aug. 30, 2019, 12:55 a.m.
On Thu, Aug 29, 2019 at 05:30:25PM -0700, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> Integer overflow in the Q16 framerate calculation was sending
> invalid values to the OMX encoder.
> 
> On the RPI4, this manifested as bitrate controls being ignored
> on video streams with 60000/1001 framerates. Video streams with
> 30000/1001 framerates were not affected.
> 
> Signed-off-by: Aman Gupta <aman@tmm1.net>
> ---
>  libavcodec/omx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/omx.c b/libavcodec/omx.c
> index 837f5df666..c745860242 100644
> --- a/libavcodec/omx.c
> +++ b/libavcodec/omx.c
> @@ -473,9 +473,9 @@ static av_cold int omx_component_init(AVCodecContext *avctx, const char *role)
>      in_port_params.format.video.nFrameWidth  = avctx->width;
>      in_port_params.format.video.nFrameHeight = avctx->height;
>      if (avctx->framerate.den > 0 && avctx->framerate.num > 0)
> -        in_port_params.format.video.xFramerate = (1 << 16) * avctx->framerate.num / avctx->framerate.den;
> +        in_port_params.format.video.xFramerate = (int64_t)(1 << 16) * avctx->framerate.num / avctx->framerate.den;

change 1 > 1LL is more simple to solve the overflow.


>      else
> -        in_port_params.format.video.xFramerate = (1 << 16) * avctx->time_base.den / avctx->time_base.num;
> +        in_port_params.format.video.xFramerate = (int64_t)(1 << 16) * avctx->time_base.den / avctx->time_base.num;
>  
>      err = OMX_SetParameter(s->handle, OMX_IndexParamPortDefinition, &in_port_params);
>      CHECK(err);
> -- 
> 2.20.1
> 
> _______________________________________________
> 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".
Aman Gupta Aug. 30, 2019, 1 a.m.
On Thu, Aug 29, 2019 at 5:56 PM Limin Wang <lance.lmwang@gmail.com> wrote:

> On Thu, Aug 29, 2019 at 05:30:25PM -0700, Aman Gupta wrote:
> > From: Aman Gupta <aman@tmm1.net>
> >
> > Integer overflow in the Q16 framerate calculation was sending
> > invalid values to the OMX encoder.
> >
> > On the RPI4, this manifested as bitrate controls being ignored
> > on video streams with 60000/1001 framerates. Video streams with
> > 30000/1001 framerates were not affected.
> >
> > Signed-off-by: Aman Gupta <aman@tmm1.net>
> > ---
> >  libavcodec/omx.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/omx.c b/libavcodec/omx.c
> > index 837f5df666..c745860242 100644
> > --- a/libavcodec/omx.c
> > +++ b/libavcodec/omx.c
> > @@ -473,9 +473,9 @@ static av_cold int omx_component_init(AVCodecContext
> *avctx, const char *role)
> >      in_port_params.format.video.nFrameWidth  = avctx->width;
> >      in_port_params.format.video.nFrameHeight = avctx->height;
> >      if (avctx->framerate.den > 0 && avctx->framerate.num > 0)
> > -        in_port_params.format.video.xFramerate = (1 << 16) *
> avctx->framerate.num / avctx->framerate.den;
> > +        in_port_params.format.video.xFramerate = (int64_t)(1 << 16) *
> avctx->framerate.num / avctx->framerate.den;
>
> change 1 > 1LL is more simple to solve the overflow.
>

Great, thank you for the tip! Updated my local patch accordingly.



>
>
> >      else
> > -        in_port_params.format.video.xFramerate = (1 << 16) *
> avctx->time_base.den / avctx->time_base.num;
> > +        in_port_params.format.video.xFramerate = (int64_t)(1 << 16) *
> avctx->time_base.den / avctx->time_base.num;
> >
> >      err = OMX_SetParameter(s->handle, OMX_IndexParamPortDefinition,
> &in_port_params);
> >      CHECK(err);
> > --
> > 2.20.1
> >
> > _______________________________________________
> > 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".
> _______________________________________________
> 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".
Nicolas George Aug. 30, 2019, 5:30 a.m.
Aman Gupta (12019-08-29):
> > > -        in_port_params.format.video.xFramerate = (1 << 16) *
> > avctx->framerate.num / avctx->framerate.den;
> > > +        in_port_params.format.video.xFramerate = (int64_t)(1 << 16) *
> > avctx->framerate.num / avctx->framerate.den;
> >
> > change 1 > 1LL is more simple to solve the overflow.
> 
> Great, thank you for the tip! Updated my local patch accordingly.

This remark looks weird: using long long directly is almost never
correct code.

And in this particular case, av_rescale_q() seems the correct choice.

Regards,
Aman Gupta Aug. 30, 2019, 3:53 p.m.
On Thu, Aug 29, 2019 at 10:30 PM Nicolas George <george@nsup.org> wrote:

> Aman Gupta (12019-08-29):
> > > > -        in_port_params.format.video.xFramerate = (1 << 16) *
> > > avctx->framerate.num / avctx->framerate.den;
> > > > +        in_port_params.format.video.xFramerate = (int64_t)(1 << 16)
> *
> > > avctx->framerate.num / avctx->framerate.den;
> > >
> > > change 1 > 1LL is more simple to solve the overflow.
> >
> > Great, thank you for the tip! Updated my local patch accordingly.
>
> This remark looks weird: using long long directly is almost never
> correct code.
>
> And in this particular case, av_rescale_q() seems the correct choice.
>

The OMX spec requires xFramerate in Q16 format, which is basically
(framerate<<16)

I'm not sure how I would use av_rescale_q to achieve this.


>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> 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".

Patch hide | download patch | download mbox

diff --git a/libavcodec/omx.c b/libavcodec/omx.c
index 837f5df666..c745860242 100644
--- a/libavcodec/omx.c
+++ b/libavcodec/omx.c
@@ -473,9 +473,9 @@  static av_cold int omx_component_init(AVCodecContext *avctx, const char *role)
     in_port_params.format.video.nFrameWidth  = avctx->width;
     in_port_params.format.video.nFrameHeight = avctx->height;
     if (avctx->framerate.den > 0 && avctx->framerate.num > 0)
-        in_port_params.format.video.xFramerate = (1 << 16) * avctx->framerate.num / avctx->framerate.den;
+        in_port_params.format.video.xFramerate = (int64_t)(1 << 16) * avctx->framerate.num / avctx->framerate.den;
     else
-        in_port_params.format.video.xFramerate = (1 << 16) * avctx->time_base.den / avctx->time_base.num;
+        in_port_params.format.video.xFramerate = (int64_t)(1 << 16) * avctx->time_base.den / avctx->time_base.num;
 
     err = OMX_SetParameter(s->handle, OMX_IndexParamPortDefinition, &in_port_params);
     CHECK(err);