[libcamera-devel] [PATCH 17/17] libcamera: pipeline: uvcvideo: Add format information and validation

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, May 27, 2019 at 02:15:43AM +0200, Niklas Söderlund wrote:
> Extend the uvcvideo pipeline with format information and validation. The
> format information is gathered by enumerating the v4l2 device. This
> enumeration approach is valid for UVC as it has a static and simple
> media graph.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/uvcvideo.cpp | 46 +++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 45260f34c8f5daba..ce9f281891a8bc5e 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -95,18 +95,42 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
>  	}
>  
>  	StreamConfiguration &cfg = config_[0];
> +	const StreamFormats &formats = cfg.formats();
>  

You can remove this blank line.

> -	/* \todo: Validate the configuration against the device capabilities. */
>  	const unsigned int pixelFormat = cfg.pixelFormat;
>  	const Size size = cfg.size;
>  
> -	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> -	cfg.size = { 640, 480 };
> +	bool found = false;
> +	for (unsigned int pixfmt : formats.pixelformats()) {
> +		if (pixfmt == pixelFormat) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (!found)
> +		cfg.pixelFormat = formats.pixelformats().front();

	auto iter = std::find(formats.begin(), formats.end(), pixelFormat);
	if (iter == formats.end()) {
		cfg.pixelFormat = formats.pixelformats().front();
		LOG(UVC, Debug)
			<< "Adjusting pixel format from " << pixelFormat
			<< " to " << cfg.pixelFormat;
		status = Adjusted;
	}

>  
> -	if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> +	if (cfg.pixelFormat != pixelFormat) {
>  		LOG(UVC, Debug)
> -			<< "Adjusting configuration from " << cfg.toString()
> -			<< " to " << cfg.size.toString() << "-YUYV";
> +			<< "Adjusting pixel format from " << pixelFormat
> +			<< " to " << cfg.pixelFormat;
> +		status = Adjusted;
> +	}
> +
> +	const std::vector<Size> &formatSizes = formats.sizes(cfg.pixelFormat);
> +	cfg.size = formatSizes.front();
> +	for (const Size &formatsSize : formatSizes) {
> +		if (formatsSize <= size)
> +			cfg.size = formatsSize;
> +
> +		if (formatsSize == size)

As the sizes are sorted, should this be >= ?

I really like this patch. Once the series stabilises, I think we should
do the same for all the other pipeline handlers, and always provide
stream formats when constructing the configuration. The stream
configuration constructor that takes no format should then be removed.

> +			break;
> +	}
> +
> +	if (cfg.size != size) {
> +		LOG(UVC, Debug)
> +			<< "Adjusting size from " << size.toString()
> +			<< " to " << cfg.size.toString();
>  		status = Adjusted;
>  	}
>  
> @@ -123,14 +147,18 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
>  CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
>  {
> +	UVCCameraData *data = cameraData(camera);
>  	CameraConfiguration *config = new UVCCameraConfiguration();
>  
>  	if (roles.empty())
>  		return config;
>  
> -	StreamConfiguration cfg{};
> -	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> -	cfg.size = { 640, 480 };
> +	V4L2DeviceFormats v4l2formats = data->video_->formats();
> +	StreamFormats formats(v4l2formats.data());
> +	StreamConfiguration cfg(formats);
> +
> +	cfg.pixelFormat = formats.pixelformats().front();
> +	cfg.size = formats.sizes(cfg.pixelFormat).back();
>  	cfg.bufferCount = 4;
>  
>  	config->addConfiguration(cfg);

-- 
Regards,

Laurent Pinchart