[v4,1/2] libcamera: camera_sensor_properties: Add sensor control delays

22018 diff mbox series

Add properties covering the sensor control application delays to both
the static CameraSensorProperties definitions. The values used are the
defaults that're in use across the library, with deviations from that
taken from Raspberry Pi's CamHelper class definitions.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v4:

	- getSensorDelays() renamed to sensorDelays()
	- sensorDelays() returns a reference to the SensorDelays struct rather
	  than each of the values.
	- Reworded the comments to make the default values seem less acceptable
	- Added delay values for AR0144

Changes in v3:

	- Rebased on top of the CameraSensorFactory introduction
	- Some rephrasing
	- Defined the sensorDelays member as empty where Raspberry Pi didn't
	  have any specific values. Check for the empty struct in
	  getSensorDelays() and return the defaults from there with a warning.

Changes in v2:

	- Rather than adding the delays to the properties ControlList, added a
	  new function in CameraSensor that allows PipelineHandlers to retreive
	  the delay values.

 include/libcamera/internal/camera_sensor.h    |   2 +
 .../internal/camera_sensor_properties.h       |  10 ++
 src/libcamera/sensor/camera_sensor.cpp        |  12 ++
 src/libcamera/sensor/camera_sensor_legacy.cpp |  18 +++
 .../sensor/camera_sensor_properties.cpp       | 121 ++++++++++++++++++
 5 files changed, 163 insertions(+)

Hi Dan

On Tue, Nov 19, 2024 at 01:03:00PM +0000, Daniel Scally wrote:
> Add properties covering the sensor control application delays to both
> the static CameraSensorProperties definitions. The values used are the
> defaults that're in use across the library, with deviations from that

This probably needs to be updated

> taken from Raspberry Pi's CamHelper class definitions.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v4:
>
> 	- getSensorDelays() renamed to sensorDelays()
> 	- sensorDelays() returns a reference to the SensorDelays struct rather
> 	  than each of the values.
> 	- Reworded the comments to make the default values seem less acceptable
> 	- Added delay values for AR0144
>
> Changes in v3:
>
> 	- Rebased on top of the CameraSensorFactory introduction
> 	- Some rephrasing
> 	- Defined the sensorDelays member as empty where Raspberry Pi didn't
> 	  have any specific values. Check for the empty struct in
> 	  getSensorDelays() and return the defaults from there with a warning.
>
> Changes in v2:
>
> 	- Rather than adding the delays to the properties ControlList, added a
> 	  new function in CameraSensor that allows PipelineHandlers to retreive
> 	  the delay values.
>
>  include/libcamera/internal/camera_sensor.h    |   2 +
>  .../internal/camera_sensor_properties.h       |  10 ++
>  src/libcamera/sensor/camera_sensor.cpp        |  12 ++
>  src/libcamera/sensor/camera_sensor_legacy.cpp |  18 +++
>  .../sensor/camera_sensor_properties.cpp       | 121 ++++++++++++++++++
>  5 files changed, 163 insertions(+)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 8aafd82e..bbdb83a1 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -20,6 +20,7 @@
>  #include <libcamera/orientation.h>
>  #include <libcamera/transform.h>
>
> +#include "libcamera/internal/camera_sensor_properties.h"
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>
> @@ -73,6 +74,7 @@ public:
>  	virtual const std::vector<controls::draft::TestPatternModeEnum> &
>  	testPatternModes() const = 0;
>  	virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;
> +	virtual const CameraSensorProperties::SensorDelays &sensorDelays() = 0;
>  };
>
>  class CameraSensorFactoryBase
> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
> index 480ac121..fe40bd7f 100644
> --- a/include/libcamera/internal/camera_sensor_properties.h
> +++ b/include/libcamera/internal/camera_sensor_properties.h
> @@ -8,6 +8,7 @@
>  #pragma once
>
>  #include <map>
> +#include <stdint.h>
>  #include <string>
>
>  #include <libcamera/control_ids.h>
> @@ -20,6 +21,15 @@ struct CameraSensorProperties {
>
>  	Size unitCellSize;
>  	std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
> +
> +	struct SensorDelays {
> +		uint8_t exposureDelay;
> +		uint8_t gainDelay;
> +		uint8_t vblankDelay;
> +		uint8_t hblankDelay;
> +	} sensorDelays;
>  };
>
> +extern const CameraSensorProperties::SensorDelays defaultSensorDelays;
> +

Fine. I thought we could have defined this inside
CameraSensor::sensorDelays() as

const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
{
        static constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
                .exposureDelay = 2,
                .gainDelay = 1,
                .vblankDelay = 2,
                .hblankDelay = 2,
        };
	if (!staticProps_ ||
	    (!staticProps_->sensorDelays.exposureDelay &&
	     !staticProps_->sensorDelays.gainDelay &&
	     !staticProps_->sensorDelays.vblankDelay &&
	     !staticProps_->sensorDelays.hblankDelay)) {
		LOG(CameraSensor, Warning)
			<< "No sensor delays found in static properties. "
			   "Assuming unverified defaults.";

		return defaultSensorDelays;
	}

	return staticProps_->sensorDelays;
}

But then I realized that if we introduced a new derived CameraSensor
class this will have to be replicated in two places. Btw, the
function will have to replicated anyhow as we don't have any concrete
base class between the abstract CameraSensor and
CameraSensorLegacy/CameraSensorRaw.

We might want to consider

        +----------------------------+
        |  Abstract                  |
        |  CameraSensorInterface     |
        +----------------------------+
                      ^
                      |
        +----------------------------+
        |     CameraSensorBase       |
        +----------------------------+
                      ^
        --------------|--------------
        |                           |
        |                           |
 +-------------+              +-------------+
 |   Legacy    |              |     RAW     |
 +-------------+              +-------------+

Where to group common functions like this one. Let's consider it when
adding the new RAW class. I wonder that for the time being defaultSensorDelays
shouldn't be moved inside CameraSensorLegacy::sensorDelays().



