| Message ID | 20190830003025.70136-1-ffmpeg@tmm1.net |
|---|---|
| State | New |
| Headers | show |
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".
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".
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,
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".
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);