[libcamera-devel,v3,05/13] libcamera: camera_sensor: Break out properties initialization

3523 diff mbox series

Refactor the CameraSensor properties initialization to a dedicated
function as it is expected to grow as we augment the number of
properties.

While at it, move documentation of the properties() method to match the
declaration order in the class definition.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/camera_sensor.cpp       | 99 +++++++++++++++------------
 src/libcamera/include/camera_sensor.h |  1 +
 2 files changed, 55 insertions(+), 45 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Fri, Apr 24, 2020 at 11:52:56PM +0200, Jacopo Mondi wrote:
> Refactor the CameraSensor properties initialization to a dedicated
> function as it is expected to grow as we augment the number of
> properties.
> 
> While at it, move documentation of the properties() method to match the
> declaration order in the class definition.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp       | 99 +++++++++++++++------------
>  src/libcamera/include/camera_sensor.h |  1 +
>  2 files changed, 55 insertions(+), 45 deletions(-)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 2219a4307436..8d7abc7147a7 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -91,45 +91,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()) {
> @@ -160,6 +121,54 @@ int CameraSensor::init()
>  	std::sort(mbusCodes_.begin(), mbusCodes_.end());
>  	std::sort(sizes_.begin(), sizes_.end());
>  
> +	return initProperties();

Out of curiosity, is there a reason to have put the call at the end
instead of where the code was removed ?

> +}
> +
> +/**
> + * \brief Initialize the camera sensor standard properties
> + *
> + * This method initializes the camera sensor standard properties, by inspecting

s/,//

> + * the control information reported by the sensor subdevice.
> + *
> + * \return 0 on success, a negative error code otherwise
> + */
> +int CameraSensor::initProperties()
> +{
> +	const ControlInfoMap &controlMap = subdev_->controls();
> +	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>();

Maybe keep the blank line here for clarity ?

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

This is a behavioural change, as we used to default to
properties::CameraLocationFront when the control had an unknown value.
I'm not opposed to that, but it should be moved to a different patch,
it's not even mentioned in the commit message. Let's keep patches that
move code around free of behavioural changes, that's easier to review.

> +		}
> +	} 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>();

Same here, I'd keep the existing else branch (not a behavioural change
though).

> +	properties_.set(properties::Rotation, propertyValue);
> +
>  	return 0;
>  }
>  
> @@ -325,12 +334,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
> @@ -361,6 +364,12 @@ 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
> + */
> +
>  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 99cff98128dc..8fa4d450f959 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -32,6 +32,7 @@ public:
>  	CameraSensor &operator=(const CameraSensor &) = delete;
>  
>  	int init();
> +	int initProperties();

Shouldn't this be a private function ?

With these small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	const MediaEntity *entity() const { return entity_; }
>  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }

On Sat, Apr 25, 2020 at 03:49:53PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Apr 24, 2020 at 11:52:56PM +0200, Jacopo Mondi wrote:
> > Refactor the CameraSensor properties initialization to a dedicated
> > function as it is expected to grow as we augment the number of
> > properties.
> >
> > While at it, move documentation of the properties() method to match the
> > declaration order in the class definition.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/camera_sensor.cpp       | 99 +++++++++++++++------------
> >  src/libcamera/include/camera_sensor.h |  1 +
> >  2 files changed, 55 insertions(+), 45 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 2219a4307436..8d7abc7147a7 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -91,45 +91,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()) {
> > @@ -160,6 +121,54 @@ int CameraSensor::init()
> >  	std::sort(mbusCodes_.begin(), mbusCodes_.end());
> >  	std::sort(sizes_.begin(), sizes_.end());
> >
> > +	return initProperties();
>
> Out of curiosity, is there a reason to have put the call at the end
> instead of where the code was removed ?
>

if we fail to find a suitable format, there is no reason to have
properties initialized, so let's first make sure we can produce the
required format, then initialize properties


> > +}
> > +
> > +/**
> > + * \brief Initialize the camera sensor standard properties
> > + *
> > + * This method initializes the camera sensor standard properties, by inspecting
>
> s/,//
>
> > + * the control information reported by the sensor subdevice.
> > + *
> > + * \return 0 on success, a negative error code otherwise
> > + */
> > +int CameraSensor::initProperties()
> > +{
> > +	const ControlInfoMap &controlMap = subdev_->controls();
> > +	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>();
>
> Maybe keep the blank line here for clarity ?
>
> > +		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;
>
> This is a behavioural change, as we used to default to
> properties::CameraLocationFront when the control had an unknown value.
> I'm not opposed to that, but it should be moved to a different patch,
> it's not even mentioned in the commit message. Let's keep patches that
> move code around free of behavioural changes, that's easier to review.

this was not intended, I think it got mixed up in between rebases,
good catch

>
> > +		}
> > +	} 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>();
>
> Same here, I'd keep the existing else branch (not a behavioural change
> though).
>

