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

8640 diff mbox series

Fill the stride and frameSize fields of the StreamConfiguration at
configuration validation time instead of at camera configuration time.

Signed-off-by: Paul Elder <paul.elder@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(-)

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