[libcamera-devel,v5,03/10] libcamera: camera_sensor: Provide fall-back for sensor properties
diff mbox series

Message ID 20210105123128.617543-4-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: camera_sensor: Make validation more strict
Related show

Commit Message

Jacopo Mondi Jan. 5, 2021, 12:31 p.m. UTC 
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
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@jmondi.org>
---
 include/libcamera/internal/camera_sensor.h |  3 ++
 src/libcamera/camera_sensor.cpp            | 57 ++++++++--------------
 2 files changed, 24 insertions(+), 36 deletions(-)

Comments

Niklas Söderlund Jan. 6, 2021, 10:39 a.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2021-01-05 13:31:21 +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
> 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@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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: "
> +			<< 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{};
> -- 
> 2.29.2
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 7, 2021, 2:32 a.m. UTC | #2 
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@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@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{};

Patch
diff mbox series 

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: "
+			<< 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{};