[libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor: Break out properties initialization

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Feb 06, 2020 at 07:52:45PM +0100, Jacopo Mondi wrote:
> Refactor the CameraSensor properties initialization to a dedicated virtual
> function that derived classes could subclass to register sensor-specific

s/subclass/override/

> properties values.

I would add, because it took me a bit of time to realize, "The private
properties_ member field is made protected to support this.".

> 
> While at it, move documentation of the properties() method to match the
> declaration order in the class definition.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp       | 114 +++++++++++++++-----------
>  src/libcamera/include/camera_sensor.h |   5 +-
>  2 files changed, 71 insertions(+), 48 deletions(-)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 06d10295a80e..08b2d2acdbfc 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -64,7 +64,7 @@ LOG_DEFINE_CATEGORY(CameraSensor);
>   * Once constructed the instance must be initialized with init().
>   */
>  CameraSensor::CameraSensor(const MediaEntity *entity)
> -	: entity_(entity), properties_(properties::properties)
> +	: properties_(properties::properties), entity_(entity)
>  {
>  	subdev_ = new V4L2Subdevice(entity);
>  }
> @@ -106,45 +106,6 @@ int CameraSensor::init()
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Retrieve and store the camera sensor properties. */
> -	const ControlInfoMap &controls = subdev_->controls();
> -	int32_t propertyValue;
> -
> -	/* Camera Location: default is front location. */
> -	const auto &locationControl = controls.find(V4L2_CID_CAMERA_SENSOR_LOCATION);
> -	if (locationControl != controls.end()) {
> -		int32_t v4l2Location =
> -			locationControl->second.def().get<int32_t>();
> -
> -		switch (v4l2Location) {
> -		default:
> -			LOG(CameraSensor, Warning)
> -				<< "Unsupported camera location "
> -				<< v4l2Location << ", setting to Front";
> -			/* Fall-through */
> -		case V4L2_LOCATION_FRONT:
> -			propertyValue = properties::CameraLocationFront;
> -			break;
> -		case V4L2_LOCATION_BACK:
> -			propertyValue = properties::CameraLocationBack;
> -			break;
> -		case V4L2_LOCATION_EXTERNAL:
> -			propertyValue = properties::CameraLocationExternal;
> -			break;
> -		}
> -	} else {
> -		propertyValue = properties::CameraLocationFront;
> -	}
> -	properties_.set(properties::Location, propertyValue);
> -
> -	/* Camera Rotation: default is 0 degrees. */
> -	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> -	if (rotationControl != controls.end())
> -		propertyValue = rotationControl->second.def().get<int32_t>();
> -	else
> -		propertyValue = 0;
> -	properties_.set(properties::Rotation, propertyValue);
> -
>  	/* Enumerate and cache media bus codes and sizes. */
>  	const ImageFormats formats = subdev_->formats(0);
>  	if (formats.isEmpty()) {
> @@ -175,6 +136,62 @@ int CameraSensor::init()
>  	std::sort(mbusCodes_.begin(), mbusCodes_.end());
>  	std::sort(sizes_.begin(), sizes_.end());
>  
> +	return initProperties(subdev_->controls());
> +}
> +
> +/**
> + * \brief Initialize the camera sensor properties
> + * \param[in] controlMap The map of control information provided by the sensor

Derived classes could call controls() to get the controls, there's no
need to pass them to this method. We could even make the subdev_ member
field protected if needed.

> + *
> + * This method initializes the camera sensor properties, by inspecting the
> + * control information reported by the sensor media entity in \a controlMap.
> + * For each supported standard V4L2 control reported by the sensor, a libcamera
> + * property is created and registered in the list of properties supported by the
> + * sensor.

That's not fully accurate, there's not necessarily a 1:1 mapping between
V4L2 controls and libcamera properties.

> + *
> + * Derived classes are free to override this method to register sensor specific

s/are free to/may/
s/sensor specific/sensor-specific/

> + * properties as they like, by inspecting custom controls or by adding

s/ as they like//

> + * properties with pre-defined values, and eventually call this base class
> + * implementation to register standard ones.

 * Derived classes may override this method to register sensor-specific
 * properties if needed. If they do so, they shall call the base
 * initProperties() method to register standard properties.

> + *
> + * \return 0 on success, a negative error code otherwise
> + */
> +int CameraSensor::initProperties(const ControlInfoMap &controlMap)
> +{
> +	int32_t propertyValue;
> +
> +	/* Camera Location: default is front location. */
> +	const auto &locationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_LOCATION);
> +	if (locationControl != controlMap.end()) {
> +		int32_t v4l2Location =
> +			locationControl->second.def().get<int32_t>();
> +		switch (v4l2Location) {
> +		case V4L2_LOCATION_EXTERNAL:
> +			propertyValue = properties::CameraLocationExternal;
> +			break;
> +		case V4L2_LOCATION_FRONT:
> +			propertyValue = properties::CameraLocationFront;
> +			break;
> +		case V4L2_LOCATION_BACK:
> +			propertyValue = properties::CameraLocationBack;
> +			break;
> +		default:
> +			LOG(CameraSensor, Error)
> +				<< "Unsupported camera location: " << v4l2Location;
> +			return -EINVAL;
> +		}
> +	} else {
> +		propertyValue = properties::CameraLocationFront;
> +	}
> +	properties_.set(properties::Location, propertyValue);
> +
> +	/* Camera Rotation: default is 0 degrees. */
> +	propertyValue = 0;
> +	const auto &rotationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> +	if (rotationControl != controlMap.end())
> +		propertyValue = rotationControl->second.def().get<int32_t>();
> +	properties_.set(properties::Rotation, propertyValue);
> +
>  	return 0;
>  }
>  
> @@ -340,12 +357,6 @@ int CameraSensor::getControls(ControlList *ctrls)
>  	return subdev_->getControls(ctrls);
>  }
>  
> -/**
> - * \fn CameraSensor::properties()
> - * \brief Retrieve the camera sensor properties
> - * \return The list of camera sensor properties
> - */
> -
>  /**
>   * \brief Write controls to the sensor
>   * \param[in] ctrls The list of controls to write
> @@ -376,6 +387,17 @@ int CameraSensor::setControls(ControlList *ctrls)
>  	return subdev_->setControls(ctrls);
>  }
>  
> +/**
> + * \fn CameraSensor::properties()
> + * \brief Retrieve the camera sensor properties
> + * \return The list of camera sensor properties
> + */
> +
> +/**
> + * \var CameraSensor::properties_
> + * \brief The list of camera sensor properties
> + */
> +
>  std::string CameraSensor::logPrefix() const
>  {
>  	return "'" + subdev_->entity()->name() + "'";
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> index 2f4a0cc8ad3f..87d3516aaae7 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -34,6 +34,7 @@ public:
>  	CameraSensor &operator=(const CameraSensor &) = delete;
>  
>  	int init();
> +	virtual int initProperties(const ControlInfoMap &controlMap);
>  
>  	const MediaEntity *entity() const { return entity_; }
>  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> @@ -51,6 +52,8 @@ public:
>  	const ControlList &properties() const { return properties_; }
>  
>  protected:
> +	ControlList properties_;
> +

Member data should go after member functions.

>  	friend class CameraSensorFactory;
>  	explicit CameraSensor(const MediaEntity *entity);
>  	std::string logPrefix() const;
> @@ -61,8 +64,6 @@ private:
>  
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
> -
> -	ControlList properties_;
>  };
>  
>  class CameraSensorFactory

-- 
Regards,

Laurent Pinchart