[FFmpeg-devel,2/3] avcodec: v4l2_m2m: remove unnecessary timeout.

Submitted by Jorge Ramirez-Ortiz on Jan. 9, 2018, 5:06 p.m.

Details

Message ID 1515517573-27292-2-git-send-email-jorge.ramirez.ortiz@gmail.com
State New
Headers show

Commit Message

Jorge Ramirez-Ortiz Jan. 9, 2018, 5:06 p.m.
From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

Qualcomm's db410c/db820 Venus driver currently present in mainline
kernel has a bug which mishandles the CMD_STOP requests causing the
decoder to block while draining [1].

This patch removes the workaround that was used to prevent that
situation.

Encoding/Decoding tested on db820c.

[1] on CMD_STOP, the driver is flushing all buffers and never raising
IPIPE which ends up in blocking on poll.
---
 libavcodec/v4l2_buffers.c | 10 ++++++++--
 libavcodec/v4l2_context.c | 48 +++++++++++++++++++++++------------------------
 libavcodec/v4l2_m2m_dec.c | 14 +++++++++++---
 libavcodec/v4l2_m2m_enc.c | 14 +++++++++++---
 4 files changed, 54 insertions(+), 32 deletions(-)

Comments

Jorge Ramirez-Ortiz Jan. 9, 2018, 10:07 p.m.
> --- a/libavcodec/v4l2_m2m_dec.c
> +++ b/libavcodec/v4l2_m2m_dec.c
> @@ -131,14 +131,22 @@ static int v4l2_receive_frame(AVCodecContext *avctx, AVFrame *frame)
>       V4L2Context *const capture = &s->capture;
>       V4L2Context *const output = &s->output;
>       AVPacket avpkt = {0};
> -    int ret;
> +    int i, ret;
>   
>       ret = ff_decode_get_packet(avctx, &avpkt);
>       if (ret < 0 && ret != AVERROR_EOF)
>           return ret;
>   
> -    if (s->draining)
> -        goto dequeue;
> +    if (s->draining) {
> +        for (i = 0; i < capture->num_buffers; i++) {
> +            if (capture->buffers[i].status == V4L2BUF_IN_DRIVER)
> +                goto dequeue;
> +        }
> +
> +        /* no more buffers in the driver, we are done */
> +        capture->done = 1;
> +        return AVERROR_EOF;
> +    }
>   
>       ret = ff_v4l2_context_enqueue_packet(output, &avpkt);
>       if (ret < 0) {
> diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> index 4c9ea1f..3d7c241 100644
> --- a/libavcodec/v4l2_m2m_enc.c
> +++ b/libavcodec/v4l2_m2m_enc.c
> @@ -253,10 +253,18 @@ static int v4l2_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
>       V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
>       V4L2Context *const capture = &s->capture;
>       V4L2Context *const output = &s->output;
> -    int ret;
> +    int i, ret;
> +
> +    if (s->draining) {

um, I shouldnt be exposing this information in this file. I'll push it 
back to v4l2_context at the time of dequeue instead so the interface 
remains clean and I dont duplicate the code in the decoder
please ignore this patch


> +        for (i = 0; i < capture->num_buffers; i++) {
> +            if (capture->buffers[i].status == V4L2BUF_IN_DRIVER)
> +                goto dequeue;
> +        }
>   
> -    if (s->draining)
> -        goto dequeue;
> +        /* no more buffers in the driver, we are done */
> +        capture->done = 1;
> +        return AVERROR_EOF;
> +    }
>   
>       if (!output->streamon) {
>           ret = ff_v4l2_context_set_status(output, VIDIOC_STREAMON);

Patch hide | download patch | download mbox

diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
index 4e68f90..8e4d4d1 100644
--- a/libavcodec/v4l2_buffers.c
+++ b/libavcodec/v4l2_buffers.c
@@ -213,8 +213,14 @@  static void v4l2_free_buffer(void *opaque, uint8_t *unused)
         if (s->reinit) {
             if (!atomic_load(&s->refcount))
                 sem_post(&s->refsync);
-        } else if (avbuf->context->streamon)
-            ff_v4l2_buffer_enqueue(avbuf);
+        } else {
+            if (s->draining) {
+                /* no need to queue more buffers to the driver */
+                avbuf->status = V4L2BUF_AVAILABLE;
+            }
+            else if (avbuf->context->streamon)
+                ff_v4l2_buffer_enqueue(avbuf);
+        }
 
         av_buffer_unref(&avbuf->context_ref);
     }
diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c
index 4c16992..fb482cf 100644
--- a/libavcodec/v4l2_context.c
+++ b/libavcodec/v4l2_context.c
@@ -217,6 +217,7 @@  static int v4l2_stop_decode(V4L2Context *ctx)
 {
     struct v4l2_decoder_cmd cmd = {
         .cmd = V4L2_DEC_CMD_STOP,
+        .flags = 0,
     };
     int ret;
 
@@ -234,6 +235,7 @@  static int v4l2_stop_encode(V4L2Context *ctx)
 {
     struct v4l2_encoder_cmd cmd = {
         .cmd = V4L2_ENC_CMD_STOP,
+        .flags = 0,
     };
     int ret;
 
@@ -260,6 +262,11 @@  static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout)
 
     if (V4L2_TYPE_IS_OUTPUT(ctx->type))
         pfd.events =  POLLOUT | POLLWRNORM;
+    else {
+        /* no need to listen to requests for more input while draining */
+        if (ctx_to_m2mctx(ctx)->draining)
+            pfd.events =  POLLIN | POLLRDNORM | POLLPRI;
+    }
 
     for (;;) {
         ret = poll(&pfd, 1, timeout);
@@ -267,11 +274,6 @@  static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout)
             break;
         if (errno == EINTR)
             continue;
-
-        /* timeout is being used to indicate last valid bufer when draining */
-        if (ctx_to_m2mctx(ctx)->draining)
-            ctx->done = 1;
-
         return NULL;
     }
 
