[libcamera-devel] [PATCH v5 03/13] libcamera: camera_sensor: Support SensorConfiguration

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Sep 21, 2023 at 06:55:40PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Add a class function to the CameraSensor class to apply a full
> configuration to the sensor.
> 
> The configuration shall be fully populated and shall apply without
> modifications to the sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/camera_sensor.h |  5 ++
>  src/libcamera/camera_sensor.cpp            | 86 ++++++++++++++++++++++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 02c77ab037da..06791c3c6425 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -29,6 +29,7 @@ namespace libcamera {
>  class BayerFormat;
>  class CameraLens;
>  class MediaEntity;
> +class SensorConfiguration;
>  
>  struct CameraSensorProperties;
>  
> @@ -58,6 +59,10 @@ public:
>  		      Transform transform = Transform::Identity);
>  	int tryFormat(V4L2SubdeviceFormat *format) const;
>  
> +	int applyConfiguration(const SensorConfiguration &config,
> +			       Transform transform = Transform::Identity,
> +			       V4L2SubdeviceFormat *sensorFormat = nullptr);
> +
>  	const ControlInfoMap &controls() const;
>  	ControlList getControls(const std::vector<uint32_t> &ids);
>  	int setControls(ControlList *ctrls);
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f3a5aa37149f..323081d25ce7 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -15,6 +15,7 @@
>  #include <math.h>
>  #include <string.h>
>  
> +#include <libcamera/camera.h>
>  #include <libcamera/property_ids.h>
>  
>  #include <libcamera/base/utils.h>
> @@ -822,6 +823,91 @@ int CameraSensor::tryFormat(V4L2SubdeviceFormat *format) const
>  				  V4L2Subdevice::Whence::TryFormat);
>  }
>  
> +/**
> + * \brief Apply a sensor configuration to the camera sensor
> + * \param[in] config The sensor configuration
> + * \param[in] transform The transform to be applied on the sensor.
> + * Defaults to Identity
> + * \param[out] sensorFormat Optional output parameter where the format
> + * actually applied to the sensor is returned to the caller

 * \param[out] sensorFormat Format applied to the sensor (optional)

Simple is more readable :-)

> + *
> + * Apply to the camera sensor the configuration \a config.
> + *
> + * \todo The configuration shall be fully populated and if any of the fields
> + * specified cannot be applied exactly, an error code is returned.

I will address this right away on top.

> + *
> + * \return 0 if \a config unpopulated or is applied correctly to the camera

The unpopulated case isn't handled by this function anymore, so
s/unpopulated or//

> + * sensor, a negative error code otherwise
> + */
> +int CameraSensor::applyConfiguration(const SensorConfiguration &config,
> +				     Transform transform,
> +				     V4L2SubdeviceFormat *sensorFormat)

The API seems a bit of a hack, and it's underdocumented as there's no
explanation about how the caller should pick applyConfiguration() or
setFormat(), or if it should call setFormat() in any case. I will likely
rework this on top.

With the above changes applied,

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

> +{
> +	if (!config.valid()) {
> +		LOG(CameraSensor, Error) << "Invalid sensor configuration";
> +		return -EINVAL;
> +	}
> +
> +	std::vector<unsigned int> filteredCodes;
> +	std::copy_if(mbusCodes_.begin(), mbusCodes_.end(),
> +		     std::back_inserter(filteredCodes),
> +		     [&config](unsigned int mbusCode) {
> +			     BayerFormat bayer = BayerFormat::fromMbusCode(mbusCode);
> +			     if (bayer.bitDepth == config.bitDepth)
> +				     return true;
> +			     return false;
> +		     });
> +	if (filteredCodes.empty()) {
> +		LOG(CameraSensor, Error)
> +			<< "Cannot find any format with bit depth "
> +			<< config.bitDepth;
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Compute the sensor's data frame size by applying the cropping
> +	 * rectangle, subsampling and output crop to the sensor's pixel array
> +	 * size.
> +	 *
> +	 * \todo The actual size computation is for now ignored and only the
> +	 * output size is considered. This implies that resolutions obtained
> +	 * with two different cropping/subsampling will look identical and
> +	 * only the first found one will be considered.
> +	 */
> +	V4L2SubdeviceFormat subdevFormat = {};
> +	for (unsigned int code : filteredCodes) {
> +		for (const Size &size : sizes(code)) {
> +			if (size.width != config.outputSize.width ||
> +			    size.height != config.outputSize.height)
> +				continue;
> +
> +			subdevFormat.mbus_code = code;
> +			subdevFormat.size = size;
> +			break;
> +		}
> +	}
> +	if (!subdevFormat.mbus_code) {
> +		LOG(CameraSensor, Error) << "Invalid output size in sensor configuration";
> +		return -EINVAL;
> +	}
> +
> +	int ret = setFormat(&subdevFormat, transform);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Return to the caller the format actually applied to the sensor.
> +	 * This is relevant if transform has changed the bayer pattern order.
> +	 */
> +	if (sensorFormat)
> +		*sensorFormat = subdevFormat;
> +
> +	/* \todo Handle AnalogCrop. Most sensors do not support set_selection */
> +	/* \todo Handle scaling in the digital domain. */
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Retrieve the supported V4L2 controls and their information
>   *

-- 
Regards,

Laurent Pinchart