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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-02-22 12:48:53 +0200, Laurent Pinchart wrote:
> 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/

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.

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.

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

-- 
Regards,
Niklas Söderlund