[PATCH v5 1/2] libcamera: camera_sensor_properties: Add sensor control delays

Dan Scally dan.scally at ideasonboard.com
Wed Nov 27 10:35:41 CET 2024


Hi Laurent

On 27/11/2024 06:59, Laurent Pinchart wrote:
> On Tue, Nov 26, 2024 at 03:12:02PM +0000, Daniel Scally wrote:
>> Add properties covering the sensor control application delays to both
>> the static CameraSensorProperties definitions. The values used are
>> taken from Raspberry Pi's CamHelper class definitions. Where no more
>> specific values are known the delay struct is defined as empty and
>> defaults supplied through the getter function.
>>
>> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
>> ---
>> Changes in v5:
>>
>> 	- Comment rewording
>> 	- Moved the defaultSensorDelays definition inside
>> 	  CameraSensorLegacy::sensorDelays()
>>
>> 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       |   8 ++
>>   src/libcamera/sensor/camera_sensor.cpp        |  14 +++
>>   src/libcamera/sensor/camera_sensor_legacy.cpp |  25 ++++
>>   .../sensor/camera_sensor_properties.cpp       | 108 ++++++++++++++++++
>>   5 files changed, 157 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"
> Alphabetical order. I'm surprised checkstyle.py didn't warn you about
> this.


Actually so am I - checkstyle is noisy for this patch because the CameraSensorProperties tables are 
not its friend. I wondered if I had missed it from that noise but I don't see it there.

>
>>   #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..75cb7839 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,13 @@ 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;
> We usually separate type definitions from usage.
>
> struct CameraSensorProperties {
> 	struct SensorDelays {
> 		uint8_t exposureDelay;
> 		uint8_t gainDelay;
> 		uint8_t vblankDelay;
> 		uint8_t hblankDelay;
> 	};
>
> 	static const CameraSensorProperties *get(const std::string &sensor);
>
> 	Size unitCellSize;
> 	std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
> 	SensorDelays sensorDelays;
> };
Ack
>>   };
>>   
>>   } /* namespace libcamera */
>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
>> index 54cf98b2..9d2c2ea7 100644
>> --- a/src/libcamera/sensor/camera_sensor.cpp
>> +++ b/src/libcamera/sensor/camera_sensor.cpp
>> @@ -336,6 +336,20 @@ CameraSensor::~CameraSensor() = default;
>>    * pattern mode for every frame thus incurs no performance penalty.
>>    */
>>   
>> +/**
>> + * \fn CameraSensor::sensorDelays()
>> + * \brief Fetch the sensor delay values
>> + *
>> + * This function retreives the sensor control delays for pipeline handlers to
> s/retreives/retrieves/

Somewhere my primary school teacher is disappointed.


Thanks

Dan

>> + * use to inform the DelayedControls. If control delays are not specified in the
> We don't usually document expected users, unless there's a very tight
> coupling between a class and its users that is relevant to understanding
> the API.
>
>> + * static sensor propertie database, this function returns a reference to a set
> s/propertie/properties/
>
>> + * of default sensor delays provided as best-effort placeholders for the actual
>> + * sensor specific delays.
> And there's a bit of implementation detail here too. You can simplify
> this to
>
>   * This function retrieves the delays that the sensor applies to controls. If
>   * the static properties database doesn't specifiy control delay values for the
>   * sensor, default delays that may be suitable are returned and a warning is
>   * logged.
>
>> + *
>> + * \return A reference to a struct CameraSensorProperties::SensorDelays holding
>> + * the delay values
> Doxygen shows the type of the return value, so you can just write
>
>   * \return The sensor delay values
>
>> + */
>> +
>>   /**
>>    * \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..17d6fa68 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,30 @@ void CameraSensorLegacy::initStaticProperties()
>>   	initTestPatternModes();
>>   }
>>   
>> +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;
>> +}
>> +
>>   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 e2305166..3dc8cf05 100644
>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
>> @@ -41,6 +41,36 @@ 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.
> checkstyle.py flagged this.
>
>> + *
>> + * This struct may be defined as empty if the actual sensor delays are not
> s/struct/structure/
>
>> + * available or have not been measured. Best effort default values are returned
>> + * in this case.
> In this context, it's not clear where the defaults are returned from.
> I'd drop the last sentence.
>
>> + */
>> +
>> +/**
>> + * \struct CameraSensorProperties::SensorDelays
>> + * \brief Sensor control application delay values
>> + *
>> + * This struct holds delay values, expressed in number of frames, between the
> s/struct/structure/
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> + * 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).
>> + *
>> + * \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
>> + *
>> + * \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
>>    */
>>   
>>   /**
>> @@ -60,6 +90,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 +105,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeColorBars, 2 },
>>   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "hi846", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -87,6 +124,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * 9: "Resolution Pattern"
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "imx214", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -97,6 +135,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>>   				{ controls::draft::TestPatternModePn9, 4 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "imx219", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -107,6 +146,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,38 +162,67 @@ 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 = { },
>>   		} },
>>   		{ "imx462", {
>>   			.unitCellSize = { 2900, 2900 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "imx477", {
>>   			.unitCellSize = { 1550, 1550 },
>>   			.testPatternModes = {},
>> +			.sensorDelays = {
>> +				.exposureDelay = 2,
>> +				.gainDelay = 2,
>> +				.vblankDelay = 3,
>> +				.hblankDelay = 3
>> +			},
>>   		} },
>>   		{ "imx519", {
>>   			.unitCellSize = { 1220, 1220 },
>> @@ -161,6 +235,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 },
>> @@ -171,6 +251,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 },
>> @@ -185,6 +271,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * 5: "Color Square"
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov2740", {
>>   			.unitCellSize = { 1400, 1400 },
>> @@ -192,6 +279,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeOff, 0 },
>>   				{ controls::draft::TestPatternModeColorBars, 1},
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov4689", {
>>   			.unitCellSize = { 2000, 2000 },
>> @@ -205,6 +293,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * colorBarType2 and colorBarType3.
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov5640", {
>>   			.unitCellSize = { 1400, 1400 },
>> @@ -212,10 +301,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 },
>> @@ -223,6 +319,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeOff, 0 },
>>   				{ controls::draft::TestPatternModeColorBars, 1 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov5675", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -230,6 +327,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeOff, 0 },
>>   				{ controls::draft::TestPatternModeColorBars, 1 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov5693", {
>>   			.unitCellSize = { 1400, 1400 },
>> @@ -242,6 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * Rolling Bar".
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov64a40", {
>>   			.unitCellSize = { 1008, 1008 },
>> @@ -255,6 +354,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 },
>> @@ -268,6 +373,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * 4: "Vertical Color Bar Type 4"
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov8865", {
>>   			.unitCellSize = { 1400, 1400 },
>> @@ -282,6 +388,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				 * 5: "Color squares with rolling bar"
>>   				 */
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   		{ "ov13858", {
>>   			.unitCellSize = { 1120, 1120 },
>> @@ -289,6 +396,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>   				{ controls::draft::TestPatternModeOff, 0 },
>>   				{ controls::draft::TestPatternModeColorBars, 1 },
>>   			},
>> +			.sensorDelays = { },
>>   		} },
>>   	};
>>   


More information about the libcamera-devel mailing list