[vlc-commits] [Git][videolan/vlc][master] 12 commits: v4l2: remove constant parameter

Felix Paul Kühne (@fkuehne) gitlab at videolan.org
Tue May 10 07:51:47 UTC 2022



Felix Paul Kühne pushed to branch master at VideoLAN / VLC


Commits:
58303ae1 by Rémi Denis-Courmont at 2022-05-08T15:38:04+03:00
v4l2: remove constant parameter

- - - - -
b8973cf8 by Rémi Denis-Courmont at 2022-05-08T16:32:33+03:00
v4l2: allocate buffer array separately

The array may be reallocated later on, so it ought to be allocated
separately from its container.

- - - - -
cbc4d8c1 by Rémi Denis-Courmont at 2022-05-08T16:32:34+03:00
v4l2: count pool references

In the corner case that the very first buffer allocation fails, the
(empty) pool was never freed, since the buffer release callback was not
invoked even once.

This adds a proper reference count with one reference for the capture
process and one reference for each allocate buffer.

- - - - -
a29b3a67 by Rémi Denis-Courmont at 2022-05-08T16:32:34+03:00
v4l2: destroy buffer directly at end

- - - - -
9aa6eef5 by Rémi Denis-Courmont at 2022-05-08T17:20:14+03:00
v4l2: track in-flight flag independently

For each buffer, track its in-flight flag independently of the other
buffers. There are no needs to fight for a single atomic bit-mask.

This flag is purely temporary and solely for clean bisection. It is
removed later on.

- - - - -
b319d719 by Rémi Denis-Courmont at 2022-05-08T17:20:46+03:00
v4l2: count unused buffers directly

As of the previous changeset, we no longer need a detailled bit-mask
with one distinct bit for each buffer.

- - - - -
613dad7b by Rémi Denis-Courmont at 2022-05-08T17:20:46+03:00
v4l2: fix ToCToU race at exit

Keep the pool lock while requeuing a buffer, so streaming cannot be
stopped in the interval.

- - - - -
f0eaf38c by Rémi Denis-Courmont at 2022-05-08T17:20:46+03:00
v4l2: store buffer index in private data

- - - - -
9e811650 by Rémi Denis-Courmont at 2022-05-08T17:20:46+03:00
v4l2: allocate buffer objects separately

To support dynamic buffer allocation later on, buffers need to be
allocated separately. Otherwise adding a buffer would potentially
change the address of all existing buffers, including in-flight ones.

- - - - -
03bfb7c3 by Rémi Denis-Courmont at 2022-05-08T17:20:46+03:00
v4l2: remove the inflight flag

Now that buffers are tracked by pointers rather than structures, the
pointer itself can be used as a flag, with NULL denoting that the
buffer is in flight.

Memory synchronisation becomes implicit, with (re)queuing as the
release side and dequeuing as the acquire side.

- - - - -
f7f7ce71 by Rémi Denis-Courmont at 2022-05-08T17:23:51+03:00
v4l2: factor out buffer allocation

- - - - -
9e4a5e2f by Rémi Denis-Courmont at 2022-05-08T17:42:42+03:00
v4l2: drop user pointer mode

User pointer allows to read the data into a memory allocation made by
the application. In the context of VLC, this serves no purpose since
memory mappings from the V4L2 device node work just as well as anonymous
memory mappings.

Furthermore most V4L2 drivers do not even support user pointer mode, so
this was a useless and untested code path.

- - - - -


4 changed files:

- modules/access/v4l2/access.c
- modules/access/v4l2/buffers.c
- modules/access/v4l2/demux.c
- modules/access/v4l2/v4l2.h


Changes:

=====================================
modules/access/v4l2/access.c
=====================================
@@ -151,7 +151,7 @@ static int InitVideo(stream_t *access, int fd, uint32_t caps)
     /* Init I/O method */
     if (caps & V4L2_CAP_STREAMING)
     {
-        sys->pool = StartMmap (VLC_OBJECT(access), fd, 16);
+        sys->pool = StartMmap (VLC_OBJECT(access), fd);
         if (sys->pool == NULL)
             return -1;
         access->pf_block = MMapBlock;


=====================================
modules/access/v4l2/buffers.c
=====================================
@@ -56,45 +56,94 @@ vlc_tick_t GetBufferPTS(const struct v4l2_buffer *buf)
     return pts;
 }
 
