Re: [FFmpeg-devel] [PATCH] avdevice/v4l2: Switch to wrapped AVFrames and implement strides

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Asahi Lina via ffmpeg-devel:
> V4L2 provides a line stride to the client for hardware that has
> alignment requirements. rawvideo cannot represent this, so switch to
> wrapped_avframe for raw video formats and calculate the plane strides
> manually.
> 
> This is slightly messy because the existing helper APIs expect
> dimensions and an alignment value, while v4l2 provides the stride of
> plane 0 and the subsequent plane strides are implied, so we need to
> open-code the logic to calculate the plane strides.
> 
> This makes vertical video work properly on Apple Macs with "1080p"
> cameras, which are actually square and can support resolutions like
> 1080x1920, which require stride padding to a multiple of 64 bytes.
> 
> In principle, this could be extended to support the V4L2 multiplanar
> API, though there seem to be practically no capture (not M2M) drivers
> that support this, so it's not terribly useful right now.
> 
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  libavdevice/v4l2-common.c |  68 +++++++++++------------
>  libavdevice/v4l2.c        | 138 +++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 151 insertions(+), 55 deletions(-)
> 
> diff --git a/libavdevice/v4l2-common.c b/libavdevice/v4l2-common.c
> index b5b4448a3154..944ffe3d87e1 100644
> --- a/libavdevice/v4l2-common.c
> +++ b/libavdevice/v4l2-common.c
> @@ -19,53 +19,53 @@
>  #include "v4l2-common.h"
>  
>  const struct fmt_map ff_fmt_conversion_table[] = {
> -    //ff_fmt              codec_id              v4l2_fmt
> -    { AV_PIX_FMT_YUV420P, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_YUV420  },
> -    { AV_PIX_FMT_YUV420P, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_YVU420  },
> -    { AV_PIX_FMT_YUV422P, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_YUV422P },
> -    { AV_PIX_FMT_YUYV422, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_YUYV    },
> -    { AV_PIX_FMT_UYVY422, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_UYVY    },
> -    { AV_PIX_FMT_YUV411P, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_YUV411P },
> -    { AV_PIX_FMT_YUV410P, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_YUV410  },
> -    { AV_PIX_FMT_YUV410P, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_YVU410  },
> -    { AV_PIX_FMT_RGB555LE,AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_RGB555  },
> -    { AV_PIX_FMT_RGB555BE,AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_RGB555X },
> -    { AV_PIX_FMT_RGB565LE,AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_RGB565  },
> -    { AV_PIX_FMT_RGB565BE,AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_RGB565X },
> -    { AV_PIX_FMT_BGR24,   AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_BGR24   },
> -    { AV_PIX_FMT_RGB24,   AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_RGB24   },
> +    //ff_fmt              codec_id                     v4l2_fmt
> +    { AV_PIX_FMT_YUV420P, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_YUV420  },
> +    { AV_PIX_FMT_YUV420P, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_YVU420  },
> +    { AV_PIX_FMT_YUV422P, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_YUV422P },
> +    { AV_PIX_FMT_YUYV422, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_YUYV    },
> +    { AV_PIX_FMT_UYVY422, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_UYVY    },
> +    { AV_PIX_FMT_YUV411P, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_YUV411P },
> +    { AV_PIX_FMT_YUV410P, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_YUV410  },
> +    { AV_PIX_FMT_YUV410P, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_YVU410  },
> +    { AV_PIX_FMT_RGB555LE,AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_RGB555  },
> +    { AV_PIX_FMT_RGB555BE,AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_RGB555X },
> +    { AV_PIX_FMT_RGB565LE,AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_RGB565  },
> +    { AV_PIX_FMT_RGB565BE,AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_RGB565X },
> +    { AV_PIX_FMT_BGR24,   AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_BGR24   },
> +    { AV_PIX_FMT_RGB24,   AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_RGB24   },
>  #ifdef V4L2_PIX_FMT_XBGR32
> -    { AV_PIX_FMT_BGR0,    AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_XBGR32  },
> -    { AV_PIX_FMT_0RGB,    AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_XRGB32  },
> -    { AV_PIX_FMT_BGRA,    AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_ABGR32  },
> -    { AV_PIX_FMT_ARGB,    AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_ARGB32  },
> +    { AV_PIX_FMT_BGR0,    AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_XBGR32  },
> +    { AV_PIX_FMT_0RGB,    AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_XRGB32  },
> +    { AV_PIX_FMT_BGRA,    AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_ABGR32  },
> +    { AV_PIX_FMT_ARGB,    AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_ARGB32  },
>  #endif
> -    { AV_PIX_FMT_BGR0,    AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_BGR32   },
> -    { AV_PIX_FMT_0RGB,    AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_RGB32   },
> -    { AV_PIX_FMT_GRAY8,   AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_GREY    },
> +    { AV_PIX_FMT_BGR0,    AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_BGR32   },
> +    { AV_PIX_FMT_0RGB,    AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_RGB32   },
> +    { AV_PIX_FMT_GRAY8,   AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_GREY    },
>  #ifdef V4L2_PIX_FMT_Y16
> -    { AV_PIX_FMT_GRAY16LE,AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_Y16     },
> +    { AV_PIX_FMT_GRAY16LE,AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_Y16     },
>  #endif
>  #ifdef V4L2_PIX_FMT_Z16
> -    { AV_PIX_FMT_GRAY16LE,AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_Z16     },
> +    { AV_PIX_FMT_GRAY16LE,AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_Z16     },
>  #endif
> -    { AV_PIX_FMT_NV12,    AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_NV12    },
> -    { AV_PIX_FMT_NONE,    AV_CODEC_ID_MJPEG,    V4L2_PIX_FMT_MJPEG   },
> -    { AV_PIX_FMT_NONE,    AV_CODEC_ID_MJPEG,    V4L2_PIX_FMT_JPEG    },
> +    { AV_PIX_FMT_NV12,    AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_NV12    },
> +    { AV_PIX_FMT_NONE,    AV_CODEC_ID_MJPEG,           V4L2_PIX_FMT_MJPEG   },
> +    { AV_PIX_FMT_NONE,    AV_CODEC_ID_MJPEG,           V4L2_PIX_FMT_JPEG    },
>  #ifdef V4L2_PIX_FMT_H264
> -    { AV_PIX_FMT_NONE,    AV_CODEC_ID_H264,     V4L2_PIX_FMT_H264    },
> +    { AV_PIX_FMT_NONE,    AV_CODEC_ID_H264,            V4L2_PIX_FMT_H264    },
>  #endif
>  #ifdef V4L2_PIX_FMT_MPEG4
> -    { AV_PIX_FMT_NONE,    AV_CODEC_ID_MPEG4,    V4L2_PIX_FMT_MPEG4   },
> +    { AV_PIX_FMT_NONE,    AV_CODEC_ID_MPEG4,           V4L2_PIX_FMT_MPEG4   },
>  #endif
>  #ifdef V4L2_PIX_FMT_CPIA1
> -    { AV_PIX_FMT_NONE,    AV_CODEC_ID_CPIA,     V4L2_PIX_FMT_CPIA1   },
> +    { AV_PIX_FMT_NONE,    AV_CODEC_ID_CPIA,            V4L2_PIX_FMT_CPIA1   },
>  #endif
>  #ifdef V4L2_PIX_FMT_SRGGB8
> -    { AV_PIX_FMT_BAYER_BGGR8, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_SBGGR8 },
> -    { AV_PIX_FMT_BAYER_GBRG8, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_SGBRG8 },
> -    { AV_PIX_FMT_BAYER_GRBG8, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_SGRBG8 },
> -    { AV_PIX_FMT_BAYER_RGGB8, AV_CODEC_ID_RAWVIDEO, V4L2_PIX_FMT_SRGGB8 },
> +    { AV_PIX_FMT_BAYER_BGGR8, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_SBGGR8 },
> +    { AV_PIX_FMT_BAYER_GBRG8, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_SGBRG8 },
> +    { AV_PIX_FMT_BAYER_GRBG8, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_SGRBG8 },
> +    { AV_PIX_FMT_BAYER_RGGB8, AV_CODEC_ID_WRAPPED_AVFRAME, V4L2_PIX_FMT_SRGGB8 },
>  #endif
>      { AV_PIX_FMT_NONE,    AV_CODEC_ID_NONE,     0                    },
>  };
> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> index 5e85d1a2b34e..534aa79b639e 100644
> --- a/libavdevice/v4l2.c
> +++ b/libavdevice/v4l2.c
> @@ -83,7 +83,10 @@ struct video_data {
>      AVClass *class;
>      int fd;
>      int pixelformat; /* V4L2_PIX_FMT_* */
> +    int pix_fmt;     /* AV_PIX_FMT_* */
>      int width, height;
> +    int bytesperline;
> +    int linesize[AV_NUM_DATA_POINTERS];
>      int frame_size;
>      int interlaced;
>      int top_field_first;
> @@ -240,6 +243,7 @@ static int device_init(AVFormatContext *ctx, int *width, int *height,
>          s->interlaced = 1;
>      }
>  
> +    s->bytesperline = fmt.fmt.pix.bytesperline;
>      return res;
>  }
>  
> @@ -501,9 +505,18 @@ static int convert_timestamp(AVFormatContext *ctx, int64_t *ts)
>      return 0;
>  }
>  
> +static void v4l2_free_frame(void *opaque, uint8_t *data)
> +{
> +    AVFrame *frame = (AVFrame*)data;
> +
> +    av_frame_free(&frame);

You should include libavutil/frame.h for this; don't rely on implicit
inclusions via avcodec.h.

> +}
> +
>  static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt)
>  {
>      struct video_data *s = ctx->priv_data;
> +    struct AVBufferRef *avbuf = NULL;
> +    struct AVFrame *frame = NULL;
>      struct v4l2_buffer buf = {
>          .type   = V4L2_BUF_TYPE_VIDEO_CAPTURE,
>          .memory = V4L2_MEMORY_MMAP
> @@ -560,13 +573,13 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt)
>      /* Image is at s->buff_start[buf.index] */
>      if (atomic_load(&s->buffers_queued) == FFMAX(s->buffers / 8, 1)) {
>          /* when we start getting low on queued buffers, fall back on copying data */
> -        res = av_new_packet(pkt, buf.bytesused);
> -        if (res < 0) {
> -            av_log(ctx, AV_LOG_ERROR, "Error allocating a packet.\n");
> +        avbuf = av_buffer_alloc(buf.bytesused);

1. This is missing the padding. av_new_packet() exists for a reason.

> +        if (!avbuf) {
> +            av_log(ctx, AV_LOG_ERROR, "Error allocating a buffer.\n");
>              enqueue_buffer(s, &buf);
>              return res;
>          }
> -        memcpy(pkt->data, s->buf_start[buf.index], buf.bytesused);
> +        memcpy(avbuf->data, s->buf_start[buf.index], buf.bytesused);
>  
>          res = enqueue_buffer(s, &buf);
>          if (res) {
> @@ -576,9 +589,6 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt)
>      } else {
>          struct buff_data *buf_descriptor;
>  
> -        pkt->data     = s->buf_start[buf.index];
> -        pkt->size     = buf.bytesused;
> -
>          buf_descriptor = av_malloc(sizeof(struct buff_data));
>          if (!buf_descriptor) {
>              /* Something went wrong... Since av_malloc() failed, we cannot even
> @@ -592,19 +602,65 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket *pkt)
>          buf_descriptor->index = buf.index;
>          buf_descriptor->s     = s;
>  
> -        pkt->buf = av_buffer_create(pkt->data, pkt->size, mmap_release_buffer,
> -                                    buf_descriptor, 0);
> -        if (!pkt->buf) {
> +        avbuf = av_buffer_create(s->buf_start[buf.index], buf.bytesused,
> +                                 mmap_release_buffer, buf_descriptor, 0);
> +        if (!avbuf) {
>              av_log(ctx, AV_LOG_ERROR, "Failed to create a buffer\n");
>              enqueue_buffer(s, &buf);
>              av_freep(&buf_descriptor);
>              return AVERROR(ENOMEM);
>          }
>      }
> +
> +    if (ctx->video_codec_id == AV_CODEC_ID_WRAPPED_AVFRAME) {
> +        frame = av_frame_alloc();
> +
> +        if (!frame) {
> +            av_log(ctx, AV_LOG_ERROR, "Failed to create an AVFrame\n");
> +            goto err_free;
> +        }
> +
> +        frame->buf[0] = avbuf;

2. You are moving ownership of avbuf to the frame here and therefore the
av_frame_unref() will free avbuf in the err_free codepath on error, but
it has already been freed in av_buffer_unref().

> +
> +        memcpy(frame->linesize, s->linesize, sizeof(s->linesize));
> +        res = av_image_fill_pointers(frame->data, s->pix_fmt, s->height,
> +                                     avbuf->data, frame->linesize);
> +        if (res < 0) {
> +            av_log(ctx, AV_LOG_ERROR, "Failed to compute data pointers\n");
> +            goto err_free;
> +        }
> +
> +        frame->format  = s->pix_fmt;
> +        frame->width   = s->width;
> +        frame->height  = s->height;
> +
> +        pkt->buf = av_buffer_create((uint8_t*)frame, sizeof(*frame),
> +                                    &v4l2_free_frame, ctx, 0);

There is no need to use an opaque here at all as v4l2_free_frame doesn't
use it at all; in fact, this buffer might outlive ctx, which is all the
more reason not to use ctx as opaque.

> +        if (!pkt->buf) {
> +            av_log(ctx, AV_LOG_ERROR, "Failed to create an AVBuffer\n");
> +            goto err_free;
> +        }
> +
> +        pkt->data   = (uint8_t*)frame;
> +        pkt->size   = sizeof(*frame);
> +        pkt->flags |= AV_PKT_FLAG_TRUSTED;
> +        frame = NULL;
> +    } else {
> +        pkt->buf      = avbuf;
> +        pkt->data     = avbuf->data;
> +        pkt->size     = buf.bytesused;
> +        avbuf = NULL;
> +    }
> +
>      pkt->pts = buf_ts.tv_sec * INT64_C(1000000) + buf_ts.tv_usec;
>      convert_timestamp(ctx, &pkt->pts);
>  
>      return pkt->size;
> +
> +err_free:
> +    av_buffer_unref(&avbuf);
> +    av_frame_unref(frame);

3. This may call av_frame_unref(NULL) in case the AVFrame could not be
allocated, which currently works, but is not guaranteed to do so (I
consider av_frame_unref(NULL) a programmer error that should not exist
and we should just let it crash). It also leaks the AVFrame in case
av_image_fill_pointers() or av_buffer_create() fails.

To fix 2. and 3., you can proceed as follows: Call
av_image_fill_pointers() before allocating the frame (store the data
pointers on the stack), then allocate the frame, then wrap it into a
buffer. If wrapping it in a buffer fails, you free the frame. You do not
free it on all other error paths; actually, you should make the AVFrame*
local to the block where you allocate it.

(This is not to say that I actually like the whole wrapped avframe hack.)

- Andreas

_______________________________________________
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".