[libcamera-devel] [PATCH v3 19/22] libcamera: simple: Fill stride and frameSize at config validation

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sat, Jul 04, 2020 at 10:31:37PM +0900, Paul Elder wrote:
> Fill the stride and frameSize fields of the StreamConfiguration at
> configuration validation time instead of at camera configuration time.

You may want to explain why this is desired (same for patch 16/22 to
18/22). Just mentioning that it allows applications to get the stride
when trying a configuration without modifying the active configuration
of the camera should be enough.

> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> New in v3
> ---
>  src/libcamera/pipeline/simple/converter.cpp | 14 +++++++++++
>  src/libcamera/pipeline/simple/converter.h   |  5 ++++
>  src/libcamera/pipeline/simple/simple.cpp    | 28 +++++++++++++++++++--
>  3 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index e5e2f0f..a015e8e 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -261,4 +261,18 @@ void SimpleConverter::outputBufferReady(FrameBuffer *buffer)
>  	}
>  }
>  
> +unsigned int SimpleConverter::stride(const Size &size,
> +				     const PixelFormat &pixelFormat) const
> +{
> +	const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
> +	return info.stride(size.width, 0);

This (and frameSize) should come from tryFormat() on the V4L2 capture
device of the converter.

> +}
> +
> +unsigned int SimpleConverter::frameSize(const Size &size,
> +					const PixelFormat &pixelFormat) const
> +{
> +	const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
> +	return info.frameSize(size);
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> index ef18cf7..0696b8f 100644
> --- a/src/libcamera/pipeline/simple/converter.h
> +++ b/src/libcamera/pipeline/simple/converter.h
> @@ -46,6 +46,11 @@ public:
>  
>  	int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>  
> +	unsigned int stride(const Size &size,
> +			    const PixelFormat &pixelFormat) const;
> +	unsigned int frameSize(const Size &size,
> +			       const PixelFormat &pixelFormat) const;

You may want to turn these two in a single function as they're always
called together (and also because stride and frameSize are not
independent, as explained in the review of previous patches).

> +
>  	Signal<FrameBuffer *, FrameBuffer *> bufferReady;
>  
>  private:
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 1ec8d0f..39a29a7 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -457,6 +457,32 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  
>  	cfg.bufferCount = 3;
>  
> +	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(data_->pipe_);
> +	SimpleConverter *converter = pipe->converter();
> +
> +	/* Set the stride and frameSize. */
> +	if (!converter) {

Even if there's a converter, if may not be used for this particular
configuration. You should check

	if (!needConversion_) {

instead. You can then move the pipe and converter variables below.

> +		V4L2DeviceFormat format = {};
> +		format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> +		format.size = cfg.size;
> +
> +		int ret = data_->video_->tryFormat(&format);
> +		if (ret < 0) {
> +			const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +			cfg.stride = info.stride(cfg.size.width, 0);
> +			cfg.frameSize = info.frameSize(cfg.size);
> +			return status;

If tryFormat() fails I think you should just return an error, there's no
point in continuing if the kernel fails on us here.

> +		}
> +
> +		cfg.stride = format.planes[0].bpl;
> +		cfg.frameSize = format.planes[0].size;
> +
> +		return status;
> +	}
> +
> +	cfg.stride = converter->stride(cfg.size, cfg.pixelFormat);
> +	cfg.frameSize = converter->frameSize(cfg.size, cfg.pixelFormat);
> +
>  	return status;
>  }
>  
> @@ -557,8 +583,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  		return -EINVAL;
>  	}
>  
> -	cfg.stride = captureFormat.planes[0].bpl;
> -
>  	/* Configure the converter if required. */
>  	useConverter_ = config->needConversion();
>  

-- 
Regards,

Laurent Pinchart