+static void DestroyBuffer(struct vlc_v4l2_buffers *pool,
+                          struct vlc_v4l2_buffer *buf)
+{
+    block_t *block = &buf->block;
+
+    v4l2_munmap(block->p_start, block->i_size);
+    free(buf);
+
+    if (vlc_atomic_rc_dec(&pool->refs)) {
+        free(pool->bufs);
+        free(pool);
+    }
+}
+
 static void ReleaseBuffer(block_t *block)
 {
     struct vlc_v4l2_buffer *buf = container_of(block, struct vlc_v4l2_buffer,
                                                block);
     struct vlc_v4l2_buffers *pool = buf->pool;
-    uint32_t index = buf - pool->bufs;
     struct v4l2_buffer buf_req = {
         .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
         .memory = V4L2_MEMORY_MMAP,
-        .index = index,
+        .index = buf->index,
     };
-    uint32_t mask;
     int fd;
 
+    assert(buf->index < pool->count);
+    assert(pool->bufs[buf->index] == NULL);
+
     vlc_mutex_lock(&pool->lock);
-    mask = atomic_fetch_and_explicit(&pool->inflight, ~(1U << index),
-                                     memory_order_relaxed);
     fd = pool->fd;
-    vlc_mutex_unlock(&pool->lock);
-    assert(mask & (1U << index));
-
-    assert(mask & (1U << index));
 
     if (likely(fd >= 0)) {
-        /* Requeue the freed buffer */
+        pool->bufs[buf->index] = buf;
         v4l2_ioctl(pool->fd, VIDIOC_QBUF, &buf_req);
-        return;
+        atomic_fetch_add(&pool->unused, 1);
     }
+    vlc_mutex_unlock(&pool->lock);
 
-    v4l2_munmap(block->p_start, block->i_size);
-
-    if (vlc_popcount(mask) == 1) /* last active buffer? */
-        free(pool);
+    if (unlikely(fd < 0))
+        DestroyBuffer(pool, buf);
 }
 
 static const struct vlc_block_callbacks vlc_v4l2_buffer_cbs = {
     ReleaseBuffer,
 };
 
+static struct vlc_v4l2_buffer *AllocateBuffer(struct vlc_v4l2_buffers *pool,
+                                              uint32_t index)
+{
+    struct v4l2_buffer buf_req = {
+        .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+        .memory = V4L2_MEMORY_MMAP,
+        .index = index,
+    };
+    int fd = pool->fd;
+
+    if (v4l2_ioctl(fd, VIDIOC_QUERYBUF, &buf_req) < 0)
+        return NULL;
+
+    struct vlc_v4l2_buffer *buf = malloc(sizeof (*buf));
+    if (unlikely(buf == NULL))
+        return NULL;
+
+    void *base = v4l2_mmap(NULL, buf_req.length, PROT_READ | PROT_WRITE,
+                           MAP_SHARED, fd, buf_req.m.offset);
+    if (base == MAP_FAILED) {
+        free(buf);
+        return NULL;
+    }
+
+    block_Init(&buf->block, &vlc_v4l2_buffer_cbs, base, buf_req.length);
+    buf->pool = pool;
+    buf->index = index;
+    vlc_atomic_rc_inc(&pool->refs);
+
+    assert(buf->index < pool->count);
+    assert(pool->bufs[index] == NULL);
+    pool->bufs[index] = buf;
+
+    if (v4l2_ioctl(fd, VIDIOC_QBUF, &buf_req) < 0) {
+        DestroyBuffer(pool, buf);
+        buf = NULL;
+    }
+
+    return buf;
+}
+
 block_t *GrabVideo(vlc_object_t *demux, struct vlc_v4l2_buffers *restrict pool)
 {
     int fd = pool->fd;
@@ -102,7 +151,6 @@ block_t *GrabVideo(vlc_object_t *demux, struct vlc_v4l2_buffers *restrict pool)
         .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
         .memory = V4L2_MEMORY_MMAP,
     };
-    uint32_t mask;
 
     /* Wait for next frame */
     if (v4l2_ioctl(fd, VIDIOC_DQBUF, &buf_req) < 0)
@@ -121,10 +169,13 @@ block_t *GrabVideo(vlc_object_t *demux, struct vlc_v4l2_buffers *restrict pool)
     }
 
     assert(buf_req.index < pool->count);
-    mask = atomic_fetch_or_explicit(&pool->inflight, 1U << buf_req.index,
-                                    memory_order_relaxed);
 
-    struct vlc_v4l2_buffer *buf = pool->bufs + buf_req.index;
+    struct vlc_v4l2_buffer *const buf = pool->bufs[buf_req.index];
+
+    assert(buf != NULL);
+    assert(buf->index == buf_req.index);
+    pool->bufs[buf_req.index] = NULL;
+
     block_t *block = &buf->block;
     /* Reinitialise the buffer */
     block->p_buffer = block->p_start;
