| Message ID | 20210729060027.9513-3-ming.qian@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v3,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, 29. Jul 14:00, Ming Qian wrote: > if client start the capture queue without waiting the source change > event, > there may be some timing issues. > For example, in client, the sequence is: > capture streamon -> source change -> capture streamoff -> capture > streamon. > but in driver side, the sequence may be: > source change -> capture streamon -> capture streamoff -> capture > streamon. > Then it may led to some unforeseen and serious problems. > So to avoid such timing issues, the client should setup the > capture queue after the first source change event is received. > > Signed-off-by: Ming Qian <ming.qian@nxp.com> > --- > libavcodec/v4l2_context.c | 6 +++++- > libavcodec/v4l2_m2m.c | 1 + > libavcodec/v4l2_m2m.h | 1 + > libavcodec/v4l2_m2m_dec.c | 34 +++++++++++++++------------------- > 4 files changed, 22 insertions(+), 20 deletions(-) > > diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c > index df41e982fc56..543fc9523cf1 100644 > --- a/libavcodec/v4l2_context.c > +++ b/libavcodec/v4l2_context.c > @@ -179,6 +179,7 @@ static int v4l2_handle_event(V4L2Context *ctx) > if (evt.type != V4L2_EVENT_SOURCE_CHANGE) > return 0; > > + s->source_change_cnt++; > 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); > @@ -272,7 +273,7 @@ static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout) > } > > /* if we are draining and there are no more capture buffers queued in the driver we are done */ > - if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx_to_m2mctx(ctx)->draining) { > + if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx_to_m2mctx(ctx)->draining && ctx->streamon) { > for (i = 0; i < ctx->num_buffers; i++) { > /* capture buffer initialization happens during decode hence > * detection happens at runtime > @@ -542,6 +543,9 @@ int ff_v4l2_context_set_status(V4L2Context* ctx, uint32_t cmd) > int type = ctx->type; > int ret; > > + if (ctx->streamon == (cmd == VIDIOC_STREAMON)) > + return 0; > + > ret = ioctl(ctx_to_m2mctx(ctx)->fd, cmd, &type); > if (ret < 0) > return AVERROR(errno); > diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c > index cdfd579810f2..e2d5d8c968a9 100644 > --- a/libavcodec/v4l2_m2m.c > +++ b/libavcodec/v4l2_m2m.c > @@ -422,6 +422,7 @@ int ff_v4l2_m2m_create_context(V4L2m2mPriv *priv, V4L2m2mContext **s) > priv->context->output.num_buffers = priv->num_output_buffers; > priv->context->self_ref = priv->context_ref; > priv->context->fd = -1; > + priv->context->source_change_cnt = 0; > > priv->context->frame = av_frame_alloc(); > if (!priv->context->frame) { > diff --git a/libavcodec/v4l2_m2m.h b/libavcodec/v4l2_m2m.h > index b67b21633109..72f1a579f1ca 100644 > --- a/libavcodec/v4l2_m2m.h > +++ b/libavcodec/v4l2_m2m.h > @@ -53,6 +53,7 @@ typedef struct V4L2m2mContext { > sem_t refsync; > atomic_uint refcount; > int reinit; > + int source_change_cnt; > > /* null frame/packet received */ > int draining; > diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c > index 224eb3d5e7be..60eb39deef88 100644 > --- a/libavcodec/v4l2_m2m_dec.c > +++ b/libavcodec/v4l2_m2m_dec.c > @@ -51,7 +51,7 @@ static int v4l2_try_start(AVCodecContext *avctx) > } > } > > - if (capture->streamon) > + if (capture->streamon || !s->source_change_cnt) > return 0; > > /* 2. get the capture format */ > @@ -146,28 +146,24 @@ static int v4l2_receive_frame(AVCodecContext *avctx, AVFrame *frame) > return ret; > } > > - if (s->draining) > - goto dequeue; > - > - ret = ff_v4l2_context_enqueue_packet(output, &s->buf_pkt); > - if (ret < 0 && ret != AVERROR(EAGAIN)) > - goto fail; > - > - /* if EAGAIN don't unref packet and try to enqueue in the next iteration */ > - if (ret != AVERROR(EAGAIN)) > - av_packet_unref(&s->buf_pkt); > - > if (!s->draining) { > - ret = v4l2_try_start(avctx); > - if (ret) { > - /* cant recover */ > - if (ret != AVERROR(ENOMEM)) > - ret = 0; > + ret = ff_v4l2_context_enqueue_packet(output, &s->buf_pkt); > + if (ret < 0 && ret != AVERROR(EAGAIN)) > goto fail; > - } > + > + /* if EAGAIN don't unref packet and try to enqueue in the next iteration */ > + if (ret != AVERROR(EAGAIN)) > + av_packet_unref(&s->buf_pkt); > + } > + > + ret = v4l2_try_start(avctx); > + if (ret) { > + /* can't recover */ > + if (ret != AVERROR(ENOMEM)) > + ret = 0; > + goto fail; > } > > -dequeue: > return ff_v4l2_context_dequeue_frame(capture, frame, -1); > fail: > av_packet_unref(&s->buf_pkt); > -- > 2.32.0 > I tested with a sample (RPi4) where the resolution change event is not triggered at the start, so I think the capture queue is not initialized at all following this change. Will share with you the sample.
diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c index df41e982fc56..543fc9523cf1 100644 --- a/libavcodec/v4l2_context.c +++ b/libavcodec/v4l2_context.c @@ -179,6 +179,7 @@ static int v4l2_handle_event(V4L2Context *ctx) if (evt.type != V4L2_EVENT_SOURCE_CHANGE) return 0; + s->source_change_cnt++; 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); @@ -272,7 +273,7 @@ static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout) } /* if we are draining and there are no more capture buffers queued in the driver we are done */ - if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx_to_m2mctx(ctx)->draining) { + if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx_to_m2mctx(ctx)->draining && ctx->streamon) { for (i = 0; i < ctx->num_buffers; i++) { /* capture buffer initialization happens during decode hence * detection happens at runtime @@ -542,6 +543,9 @@ int ff_v4l2_context_set_status(V4L2Context* ctx, uint32_t cmd) int type = ctx->type; int ret; + if (ctx->streamon == (cmd == VIDIOC_STREAMON)) + return 0; + ret = ioctl(ctx_to_m2mctx(ctx)->fd, cmd, &type); if (ret < 0) return AVERROR(errno); diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c index cdfd579810f2..e2d5d8c968a9 100644 --- a/libavcodec/v4l2_m2m.c +++ b/libavcodec/v4l2_m2m.c @@ -422,6 +422,7 @@ int ff_v4l2_m2m_create_context(V4L2m2mPriv *priv, V4L2m2mContext **s) priv->context->output.num_buffers = priv->num_output_buffers; priv->context->self_ref = priv->context_ref; priv->context->fd = -1; + priv->context->source_change_cnt = 0; priv->context->frame = av_frame_alloc(); if (!priv->context->frame) { diff --git a/libavcodec/v4l2_m2m.h b/libavcodec/v4l2_m2m.h index b67b21633109..72f1a579f1ca 100644 --- a/libavcodec/v4l2_m2m.h +++ b/libavcodec/v4l2_m2m.h @@ -53,6 +53,7 @@ typedef struct V4L2m2mContext { sem_t refsync; atomic_uint refcount; int reinit; + int source_change_cnt; /* null frame/packet received */ int draining; diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c index 224eb3d5e7be..60eb39deef88 100644 --- a/libavcodec/v4l2_m2m_dec.c +++ b/libavcodec/v4l2_m2m_dec.c @@ -51,7 +51,7 @@ static int v4l2_try_start(AVCodecContext *avctx) } } - if (capture->streamon) + if (capture->streamon || !s->source_change_cnt) return 0; /* 2. get the capture format */ @@ -146,28 +146,24 @@ static int v4l2_receive_frame(AVCodecContext *avctx, AVFrame *frame) return ret; } - if (s->draining) - goto dequeue; - - ret = ff_v4l2_context_enqueue_packet(output, &s->buf_pkt); - if (ret < 0 && ret != AVERROR(EAGAIN)) - goto fail; - - /* if EAGAIN don't unref packet and try to enqueue in the next iteration */ - if (ret != AVERROR(EAGAIN)) - av_packet_unref(&s->buf_pkt); - if (!s->draining) { - ret = v4l2_try_start(avctx); - if (ret) { - /* cant recover */ - if (ret != AVERROR(ENOMEM)) - ret = 0; + ret = ff_v4l2_context_enqueue_packet(output, &s->buf_pkt); + if (ret < 0 && ret != AVERROR(EAGAIN)) goto fail; - } + + /* if EAGAIN don't unref packet and try to enqueue in the next iteration */ + if (ret != AVERROR(EAGAIN)) + av_packet_unref(&s->buf_pkt); + } + + ret = v4l2_try_start(avctx); + if (ret) { + /* can't recover */ + if (ret != AVERROR(ENOMEM)) + ret = 0; + goto fail; } -dequeue: return ff_v4l2_context_dequeue_frame(capture, frame, -1); fail: av_packet_unref(&s->buf_pkt);
if client start the capture queue without waiting the source change event, there may be some timing issues. For example, in client, the sequence is: capture streamon -> source change -> capture streamoff -> capture streamon. but in driver side, the sequence may be: source change -> capture streamon -> capture streamoff -> capture streamon. Then it may led to some unforeseen and serious problems. So to avoid such timing issues, the client should setup the capture queue after the first source change event is received. Signed-off-by: Ming Qian <ming.qian@nxp.com> --- libavcodec/v4l2_context.c | 6 +++++- libavcodec/v4l2_m2m.c | 1 + libavcodec/v4l2_m2m.h | 1 + libavcodec/v4l2_m2m_dec.c | 34 +++++++++++++++------------------- 4 files changed, 22 insertions(+), 20 deletions(-)