| Message ID | 20210819085533.1174-1-ming.qian@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v4,1/3] avcodec/v4l2_context: don't reinit output queue when dynamic resolution change | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Thu, 19. Aug 16:55, Ming Qian wrote: > in the v4l2 stateful video document, we can see the following > description: > During the resolution change sequence, the OUTPUT queue must remain > streaming. Calling VIDIOC_STREAMOFF() on the OUTPUT queue would > abort the sequence and initiate a seek. > > In principle, the OUTPUT queue operates separately from the CAPTURE > queue and this remains true for the duration of the entire > resolution change sequence as well. > > so don't reinit the output queue when handling the resolution change > event > > Signed-off-by: Ming Qian <ming.qian@nxp.com> > --- > libavcodec/v4l2_context.c | 27 ++------------------------- > 1 file changed, 2 insertions(+), 25 deletions(-) > > diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c > index ff1ea8e57b08..dda5157698c3 100644 > --- a/libavcodec/v4l2_context.c > +++ b/libavcodec/v4l2_context.c > @@ -162,9 +162,8 @@ static int v4l2_handle_event(V4L2Context *ctx) > { > V4L2m2mContext *s = ctx_to_m2mctx(ctx); > struct v4l2_format cap_fmt = s->capture.format; > - struct v4l2_format out_fmt = s->output.format; > struct v4l2_event evt = { 0 }; > - int full_reinit, reinit, ret; > + int reinit, ret; > > ret = ioctl(s->fd, VIDIOC_DQEVENT, &evt); > if (ret < 0) { > @@ -180,25 +179,12 @@ static int v4l2_handle_event(V4L2Context *ctx) > if (evt.type != V4L2_EVENT_SOURCE_CHANGE) > return 0; > > - ret = ioctl(s->fd, VIDIOC_G_FMT, &out_fmt); > - if (ret) { > - av_log(logger(ctx), AV_LOG_ERROR, "%s VIDIOC_G_FMT\n", s->output.name); > - return 0; > - } > - > ret = ioctl(s->fd, VIDIOC_G_FMT, &cap_fmt); > if (ret) { > av_log(logger(ctx), AV_LOG_ERROR, "%s VIDIOC_G_FMT\n", s->capture.name); > return 0; > } > > - full_reinit = v4l2_resolution_changed(&s->output, &out_fmt); > - if (full_reinit) { > - s->output.height = v4l2_get_height(&out_fmt); > - s->output.width = v4l2_get_width(&out_fmt); > - s->output.sample_aspect_ratio = v4l2_get_sar(&s->output); > - } > - > reinit = v4l2_resolution_changed(&s->capture, &cap_fmt); > if (reinit) { > s->capture.height = v4l2_get_height(&cap_fmt); > @@ -206,18 +192,9 @@ static int v4l2_handle_event(V4L2Context *ctx) > s->capture.sample_aspect_ratio = v4l2_get_sar(&s->capture); > } > > - if (full_reinit || reinit) > + if (reinit) > s->reinit = 1; > > - if (full_reinit) { > - ret = ff_v4l2_m2m_codec_full_reinit(s); This is the only use of the function ff_v4l2_m2m_codec_full_reinit(). An option is to remove the function in the commit. I can see how this private function could be useful in the future though.. Do we then remove the function and add it back in the future or just let it be? Suggestion on IRC was to remove, but I also want to check on ML. > - if (ret) { > - av_log(logger(ctx), AV_LOG_ERROR, "v4l2_m2m_codec_full_reinit\n"); > - return AVERROR(EINVAL); > - } > - goto reinit_run; > - } > - > if (reinit) { > if (s->avctx) > ret = ff_set_dimensions(s->avctx, s->capture.width, s->capture.height); Otherwise, the patch looks good to me. Sorry for the delay. Thanks,
> -----Original Message----- > From: Andriy Gelman [mailto:andriy.gelman@gmail.com] > Sent: Monday, January 3, 2022 12:24 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Cc: Ming Qian <ming.qian@nxp.com> > Subject: [EXT] Re: [FFmpeg-devel] [PATCH v4 1/3] avcodec/v4l2_context: don't > reinit output queue when dynamic resolution change > > Caution: EXT Email > > On Thu, 19. Aug 16:55, Ming Qian wrote: > > in the v4l2 stateful video document, we can see the following > > description: > > During the resolution change sequence, the OUTPUT queue must > remain > > streaming. Calling VIDIOC_STREAMOFF() on the OUTPUT queue would > > abort the sequence and initiate a seek. > > > > In principle, the OUTPUT queue operates separately from the CAPTURE > > queue and this remains true for the duration of the entire > > resolution change sequence as well. > > > > so don't reinit the output queue when handling the resolution change > > event > > > > Signed-off-by: Ming Qian <ming.qian@nxp.com> > > --- > > libavcodec/v4l2_context.c | 27 ++------------------------- > > 1 file changed, 2 insertions(+), 25 deletions(-) > > > > diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c > > index ff1ea8e57b08..dda5157698c3 100644 > > --- a/libavcodec/v4l2_context.c > > +++ b/libavcodec/v4l2_context.c > > @@ -162,9 +162,8 @@ static int v4l2_handle_event(V4L2Context *ctx) { > > V4L2m2mContext *s = ctx_to_m2mctx(ctx); > > struct v4l2_format cap_fmt = s->capture.format; > > - struct v4l2_format out_fmt = s->output.format; > > struct v4l2_event evt = { 0 }; > > - int full_reinit, reinit, ret; > > + int reinit, ret; > > > > ret = ioctl(s->fd, VIDIOC_DQEVENT, &evt); > > if (ret < 0) { > > @@ -180,25 +179,12 @@ static int v4l2_handle_event(V4L2Context *ctx) > > if (evt.type != V4L2_EVENT_SOURCE_CHANGE) > > return 0; > > > > - ret = ioctl(s->fd, VIDIOC_G_FMT, &out_fmt); > > - if (ret) { > > - av_log(logger(ctx), AV_LOG_ERROR, "%s VIDIOC_G_FMT\n", > s->output.name); > > - return 0; > > - } > > - > > ret = ioctl(s->fd, VIDIOC_G_FMT, &cap_fmt); > > if (ret) { > > av_log(logger(ctx), AV_LOG_ERROR, "%s VIDIOC_G_FMT\n", > s->capture.name); > > return 0; > > } > > > > - full_reinit = v4l2_resolution_changed(&s->output, &out_fmt); > > - if (full_reinit) { > > - s->output.height = v4l2_get_height(&out_fmt); > > - s->output.width = v4l2_get_width(&out_fmt); > > - s->output.sample_aspect_ratio = v4l2_get_sar(&s->output); > > - } > > - > > reinit = v4l2_resolution_changed(&s->capture, &cap_fmt); > > if (reinit) { > > s->capture.height = v4l2_get_height(&cap_fmt); > > @@ -206,18 +192,9 @@ static int v4l2_handle_event(V4L2Context *ctx) > > s->capture.sample_aspect_ratio = v4l2_get_sar(&s->capture); > > } > > > > - if (full_reinit || reinit) > > + if (reinit) > > s->reinit = 1; > > > > - if (full_reinit) { > > > - ret = ff_v4l2_m2m_codec_full_reinit(s); > > This is the only use of the function ff_v4l2_m2m_codec_full_reinit(). An option > is to remove the function in the commit. I can see how this private > function could be useful in the future though.. Do we then remove the function > and add > it back in the future or just let it be? > > Suggestion on IRC was to remove, but I also want to check on ML. Hi Andriy, I think we can just keep it, if you really need to remove it, I think we can submit a single patch that remove it. > > > - if (ret) { > > - av_log(logger(ctx), AV_LOG_ERROR, > "v4l2_m2m_codec_full_reinit\n"); > > - return AVERROR(EINVAL); > > - } > > - goto reinit_run; > > - } > > - > > if (reinit) { > > if (s->avctx) > > ret = ff_set_dimensions(s->avctx, s->capture.width, > s->capture.height); > > Otherwise, the patch looks good to me. > Sorry for the delay. > > Thanks, > -- > Andriy
diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c index ff1ea8e57b08..dda5157698c3 100644 --- a/libavcodec/v4l2_context.c +++ b/libavcodec/v4l2_context.c @@ -162,9 +162,8 @@ static int v4l2_handle_event(V4L2Context *ctx) { V4L2m2mContext *s = ctx_to_m2mctx(ctx); struct v4l2_format cap_fmt = s->capture.format; - struct v4l2_format out_fmt = s->output.format; struct v4l2_event evt = { 0 }; - int full_reinit, reinit, ret; + int reinit, ret; ret = ioctl(s->fd, VIDIOC_DQEVENT, &evt); if (ret < 0) { @@ -180,25 +179,12 @@ static int v4l2_handle_event(V4L2Context *ctx) if (evt.type != V4L2_EVENT_SOURCE_CHANGE) return 0; - ret = ioctl(s->fd, VIDIOC_G_FMT, &out_fmt); - if (ret) { - av_log(logger(ctx), AV_LOG_ERROR, "%s VIDIOC_G_FMT\n", s->output.name); - return 0; - } - ret = ioctl(s->fd, VIDIOC_G_FMT, &cap_fmt); if (ret) { av_log(logger(ctx), AV_LOG_ERROR, "%s VIDIOC_G_FMT\n", s->capture.name); return 0; } - full_reinit = v4l2_resolution_changed(&s->output, &out_fmt); - if (full_reinit) { - s->output.height = v4l2_get_height(&out_fmt); - s->output.width = v4l2_get_width(&out_fmt); - s->output.sample_aspect_ratio = v4l2_get_sar(&s->output); - } - reinit = v4l2_resolution_changed(&s->capture, &cap_fmt); if (reinit) { s->capture.height = v4l2_get_height(&cap_fmt); @@ -206,18 +192,9 @@ static int v4l2_handle_event(V4L2Context *ctx) s->capture.sample_aspect_ratio = v4l2_get_sar(&s->capture); } - if (full_reinit || reinit) + if (reinit) s->reinit = 1; - if (full_reinit) { - ret = ff_v4l2_m2m_codec_full_reinit(s); - if (ret) { - av_log(logger(ctx), AV_LOG_ERROR, "v4l2_m2m_codec_full_reinit\n"); - return AVERROR(EINVAL); - } - goto reinit_run; - } - if (reinit) { if (s->avctx) ret = ff_set_dimensions(s->avctx, s->capture.width, s->capture.height);
in the v4l2 stateful video document, we can see the following description: During the resolution change sequence, the OUTPUT queue must remain streaming. Calling VIDIOC_STREAMOFF() on the OUTPUT queue would abort the sequence and initiate a seek. In principle, the OUTPUT queue operates separately from the CAPTURE queue and this remains true for the duration of the entire resolution change sequence as well. so don't reinit the output queue when handling the resolution change event Signed-off-by: Ming Qian <ming.qian@nxp.com> --- libavcodec/v4l2_context.c | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-)