[libcamera-devel] [PATCH/RFC 11/12] libcamera: pipeline: uvcvideo: Validate format in UVCCameraConfiguration::validate()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sat, May 18, 2019 at 02:06:20AM +0300, Laurent Pinchart wrote:
> From: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> Validate and potentially adjust the requested format with a list of
> discrete formats retrieved from the video device.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/uvcvideo.cpp | 44 ++++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 286d19b0af01..e02d5b19e82a 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -37,6 +37,7 @@ public:
>  
>  	V4L2Device *video_;
>  	Stream stream_;
> +	StreamFormats formats_;
>  };
>  
>  class UVCCameraConfiguration : public CameraConfiguration
> @@ -45,6 +46,7 @@ public:
>  	UVCCameraConfiguration();
>  
>  	Status validate() override;
> +	StreamFormats formats;

You can instead store a pointer to the UVCCameraData in
UVCCameraConfiguration (see the IPU3 or RkISP1 pipeline handlers to see
how this should be done), and access the formats from there.

>  };
>  
>  class PipelineHandlerUVC : public PipelineHandler
> @@ -96,21 +98,42 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
>  
>  	StreamConfiguration &cfg = config_[0];
>  
> -	/* \todo: Validate the configuration against the device capabilities. */
>  	const unsigned int pixelFormat = cfg.pixelFormat;
>  	const Size size = cfg.size;
> +	const unsigned int bufferCount = cfg.bufferCount;
>  
>  	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> -	cfg.size = { 640, 480 };
> +	if (cfg.pixelFormat != pixelFormat) {
> +		LOG(UVC, Debug)
> +			<< "Adjusting pixel format from " << pixelFormat
> +			<< " to " << cfg.pixelFormat;
> +		status = Adjusted;
> +	}

We shouldn't hardcode the format to YUYV but instead support any pixel
format supported by the camera (or at the very least both YUYV and
MJPEG).

>  
> -	if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> +	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)
> +			break;
> +	}
> +
> +	if (cfg.size != size) {
>  		LOG(UVC, Debug)
> -			<< "Adjusting configuration from " << cfg.toString()
> -			<< " to " << cfg.size.toString() << "-YUYV";
> +			<< "Adjusting size from " << size.toString()
> +			<< " to " << cfg.size.toString();
>  		status = Adjusted;
>  	}
>  
> -	cfg.bufferCount = 4;
> +	if (bufferCount < 4) {
> +		cfg.bufferCount = 4;
> +		LOG(UVC, Debug)
> +			<< "Adjusting buffer count from " << bufferCount
> +			<< " to " << cfg.bufferCount;
> +		status = Adjusted;
> +	}

I've left this out for now, with a \todo in one of the patches to decide
on how to handle buffers count adjustement. Let's not mix it with this
patc.

>  
>  	return status;
>  }
> @@ -123,7 +146,10 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
>  CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
>  {
> -	CameraConfiguration *config = new UVCCameraConfiguration();
> +	UVCCameraData *data = cameraData(camera);
> +	UVCCameraConfiguration *config = new UVCCameraConfiguration();
> +
> +	config->formats = data->formats_;
>  
>  	if (!roles.empty()) {
>  		StreamConfiguration cfg{};

Should we pick a default configuration based on the enumerated formats ?

> @@ -242,6 +268,10 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	if (data->video_->open())
>  		return false;
>  
> +	data->formats_ = StreamFormats(data->video_->enumerateFrameSizes({ V4L2_PIX_FMT_YUYV }));

No need for the StreamFormats(), is there ?

> +	for (const Size &size : data->formats_.sizes(V4L2_PIX_FMT_YUYV))
> +		LOG(UVC, Info) << size.toString();

This should be debug messages. Or if you think they're useful as info
messages, should we print all resolutions on a single line ?

> +
>  	data->video_->bufferReady.connect(data.get(), &UVCCameraData::bufferReady);
>  
>  	/* Create and register the camera. */

-- 
Regards,

Laurent Pinchart