[libcamera-devel] [PATCH v2 05/16] libcamera: v4l2_videodevice: Add helper to queue all buffers
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Jul 14 12:46:06 CEST 2019
Hi Laurent,
On 2019-07-14 13:43:07 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> On Sun, Jul 14, 2019 at 07:33:06PM +0900, Niklas Söderlund wrote:
> > On 2019-07-13 20:23:40 +0300, Laurent Pinchart wrote:
> > > When starting the stream on a capture video device it is often needed to
> > > queue all the allocated buffers. Add a helper method to do so, and
> > > refactor the existing queueBuffer() method to make it clearer.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > src/libcamera/include/v4l2_videodevice.h | 1 +
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 ++---
> > > src/libcamera/v4l2_videodevice.cpp | 63 +++++++++++++++++++++---
> > > test/v4l2_videodevice/buffer_sharing.cpp | 8 ++-
> > > test/v4l2_videodevice/capture_async.cpp | 8 ++-
> > > 5 files changed, 69 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > > index b92df882568f..f948a41fb8c1 100644
> > > --- a/src/libcamera/include/v4l2_videodevice.h
> > > +++ b/src/libcamera/include/v4l2_videodevice.h
> > > @@ -139,6 +139,7 @@ public:
> > > int releaseBuffers();
> > >
> > > int queueBuffer(Buffer *buffer);
> > > + int queueAllBuffers(std::vector<std::unique_ptr<Buffer>> *buffers);
> >
> > Could this function return the vector instead of taking it as an
> > argument? I think that would make sens as the queueAllBuffers() is an
> > "output" while queueBuffer() is an "input".
>
> I'll do that.
>
> > > Signal<Buffer *> bufferReady;
> > >
> > > int streamOn();
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 1abd20e5fee5..50457891e288 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -131,6 +131,7 @@ public:
> > > CameraSensor *sensor_;
> > >
> > > BufferPool pool_;
> > > + std::vector<std::unique_ptr<Buffer>> buffers_;
> > > };
> > >
> > > class IPU3Stream : public Stream
> > > @@ -1430,11 +1431,9 @@ int CIO2Device::start()
> > > {
> > > int ret;
> > >
> > > - for (Buffer &buffer : pool_.buffers()) {
> > > - ret = output_->queueBuffer(&buffer);
> > > - if (ret)
> > > - return ret;
> > > - }
> > > + ret = output_->queueAllBuffers(&buffers_);
> > > + if (ret)
> > > + return ret;
> > >
> > > ret = output_->streamOn();
> > > if (ret)
> > > @@ -1445,7 +1444,9 @@ int CIO2Device::start()
> > >
> > > int CIO2Device::stop()
> > > {
> > > - return output_->streamOff();
> > > + int ret = output_->streamOff();
> > > + buffers_.clear();
> > > + return ret;
> > > }
> > >
> > > int CIO2Device::mediaBusToFormat(unsigned int code)
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 2d1e87a76c6f..af5ba803de45 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -894,8 +894,8 @@ int V4L2VideoDevice::releaseBuffers()
> > > */
> > > int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> > > {
> > > + struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
> > > struct v4l2_buffer buf = {};
> > > - struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
> > > int ret;
> > >
> > > buf.index = buffer->index();
> > > @@ -904,21 +904,20 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> > > buf.field = V4L2_FIELD_NONE;
> > >
> > > bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> > > + const std::vector<Plane> &planes = buffer->planes();
> > >
> > > if (buf.memory == V4L2_MEMORY_DMABUF) {
> > > if (multiPlanar) {
> > > - for (unsigned int p = 0;
> > > - p < buffer->planes().size();
> > > - p++)
> > > - planes[p].m.fd = buffer->planes()[p].dmabuf();
> > > + for (unsigned int p = 0; p < planes.size(); ++p)
> > > + v4l2Planes[p].m.fd = planes[p].dmabuf();
> > > } else {
> > > - buf.m.fd = buffer->planes()[0].dmabuf();
> > > + buf.m.fd = planes[0].dmabuf();
> > > }
> > > }
> > >
> > > if (multiPlanar) {
> > > - buf.length = buffer->planes().size();
> > > - buf.m.planes = planes;
> > > + buf.length = planes.size();
> > > + buf.m.planes = v4l2Planes;
> > > }
> >
> > Hum, this seems like an unrelated change which could be in its own
> > commit ;-)
>
> Didn't the commit message mention refactoring queueBuffer() ? :-)
Ohh my bad :-)
With the return type fixed for queueAllBuffers(),
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > >
> > > if (V4L2_TYPE_IS_OUTPUT(bufferType_)) {
> > > @@ -944,6 +943,54 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> > > return 0;
> > > }
> > >
> > > +/**
> > > + * \brief Queue all buffers into the video device
> > > + * \param[out] buffers A vector of queued Buffer instances
> > > + *
> > > + * When starting video capture users of the video device often need to queue
> > > + * all allocated buffers to the device. This helper method simplifies the
> > > + * implementation of the user by queuing all buffers and populating the
> > > + * \a buffers vector with Buffer instances for each queued buffer.
> > > + *
> > > + * This method is meant to be used with video capture devices internal to a
> > > + * pipeline handler, such as ISP statistics capture devices, or raw CSI-2
> > > + * receivers. For video capture devices facing applications, buffers shall
> > > + * instead be queued when requests are received, and for video output devices,
> > > + * buffers shall be queued when frames are ready to be output.
> > > + *
> > > + * The caller shall ensure that the \a buffers vector remains valid until all
> > > + * the queued buffers are dequeued, either during capture, or by stopping the
> > > + * video device.
> > > + *
> > > + * Calling this method on an output device or on a device that has buffers
> > > + * already queued is an error and will return -EINVAL.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2VideoDevice::queueAllBuffers(std::vector<std::unique_ptr<Buffer>> *buffers)
> > > +{
> > > + int ret;
> > > +
> > > + if (queuedBuffersCount_)
> > > + return -EINVAL;
> > > +
> > > + if (V4L2_TYPE_IS_OUTPUT(bufferType_))
> > > + return -EINVAL;
> > > +
> > > + buffers->clear();
> > > +
> > > + for (unsigned int i = 0; i < bufferPool_->count(); ++i) {
> > > + Buffer *buffer = new Buffer();
> > > + buffer->index_ = i;
> > > + buffers->emplace_back(buffer);
> > > + ret = queueBuffer(buffer);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /**
> > > * \brief Dequeue the next available buffer from the video device
> > > *
> > > diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp
> > > index cc724b226619..459bd238fe97 100644
> > > --- a/test/v4l2_videodevice/buffer_sharing.cpp
> > > +++ b/test/v4l2_videodevice/buffer_sharing.cpp
> > > @@ -117,11 +117,9 @@ protected:
> > > capture_->bufferReady.connect(this, &BufferSharingTest::captureBufferReady);
> > > output_->bufferReady.connect(this, &BufferSharingTest::outputBufferReady);
> > >
> > > - /* Queue all the buffers to the capture device. */
> > > - for (Buffer &buffer : pool_.buffers()) {
> > > - if (capture_->queueBuffer(&buffer))
> > > - return TestFail;
> > > - }
> > > + std::vector<std::unique_ptr<Buffer>> buffers;
> > > + if (capture_->queueAllBuffers(&buffers))
> > > + return TestFail;
> > >
> > > ret = capture_->streamOn();
> > > if (ret) {
> > > diff --git a/test/v4l2_videodevice/capture_async.cpp b/test/v4l2_videodevice/capture_async.cpp
> > > index cea4fffbf7a5..c6d6ec6b74bd 100644
> > > --- a/test/v4l2_videodevice/capture_async.cpp
> > > +++ b/test/v4l2_videodevice/capture_async.cpp
> > > @@ -46,11 +46,9 @@ protected:
> > >
> > > capture_->bufferReady.connect(this, &CaptureAsyncTest::receiveBuffer);
> > >
> > > - /* Queue all the buffers to the device. */
> > > - for (Buffer &b : pool_.buffers()) {
> > > - if (capture_->queueBuffer(&b))
> > > - return TestFail;
> > > - }
> > > + std::vector<std::unique_ptr<Buffer>> buffers;
> > > + if (capture_->queueAllBuffers(&buffers))
> > > + return TestFail;
> > >
> > > ret = capture_->streamOn();
> > > if (ret)
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list