[libcamera-devel] [PATCH v2 05/16] libcamera: v4l2_videodevice: Add helper to queue all buffers

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

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

>  	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 ;-)

>  
>  	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