>  } /* namespace libcamera */
> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> index 54cf98b2..e9ca291c 100644
> --- a/src/libcamera/sensor/camera_sensor.cpp
> +++ b/src/libcamera/sensor/camera_sensor.cpp
> @@ -336,6 +336,18 @@ CameraSensor::~CameraSensor() = default;
>   * pattern mode for every frame thus incurs no performance penalty.
>   */
>
> +/**
> + * \fn CameraSensor::sensorDelays()
> + * \brief Fetch the sensor delay values
> + *
> + * This function fills in sensor control delays for pipeline handlers to use to

s/fills in/retreieves the/

> + * inform the DelayedControls. If no static properties are available it returns
> + * a reference to the default sensor delays.

What are default ? How have they been measured and how accurate they
are ? I would

* inform the DelayedControls. If controls delays are not specied in the static
* sensor properties database, this function returns a reference to a
* set of default sensor delays provided as best-effort place-holders
* for the actual sensor-specific delays.
*
* \sa defaultSensorDelays

> + *
> + * \return A reference to a struct CameraSensorProperties::SensorDelays holding
> + * the delay values.

remove the full stop please

> + */
> +
>  /**
>   * \class CameraSensorFactoryBase
>   * \brief Base class for camera sensor factories
> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> index a9b15c03..f9202606 100644
> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> @@ -95,6 +95,7 @@ public:
>  	const std::vector<controls::draft::TestPatternModeEnum> &
>  	testPatternModes() const override { return testPatternModes_; }
>  	int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;
> +	const CameraSensorProperties::SensorDelays &sensorDelays() override;
>
>  protected:
>  	std::string logPrefix() const override;
> @@ -482,6 +483,23 @@ void CameraSensorLegacy::initStaticProperties()
>  	initTestPatternModes();
>  }
>
> +const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
> +{
> +	if (!staticProps_ ||
> +	    (!staticProps_->sensorDelays.exposureDelay &&
> +	     !staticProps_->sensorDelays.gainDelay &&
> +	     !staticProps_->sensorDelays.vblankDelay &&
> +	     !staticProps_->sensorDelays.hblankDelay)) {
> +		LOG(CameraSensor, Warning)
> +			<< "No sensor delays found in static properties. "
> +			   "Assuming unverified defaults.";
> +
> +		return defaultSensorDelays;
> +	}
> +
> +	return staticProps_->sensorDelays;
> +}
> +
>  void CameraSensorLegacy::initTestPatternModes()
>  {
>  	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> index 6d4136d0..97202bee 100644
> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> @@ -41,7 +41,51 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)
>   * \brief Map that associates the TestPattern control value with the indexes of
>   * the corresponding sensor test pattern modes as returned by
>   * V4L2_CID_TEST_PATTERN.
> + *
> + * \var CameraSensorProperties::sensorDelays
> + * \brief Sensor control application delays.
> + *
> + * This struct may be defined as empty, in which case the CameraSensor
> + * derivative should provide some appropriate default values.

True this is internal stuff, but this seems an implementation detail I
would

* This struct may be defined as empty if the actual sensor delays are
* not available or have not been measured. Best-effort default values
* are returned in this case.
*
* \sa defaultSensorDelays

> + */
> +
> +/**
> + * \struct CameraSensorProperties::SensorDelays
> + * \brief Sensor control application delay values
> + *
> + * This struct holds delay values, expressed in number of frames, between the
> + * time a control value is applied to the sensor and the time that value is
> + * reflected in the output. For example "2 frames delay" means that parameters
> + * set during frame N will take effect for frame N+2 (and by extension a delay
> + * of 0 would mean the parameter is applied immediately to the current frame).

"to the current frame", if you mean the one currently being exposed, seems
unlikely to me, as I presume changes take effect at least in the next frame.

> + *
> + * \var CameraSensorProperties::SensorDelays::exposureDelay
> + * \brief Number of frames between application of exposure control and effect
> + *
> + * \var CameraSensorProperties::SensorDelays::gainDelay
> + * \brief Number of frames between application of analogue gain control and effect

Will we ever have digital gain here ?

> + *
> + * \var CameraSensorProperties::SensorDelays::vblankDelay
> + * \brief Number of frames between application of vblank control and effect
> + *
> + * \var CameraSensorProperties::SensorDelays::hblankDelay
> + * \brief Number of frames between application of hblank control and effect
> + */
> +
> +/**
> + * \brief Default sensor control application delay values

"application" sounds confusing to me, I would drop it.

> + *
> + * These sensor control delays are intended to be used where no specific values
> + * are defined for a particular sensor in its CameraSensorProperties. These are
> + * not verified for use with any particular sensor and so should be used with
> + * caution.
>   */
> +constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
> +	.exposureDelay = 2,
> +	.gainDelay = 1,
> +	.vblankDelay = 2,
> +	.hblankDelay = 2,
> +};
>
>  /**
>   * \brief Retrieve the properties associated with a sensor
> @@ -60,6 +104,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeColorBars, 2 },
>  				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>  			},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 2,
> +				.hblankDelay = 2
> +			},
>  		} },
>  		{ "ar0521", {
>  			.unitCellSize = { 2200, 2200 },
> @@ -69,6 +119,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeColorBars, 2 },
>  				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "hi846", {
>  			.unitCellSize = { 1120, 1120 },
> @@ -87,6 +138,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				 * 9: "Resolution Pattern"
>  				 */
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "imx214", {
>  			.unitCellSize = { 1120, 1120 },
> @@ -97,6 +149,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>  				{ controls::draft::TestPatternModePn9, 4 },
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "imx219", {
>  			.unitCellSize = { 1120, 1120 },
> @@ -107,6 +160,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>  				{ controls::draft::TestPatternModePn9, 4 },
>  			},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 1,
> +				.vblankDelay = 2,
> +				.hblankDelay = 2
> +			},
>  		} },
>  		{ "imx258", {
>  			.unitCellSize = { 1120, 1120 },
> @@ -117,34 +176,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>  				{ controls::draft::TestPatternModePn9, 4 },
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "imx283", {
>  			.unitCellSize = { 2400, 2400 },
>  			.testPatternModes = {},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 2,
> +				.hblankDelay = 2
> +			},
>  		} },
>  		{ "imx290", {
>  			.unitCellSize = { 2900, 2900 },
>  			.testPatternModes = {},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 2,
> +				.hblankDelay = 2
> +			},
>  		} },
>  		{ "imx296", {
>  			.unitCellSize = { 3450, 3450 },
>  			.testPatternModes = {},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 2,
> +				.hblankDelay = 2
> +			},
>  		} },
>  		{ "imx327", {
>  			.unitCellSize = { 2900, 2900 },
>  			.testPatternModes = {},
> +			.sensorDelays = { },
>  		} },
>  		{ "imx335", {
>  			.unitCellSize = { 2000, 2000 },
>  			.testPatternModes = {},
> +			.sensorDelays = { },
>  		} },
>  		{ "imx415", {
>  			.unitCellSize = { 1450, 1450 },
>  			.testPatternModes = {},
> +			.sensorDelays = { },
>  		} },
>  		{ "imx477", {
>  			.unitCellSize = { 1550, 1550 },
>  			.testPatternModes = {},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 3,
> +				.hblankDelay = 3
> +			},
>  		} },
>  		{ "imx519", {
>  			.unitCellSize = { 1220, 1220 },
> @@ -157,6 +244,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				 * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).
>  				 */
>  			},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 3,
> +				.hblankDelay = 3
> +			},
>  		} },
>  		{ "imx708", {
>  			.unitCellSize = { 1400, 1400 },
> @@ -167,6 +260,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>  				{ controls::draft::TestPatternModePn9, 4 },
>  			},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 3,
> +				.hblankDelay = 3
> +			},
>  		} },
>  		{ "ov2685", {
>  			.unitCellSize = { 1750, 1750 },
> @@ -181,6 +280,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				 * 5: "Color Square"
>  				 */
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "ov2740", {
>  			.unitCellSize = { 1400, 1400 },
> @@ -188,6 +288,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeOff, 0 },
>  				{ controls::draft::TestPatternModeColorBars, 1},
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "ov4689", {
>  			.unitCellSize = { 2000, 2000 },
> @@ -201,6 +302,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				 * colorBarType2 and colorBarType3.
>  				 */
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "ov5640", {
>  			.unitCellSize = { 1400, 1400 },
> @@ -208,10 +310,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeOff, 0 },
>  				{ controls::draft::TestPatternModeColorBars, 1 },
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "ov5647", {
>  			.unitCellSize = { 1400, 1400 },
>  			.testPatternModes = {},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 2,
> +				.hblankDelay = 2
> +			},
>  		} },
>  		{ "ov5670", {
>  			.unitCellSize = { 1120, 1120 },
> @@ -219,6 +328,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeOff, 0 },
>  				{ controls::draft::TestPatternModeColorBars, 1 },
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "ov5675", {
>  			.unitCellSize = { 1120, 1120 },
> @@ -226,6 +336,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeOff, 0 },
>  				{ controls::draft::TestPatternModeColorBars, 1 },
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "ov5693", {
>  			.unitCellSize = { 1400, 1400 },
> @@ -238,6 +349,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				 * Rolling Bar".
>  				 */
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "ov64a40", {
>  			.unitCellSize = { 1008, 1008 },
> @@ -251,6 +363,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				 * 4: "Vertical Color Bar Type 4"
>  				 */
>  			},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 2,
> +				.hblankDelay = 2
> +			},
>  		} },
>  		{ "ov8858", {
>  			.unitCellSize = { 1120, 1120 },
> @@ -264,6 +382,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				 * 4: "Vertical Color Bar Type 4"
>  				 */
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "ov8865", {
>  			.unitCellSize = { 1400, 1400 },
> @@ -278,6 +397,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				 * 5: "Color squares with rolling bar"
>  				 */
>  			},
> +			.sensorDelays = { },
>  		} },
>  		{ "ov13858", {
>  			.unitCellSize = { 1120, 1120 },
> @@ -285,6 +405,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeOff, 0 },
>  				{ controls::draft::TestPatternModeColorBars, 1 },
>  			},
> +			.sensorDelays = { },
>  		} },
>  	};
>
> --
> 2.34.1
>

