[FFmpeg-devel] [PATCH v3 3/3] avcodec/v4l2_m2m_dec: start capture after source change event is received

[Andriy Gelman]

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 at 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.

-- 
Andriy