@@ -132,7 +183,7 @@ block_t *GrabVideo(vlc_object_t *demux, struct vlc_v4l2_buffers *restrict pool)
     block->i_buffer = buf_req.bytesused;
     block->p_next = NULL;
 
-    if ((size_t)vlc_popcount(mask) == pool->count - 1) {
+    if (atomic_fetch_sub(&pool->unused, 1) <= 2) {
         /* Running out of buffers! Memory copy forced. */
         block = block_Duplicate(block);
         block_Release(&buf->block);
@@ -144,14 +195,13 @@ block_t *GrabVideo(vlc_object_t *demux, struct vlc_v4l2_buffers *restrict pool)
 
 /**
  * Allocates memory-mapped buffers, queues them and start streaming.
- * @param n requested buffers count
  * @return array of allocated buffers (use free()), or NULL on error.
  */
-struct vlc_v4l2_buffers *StartMmap(vlc_object_t *obj, int fd, unsigned int n)
+struct vlc_v4l2_buffers *StartMmap(vlc_object_t *obj, int fd)
 {
     struct vlc_v4l2_buffers *pool;
     struct v4l2_requestbuffers req = {
-        .count = n,
+        .count = 16,
         .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
         .memory = V4L2_MEMORY_MMAP,
     };
@@ -168,53 +218,37 @@ struct vlc_v4l2_buffers *StartMmap(vlc_object_t *obj, int fd, unsigned int n)
         return NULL;
     }
 
-    pool = malloc(sizeof (*pool) + req.count * sizeof (pool->bufs[0]));
+    pool = malloc(sizeof (*pool));
     if (unlikely(pool == NULL))
         return NULL;
 
+    pool->bufs = calloc(req.count, sizeof (pool->bufs[0]));
+    if (unlikely(pool->bufs == NULL)) {
+        free(pool);
+        return NULL;
+    }
+
+    pool->count = req.count;
+    for (size_t i = 0; i < req.count; i++)
+        pool->bufs[i] = NULL;
+
     pool->fd = fd;
-    pool->inflight = 0;
-    pool->count = 0;
+    vlc_atomic_rc_init(&pool->refs);
     vlc_mutex_init(&pool->lock);
 
-    while (pool->count < req.count)
+    for (uint32_t index = 0; index < req.count; index++)
     {
-        struct vlc_v4l2_buffer *const buf = pool->bufs + pool->count;
-        struct v4l2_buffer buf_req = {
-            .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
-            .memory = V4L2_MEMORY_MMAP,
-            .index = pool->count,
-        };
-
-        if (v4l2_ioctl(fd, VIDIOC_QUERYBUF, &buf_req) < 0)
-        {
-            msg_Err(obj, "cannot query buffer %zu: %s", pool->count,
-                    vlc_strerror_c(errno));
-            goto error;
-        }
+        struct vlc_v4l2_buffer *buf = AllocateBuffer(pool, index);
 
-        void *base = v4l2_mmap(NULL, buf_req.length, PROT_READ | PROT_WRITE,
-                               MAP_SHARED, fd, buf_req.m.offset);
-        if (base == MAP_FAILED)
-        {
-            msg_Err(obj, "cannot map buffer %"PRIu32": %s", buf_req.index,
+        if (unlikely(buf == NULL)) {
+            msg_Err(obj, "cannot allocate buffer %"PRIu32": %s", index,
                     vlc_strerror_c(errno));
             goto error;
         }
-
-        block_Init(&buf->block, &vlc_v4l2_buffer_cbs, base, buf_req.length);
-        buf->pool = pool;
-        pool->count++;
-
-        /* Some drivers refuse to queue buffers before they are mapped. Bug? */
-        if (v4l2_ioctl(fd, VIDIOC_QBUF, &buf_req) < 0)
-        {
-            msg_Err(obj, "cannot queue buffer %"PRIu32": %s", buf_req.index,
-                     vlc_strerror_c(errno));
-            goto error;
-        }
     }
 
+    atomic_init(&pool->unused, pool->count);
+
     enum v4l2_buf_type type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
     if (v4l2_ioctl(fd, VIDIOC_STREAMON, &type) < 0)
     {
@@ -231,48 +265,24 @@ void StopMmap(struct vlc_v4l2_buffers *pool)
 {
     enum v4l2_buf_type type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
     const size_t count = pool->count;
-    uint32_t unused;
-
-    /* STREAMOFF implicitly dequeues all buffers */
-    v4l2_ioctl(pool->fd, VIDIOC_STREAMOFF, &type);
+    int fd = pool->fd;
 
     vlc_mutex_lock(&pool->lock);
     pool->fd = -1;
-    unused = (~atomic_load_explicit(&pool->inflight, memory_order_relaxed))
-             & ((UINT64_C(1) << count) - 1);
-    atomic_fetch_or_explicit(&pool->inflight, unused, memory_order_relaxed);
     vlc_mutex_unlock(&pool->lock);
 
-    for (size_t i = 0; i < count; i++)
-        if (unused & (1u << i))
-            block_Release(&pool->bufs[i].block);
-    /* Pool is freed whence all buffers are released (possibly here) */
-}
+    /* STREAMOFF implicitly dequeues all buffers */
+    v4l2_ioctl(fd, VIDIOC_STREAMOFF, &type);
 
-/**
- * Allocates user pointer buffers, and start streaming.
- */
-int StartUserPtr(vlc_object_t *obj, int fd)
-{
-    struct v4l2_requestbuffers reqbuf = {
-        .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
-        .memory = V4L2_MEMORY_USERPTR,
-        .count = 2,
-    };
+    for (size_t i = 0; i < count; i++) {
+        struct vlc_v4l2_buffer *const buf = pool->bufs[i];
 
-    if (v4l2_ioctl(fd, VIDIOC_REQBUFS, &reqbuf) < 0)
-    {
-        if (errno != EINVAL)
-            msg_Err(obj, "cannot reserve user buffers: %s",
-                    vlc_strerror_c(errno));
-        else
-            msg_Dbg(obj, "user buffers not supported");
-        return -1;
+        if (buf != NULL)
+            DestroyBuffer(pool, buf);
     }
-    if (v4l2_ioctl(fd, VIDIOC_STREAMON, &reqbuf.type) < 0)
-    {
-        msg_Err(obj, "cannot start streaming: %s", vlc_strerror_c(errno));
-        return -1;
+
+    if (vlc_atomic_rc_dec(&pool->refs)) {
+        free(pool->bufs);
+        free(pool);
     }
-    return 0;
 }


=====================================
modules/access/v4l2/demux.c
=====================================
@@ -31,10 +31,6 @@
 #include <errno.h>
 #include <assert.h>
 #include <sys/ioctl.h>
-#include <sys/mman.h>
-#ifndef MAP_ANONYMOUS
-# define MAP_ANONYMOUS MAP_ANON
-#endif
 #include <poll.h>
 
 #include <vlc_common.h>
@@ -61,93 +57,6 @@ typedef struct
 #endif
 } demux_sys_t;
 
-/** Allocates and queue a user buffer using mmap(). */
-static block_t *UserPtrQueue(vlc_object_t *obj, int fd, size_t length)
-{
-    void *ptr = mmap(NULL, length, PROT_READ | PROT_WRITE,
-                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
-    if (ptr == MAP_FAILED)
-    {
-        msg_Err(obj, "cannot allocate %zu-bytes buffer: %s", length,
-                vlc_strerror_c(errno));
-        return NULL;
-    }
-
-    block_t *block = block_mmap_Alloc(ptr, length);
-    if (unlikely(block == NULL))
-    {
-        munmap(ptr, length);
-        return NULL;
-    }
-
-    struct v4l2_buffer buf = {
-        .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
-        .memory = V4L2_MEMORY_USERPTR,
-        .m = {
-            .userptr = (uintptr_t)ptr,
-        },
-        .length = length,
-    };
-
-    if (v4l2_ioctl(fd, VIDIOC_QBUF, &buf) < 0)
-    {
-        msg_Err(obj, "cannot queue buffer: %s", vlc_strerror_c(errno));
-        block_Release(block);
-        return NULL;
-    }
-    return block;
-}
-
-static void *UserPtrThread(void *data)
-{
-    demux_t *demux = data;
-    demux_sys_t *sys = demux->p_sys;
-    int fd = sys->fd;
-    struct pollfd ufd[2];
-    nfds_t numfds = 1;
-
-    ufd[0].fd = fd;
-    ufd[0].events = POLLIN;
-
-    int canc = vlc_savecancel();
-    for (;;)
-    {
-        struct v4l2_buffer buf = {
-            .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
-            .memory = V4L2_MEMORY_USERPTR,
-        };
-        block_t *block = UserPtrQueue(VLC_OBJECT(demux), fd, sys->blocksize);
-        if (block == NULL)
-            break;
-
-        /* Wait for data */
-        vlc_restorecancel(canc);
-        block_cleanup_push(block);
-        while (poll(ufd, numfds, -1) == -1)
-           if (errno != EINTR)
-               msg_Err(demux, "poll error: %s", vlc_strerror_c(errno));
-        vlc_cleanup_pop();
-        canc = vlc_savecancel();
-
-        if (v4l2_ioctl(fd, VIDIOC_DQBUF, &buf) < 0)
-        {
-            msg_Err(demux, "cannot dequeue buffer: %s",
-                    vlc_strerror_c(errno));
-            block_Release(block);
-            continue;
-        }
-
-        assert(block->p_buffer == (void *)buf.m.userptr);
-        block->i_buffer = buf.length;
-        block->i_pts = block->i_dts = GetBufferPTS(&buf);
-        block->i_flags |= sys->block_flags;
-        es_out_SetPCR(demux->out, block->i_pts);
-        es_out_Send(demux->out, sys->es, block);
-    }
-    vlc_restorecancel(canc); /* <- hmm, this is purely cosmetic */
-    return NULL;
-}
-
 static void *MmapThread(void *data)
 {
     demux_t *demux = data;
@@ -315,28 +224,12 @@ static int InitVideo (demux_t *demux, int fd, uint32_t caps)
     void *(*entry) (void *);
     if (caps & V4L2_CAP_STREAMING)
     {
-        if (StartUserPtr(VLC_OBJECT(demux), fd) == 0)
-        {
-            /* In principles, mmap() will pad the length to a multiple of the
-             * page size, so there is no need to care. Nevertheless with the
-             * page size, block->i_size can be set optimally. */
-            const long pagemask = sysconf (_SC_PAGE_SIZE) - 1;
-
-            sys->pool = NULL;
-            sys->blocksize = (sys->blocksize + pagemask) & ~pagemask;
-            entry = UserPtrThread;
-            msg_Dbg (demux, "streaming with %"PRIu32"-bytes user buffers",
-                     sys->blocksize);
-        }
-        else /* fall back to memory map */
-        {
-            sys->pool = StartMmap(VLC_OBJECT(demux), fd, 16);
-            if (sys->pool == NULL)
-                return -1;
-            entry = MmapThread;
-            msg_Dbg(demux, "streaming with %zu memory-mapped buffers",
-                    sys->pool->count);
-        }
+        sys->pool = StartMmap(VLC_OBJECT(demux), fd);
+        if (sys->pool == NULL)
+            return -1;
+        entry = MmapThread;
+        msg_Dbg(demux, "streaming with %zu memory-mapped buffers",
+                sys->pool->count);
     }
     else if (caps & V4L2_CAP_READWRITE)
     {


=====================================
modules/access/v4l2/v4l2.h
=====================================
@@ -37,19 +37,23 @@ extern int (*v4l2_munmap) (void *, size_t);
 
 typedef struct vlc_v4l2_ctrl vlc_v4l2_ctrl_t;
 
+#include <vlc_atomic.h>
 #include <vlc_block.h>
 
 struct vlc_v4l2_buffer {
     block_t block;
     struct vlc_v4l2_buffers *pool;
+    uint32_t index;
 };
 
 struct vlc_v4l2_buffers {
+    size_t count;
+    struct vlc_v4l2_buffer **bufs;
+
     int fd;
-    _Atomic uint32_t inflight;
+    vlc_atomic_rc_t refs;
+    _Atomic size_t unused;
     vlc_mutex_t lock;
-    size_t count;
-    struct vlc_v4l2_buffer bufs[];
 };
 
 /* v4l2.c */
@@ -62,8 +66,7 @@ int SetupTuner (vlc_object_t *, int fd, uint32_t);
 int SetupVideo(vlc_object_t *, int fd, uint32_t,
                es_format_t *, uint32_t *, uint32_t *);
 
-int StartUserPtr (vlc_object_t *, int);
-struct vlc_v4l2_buffers *StartMmap(vlc_object_t *, int, unsigned int);
+struct vlc_v4l2_buffers *StartMmap(vlc_object_t *, int);
 void StopMmap(struct vlc_v4l2_buffers *);
 
 vlc_tick_t GetBufferPTS (const struct v4l2_buffer *);



View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/32819382024f80a220f287915f979c16a58f0f3d...9e4a5e2f16fb91da1b93cac4a3f27c4d5667ff56

-- 
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/32819382024f80a220f287915f979c16a58f0f3d...9e4a5e2f16fb91da1b93cac4a3f27c4d5667ff56
You're receiving this email because of your account on code.videolan.org.


VideoLAN code repository instance


More information about the vlc-commits mailing list