Hi Jacopo - thank you for the review

On 19/11/2024 15:57, Jacopo Mondi wrote:
> Hi Dan
>
> On Tue, Nov 19, 2024 at 01:03:00PM +0000, Daniel Scally wrote:
>> Add properties covering the sensor control application delays to both
>> the static CameraSensorProperties definitions. The values used are the
>> defaults that're in use across the library, with deviations from that
> This probably needs to be updated

Indeed

>
>> taken from Raspberry Pi's CamHelper class definitions.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> Changes in v4:
>>
>> 	- getSensorDelays() renamed to sensorDelays()
>> 	- sensorDelays() returns a reference to the SensorDelays struct rather
>> 	  than each of the values.
>> 	- Reworded the comments to make the default values seem less acceptable
>> 	- Added delay values for AR0144
>>
>> Changes in v3:
>>
>> 	- Rebased on top of the CameraSensorFactory introduction
>> 	- Some rephrasing
>> 	- Defined the sensorDelays member as empty where Raspberry Pi didn't
>> 	  have any specific values. Check for the empty struct in
>> 	  getSensorDelays() and return the defaults from there with a warning.
>>
>> Changes in v2:
>>
>> 	- Rather than adding the delays to the properties ControlList, added a
>> 	  new function in CameraSensor that allows PipelineHandlers to retreive
>> 	  the delay values.
>>
>>   include/libcamera/internal/camera_sensor.h    |   2 +
>>   .../internal/camera_sensor_properties.h       |  10 ++
>>   src/libcamera/sensor/camera_sensor.cpp        |  12 ++
>>   src/libcamera/sensor/camera_sensor_legacy.cpp |  18 +++
>>   .../sensor/camera_sensor_properties.cpp       | 121 ++++++++++++++++++
>>   5 files changed, 163 insertions(+)
>>
>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
>> index 8aafd82e..bbdb83a1 100644
>> --- a/include/libcamera/internal/camera_sensor.h
>> +++ b/include/libcamera/internal/camera_sensor.h
>> @@ -20,6 +20,7 @@
>>   #include <libcamera/orientation.h>
>>   #include <libcamera/transform.h>
>>
>> +#include "libcamera/internal/camera_sensor_properties.h"
>>   #include "libcamera/internal/bayer_format.h"
>>   #include "libcamera/internal/v4l2_subdevice.h"
>>
>> @@ -73,6 +74,7 @@ public:
>>   	virtual const std::vector<controls::draft::TestPatternModeEnum> &
>>   	testPatternModes() const = 0;
>>   	virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;
>> +	virtual const CameraSensorProperties::SensorDelays &sensorDelays() = 0;
>>   };
>>
>>   class CameraSensorFactoryBase
>> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
>> index 480ac121..fe40bd7f 100644
>> --- a/include/libcamera/internal/camera_sensor_properties.h
>> +++ b/include/libcamera/internal/camera_sensor_properties.h
>> @@ -8,6 +8,7 @@
>>   #pragma once
>>
>>   #include <map>
>> +#include <stdint.h>
>>   #include <string>
>>
>>   #include <libcamera/control_ids.h>
>> @@ -20,6 +21,15 @@ struct CameraSensorProperties {
>>
>>   	Size unitCellSize;
>>   	std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
>> +
>> +	struct SensorDelays {
>> +		uint8_t exposureDelay;
>> +		uint8_t gainDelay;
>> +		uint8_t vblankDelay;
>> +		uint8_t hblankDelay;
>> +	} sensorDelays;
>>   };
>>
>> +extern const CameraSensorProperties::SensorDelays defaultSensorDelays;
>> +
> Fine. I thought we could have defined this inside
> CameraSensor::sensorDelays() as
>
> const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
> {
>          static constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
>                  .exposureDelay = 2,
>                  .gainDelay = 1,
>                  .vblankDelay = 2,
>                  .hblankDelay = 2,
>          };
Are references to that safe from outside the function scope? If so then sure we could do that instead...
> 	if (!staticProps_ ||
> 	    (!staticProps_->sensorDelays.exposureDelay &&
> 	     !staticProps_->sensorDelays.gainDelay &&
> 	     !staticProps_->sensorDelays.vblankDelay &&
> 	     !staticProps_->sensorDelays.hblankDelay)) {
> 		LOG(CameraSensor, Warning)
> 			<< "No sensor delays found in static properties. "
> 			   "Assuming unverified defaults.";
>
> 		return defaultSensorDelays;
> 	}
>
> 	return staticProps_->sensorDelays;
> }
>
> But then I realized that if we introduced a new derived CameraSensor
> class this will have to be replicated in two places. Btw, the
> function will have to replicated anyhow as we don't have any concrete
> base class between the abstract CameraSensor and
> CameraSensorLegacy/CameraSensorRaw.
I don't know that that's such a problem...I'm not following the plans for CameraSensorRaw but I 
wondered about a CameraSensorTPG or something and we'd presumably want to define the delays 
differently there anyway.
>
> We might want to consider
>
>          +----------------------------+
>          |  Abstract                  |
>          |  CameraSensorInterface     |
>          +----------------------------+
>                        ^
>                        |
>          +----------------------------+
>          |     CameraSensorBase       |
>          +----------------------------+
>                        ^
>          --------------|--------------
>          |                           |
>          |                           |
>   +-------------+              +-------------+
>   |   Legacy    |              |     RAW     |
>   +-------------+              +-------------+
>
> Where to group common functions like this one. Let's consider it when
> adding the new RAW class. I wonder that for the time being defaultSensorDelays
> shouldn't be moved inside CameraSensorLegacy::sensorDelays().
>
>
>
>>   } /* namespace libcamera */
>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
>> index 54cf98b2..e9ca291c 100644
>> --- a/src/libcamera/sensor/camera_sensor.cpp
>> +++ b/src/libcamera/sensor/camera_sensor.cpp
>> @@ -336,6 +336,18 @@ CameraSensor::~CameraSensor() = default;
>>    * pattern mode for every frame thus incurs no performance penalty.
>>    */
>>
>> +/**
>> + * \fn CameraSensor::sensorDelays()
>> + * \brief Fetch the sensor delay values
>> + *
>> + * This function fills in sensor control delays for pipeline handlers to use to
> s/fills in/retreieves the/
ack
>
>> + * inform the DelayedControls. If no static properties are available it returns
>> + * a reference to the default sensor delays.
> What are default ? How have they been measured and how accurate they
> are ? I would
>
> * inform the DelayedControls. If controls delays are not specied in the static
> * sensor properties database, this function returns a reference to a
> * set of default sensor delays provided as best-effort place-holders
> * for the actual sensor-specific delays.
> *
> * \sa defaultSensorDelays


Sure that sounds fine to me

>
>> + *
>> + * \return A reference to a struct CameraSensorProperties::SensorDelays holding
>> + * the delay values.
> remove the full stop please


Okedokey

>
>> + */
>> +
>>   /**
>>    * \class CameraSensorFactoryBase
>>    * \brief Base class for camera sensor factories
>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
>> index a9b15c03..f9202606 100644
>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
>> @@ -95,6 +95,7 @@ public:
>>   	const std::vector<controls::draft::TestPatternModeEnum> &
>>   	testPatternModes() const override { return testPatternModes_; }
>>   	int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;
>> +	const CameraSensorProperties::SensorDelays &sensorDelays() override;
>>
>>   protected:
>>   	std::string logPrefix() const override;
>> @@ -482,6 +483,23 @@ void CameraSensorLegacy::initStaticProperties()
>>   	initTestPatternModes();
>>   }
>>
>> +const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
>> +{
>> +	if (!staticProps_ ||
>> +	    (!staticProps_->sensorDelays.exposureDelay &&
>> +	     !staticProps_->sensorDelays.gainDelay &&
>> +	     !staticProps_->sensorDelays.vblankDelay &&
>> +	     !staticProps_->sensorDelays.hblankDelay)) {
>> +		LOG(CameraSensor, Warning)
>> +			<< "No sensor delays found in static properties. "
>> +			   "Assuming unverified defaults.";
>> +
>> +		return defaultSensorDelays;
>> +	}
>> +
>> +	return staticProps_->sensorDelays;
>> +}
>> +
>>   void CameraSensorLegacy::initTestPatternModes()
>>   {
>>   	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
>> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
>> index 6d4136d0..97202bee 100644
>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
>> @@ -41,7 +41,51 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)
>>    * \brief Map that associates the TestPattern control value with the indexes of
>>    * the corresponding sensor test pattern modes as returned by
>>    * V4L2_CID_TEST_PATTERN.
>> + *
>> + * \var CameraSensorProperties::sensorDelays
>> + * \brief Sensor control application delays.
>> + *
>> + * This struct may be defined as empty, in which case the CameraSensor
>> + * derivative should provide some appropriate default values.
> True this is internal stuff, but this seems an implementation detail I
> would
>
> * This struct may be defined as empty if the actual sensor delays are
> * not available or have not been measured. Best-effort default values
> * are returned in this case.
> *
> * \sa defaultSensorDelays


Okedokey

>
>> + */
>> +
>> +/**
>> + * \struct CameraSensorProperties::SensorDelays
>> + * \brief Sensor control application delay values
>> + *
>> + * This struct holds delay values, expressed in number of frames, between the
>> + * time a control value is applied to the sensor and the time that value is
>> + * reflected in the output. For example "2 frames delay" means that parameters
>> + * set during frame N will take effect for frame N+2 (and by extension a delay
>> + * of 0 would mean the parameter is applied immediately to the current frame).
> "to the current frame", if you mean the one currently being exposed, seems
> unlikely to me, as I presume changes take effect at least in the next frame.

Me too, I just took Laurent's wording as I thought it made it very clear.

>
>> + *
>> + * \var CameraSensorProperties::SensorDelays::exposureDelay
>> + * \brief Number of frames between application of exposure control and effect
>> + *
>> + * \var CameraSensorProperties::SensorDelays::gainDelay
>> + * \brief Number of frames between application of analogue gain control and effect
> Will we ever have digital gain here ?
Possibly - it's a topic for the discussion with Anthony tomorrow in fact.
>
>> + *
>> + * \var CameraSensorProperties::SensorDelays::vblankDelay
>> + * \brief Number of frames between application of vblank control and effect
>> + *
>> + * \var CameraSensorProperties::SensorDelays::hblankDelay
>> + * \brief Number of frames between application of hblank control and effect
>> + */
>> +
>> +/**
>> + * \brief Default sensor control application delay values
> "application" sounds confusing to me, I would drop it.
Hm, application in this sense meaning "the action of putting something into operation", does just 
"sensor control delay values" convey that meaning already? If so it's fine to drop.
>
>> + *
>> + * These sensor control delays are intended to be used where no specific values
>> + * are defined for a particular sensor in its CameraSensorProperties. These are
>> + * not verified for use with any particular sensor and so should be used with
>> + * caution.
>>    */
>> +constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
>> +	.exposureDelay = 2,
>> +	.gainDelay = 1,
>> +	.vblankDelay = 2,
>> +	.hblankDelay = 2,
>> +};
>>
>>   /**
>>    * \brief Retrieve the properties associated with a sensor
>> @@ -60,6 +104,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeColorBars, 2 },
>>   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>>   			},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 2,
>> +				.hblankDelay = 2
>> +			},
>>   		} },
>>   		{ "ar0521", {
>>   			.unitCellSize = { 2200, 2200 },
>> @@ -69,6 +119,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeColorBars, 2 },
>>   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "hi846", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -87,6 +138,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * 9: "Resolution Pattern"
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "imx214", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -97,6 +149,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>>   				{ controls::draft::TestPatternModePn9, 4 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "imx219", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -107,6 +160,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>>   				{ controls::draft::TestPatternModePn9, 4 },
>>   			},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 1,
>> +				.vblankDelay = 2,
>> +				.hblankDelay = 2
>> +			},
>>   		} },
>>   		{ "imx258", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -117,34 +176,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>>   				{ controls::draft::TestPatternModePn9, 4 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "imx283", {
>>   			.unitCellSize = { 2400, 2400 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 2,
>> +				.hblankDelay = 2
>> +			},
>>   		} },
>>   		{ "imx290", {
>>   			.unitCellSize = { 2900, 2900 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 2,
>> +				.hblankDelay = 2
>> +			},
>>   		} },
>>   		{ "imx296", {
>>   			.unitCellSize = { 3450, 3450 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 2,
>> +				.hblankDelay = 2
>> +			},
>>   		} },
>>   		{ "imx327", {
>>   			.unitCellSize = { 2900, 2900 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "imx335", {
>>   			.unitCellSize = { 2000, 2000 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "imx415", {
>>   			.unitCellSize = { 1450, 1450 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "imx477", {
>>   			.unitCellSize = { 1550, 1550 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 3,
>> +				.hblankDelay = 3
>> +			},
>>   		} },
>>   		{ "imx519", {
>>   			.unitCellSize = { 1220, 1220 },
>> @@ -157,6 +244,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).
>>   				 */
>>   			},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 3,
>> +				.hblankDelay = 3
>> +			},
>>   		} },
>>   		{ "imx708", {
>>   			.unitCellSize = { 1400, 1400 },
>> @@ -167,6 +260,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>>   				{ controls::draft::TestPatternModePn9, 4 },
>>   			},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 3,
>> +				.hblankDelay = 3
>> +			},
>>   		} },
>>   		{ "ov2685", {
>>   			.unitCellSize = { 1750, 1750 },
>> @@ -181,6 +280,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * 5: "Color Square"
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov2740", {
>>   			.unitCellSize = { 1400, 1400 },
>> @@ -188,6 +288,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeOff, 0 },
>>   				{ controls::draft::TestPatternModeColorBars, 1},
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov4689", {
>>   			.unitCellSize = { 2000, 2000 },
>> @@ -201,6 +302,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * colorBarType2 and colorBarType3.
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov5640", {
>>   			.unitCellSize = { 1400, 1400 },
>> @@ -208,10 +310,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeOff, 0 },
>>   				{ controls::draft::TestPatternModeColorBars, 1 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov5647", {
>>   			.unitCellSize = { 1400, 1400 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 2,
>> +				.hblankDelay = 2
>> +			},
>>   		} },
>>   		{ "ov5670", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -219,6 +328,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeOff, 0 },
>>   				{ controls::draft::TestPatternModeColorBars, 1 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov5675", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -226,6 +336,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeOff, 0 },
>>   				{ controls::draft::TestPatternModeColorBars, 1 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov5693", {
>>   			.unitCellSize = { 1400, 1400 },
>> @@ -238,6 +349,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * Rolling Bar".
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov64a40", {
>>   			.unitCellSize = { 1008, 1008 },
>> @@ -251,6 +363,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * 4: "Vertical Color Bar Type 4"
>>   				 */
>>   			},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 2,
>> +				.hblankDelay = 2
>> +			},
>>   		} },
>>   		{ "ov8858", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -264,6 +382,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * 4: "Vertical Color Bar Type 4"
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov8865", {
>>   			.unitCellSize = { 1400, 1400 },
>> @@ -278,6 +397,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * 5: "Color squares with rolling bar"
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov13858", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -285,6 +405,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeOff, 0 },
>>   				{ controls::draft::TestPatternModeColorBars, 1 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   	};
>>
>> --
>> 2.34.1
>>

Hi Dan

On Tue, Nov 19, 2024 at 04:20:16PM +0000, Dan Scally wrote:
> Hi Jacopo - thank you for the review
>
> On 19/11/2024 15:57, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Tue, Nov 19, 2024 at 01:03:00PM +0000, Daniel Scally wrote:
> > > Add properties covering the sensor control application delays to both
> > > the static CameraSensorProperties definitions. The values used are the
> > > defaults that're in use across the library, with deviations from that
> > This probably needs to be updated
>
> Indeed
>
> >
> > > taken from Raspberry Pi's CamHelper class definitions.
> > >
> > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > ---
> > > Changes in v4:
> > >
> > > 	- getSensorDelays() renamed to sensorDelays()
> > > 	- sensorDelays() returns a reference to the SensorDelays struct rather
> > > 	  than each of the values.
> > > 	- Reworded the comments to make the default values seem less acceptable
> > > 	- Added delay values for AR0144
> > >
> > > Changes in v3:
> > >
> > > 	- Rebased on top of the CameraSensorFactory introduction
> > > 	- Some rephrasing
> > > 	- Defined the sensorDelays member as empty where Raspberry Pi didn't
> > > 	  have any specific values. Check for the empty struct in
> > > 	  getSensorDelays() and return the defaults from there with a warning.
> > >
> > > Changes in v2:
> > >
> > > 	- Rather than adding the delays to the properties ControlList, added a
> > > 	  new function in CameraSensor that allows PipelineHandlers to retreive
> > > 	  the delay values.
> > >
> > >   include/libcamera/internal/camera_sensor.h    |   2 +
> > >   .../internal/camera_sensor_properties.h       |  10 ++
> > >   src/libcamera/sensor/camera_sensor.cpp        |  12 ++
> > >   src/libcamera/sensor/camera_sensor_legacy.cpp |  18 +++
> > >   .../sensor/camera_sensor_properties.cpp       | 121 ++++++++++++++++++
> > >   5 files changed, 163 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index 8aafd82e..bbdb83a1 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -20,6 +20,7 @@
> > >   #include <libcamera/orientation.h>
> > >   #include <libcamera/transform.h>
> > >
> > > +#include "libcamera/internal/camera_sensor_properties.h"
> > >   #include "libcamera/internal/bayer_format.h"
> > >   #include "libcamera/internal/v4l2_subdevice.h"
> > >
> > > @@ -73,6 +74,7 @@ public:
> > >   	virtual const std::vector<controls::draft::TestPatternModeEnum> &
> > >   	testPatternModes() const = 0;
> > >   	virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;
> > > +	virtual const CameraSensorProperties::SensorDelays &sensorDelays() = 0;
> > >   };
> > >
> > >   class CameraSensorFactoryBase
> > > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
> > > index 480ac121..fe40bd7f 100644
> > > --- a/include/libcamera/internal/camera_sensor_properties.h
> > > +++ b/include/libcamera/internal/camera_sensor_properties.h
> > > @@ -8,6 +8,7 @@
> > >   #pragma once
> > >
> > >   #include <map>
> > > +#include <stdint.h>
> > >   #include <string>
> > >
> > >   #include <libcamera/control_ids.h>
> > > @@ -20,6 +21,15 @@ struct CameraSensorProperties {
> > >
> > >   	Size unitCellSize;
> > >   	std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
> > > +
> > > +	struct SensorDelays {
> > > +		uint8_t exposureDelay;
> > > +		uint8_t gainDelay;
> > > +		uint8_t vblankDelay;
> > > +		uint8_t hblankDelay;
> > > +	} sensorDelays;
> > >   };
> > >
> > > +extern const CameraSensorProperties::SensorDelays defaultSensorDelays;
> > > +
> > Fine. I thought we could have defined this inside
> > CameraSensor::sensorDelays() as
> >
> > const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
> > {
> >          static constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
> >                  .exposureDelay = 2,
> >                  .gainDelay = 1,
> >                  .vblankDelay = 2,
> >                  .hblankDelay = 2,
> >          };
> Are references to that safe from outside the function scope? If so then sure we could do that instead...

