[libcamera-devel] [PATCH v3 06/13] libcamera: camera_sensor: Collect pixel array properties

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Apr 24, 2020 at 11:52:57PM +0200, Jacopo Mondi wrote:
> Collect the sensor pixel array properties by retrieving the subdevice
> native size and active pixel array size.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 8d7abc7147a7..a54751fecf5a 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -169,6 +169,29 @@ int CameraSensor::initProperties()
>  		propertyValue = rotationControl->second.def().get<int32_t>();
>  	properties_.set(properties::Rotation, propertyValue);
>  
> +	/*
> +	 * Sensor pixel array properties. Conditionally register them if the
> +	 * sub-device provides support for the selection API.
> +	 */
> +	Size size{};
> +	int ret = subdev_->getNativeSize(0, &size);
> +	if (ret && ret != -ENOTTY)
> +		return ret;

This answers my previous question :-)

Should failures (other than -ENOTTY) be considered fatal, or should we
continue with other properties ?

> +	if (!ret)
> +		properties_.set(properties::PixelArray, { static_cast<int>(size.width),
> +							  static_cast<int>(size.height) });
> +
> +	/*
> +	 * \todo The sub-device API only support a single active area rectangle

I don't think it will ever support more, I think you can drop this
comment.

> +	 */
> +	Rectangle rect{};
> +	ret = subdev_->getActiveArea(0, &rect);
> +	if (ret && ret != -ENOTTY)
> +		return ret;
> +	if (!ret)
> +		properties_.set(properties::ActiveAreas, { rect.x, rect.y,
> +							   static_cast<int>(rect.width),
> +							   static_cast<int>(rect.height) });

How about adding two control types for Size and Rectangle, and using
them for these properties ? I wrote this some time ago as a test patch:

commit 08f1cb4e8477cbf1062dcc1d6bf99a7c72347e9b
Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Date:   Sat Feb 29 03:39:46 2020 +0200

    [DNI] How easy is it to add a Size control type ?

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

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 4b2e7e9cdd6c..89d5a6a72820 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -13,6 +13,7 @@
 #include <string>
 #include <unordered_map>

+#include <libcamera/geometry.h>
 #include <libcamera/span.h>

 namespace libcamera {
@@ -27,6 +28,7 @@ enum ControlType {
 	ControlTypeInteger64,
 	ControlTypeFloat,
 	ControlTypeString,
+	ControlTypeSize,
 };

 namespace details {
@@ -70,6 +72,11 @@ struct control_type<std::string> {
 	static constexpr ControlType value = ControlTypeString;
 };

+template<>
+struct control_type<Size> {
+	static constexpr ControlType value = ControlTypeSize;
+};
+
 template<typename T, std::size_t N>
 struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
 };
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
index 83555c021b6c..97ac0c7b3942 100644
--- a/src/libcamera/control_ids.yaml
+++ b/src/libcamera/control_ids.yaml
@@ -58,4 +58,14 @@ controls:
   - SensorModel:
       type: string
       description: The sensor model name
+
+  - TheSize:
+      type: Size
+      description: A Size property
+
+  - TheSizes:
+      type: Size
+      description: A Size array property
+      size: [n]
+
 ...
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 540cc026417a..a1ec994900a5 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -58,6 +58,7 @@ static constexpr size_t ControlValueSize[] = {
 	[ControlTypeInteger64]		= sizeof(int64_t),
 	[ControlTypeFloat]		= sizeof(float),
 	[ControlTypeString]		= sizeof(char),
+	[ControlTypeSize]		= sizeof(Size),
 };

 } /* namespace */
@@ -242,6 +243,12 @@ std::string ControlValue::toString() const
 			str += std::to_string(*value);
 			break;
 		}
+		case ControlTypeSize: {
+			const Size *value = reinterpret_cast<const Size *>(data);
+			str += std::to_string(value->width) + "x"
+			     + std::to_string(value->height);
+			break;
+		}
 		case ControlTypeNone:
 		case ControlTypeString:
 			break;
diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp
index e1d0055d139c..4885501bdcf3 100644
--- a/test/serialization/control_serialization.cpp
+++ b/test/serialization/control_serialization.cpp
@@ -47,6 +47,8 @@ protected:
 		list.set(controls::Saturation, 50);
 		list.set(controls::BayerGains, { 1.0f });
 		list.set(controls::SensorModel, "VIMC Sensor B");
+		list.set(controls::TheSize, Size{ 640, 480 });
+		list.set(controls::TheSizes, { Size{ 640, 480 }, Size{ 1280, 720 } });

 		/*
 		 * Serialize the control list, this should fail as the control

I think it would lead to cleaner code than storing rectangles and sizes
in integer arrays.

>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart