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

[Kieran Bingham]

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.


> 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 ...


> 
> 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