[libcamera-devel] [PATCH v5 03/10] libcamera: camera_sensor: Provide fall-back for sensor properties

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 05, 2021 at 01:31:21PM +0100, Jacopo Mondi wrote:
> Support for the V4L2 selection API is currently optional in the
> CameraSensor class. Properties registered by using values read through
> that API are defaulted in several different places (the Android camera
> HAL or the CameraSensor class).
> 
> In future support for the selection API will be made mandatory, but to

s/In future/In the future/

> give time to sensor drivers in all test platforms to be updated, use
> sensor resolution as fallback values for sensor pixel array properties
> and cache them as class member variables.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  3 ++
>  src/libcamera/camera_sensor.cpp            | 57 ++++++++--------------
>  2 files changed, 24 insertions(+), 36 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index aee10aa6e3c7..86902b85ada8 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -84,6 +84,9 @@ private:
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
>  
> +	Size pixelArraySize_;
> +	Rectangle activeArea_;
> +
>  	ControlList properties_;
>  };
>  
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 048fcd6a1fae..92c90862215d 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -269,15 +269,21 @@ int CameraSensor::validateSensorDriver()
>  	Rectangle rect;
>  	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);
>  	if (ret) {
> +		rect = Rectangle(resolution());
>  		LOG(CameraSensor, Warning)
> -			<< "Failed to retrieve the readable pixel array size";
> +			<< "The PixelArraySize property has been defaulted to "
> +			<< rect.toString();
>  		err = -EINVAL;
>  	}
> +	pixelArraySize_.width = rect.width;
> +	pixelArraySize_.height = rect.height;
>  
> -	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
> +	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &activeArea_);
>  	if (ret) {
> +		activeArea_ = Rectangle(pixelArraySize_);
>  		LOG(CameraSensor, Warning)
> -			<< "Failed to retrieve the active pixel array size";
> +			<< "The PixelArrayActiveAreas property has been defaulted to: "

s/to:/to/

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +			<< activeArea_.toString();
>  		err = -EINVAL;
>  	}
>  
> @@ -373,24 +379,8 @@ int CameraSensor::initProperties()
>  		propertyValue = 0;
>  	properties_.set(properties::Rotation, propertyValue);
>  
> -	Rectangle bounds;
> -	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &bounds);
> -	if (!ret)
> -		properties_.set(properties::PixelArraySize, bounds.size());
> -
> -	Rectangle crop;
> -	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &crop);
> -	if (!ret) {
> -		/*
> -		 * V4L2_SEL_TGT_CROP_DEFAULT and V4L2_SEL_TGT_CROP_BOUNDS are
> -		 * defined relatively to the sensor full pixel array size,
> -		 * while properties::PixelArrayActiveAreas is defined relatively
> -		 * to properties::PixelArraySize. Adjust it.
> -		 */
> -		crop.x -= bounds.x;
> -		crop.y -= bounds.y;
> -		properties_.set(properties::PixelArrayActiveAreas, { crop });
> -	}
> +	properties_.set(properties::PixelArraySize, pixelArraySize_);
> +	properties_.set(properties::PixelArrayActiveAreas, { activeArea_ });
>  
>  	/* Color filter array pattern, register only for RAW sensors. */
>  	for (const auto &format : formats_) {
> @@ -646,20 +636,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  {
>  	info->model = model();
>  
> -	/* Get the active area size. */
> -	Rectangle rect;
> -	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
> -	if (ret) {
> -		LOG(CameraSensor, Error)
> -			<< "Failed to construct camera sensor info: "
> -			<< "the camera sensor does not report the active area";
> -
> -		return ret;
> -	}
> -	info->activeAreaSize = { rect.width, rect.height };
> +	/*
> +	 * The active area size is a static property, while the crop
> +	 * rectangle needs to be re-read as it depends on the sensor
> +	 * configuration.
> +	 */
> +	info->activeAreaSize = { activeArea_.width, activeArea_.height };
>  
>  	/* It's mandatory for the subdevice to report its crop rectangle. */
> -	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
> +	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
>  	if (ret) {
>  		LOG(CameraSensor, Error)
>  			<< "Failed to construct camera sensor info: "
> @@ -672,10 +657,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	 * are defined relatively to the active pixel area, while V4L2's
>  	 * TGT_CROP target is defined in respect to the full pixel array.
>  	 *
> -	 * Compensate it by subtracting the active areas offset.
> +	 * Compensate it by subtracting the active area offset.
>  	 */
> -	info->analogCrop.x -= rect.x;
> -	info->analogCrop.y -= rect.y;
> +	info->analogCrop.x -= activeArea_.x;
> +	info->analogCrop.y -= activeArea_.y;
>  
>  	/* The bit depth and image size depend on the currently applied format. */
>  	V4L2SubdeviceFormat format{};

-- 
Regards,

Laurent Pinchart