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

[Laurent Pinchart]

Hi Jacopo,

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

I think you're right, it's probably best to fail to ensure drivers are
correctly implemented.

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

And the fields will also be easier to access, you won't have to convert
back and forth between Size/Rectangle and integer arrays.

I'll post the patch below, as well as another one for Rectangle
controls.

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

What about it ? :-)

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

-- 
Regards,

Laurent Pinchart