[libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix frame lengths calculated by sensorInfo()

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Tue, Apr 06, 2021 at 11:40:49AM +0100, David Plowman wrote:

Oh my, that's such a long delay :-( Sorry about that.

> The minimum and maximum vblanking can change when a new format is
> applied to the sensor subdevice, so be sure to retrieve up-to-date
> values.
> 
> The V4L2Device acquires the new updateControlInfos() method to perform
> this function, and which the CameraSensor calls automatically if its
> setFormat method is used to update the sensor.
> 
> However, not all pipeline handlers invoke the setFormat method
> directly, so the new method must be made publicly available for
> pipeline handlers to call if they need to.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/internal/camera_sensor.h |  2 ++
>  include/libcamera/internal/v4l2_device.h   |  2 ++
>  src/libcamera/camera_sensor.cpp            | 31 +++++++++++++++++++++-
>  src/libcamera/v4l2_device.cpp              | 26 ++++++++++++++++++

I may have split the patch in two, but it's no big deal.

>  4 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 3e98f71b..c94744f0 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -68,6 +68,8 @@ public:
>  	const ControlList &properties() const { return properties_; }
>  	int sensorInfo(CameraSensorInfo *info) const;
>  
> +	void updateControlInfos();

Let's name this updateControlInfo() as information is uncountable. Same
below.

> +
>  protected:
>  	std::string logPrefix() const override;
>  
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index d006bf68..274cbe65 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -41,6 +41,8 @@ public:
>  	int setFrameStartEnabled(bool enable);
>  	Signal<uint32_t> frameStart;
>  
> +	void updateControlInfos();
> +
>  protected:
>  	V4L2Device(const std::string &deviceNode);
>  	~V4L2Device();
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f7ed91d9..5dbe47a7 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
>  /**
>   * \brief Set the sensor output format
>   * \param[in] format The desired sensor output format
> + *
> + * The sensor output format is set. The ranges of any controls associated
> + * with the sensor are also updated.

You can drop the first sentence.

> + *
>   * \return 0 on success or a negative error code otherwise
>   */
>  int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
>  {
> -	return subdev_->setFormat(pad_, format);
> +	int ret = subdev_->setFormat(pad_, format);
> +
> +	if (ret == 0)
> +		updateControlInfos();

We tend to return immediately on error.

	int ret = subdev_->setFormat(pad_, format);
	if (ret)
		return ret;

	updateControlInfos();
	return 0;

> +
> +	return ret;
>  }
>  
>  /**
>   * \brief Retrieve the supported V4L2 controls and their information
> + *
> + * Pipeline handlers that do not change the sensor format using the
> + * CameraSensor::setFormat method may need to call

Just "setFormat()" will be enough for doxygen to pick it up, as we're in
the same class. Same below.

> + * CameraSensor::updateControlInfos beforehand, to ensure all the control
> + * ranges are up to date.

I'd go the other way around, saying that the control information is
updated automatically when setFormat() is called.

 * Control information is updated automatically to reflect the current sensor
 * configuration when the setFormat() function is called, without invalidating
 * any iterator on the ControlInfoMap. A manual update can also be forced by
 * calling the updateControlInfo() function for pipeline handlers that change
 * the sensor configuration without using setFormat().

Note that we really don't want to see more pipeline handlers doing this,
and I'd like to deprecate this method in the future and switch to the
regular CameraSensor::setFormat() for the Raspberry Pi pipeline handler
too.

Please also note that we're considering removing the V4L2 dependency in
the API of the CameraSensor class. If that happens, we will really want
to drop the manual updateControlInfo().

> + *
>   * \return A map of the V4L2 controls supported by the sensor
>   */
>  const ControlInfoMap &CameraSensor::controls() const
> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)
>   * Sensor information is only available for raw sensors. When called for a YUV
>   * sensor, this function returns -EINVAL.
>   *
> + * Pipeline handlers that do not change the sensor format using the
> + * CameraSensor::setFormat method may need to call
> + * CameraSensor::updateControlInfos beforehand, to ensure all the control
> + * ranges are up to date.

Based on the comment above, I'm tempted to drop this one.

> + *
>   * \return 0 on success, a negative error code otherwise
>   */
>  int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> @@ -821,6 +841,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	return 0;
>  }
>  
> +/**
> + * \fn void CameraSensor::updateControlInfos()
> + * \brief Update the sensor's ControlInfos in case they have changed

It's not ControlInfos but ControlInfoMap. You can add a \sa
V4L2Device::updateControlInfo()

> + */
> +void CameraSensor::updateControlInfos()
> +{
> +	subdev_->updateControlInfos();
> +}
> +
>  std::string CameraSensor::logPrefix() const
>  {
>  	return "'" + entity_->name() + "'";
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index decd19ef..9678962c 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -514,6 +514,32 @@ void V4L2Device::listControls()
>  	controls_ = std::move(ctrls);
>  }
>  
> +/*
> + * \brief Update the control information that was previously stored by
> + * listControls(). Some parts of this information, such as min and max
> + * values of some controls, are liable to change when a new format is set.

That's not very brief :-) Let's split it. Also, listControls() being a
private function, it's not great to document a public API based on it.
And finally, we should specify the behaviour more precisely, as we're
updating information live.

 * \brief Update the information for all device controls
 *
 * The V4L2Device class caches information about all controls supported by the
 * device and exposes it through the controls() and controlInfo() functions.
 * Control information may change at runtime, for instance when formats on a
 * subdev are modified. When this occurs, this function can be used to refresh
 * control information. The information is refreshed in-place, all pointers to
 * v4l2_query_ext_ctrl instances previously returned by controlInfo() and
 * iterators to the ControlInfoMap returned by controls() remain valid.
 *
 * Note that control information isn't refreshed automatically is it may be an
 * expensive operation. The V4L2Device users are responsible for calling this
 * function when required, based on their usage pattern of the class.

> + */
> +void V4L2Device::updateControlInfos()
> +{
> +	for (auto &controlId : controlIds_) {
> +		unsigned int id = controlId->id();
> +
> +		struct v4l2_query_ext_ctrl ctrl = {};
> +		ctrl.id = id;
> +
> +		if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {
> +			LOG(V4L2, Debug)
> +				<< "Could not refresh control "
> +				<< utils::hex(ctrl.id);

s/ctrl.id/id/

> +			continue;
> +		}
> +
> +		/* Assume these are present - listControls() made them. */
> +		controlInfo_[id] = ctrl;
> +		controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);

That's two lookups for each entry, I think we can do better by iterating
over controls_ instead of controlIds_. It could also be possibly to
avoid the copy by operating on the controlInfo_ entry directly.

	for (auto &[controlId, info] : controls_) {
		unsigned int id = controlId->id();

		/*
		 * Assume controlInfo_ has a corresponding entry, as it has been
		 * generated by listControls().
		 */
		struct v4l2_query_ext_ctrl &ctrl = controlInfo_[id];

		if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {
			LOG(V4L2, Debug)
				<< "Could not refresh control "
				<< utils::hex(id);
			continue;
		}

		info = V4L2ControlInfo(ctrl);
	}

> +	}
> +}
> +
>  /*
>   * \brief Update the value of the first \a count V4L2 controls in \a ctrls using
>   * values in \a v4l2Ctrls

-- 
Regards,

Laurent Pinchart