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

[Jacopo Mondi]

On Sat, Apr 25, 2020 at 03:54:02PM +0300, Laurent Pinchart wrote:
> 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 ?

An failure != -ENOTTY mean something went wrong, possibly on the
kernel side, so I would rather fail here. The failure is loud in the
V4L2Subdevice class already. Do you think we should continue instead ?

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

ack, although the property defines more rectangles, and one could
wonder why we ignore that

> > +	 */
> > +	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:

I would be glad to do this after the series went in, the gain is
minimal imho, the main advantage I see is that it won't be possible to
get the order of fields wrong.


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

What about control serialization ?

>
> I think it would lead to cleaner code than storing rectangles and sizes
> in integer arrays.
>
> >  	return 0;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart