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

[Laurent Pinchart]

Hi Niklas,

On Sat, Feb 23, 2019 at 02:39:01AM +0100, Niklas Söderlund wrote:
> On 2019-02-22 12:48:53 +0200, Laurent Pinchart wrote:
> > 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/
> 
> I agree, will update this for 2/4 and 3/4 for next version.
> 
> >> 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.
> 
> I concur that that this is a short- to mid- term solution that allows us 
> to move forward. Once we have a more complete StreamConfiguration 
> implementation and more pipeline handler implementations I think we 
> might be able to draw some more insights. At that point I think we will 
> be able to create one or more helper functions to assist pipeline 
> handlers, likely using the *_TRY_* IOCTLs. That way we could try a 
> complete set of configurations in a none invasive fashion and fail in a 
> more safe way if needed.

The TRY ioctls will usually not help much as they're limited to video
nodes. Something more complex will be needed for MC-based pipelines, but
we can do that.

> Bonus for *_TRY_* IOCTLs support will be that applications would be able 
> to call that themself before calling configureStreams() to 'update' and 
> inspect the format they request to hardware/driver limitations before 
> asking the hardware to be reprogrammed. At least this is how I model the 
> future of this part of the library in my head, if anyone have other 
> ideas please let me know so I can try to have that in mind when working 
> on the interactions between the Camera and applications.

I'm not sure if that will be feasible. While this is the model exposed
by V4L2, I fell it may not be the most suitable for applications. We'll
have to think about it.

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

-- 
Regards,

Laurent Pinchart