this was intended, but I can revert it back

> > +	properties_.set(properties::Rotation, propertyValue);
> > +
> >  	return 0;
> >  }
> >
> > @@ -325,12 +334,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
> > @@ -361,6 +364,12 @@ 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
> > + */
> > +
> >  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 99cff98128dc..8fa4d450f959 100644
> > --- a/src/libcamera/include/camera_sensor.h
> > +++ b/src/libcamera/include/camera_sensor.h
> > @@ -32,6 +32,7 @@ public:
> >  	CameraSensor &operator=(const CameraSensor &) = delete;
> >
> >  	int init();
> > +	int initProperties();
>
> Shouldn't this be a private function ?
>
> With these small issues fixed,

Indeed, leftover from when this was a protected virtual

Thanks
  j

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >
> >  	const MediaEntity *entity() const { return entity_; }
> >  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Sat, Apr 25, 2020 at 03:42:24PM +0200, Jacopo Mondi wrote:
> On Sat, Apr 25, 2020 at 03:49:53PM +0300, Laurent Pinchart wrote:
> > On Fri, Apr 24, 2020 at 11:52:56PM +0200, Jacopo Mondi wrote:
> > > Refactor the CameraSensor properties initialization to a dedicated
> > > function as it is expected to grow as we augment the number of
> > > properties.
> > >
> > > While at it, move documentation of the properties() method to match the
> > > declaration order in the class definition.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/libcamera/camera_sensor.cpp       | 99 +++++++++++++++------------
> > >  src/libcamera/include/camera_sensor.h |  1 +
> > >  2 files changed, 55 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index 2219a4307436..8d7abc7147a7 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -91,45 +91,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()) {
> > > @@ -160,6 +121,54 @@ int CameraSensor::init()
> > >  	std::sort(mbusCodes_.begin(), mbusCodes_.end());
> > >  	std::sort(sizes_.begin(), sizes_.end());
> > >
> > > +	return initProperties();
> >
> > Out of curiosity, is there a reason to have put the call at the end
> > instead of where the code was removed ?
> 
> if we fail to find a suitable format, there is no reason to have
> properties initialized, so let's first make sure we can produce the
> required format, then initialize properties
> 
> > > +}
> > > +
> > > +/**
> > > + * \brief Initialize the camera sensor standard properties
> > > + *
> > > + * This method initializes the camera sensor standard properties, by inspecting
> >
> > s/,//
> >
> > > + * the control information reported by the sensor subdevice.
> > > + *
> > > + * \return 0 on success, a negative error code otherwise
> > > + */
> > > +int CameraSensor::initProperties()
> > > +{
> > > +	const ControlInfoMap &controlMap = subdev_->controls();
> > > +	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>();
> >
> > Maybe keep the blank line here for clarity ?
> >
> > > +		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;
> >
> > This is a behavioural change, as we used to default to
> > properties::CameraLocationFront when the control had an unknown value.
> > I'm not opposed to that, but it should be moved to a different patch,
> > it's not even mentioned in the commit message. Let's keep patches that
> > move code around free of behavioural changes, that's easier to review.
> 
> this was not intended, I think it got mixed up in between rebases,
> good catch
> 
> > > +		}
> > > +	} 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>();
> >
> > Same here, I'd keep the existing else branch (not a behavioural change
> > though).
> 
> this was intended, but I can revert it back

If that's fine with you, let's minmize the changes in patches that move
code, and then perform them on top.

> > > +	properties_.set(properties::Rotation, propertyValue);
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -325,12 +334,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
> > > @@ -361,6 +364,12 @@ 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
> > > + */
> > > +
> > >  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 99cff98128dc..8fa4d450f959 100644
> > > --- a/src/libcamera/include/camera_sensor.h
> > > +++ b/src/libcamera/include/camera_sensor.h
> > > @@ -32,6 +32,7 @@ public:
> > >  	CameraSensor &operator=(const CameraSensor &) = delete;
> > >
> > >  	int init();
> > > +	int initProperties();
> >
> > Shouldn't this be a private function ?
> >
> > With these small issues fixed,
> 
> Indeed, leftover from when this was a protected virtual
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > >
> > >  	const MediaEntity *entity() const { return entity_; }
> > >  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }