[libcamera-devel] [PATCH v2 3/3] libcamera: pipeline: uvc: Use the device to validate formats

[Jacopo Mondi]

Hi Kieran,

On Fri, May 24, 2019 at 09:25:16AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 24/05/2019 08:56, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Thu, May 23, 2019 at 02:59:00PM +0100, Kieran Bingham wrote:
> >> UVC pipelines are highly variable, and we can not define their
> >> configuration restrictions within the UVC pipeline handler directly.
> >>
> >> Update the UVCCameraConfiguration to store the UVCCameraData (storing
> >> a reference to the camera as a means of borrowing a reference to the
> >> camera data).
> >>
> >> We then validate the configuration by the tryFormat() operation on the
> >> UVC V4L2Device.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  src/libcamera/pipeline/uvcvideo.cpp | 36 +++++++++++++++++++++--------
> >>  1 file changed, 26 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> >> index 45260f34c8f5..df321c6e64a6 100644
> >> --- a/src/libcamera/pipeline/uvcvideo.cpp
> >> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> >> @@ -42,9 +42,18 @@ public:
> >>  class UVCCameraConfiguration : public CameraConfiguration
> >>  {
> >>  public:
> >> -	UVCCameraConfiguration();
> >> +	UVCCameraConfiguration(Camera *camera, UVCCameraData *data);
> >
> > Minor: do you need 'camera' in UVCCameraConfiguration or just 'data' ?
> > You can pass data to the UVCCameraConfiguration constructor at
> > generateConfiguration() time, and save storing one shared pointer.
>
> I added it due to my interpretation of the comment in
> RkISP1CameraConfiguration (which I have replicated below) as requiring a
> reference to the Camera to ensure that the CameraData is valid.

Correct, I've also been told offline just yesterday that this is
mostly for reference counting on the Camera instance. Please ignore my
comment then!

>
>
> > Apart from this minor thing:
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thank you,
>
> I think Laurent has some reservations about this ... as Niklas is
> working on some different enumerations.
>
> But I still need this series to be able to utilise libcamera on my
> development laptop, so of course I'd like it to go in ...

Looking at the code I would have preferred too to see the UVC pipeline
handler matching the required configuration against the list of
enumerated formats, but since we don't have it yet... :)

Anyway, the patch itself is good to me, let's wait to hear from others
if we want to wait for format enumeration support to land first.

>
>
> >
> > Thanks
> >   j
> >
> >>
> >>  	Status validate() override;
> >> +
> >> +private:
> >> +	/*
> >> +	 * The UVCCameraConfiguration instance is guaranteed to be valid as long
> >> +	 * as the corresponding Camera instance is valid. In order to borrow a
> >> +	 * reference to the camera data, store a new reference to the camera.
> >> +	 */
>
> Here:     ^^^^^^^^^^^^^^^^^^^
>
>
>
> >> +	std::shared_ptr<Camera> camera_;
> >> +	const UVCCameraData *data_;
> >>  };
> >>
> >>  class PipelineHandlerUVC : public PipelineHandler
> >> @@ -76,9 +85,12 @@ private:
> >>  	}
> >>  };
> >>
> >> -UVCCameraConfiguration::UVCCameraConfiguration()
> >> +UVCCameraConfiguration::UVCCameraConfiguration(Camera *camera,
> >> +					       UVCCameraData *data)
> >>  	: CameraConfiguration()
> >>  {
> >> +	camera_ = camera->shared_from_this();
> >> +	data_ = data;
> >>  }
> >>
> >>  CameraConfiguration::Status UVCCameraConfiguration::validate()
> >> @@ -96,17 +108,20 @@ 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;
> >> +	V4L2DeviceFormat format;
> >> +	format.fourcc = cfg.pixelFormat;
> >> +	format.size = cfg.size;
> >>
> >> -	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> >> -	cfg.size = { 640, 480 };
> >> +	/* Validate the format on the device. */
> >> +	data_->video_->tryFormat(&format);
> >>
> >> -	if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> >> +	if (cfg.pixelFormat != format.fourcc || cfg.size != format.size) {
> >>  		LOG(UVC, Debug)
> >>  			<< "Adjusting configuration from " << cfg.toString()
> >> -			<< " to " << cfg.size.toString() << "-YUYV";
> >> +			<< " to " << format.toString();
> >> +
> >> +		cfg.pixelFormat = format.fourcc;
> >> +		cfg.size = format.size;
> >>  		status = Adjusted;
> >>  	}
> >>
> >> @@ -123,7 +138,8 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> >>  CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> >>  	const StreamRoles &roles)
> >>  {
> >> -	CameraConfiguration *config = new UVCCameraConfiguration();
> >> +	UVCCameraData *data = cameraData(camera);
> >> +	CameraConfiguration *config = new UVCCameraConfiguration(camera, data);
> >>
> >>  	if (roles.empty())
> >>  		return config;
> >> --
> >> 2.20.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190524/a5f1eab5/attachment.sig>