[libcamera-devel,17/20] libcamera: pipeline: simple: Hardcode the number of internal buffers

11096 diff mbox series

The number of internal buffers, used between the capture device and the
converter, doesn't need to depend on the number of buffers allocated for
the output stream of the pipeline. Hardcode it to a fixed value.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Hi Laurent,

On Mon, Feb 01, 2021 at 12:46:59AM +0200, Laurent Pinchart wrote:
> The number of internal buffers, used between the capture device and the
> converter, doesn't need to depend on the number of buffers allocated for
> the output stream of the pipeline. Hardcode it to a fixed value.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 9dffe64ee870..c987e1a0d9cb 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -228,6 +228,8 @@ protected:
>  	int queueRequestDevice(Camera *camera, Request *request) override;
>  
>  private:
> +	static constexpr unsigned int kNumInternalBuffers = 3;
> +
>  	SimpleCameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<SimpleCameraData *>(
> @@ -699,7 +701,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  		inputCfg.pixelFormat = pipeConfig->captureFormat;
>  		inputCfg.size = pipeConfig->captureSize;
>  		inputCfg.stride = captureFormat.planes[0].bpl;
> -		inputCfg.bufferCount = cfg.bufferCount;
> +		inputCfg.bufferCount = kNumInternalBuffers;
>  
>  		ret = converter_->configure(inputCfg, { cfg });
>  		if (ret < 0) {
> @@ -736,13 +738,20 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *c
>  {
>  	SimpleCameraData *data = cameraData(camera);
>  	V4L2VideoDevice *video = data->video_;
> -	unsigned int count = data->streams_[0].configuration().bufferCount;
>  	int ret;
>  
> -	if (data->useConverter_)
> -		ret = video->allocateBuffers(count, &data->converterBuffers_);
> -	else
> -		ret = video->importBuffers(count);
> +	if (data->useConverter_) {
> +		/*
> +		 * When using the converter allocate a fixed number of internal
> +		 * buffers.
> +		 */
> +		ret = video->allocateBuffers(kNumInternalBuffers,
> +					     &data->converterBuffers_);
> +	} else {
> +		/* Otherwise, prepare for using buffers from the only stream. */
> +		Stream *stream = &data->streams_[0];
> +		ret = video->importBuffers(stream->configuration().bufferCount);
> +	}
>  	if (ret < 0)
>  		return ret;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

On 31/01/2021 22:46, Laurent Pinchart wrote:
> The number of internal buffers, used between the capture device and the
> converter, doesn't need to depend on the number of buffers allocated for
> the output stream of the pipeline. Hardcode it to a fixed value.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 9dffe64ee870..c987e1a0d9cb 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -228,6 +228,8 @@ protected:
>  	int queueRequestDevice(Camera *camera, Request *request) override;
>  
>  private:
> +	static constexpr unsigned int kNumInternalBuffers = 3;

Will it be obvious that these are 'converter' buffers?

> +
>  	SimpleCameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<SimpleCameraData *>(
> @@ -699,7 +701,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  		inputCfg.pixelFormat = pipeConfig->captureFormat;
>  		inputCfg.size = pipeConfig->captureSize;
>  		inputCfg.stride = captureFormat.planes[0].bpl;
> -		inputCfg.bufferCount = cfg.bufferCount;
> +		inputCfg.bufferCount = kNumInternalBuffers;
>  
>  		ret = converter_->configure(inputCfg, { cfg });
>  		if (ret < 0) {
> @@ -736,13 +738,20 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *c
>  {
>  	SimpleCameraData *data = cameraData(camera);
>  	V4L2VideoDevice *video = data->video_;
> -	unsigned int count = data->streams_[0].configuration().bufferCount;
>  	int ret;
>  
> -	if (data->useConverter_)
> -		ret = video->allocateBuffers(count, &data->converterBuffers_);
> -	else
> -		ret = video->importBuffers(count);
> +	if (data->useConverter_) {
> +		/*
> +		 * When using the converter allocate a fixed number of internal
> +		 * buffers.
> +		 */
> +		ret = video->allocateBuffers(kNumInternalBuffers,
> +					     &data->converterBuffers_);

I think that means probably yes ;-)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +	} else {
> +		/* Otherwise, prepare for using buffers from the only stream. */
> +		Stream *stream = &data->streams_[0];
> +		ret = video->importBuffers(stream->configuration().bufferCount);
> +	}
>  	if (ret < 0)
>  		return ret;
>  
>