[libcamera-devel] [PATCH 2/4] libcamera: pipeline: uvcvideo: enforce stream configuration

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Feb 21, 2019 at 12:59:37AM +0100, Niklas Söderlund wrote:
> The format requested by configureStreams() should exactly match what

s/what$/what is/

> programmed on the video device it self. If they do not match the call

s/it self/itself/ or simply s/ it self//

> should fail as the application in that case will not know what
> configuration actually was programmed.

s/actually was/was actually/

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/uvcvideo.cpp | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 37a3477c2c56f3ea..b7b8ff109109e9c5 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -84,6 +84,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,
>  					 std::map<Stream *, StreamConfiguration> &config)
>  {
>  	StreamConfiguration *cfg = &config[&stream_];
> +	int ret;
>  
>  	LOG(UVC, Debug) << "Configure the camera for resolution "
>  			<< cfg->width << "x" << cfg->height;
> @@ -93,7 +94,16 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,
>  	format.height = cfg->height;
>  	format.fourcc = cfg->pixelFormat;
>  
> -	return video_->setFormat(&format);
> +	ret = video_->setFormat(&format);
> +	if (ret)
> +		return ret;
> +
> +	if (format.width != cfg->width ||
> +	    format.height != cfg->height ||
> +	    format.fourcc != cfg->pixelFormat)
> +		return -EINVAL;

This is fine for now, but I wonder if longer term we should enumerate
the available formats and resolutions, and avoiding calling setFormat()
is the requested format can't be achieved. The documentation in patch
1/4 explains that no configuration should be applied if an exact match
is impossible, and calling setFormat() modifies the hardware
configuration. On the other hand this change in hardware configuration
isn't visible from the application, so it won't matter much.

> +
> +	return 0;
>  }
>  
>  int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)

-- 
Regards,

Laurent Pinchart