[RFC,v1,27/27] v4l2: v4l2_camera: Provide buffers one by one

26991 diff mbox series

Instead of processing a set of buffers at a time and cycling through
the buffers using `V4L2CameraProxy::currentBuf_`, add a new function
to query just the least recently completed buffer and use that to
service `VIDIOC_DQBUF`. This ensures that the reported index will denote
the buffer that was actually completed after the request-buffer split.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/v4l2/v4l2_camera.cpp       | 62 ++++++++++---------------
 src/v4l2/v4l2_camera.h         | 13 ++----
 src/v4l2/v4l2_camera_proxy.cpp | 84 +++++++++++++++++-----------------
 src/v4l2/v4l2_camera_proxy.h   |  2 -
 4 files changed, 69 insertions(+), 92 deletions(-)

Quoting Barnabás Pőcze (2026-06-18 21:38:44)
> Instead of processing a set of buffers at a time and cycling through
> the buffers using `V4L2CameraProxy::currentBuf_`, add a new function
> to query just the least recently completed buffer and use that to
> service `VIDIOC_DQBUF`. This ensures that the reported index will denote
> the buffer that was actually completed after the request-buffer split.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/v4l2/v4l2_camera.cpp       | 62 ++++++++++---------------
>  src/v4l2/v4l2_camera.h         | 13 ++----
>  src/v4l2/v4l2_camera_proxy.cpp | 84 +++++++++++++++++-----------------
>  src/v4l2/v4l2_camera_proxy.h   |  2 -
>  4 files changed, 69 insertions(+), 92 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 8c7de8e92b..90773e00bf 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -20,7 +20,7 @@ LOG_DECLARE_CATEGORY(V4L2Compat)
>  
>  V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
>         : camera_(camera), controls_(controls::controls), isRunning_(false),
> -         efd_(-1), bufferAvailableCount_(0)
> +         efd_(-1)
>  {
>         camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
>  }
> @@ -68,38 +68,25 @@ void V4L2Camera::unbind()
>         efd_ = -1;
>  }
>  
> -std::vector<V4L2Camera::CompletedBuffer> V4L2Camera::completedBuffers()
> -{
> -       MutexLocker lock(bufferLock_);
> -       std::vector v(std::move_iterator(completedBuffers_.begin()),
> -                     std::move_iterator(completedBuffers_.end()));
> -
> -       completedBuffers_.clear();
> -
> -       return v;
> -}
> -
>  void V4L2Camera::requestComplete(Request *request)
>  {
>         if (request->status() == Request::RequestCancelled)
>                 return;
>  
>         /* We only have one stream at the moment. */
> -       bufferLock_.lock();
> -       FrameBuffer *buffer = request->buffers().begin()->second;
> -       completedBuffers_.emplace_back(buffer->cookie(), buffer->metadata());
> -       bufferLock_.unlock();
> -
> -       uint64_t data = 1;
> -       int ret = ::write(efd_, &data, sizeof(data));
> -       if (ret != sizeof(data))
> -               LOG(V4L2Compat, Error) << "Failed to signal eventfd POLLIN";
> -
> -       request->reuse();
>         {
>                 MutexLocker locker(bufferMutex_);
> -               bufferAvailableCount_++;
> +               FrameBuffer *buffer = request->buffers().begin()->second;
> +               completedBuffers_.emplace_back(buffer->cookie(), buffer->metadata());
> +
> +               uint64_t data = 1;
> +               int ret = ::write(efd_, &data, sizeof(data));
> +               if (ret != sizeof(data))
> +                       LOG(V4L2Compat, Error) << "Failed to signal eventfd POLLIN";
> +
> +               request->reuse();
>         }
> +
>         bufferCV_.notify_all();
>  }
>  
> @@ -280,24 +267,23 @@ int V4L2Camera::qbuf(unsigned int index)
>         return 0;
>  }
>  
> -void V4L2Camera::waitForBufferAvailable()
> +std::optional<V4L2Camera::CompletedBuffer> V4L2Camera::nextBuffer(bool wait)
>  {
>         MutexLocker locker(bufferMutex_);
> -       bufferCV_.wait(locker, [&]() LIBCAMERA_TSA_REQUIRES(bufferMutex_) {
> -                              return bufferAvailableCount_ >= 1 || !isRunning_;
> -                      });
> -       if (isRunning_)
> -               bufferAvailableCount_--;
> -}
>  
> -bool V4L2Camera::isBufferAvailable()
> -{
> -       MutexLocker locker(bufferMutex_);
> -       if (bufferAvailableCount_ < 1)
> -               return false;
> +       if (wait) {
> +               bufferCV_.wait(locker, [&]() LIBCAMERA_TSA_REQUIRES(bufferMutex_) {
> +                      return !completedBuffers_.empty() || !isRunning_;
> +               });
> +       }
> +
> +       if (!isRunning_)
> +               return {};
> +
> +       auto buffer = std::move(completedBuffers_.front());
> +       completedBuffers_.pop_front();
>  
> -       bufferAvailableCount_--;
> -       return true;
> +       return buffer;
>  }
>  
>  bool V4L2Camera::isRunning()
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index 1d90eb43e4..06e37077b2 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -9,6 +9,7 @@
>  
>  #include <deque>
>  #include <memory>
> +#include <optional>
>  #include <vector>
>  
>  #include <libcamera/base/mutex.h>
> @@ -41,8 +42,6 @@ public:
>         void bind(int efd);
>         void unbind();
>  
> -       std::vector<CompletedBuffer> completedBuffers() LIBCAMERA_TSA_EXCLUDES(bufferLock_);
> -
>         int configure(libcamera::StreamConfiguration *streamConfigOut,
>                       const libcamera::Size &size,
>                       const libcamera::PixelFormat &pixelformat,
> @@ -63,14 +62,12 @@ public:
>  
>         int qbuf(unsigned int index);
>  
> -       void waitForBufferAvailable() LIBCAMERA_TSA_EXCLUDES(bufferMutex_);
> -       bool isBufferAvailable() LIBCAMERA_TSA_EXCLUDES(bufferMutex_);
> +       std::optional<CompletedBuffer> nextBuffer(bool wait) LIBCAMERA_TSA_EXCLUDES(bufferMutex_);
>  
>         bool isRunning();
>  
>  private:
> -       void requestComplete(libcamera::Request *request)
> -               LIBCAMERA_TSA_EXCLUDES(bufferLock_);
> +       void requestComplete(libcamera::Request *request);
>  
>         std::shared_ptr<libcamera::Camera> camera_;
>         std::unique_ptr<libcamera::CameraConfiguration> config_;
> @@ -79,18 +76,16 @@ private:
>  
>         bool isRunning_;
>  
> -       libcamera::Mutex bufferLock_;
>         std::unique_ptr<libcamera::FrameBufferAllocator> bufferAllocator_;
>  
>         std::vector<std::unique_ptr<libcamera::Request>> requestPool_;
>  
>         std::deque<libcamera::Request *> pendingRequests_;
>         std::deque<CompletedBuffer> completedBuffers_
> -               LIBCAMERA_TSA_GUARDED_BY(bufferLock_);
> +               LIBCAMERA_TSA_GUARDED_BY(bufferMutex_);
>  
>         int efd_;
>  
>         libcamera::Mutex bufferMutex_;
>         libcamera::ConditionVariable bufferCV_;
> -       unsigned int bufferAvailableCount_ LIBCAMERA_TSA_GUARDED_BY(bufferMutex_);
>  };
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 820a417a9c..3a34e84071 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -39,9 +39,37 @@ using namespace std::literals::chrono_literals;
>  
>  LOG_DECLARE_CATEGORY(V4L2Compat)
>  
> +namespace {
> +
> +void updateBuffer(v4l2_buffer &buf, const FrameMetadata &fmd)
> +{
> +       switch (fmd.status) {
> +       case FrameMetadata::FrameSuccess:
> +               buf.bytesused = std::accumulate(fmd.planes().begin(),
> +                                               fmd.planes().end(), 0,
> +                                               [](unsigned int total, const auto &plane) {
> +                                                       return total + plane.bytesused;
> +                                               });
> +               buf.field = V4L2_FIELD_NONE;
> +               buf.timestamp.tv_sec = fmd.timestamp / 1000000000;
> +               buf.timestamp.tv_usec = (fmd.timestamp / 1000) % 1000000;
> +               buf.sequence = fmd.sequence;
> +
> +               buf.flags |= V4L2_BUF_FLAG_DONE;
> +               break;
> +       case FrameMetadata::FrameError:
> +               buf.flags |= V4L2_BUF_FLAG_ERROR;
> +               break;
> +       default:
> +               break;
> +       }
> +}
> +
> +} /* namespace */
> +
>  V4L2CameraProxy::V4L2CameraProxy(unsigned int index,
>                                  std::shared_ptr<Camera> camera)
> -       : refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),
> +       : refcount_(0), index_(index), bufferCount_(0),
>           vcam_(std::make_unique<V4L2Camera>(camera)), owner_(nullptr)
>  {
>         querycap(camera);
> @@ -241,36 +269,6 @@ void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)
>         memset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));
>  }
>  
> -void V4L2CameraProxy::updateBuffers()
> -{
> -       std::vector<V4L2Camera::CompletedBuffer> completedBuffers = vcam_->completedBuffers();
> -       for (const V4L2Camera::CompletedBuffer &buffer : completedBuffers) {
> -               const FrameMetadata &fmd = buffer.data_;
> -               struct v4l2_buffer &buf = buffers_[buffer.index_];
> -
> -               switch (fmd.status) {
> -               case FrameMetadata::FrameSuccess:
> -                       buf.bytesused = std::accumulate(fmd.planes().begin(),
> -                                                       fmd.planes().end(), 0,
> -                                                       [](unsigned int total, const auto &plane) {
> -                                                               return total + plane.bytesused;
> -                                                       });
> -                       buf.field = V4L2_FIELD_NONE;
> -                       buf.timestamp.tv_sec = fmd.timestamp / 1000000000;
> -                       buf.timestamp.tv_usec = (fmd.timestamp / 1000) % 1000000;
> -                       buf.sequence = fmd.sequence;
> -
> -                       buf.flags |= V4L2_BUF_FLAG_DONE;
> -                       break;
> -               case FrameMetadata::FrameError:
> -                       buf.flags |= V4L2_BUF_FLAG_ERROR;
> -                       break;
> -               default:
> -                       break;
> -               }
> -       }
> -}
> -
>  int V4L2CameraProxy::vidioc_querycap(V4L2CameraFile *file, struct v4l2_capability *arg)
>  {
>         LOG(V4L2Compat, Debug)
> @@ -585,8 +583,6 @@ int V4L2CameraProxy::vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *a
>             arg->index >= bufferCount_)
>                 return -EINVAL;
>  
> -       updateBuffers();
> -
>         *arg = buffers_[arg->index];
>  
>         return 0;
> @@ -674,30 +670,34 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
>             !validateMemoryType(arg->memory))
>                 return -EINVAL;
>  
> +       std::optional<V4L2Camera::CompletedBuffer> b;
> +
>         if (!file->nonBlocking()) {
>                 lock->unlock();
> -               vcam_->waitForBufferAvailable();
> +               b = vcam_->nextBuffer(true);
>                 lock->lock();
> -       } else if (!vcam_->isBufferAvailable())
> -               return -EAGAIN;
> +       } else {
> +               b = vcam_->nextBuffer(false);
> +       }
>  
>         /*
>          * We need to check here again in case stream was turned off while we
> -        * were blocked on waitForBufferAvailable().
> +        * were blocked in nextBuffer().
>          */
>         if (!vcam_->isRunning())
>                 return -EINVAL;
>  
> -       updateBuffers();
> +       if (!b)
> +               return -EAGAIN;
> +
> +       struct v4l2_buffer &buf = buffers_[b->index_];
>  
> -       struct v4l2_buffer &buf = buffers_[currentBuf_];
> +       updateBuffer(buf, b->data_);
>  
>         buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_PREPARED);
>         buf.length = sizeimage_;
>         *arg = buf;
>  
> -       currentBuf_ = (currentBuf_ + 1) % bufferCount_;
> -
>         uint64_t data;
>         int ret = ::read(file->efd(), &data, sizeof(data));
>         if (ret != sizeof(data))
> @@ -753,8 +753,6 @@ int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)
>         if (vcam_->isRunning())
>                 return 0;
>  
> -       currentBuf_ = 0;
> -
>         return vcam_->streamOn();
>  }
>  
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index 5aa352c36f..e8f3583aa6 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -44,7 +44,6 @@ private:
>         void querycap(std::shared_ptr<libcamera::Camera> camera);
>         int tryFormat(struct v4l2_format *arg);
>         enum v4l2_priority maxPriority();
> -       void updateBuffers();
>         void freeBuffers();
>  
>         int vidioc_querycap(V4L2CameraFile *file, struct v4l2_capability *arg);
> @@ -81,7 +80,6 @@ private:
>  
>         libcamera::StreamConfiguration streamConfig_;
>         unsigned int bufferCount_;
> -       unsigned int currentBuf_;
>         unsigned int sizeimage_;
>  
>         struct v4l2_capability capabilities_;
> -- 
> 2.54.0
>