I don't see any code reference outside of this, but indeed
documentation links would be broken.



> > 	if (!staticProps_ ||
> > 	    (!staticProps_->sensorDelays.exposureDelay &&
> > 	     !staticProps_->sensorDelays.gainDelay &&
> > 	     !staticProps_->sensorDelays.vblankDelay &&
> > 	     !staticProps_->sensorDelays.hblankDelay)) {
> > 		LOG(CameraSensor, Warning)
> > 			<< "No sensor delays found in static properties. "
> > 			   "Assuming unverified defaults.";
> >
> > 		return defaultSensorDelays;
> > 	}
> >
> > 	return staticProps_->sensorDelays;
> > }
> >
> > But then I realized that if we introduced a new derived CameraSensor
> > class this will have to be replicated in two places. Btw, the
> > function will have to replicated anyhow as we don't have any concrete
> > base class between the abstract CameraSensor and
> > CameraSensorLegacy/CameraSensorRaw.
> I don't know that that's such a problem...I'm not following the plans for
> CameraSensorRaw but I wondered about a CameraSensorTPG or something and we'd
> presumably want to define the delays differently there anyway.

Fair point

> >
> > We might want to consider
> >
> >          +----------------------------+
> >          |  Abstract                  |
> >          |  CameraSensorInterface     |
> >          +----------------------------+
> >                        ^
> >                        |
> >          +----------------------------+
> >          |     CameraSensorBase       |
> >          +----------------------------+
> >                        ^
> >          --------------|--------------
> >          |                           |
> >          |                           |
> >   +-------------+              +-------------+
> >   |   Legacy    |              |     RAW     |
> >   +-------------+              +-------------+
> >
> > Where to group common functions like this one. Let's consider it when
> > adding the new RAW class. I wonder that for the time being defaultSensorDelays
> > shouldn't be moved inside CameraSensorLegacy::sensorDelays().
> >
> >
> >
> > >   } /* namespace libcamera */
> > > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> > > index 54cf98b2..e9ca291c 100644
> > > --- a/src/libcamera/sensor/camera_sensor.cpp
> > > +++ b/src/libcamera/sensor/camera_sensor.cpp
> > > @@ -336,6 +336,18 @@ CameraSensor::~CameraSensor() = default;
> > >    * pattern mode for every frame thus incurs no performance penalty.
> > >    */
> > >
> > > +/**
> > > + * \fn CameraSensor::sensorDelays()
> > > + * \brief Fetch the sensor delay values
> > > + *
> > > + * This function fills in sensor control delays for pipeline handlers to use to
> > s/fills in/retreieves the/
> ack
> >
> > > + * inform the DelayedControls. If no static properties are available it returns
> > > + * a reference to the default sensor delays.
> > What are default ? How have they been measured and how accurate they
> > are ? I would
> >
> > * inform the DelayedControls. If controls delays are not specied in the static
> > * sensor properties database, this function returns a reference to a
> > * set of default sensor delays provided as best-effort place-holders
> > * for the actual sensor-specific delays.
> > *
> > * \sa defaultSensorDelays
>
>
> Sure that sounds fine to me
>
> >
> > > + *
> > > + * \return A reference to a struct CameraSensorProperties::SensorDelays holding
> > > + * the delay values.
> > remove the full stop please
>
>
> Okedokey
>
> >
> > > + */
> > > +
> > >   /**
> > >    * \class CameraSensorFactoryBase
> > >    * \brief Base class for camera sensor factories
> > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > index a9b15c03..f9202606 100644
> > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > @@ -95,6 +95,7 @@ public:
> > >   	const std::vector<controls::draft::TestPatternModeEnum> &
> > >   	testPatternModes() const override { return testPatternModes_; }
> > >   	int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;
> > > +	const CameraSensorProperties::SensorDelays &sensorDelays() override;
> > >
> > >   protected:
> > >   	std::string logPrefix() const override;
> > > @@ -482,6 +483,23 @@ void CameraSensorLegacy::initStaticProperties()
> > >   	initTestPatternModes();
> > >   }
> > >
> > > +const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
> > > +{
> > > +	if (!staticProps_ ||
> > > +	    (!staticProps_->sensorDelays.exposureDelay &&
> > > +	     !staticProps_->sensorDelays.gainDelay &&
> > > +	     !staticProps_->sensorDelays.vblankDelay &&
> > > +	     !staticProps_->sensorDelays.hblankDelay)) {
> > > +		LOG(CameraSensor, Warning)
> > > +			<< "No sensor delays found in static properties. "
> > > +			   "Assuming unverified defaults.";
> > > +
> > > +		return defaultSensorDelays;
> > > +	}
> > > +
> > > +	return staticProps_->sensorDelays;
> > > +}
> > > +
> > >   void CameraSensorLegacy::initTestPatternModes()
> > >   {
> > >   	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > index 6d4136d0..97202bee 100644
> > > --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> > > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > @@ -41,7 +41,51 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)
> > >    * \brief Map that associates the TestPattern control value with the indexes of
> > >    * the corresponding sensor test pattern modes as returned by
> > >    * V4L2_CID_TEST_PATTERN.
> > > + *
> > > + * \var CameraSensorProperties::sensorDelays
> > > + * \brief Sensor control application delays.
> > > + *
> > > + * This struct may be defined as empty, in which case the CameraSensor
> > > + * derivative should provide some appropriate default values.
> > True this is internal stuff, but this seems an implementation detail I
> > would
> >
> > * This struct may be defined as empty if the actual sensor delays are
> > * not available or have not been measured. Best-effort default values
> > * are returned in this case.
> > *
> > * \sa defaultSensorDelays
>
>
> Okedokey
>
> >
> > > + */
> > > +
> > > +/**
> > > + * \struct CameraSensorProperties::SensorDelays
> > > + * \brief Sensor control application delay values
> > > + *
> > > + * This struct holds delay values, expressed in number of frames, between the
> > > + * time a control value is applied to the sensor and the time that value is
> > > + * reflected in the output. For example "2 frames delay" means that parameters
> > > + * set during frame N will take effect for frame N+2 (and by extension a delay
> > > + * of 0 would mean the parameter is applied immediately to the current frame).
> > "to the current frame", if you mean the one currently being exposed, seems
> > unlikely to me, as I presume changes take effect at least in the next frame.
>
> Me too, I just took Laurent's wording as I thought it made it very clear.
>

Ah, let's wait for him to comment then

> >
> > > + *
> > > + * \var CameraSensorProperties::SensorDelays::exposureDelay
> > > + * \brief Number of frames between application of exposure control and effect
> > > + *
> > > + * \var CameraSensorProperties::SensorDelays::gainDelay
> > > + * \brief Number of frames between application of analogue gain control and effect
> > Will we ever have digital gain here ?
> Possibly - it's a topic for the discussion with Anthony tomorrow in fact.
> >
> > > + *
> > > + * \var CameraSensorProperties::SensorDelays::vblankDelay
> > > + * \brief Number of frames between application of vblank control and effect
> > > + *
> > > + * \var CameraSensorProperties::SensorDelays::hblankDelay
> > > + * \brief Number of frames between application of hblank control and effect
> > > + */
> > > +
> > > +/**
> > > + * \brief Default sensor control application delay values
> > "application" sounds confusing to me, I would drop it.
> Hm, application in this sense meaning "the action of putting something into
> operation", does just "sensor control delay values" convey that meaning
> already? If so it's fine to drop.

Maybe it's just me being confused by what "application" usually means
then :)

Up to you, really

Thanks
  j

> >
> > > + *
> > > + * These sensor control delays are intended to be used where no specific values
> > > + * are defined for a particular sensor in its CameraSensorProperties. These are
> > > + * not verified for use with any particular sensor and so should be used with
> > > + * caution.
> > >    */
> > > +constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
> > > +	.exposureDelay = 2,
> > > +	.gainDelay = 1,
> > > +	.vblankDelay = 2,
> > > +	.hblankDelay = 2,
> > > +};
> > >
> > >   /**
> > >    * \brief Retrieve the properties associated with a sensor
> > > @@ -60,6 +104,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeColorBars, 2 },
> > >   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > >   			},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 2,
> > > +				.vblankDelay = 2,
> > > +				.hblankDelay = 2
> > > +			},
> > >   		} },
> > >   		{ "ar0521", {
> > >   			.unitCellSize = { 2200, 2200 },
> > > @@ -69,6 +119,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeColorBars, 2 },
> > >   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "hi846", {
> > >   			.unitCellSize = { 1120, 1120 },
> > > @@ -87,6 +138,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				 * 9: "Resolution Pattern"
> > >   				 */
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "imx214", {
> > >   			.unitCellSize = { 1120, 1120 },
> > > @@ -97,6 +149,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > >   				{ controls::draft::TestPatternModePn9, 4 },
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "imx219", {
> > >   			.unitCellSize = { 1120, 1120 },
> > > @@ -107,6 +160,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > >   				{ controls::draft::TestPatternModePn9, 4 },
> > >   			},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 1,
> > > +				.vblankDelay = 2,
> > > +				.hblankDelay = 2
> > > +			},
> > >   		} },
> > >   		{ "imx258", {
> > >   			.unitCellSize = { 1120, 1120 },
> > > @@ -117,34 +176,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > >   				{ controls::draft::TestPatternModePn9, 4 },
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "imx283", {
> > >   			.unitCellSize = { 2400, 2400 },
> > >   			.testPatternModes = {},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 2,
> > > +				.vblankDelay = 2,
> > > +				.hblankDelay = 2
> > > +			},
> > >   		} },
> > >   		{ "imx290", {
> > >   			.unitCellSize = { 2900, 2900 },
> > >   			.testPatternModes = {},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 2,
> > > +				.vblankDelay = 2,
> > > +				.hblankDelay = 2
> > > +			},
> > >   		} },
> > >   		{ "imx296", {
> > >   			.unitCellSize = { 3450, 3450 },
> > >   			.testPatternModes = {},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 2,
> > > +				.vblankDelay = 2,
> > > +				.hblankDelay = 2
> > > +			},
> > >   		} },
> > >   		{ "imx327", {
> > >   			.unitCellSize = { 2900, 2900 },
> > >   			.testPatternModes = {},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "imx335", {
> > >   			.unitCellSize = { 2000, 2000 },
> > >   			.testPatternModes = {},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "imx415", {
> > >   			.unitCellSize = { 1450, 1450 },
> > >   			.testPatternModes = {},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "imx477", {
> > >   			.unitCellSize = { 1550, 1550 },
> > >   			.testPatternModes = {},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 2,
> > > +				.vblankDelay = 3,
> > > +				.hblankDelay = 3
> > > +			},
> > >   		} },
> > >   		{ "imx519", {
> > >   			.unitCellSize = { 1220, 1220 },
> > > @@ -157,6 +244,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				 * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).
> > >   				 */
> > >   			},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 2,
> > > +				.vblankDelay = 3,
> > > +				.hblankDelay = 3
> > > +			},
> > >   		} },
> > >   		{ "imx708", {
> > >   			.unitCellSize = { 1400, 1400 },
> > > @@ -167,6 +260,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > >   				{ controls::draft::TestPatternModePn9, 4 },
> > >   			},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 2,
> > > +				.vblankDelay = 3,
> > > +				.hblankDelay = 3
> > > +			},
> > >   		} },
> > >   		{ "ov2685", {
> > >   			.unitCellSize = { 1750, 1750 },
> > > @@ -181,6 +280,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				 * 5: "Color Square"
> > >   				 */
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "ov2740", {
> > >   			.unitCellSize = { 1400, 1400 },
> > > @@ -188,6 +288,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeOff, 0 },
> > >   				{ controls::draft::TestPatternModeColorBars, 1},
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "ov4689", {
> > >   			.unitCellSize = { 2000, 2000 },
> > > @@ -201,6 +302,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				 * colorBarType2 and colorBarType3.
> > >   				 */
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "ov5640", {
> > >   			.unitCellSize = { 1400, 1400 },
> > > @@ -208,10 +310,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeOff, 0 },
> > >   				{ controls::draft::TestPatternModeColorBars, 1 },
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "ov5647", {
> > >   			.unitCellSize = { 1400, 1400 },
> > >   			.testPatternModes = {},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 2,
> > > +				.vblankDelay = 2,
> > > +				.hblankDelay = 2
> > > +			},
> > >   		} },
> > >   		{ "ov5670", {
> > >   			.unitCellSize = { 1120, 1120 },
> > > @@ -219,6 +328,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeOff, 0 },
> > >   				{ controls::draft::TestPatternModeColorBars, 1 },
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "ov5675", {
> > >   			.unitCellSize = { 1120, 1120 },
> > > @@ -226,6 +336,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeOff, 0 },
> > >   				{ controls::draft::TestPatternModeColorBars, 1 },
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "ov5693", {
> > >   			.unitCellSize = { 1400, 1400 },
> > > @@ -238,6 +349,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				 * Rolling Bar".
> > >   				 */
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "ov64a40", {
> > >   			.unitCellSize = { 1008, 1008 },
> > > @@ -251,6 +363,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				 * 4: "Vertical Color Bar Type 4"
> > >   				 */
> > >   			},
> > > +			.sensorDelays = {
> > > +				.exposureDelay = 2,
> > > +				.gainDelay = 2,
> > > +				.vblankDelay = 2,
> > > +				.hblankDelay = 2
> > > +			},
> > >   		} },
> > >   		{ "ov8858", {
> > >   			.unitCellSize = { 1120, 1120 },
> > > @@ -264,6 +382,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				 * 4: "Vertical Color Bar Type 4"
> > >   				 */
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "ov8865", {
> > >   			.unitCellSize = { 1400, 1400 },
> > > @@ -278,6 +397,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				 * 5: "Color squares with rolling bar"
> > >   				 */
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   		{ "ov13858", {
> > >   			.unitCellSize = { 1120, 1120 },
> > > @@ -285,6 +405,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >   				{ controls::draft::TestPatternModeOff, 0 },
> > >   				{ controls::draft::TestPatternModeColorBars, 1 },
> > >   			},
> > > +			.sensorDelays = { },
> > >   		} },
> > >   	};
> > >
> > > --
> > > 2.34.1
> > >

Quoting Jacopo Mondi (2024-11-19 16:50:43)
> Hi Dan
> 
> On Tue, Nov 19, 2024 at 04:20:16PM +0000, Dan Scally wrote:
> > Hi Jacopo - thank you for the review
> >
> > On 19/11/2024 15:57, Jacopo Mondi wrote:
> > > Hi Dan
> > >
> > > On Tue, Nov 19, 2024 at 01:03:00PM +0000, Daniel Scally wrote:
> > > > Add properties covering the sensor control application delays to both
> > > > the static CameraSensorProperties definitions. The values used are the
> > > > defaults that're in use across the library, with deviations from that
> > > This probably needs to be updated
> >
> > Indeed
> >
> > >
> > > > taken from Raspberry Pi's CamHelper class definitions.
> > > >
> > > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > > ---
> > > > Changes in v4:
> > > >
> > > >   - getSensorDelays() renamed to sensorDelays()
> > > >   - sensorDelays() returns a reference to the SensorDelays struct rather
> > > >     than each of the values.
> > > >   - Reworded the comments to make the default values seem less acceptable
> > > >   - Added delay values for AR0144
> > > >
> > > > Changes in v3:
> > > >
> > > >   - Rebased on top of the CameraSensorFactory introduction
> > > >   - Some rephrasing
> > > >   - Defined the sensorDelays member as empty where Raspberry Pi didn't
> > > >     have any specific values. Check for the empty struct in
> > > >     getSensorDelays() and return the defaults from there with a warning.
> > > >
> > > > Changes in v2:
> > > >
> > > >   - Rather than adding the delays to the properties ControlList, added a
> > > >     new function in CameraSensor that allows PipelineHandlers to retreive
> > > >     the delay values.
> > > >
> > > >   include/libcamera/internal/camera_sensor.h    |   2 +
> > > >   .../internal/camera_sensor_properties.h       |  10 ++
> > > >   src/libcamera/sensor/camera_sensor.cpp        |  12 ++
> > > >   src/libcamera/sensor/camera_sensor_legacy.cpp |  18 +++
> > > >   .../sensor/camera_sensor_properties.cpp       | 121 ++++++++++++++++++
> > > >   5 files changed, 163 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > index 8aafd82e..bbdb83a1 100644
> > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > @@ -20,6 +20,7 @@
> > > >   #include <libcamera/orientation.h>
> > > >   #include <libcamera/transform.h>
> > > >
> > > > +#include "libcamera/internal/camera_sensor_properties.h"
> > > >   #include "libcamera/internal/bayer_format.h"
> > > >   #include "libcamera/internal/v4l2_subdevice.h"
> > > >
> > > > @@ -73,6 +74,7 @@ public:
> > > >           virtual const std::vector<controls::draft::TestPatternModeEnum> &
> > > >           testPatternModes() const = 0;
> > > >           virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;
> > > > + virtual const CameraSensorProperties::SensorDelays &sensorDelays() = 0;
> > > >   };
> > > >
> > > >   class CameraSensorFactoryBase
> > > > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
> > > > index 480ac121..fe40bd7f 100644
> > > > --- a/include/libcamera/internal/camera_sensor_properties.h
> > > > +++ b/include/libcamera/internal/camera_sensor_properties.h
> > > > @@ -8,6 +8,7 @@
> > > >   #pragma once
> > > >
> > > >   #include <map>
> > > > +#include <stdint.h>
> > > >   #include <string>
> > > >
> > > >   #include <libcamera/control_ids.h>
> > > > @@ -20,6 +21,15 @@ struct CameraSensorProperties {
> > > >
> > > >           Size unitCellSize;
> > > >           std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
> > > > +
> > > > + struct SensorDelays {
> > > > +         uint8_t exposureDelay;
> > > > +         uint8_t gainDelay;
> > > > +         uint8_t vblankDelay;
> > > > +         uint8_t hblankDelay;
> > > > + } sensorDelays;
> > > >   };
> > > >
> > > > +extern const CameraSensorProperties::SensorDelays defaultSensorDelays;
> > > > +
> > > Fine. I thought we could have defined this inside
> > > CameraSensor::sensorDelays() as
> > >
> > > const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
> > > {
> > >          static constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
> > >                  .exposureDelay = 2,
> > >                  .gainDelay = 1,
> > >                  .vblankDelay = 2,
> > >                  .hblankDelay = 2,
> > >          };
> > Are references to that safe from outside the function scope? If so then sure we could do that instead...

Yes, because it's a static constexpr - I believe it's safe to define it
in scope here - and it will be part of the static data - and valid when
we return to the callers with the 'address/reference' to this const
data.

> 
> I don't see any code reference outside of this, but indeed
> documentation links would be broken.

I don't think we need to document the defaultSensorDelays specifically.

--
Kieran