[libcamera-devel] [PATCH v3 07/22] v4l2: v4l2_camera: Add validateConfiguration

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sat, Jul 04, 2020 at 10:31:25PM +0900, Paul Elder wrote:
> In V4L2CameraProxy, we need a way to validate formats and sizes, to
> implement try_fmt. Instead of manually checking it against the cached
> list of formats and sizes, add V4L2Camera::validateConfiguration that we
> can use to implement try_fmt.

I'd squash this with patch 10/22, as it's more difficult to review the
function without seeing how it's used. If the function and/or the user
where large it may make sense to split them in separate patches, but
here a single patch is manageable.

> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> New in v3
> ---
>  src/v4l2/v4l2_camera.cpp | 29 +++++++++++++++++++++++++++++
>  src/v4l2/v4l2_camera.h   |  3 +++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index ffc1230..326e1c2 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -138,6 +138,35 @@ int V4L2Camera::configure(StreamConfiguration *streamConfigOut,
>  	return 0;
>  }
>  
> +int V4L2Camera::validateConfiguration(StreamConfiguration *streamConfigOut,
> +				      const Size &size,
> +				      const PixelFormat &pixelFormat)

We usually put output parameters last, and pixel format before size
(probably because we consider that the size qualifies the pixel format).

> +{
> +	if (!streamConfigOut)
> +		return -EINVAL;

That can't happen, it's a private function, and the caller always passed
a non-null pointer.

> +
> +	std::unique_ptr<CameraConfiguration> config =
> +		camera_->generateConfiguration({ StreamRole::Viewfinder });
> +	StreamConfiguration &cfg = config->at(0);
> +	cfg.size.width = size.width;
> +	cfg.size.height = size.height;

	cfg.size = size;

> +	cfg.pixelFormat = pixelFormat;
> +	cfg.bufferCount = 1;
> +
> +	CameraConfiguration::Status validation = config->validate();
> +	if (validation == CameraConfiguration::Invalid)
> +		return -EINVAL;
> +
> +	streamConfigOut->pixelFormat = cfg.pixelFormat;
> +	streamConfigOut->size.width  = cfg.size.width;
> +	streamConfigOut->size.height = cfg.size.height;

	streamConfigOut->size = cfg.size;

> +	streamConfigOut->bufferCount = cfg.bufferCount;
> +	streamConfigOut->stride      = cfg.stride;
> +	streamConfigOut->frameSize   = cfg.frameSize;

Or just

	*streamConfigOut = cfg;

?

> +
> +	return 0;
> +}
> +
>  int V4L2Camera::allocBuffers(unsigned int count)
>  {
>  	Stream *stream = *camera_->streams().begin();
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index 515e906..15a9363 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -47,6 +47,9 @@ public:
>  	int configure(StreamConfiguration *streamConfigOut,
>  		      const Size &size, const PixelFormat &pixelformat,
>  		      unsigned int bufferCount);
> +	int validateConfiguration(StreamConfiguration *streamConfigOut,
> +				  const Size &size,
> +				  const PixelFormat &pixelformat);
>  
>  	int allocBuffers(unsigned int count);
>  	void freeBuffers();

-- 
Regards,

Laurent Pinchart