@@ -286,7 +288,7 @@  static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout)
         ret = v4l2_handle_event(ctx);
         if (ret < 0) {
             /* if re-init failed, abort */
-            ctx->done = EINVAL;
+            ctx->done = 1;
             return NULL;
         }
         if (ret) {
@@ -325,23 +327,25 @@  dequeue:
         ret = ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_DQBUF, &buf);
         if (ret) {
             if (errno != EAGAIN) {
-                ctx->done = errno;
+                ctx->done = 1;
                 if (errno != EPIPE)
                     av_log(logger(ctx), AV_LOG_DEBUG, "%s VIDIOC_DQBUF, errno (%s)\n",
                         ctx->name, av_err2str(AVERROR(errno)));
             }
-        } else {
-            avbuf = &ctx->buffers[buf.index];
-            avbuf->status = V4L2BUF_AVAILABLE;
-            avbuf->buf = buf;
-            if (V4L2_TYPE_IS_MULTIPLANAR(ctx->type)) {
-                memcpy(avbuf->planes, planes, sizeof(planes));
-                avbuf->buf.m.planes = avbuf->planes;
-            }
+            return NULL;
+        }
+
+        avbuf = &ctx->buffers[buf.index];
+        avbuf->status = V4L2BUF_AVAILABLE;
+        avbuf->buf = buf;
+        if (V4L2_TYPE_IS_MULTIPLANAR(ctx->type)) {
+            memcpy(avbuf->planes, planes, sizeof(planes));
+            avbuf->buf.m.planes = avbuf->planes;
         }
+        return avbuf;
     }
 
-    return avbuf;
+    return NULL;
 }
 
 static V4L2Buffer* v4l2_getfree_v4l2buf(V4L2Context *ctx)
@@ -552,14 +556,12 @@  int ff_v4l2_context_dequeue_frame(V4L2Context* ctx, AVFrame* frame)
 {
     V4L2Buffer* avbuf = NULL;
 
-    /* if we are draining, we are no longer inputing data, therefore enable a
-     * timeout so we can dequeue and flag the last valid buffer.
-     *
+    /*
      * blocks until:
      *  1. decoded frame available
      *  2. an input buffer is ready to be dequeued
      */
-    avbuf = v4l2_dequeue_v4l2buf(ctx, ctx_to_m2mctx(ctx)->draining ? 200 : -1);
+    avbuf = v4l2_dequeue_v4l2buf(ctx, -1);
     if (!avbuf) {
         if (ctx->done)
             return AVERROR_EOF;
@@ -574,14 +576,12 @@  int ff_v4l2_context_dequeue_packet(V4L2Context* ctx, AVPacket* pkt)
 {
     V4L2Buffer* avbuf = NULL;
 
-    /* if we are draining, we are no longer inputing data, therefore enable a
-     * timeout so we can dequeue and flag the last valid buffer.
-     *
+    /*
      * blocks until:
      *  1. encoded packet available
      *  2. an input buffer ready to be dequeued
      */
-    avbuf = v4l2_dequeue_v4l2buf(ctx, ctx_to_m2mctx(ctx)->draining ? 200 : -1);
+    avbuf = v4l2_dequeue_v4l2buf(ctx, -1);
     if (!avbuf) {
         if (ctx->done)
             return AVERROR_EOF;
diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c
index bca45be..34d1f9c 100644
--- a/libavcodec/v4l2_m2m_dec.c
+++ b/libavcodec/v4l2_m2m_dec.c
@@ -131,14 +131,22 @@  static int v4l2_receive_frame(AVCodecContext *avctx, AVFrame *frame)
     V4L2Context *const capture = &s->capture;
     V4L2Context *const output = &s->output;
     AVPacket avpkt = {0};
-    int ret;
+    int i, ret;
 
     ret = ff_decode_get_packet(avctx, &avpkt);
     if (ret < 0 && ret != AVERROR_EOF)
         return ret;
 
-    if (s->draining)
-        goto dequeue;
+    if (s->draining) {
+        for (i = 0; i < capture->num_buffers; i++) {
+            if (capture->buffers[i].status == V4L2BUF_IN_DRIVER)
+                goto dequeue;
+        }
+
+        /* no more buffers in the driver, we are done */
+        capture->done = 1;
+        return AVERROR_EOF;
+    }
 
     ret = ff_v4l2_context_enqueue_packet(output, &avpkt);
     if (ret < 0) {
diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
index 4c9ea1f..3d7c241 100644
--- a/libavcodec/v4l2_m2m_enc.c
+++ b/libavcodec/v4l2_m2m_enc.c
@@ -253,10 +253,18 @@  static int v4l2_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
     V4L2m2mContext *s = ((V4L2m2mPriv*)avctx->priv_data)->context;
     V4L2Context *const capture = &s->capture;
     V4L2Context *const output = &s->output;
-    int ret;
+    int i, ret;
+
+    if (s->draining) {
+        for (i = 0; i < capture->num_buffers; i++) {
+            if (capture->buffers[i].status == V4L2BUF_IN_DRIVER)
+                goto dequeue;
+        }
 
-    if (s->draining)
-        goto dequeue;
+        /* no more buffers in the driver, we are done */
+        capture->done = 1;
+        return AVERROR_EOF;
+    }
 
     if (!output->streamon) {
         ret = ff_v4l2_context_set_status(output, VIDIOC_STREAMON);