| Message ID | 20241115074628.417215-6-dan.scally@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Daniel Scally (2024-11-15 07:46:27) > 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 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. Sounds resonable to me. > > 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 | 9 +++ > src/libcamera/sensor/camera_sensor.cpp | 13 +++ > src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++ > .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++ > 5 files changed, 136 insertions(+) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index 8aafd82e..a9b2d494 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -73,6 +73,8 @@ public: > virtual const std::vector<controls::draft::TestPatternModeEnum> & > testPatternModes() const = 0; > virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0; > + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0; Could/Should this return a const pointer to a struct SensorDelays now ? > }; > > class CameraSensorFactoryBase > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h > index 480ac121..56d5c15d 100644 > --- a/include/libcamera/internal/camera_sensor_properties.h > +++ b/include/libcamera/internal/camera_sensor_properties.h > @@ -10,6 +10,8 @@ > #include <map> > #include <string> > > +#include <stdint.h> > + > #include <libcamera/control_ids.h> > #include <libcamera/geometry.h> > > @@ -20,6 +22,13 @@ struct CameraSensorProperties { > > Size unitCellSize; > std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; > + > + struct { > + uint8_t exposureDelay; > + uint8_t gainDelay; > + uint8_t vblankDelay; > + uint8_t hblankDelay; > + } sensorDelays; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > index 54cf98b2..61420873 100644 > --- a/src/libcamera/sensor/camera_sensor.cpp > +++ b/src/libcamera/sensor/camera_sensor.cpp > @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default; > * pattern mode for every frame thus incurs no performance penalty. > */ > > +/** > + * \fn CameraSensor::getSensorDelays() > + * \brief Fetch the sensor delay values > + * \param[out] exposureDelay The exposure delay > + * \param[out] gainDelay The analogue gain delay > + * \param[out] vblankDelay The vblank delay > + * \param[out] hblankDelay The hblank delay > + * > + * This function fills in sensor control delays for pipeline handlers to use to > + * inform the DelayedControls. If no static properties are available it fills in > + * some widely applicable default 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..84972f02 100644 > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > @@ -95,6 +95,8 @@ public: > const std::vector<controls::draft::TestPatternModeEnum> & > testPatternModes() const override { return testPatternModes_; } > int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override; > + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > + uint8_t &vblankDelay, uint8_t &hblankDelay) override; > > protected: > std::string logPrefix() const override; > @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties() > initTestPatternModes(); > } > > +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > + uint8_t &vblankDelay, uint8_t &hblankDelay) > +{ > + > + /* > + * These defaults are applicable to many sensors, however more specific > + * values can be added to the CameraSensorProperties for a sensor if > + * required. > + */ > + 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."; > + > + exposureDelay = 2; > + gainDelay = 1; > + vblankDelay = 2; > + hblankDelay = 2; > + return; > + } > + > + exposureDelay = staticProps_->sensorDelays.exposureDelay; > + gainDelay = staticProps_->sensorDelays.gainDelay; > + vblankDelay = staticProps_->sensorDelays.vblankDelay; > + hblankDelay = staticProps_->sensorDelays.hblankDelay; Which would make this just return staticProps_->sensorDelays; > +} I'm glad this function prints a warning! > + > 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..6dda7ba9 100644 > --- a/src/libcamera/sensor/camera_sensor_properties.cpp > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp > @@ -41,6 +41,13 @@ 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 Holds the delays, expressed in number of frames, between the time a > + * control is applied to the sensor and the time it actually takes effect. > + * Delays are recorded for the exposure time, analogue gain, vertical and > + * horizontal blanking. These values may be defined as empty, in which case the > + * CameraSensor derivative should provide default values. > */ > > /** > @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeColorBars, 2 }, > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > }, > + .sensorDelays = { }, > } }, > { "ar0521", { > .unitCellSize = { 2200, 2200 }, > @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeColorBars, 2 }, > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > }, > + .sensorDelays = { }, > } }, > { "hi846", { > .unitCellSize = { 1120, 1120 }, > @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > * 9: "Resolution Pattern" > */ > }, > + .sensorDelays = { }, > } }, > { "imx214", { > .unitCellSize = { 1120, 1120 }, > @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > { controls::draft::TestPatternModePn9, 4 }, > }, > + .sensorDelays = { }, > } }, > { "imx219", { > .unitCellSize = { 1120, 1120 }, > @@ -107,6 +118,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 +134,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 +202,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 +218,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 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > * 5: "Color Square" > */ > }, > + .sensorDelays = { }, > } }, > { "ov2740", { > .unitCellSize = { 1400, 1400 }, > @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeOff, 0 }, > { controls::draft::TestPatternModeColorBars, 1}, > }, > + .sensorDelays = { }, > } }, > { "ov4689", { > .unitCellSize = { 2000, 2000 }, > @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > * colorBarType2 and colorBarType3. > */ > }, > + .sensorDelays = { }, > } }, > { "ov5640", { > .unitCellSize = { 1400, 1400 }, > @@ -208,10 +268,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 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeOff, 0 }, > { controls::draft::TestPatternModeColorBars, 1 }, > }, > + .sensorDelays = { }, > } }, > { "ov5675", { > .unitCellSize = { 1120, 1120 }, > @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeOff, 0 }, > { controls::draft::TestPatternModeColorBars, 1 }, > }, > + .sensorDelays = { }, > } }, > { "ov5693", { > .unitCellSize = { 1400, 1400 }, > @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > * Rolling Bar". > */ > }, > + .sensorDelays = { }, > } }, > { "ov64a40", { > .unitCellSize = { 1008, 1008 }, > @@ -251,6 +321,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 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > * 4: "Vertical Color Bar Type 4" > */ > }, > + .sensorDelays = { }, > } }, > { "ov8865", { > .unitCellSize = { 1400, 1400 }, > @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > * 5: "Color squares with rolling bar" > */ > }, > + .sensorDelays = { }, > } }, > { "ov13858", { > .unitCellSize = { 1120, 1120 }, > @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeOff, 0 }, > { controls::draft::TestPatternModeColorBars, 1 }, > }, > + .sensorDelays = { }, > } }, > }; > > -- > 2.30.2 >
Hi Kieran On 15/11/2024 12:10, Kieran Bingham wrote: > Quoting Daniel Scally (2024-11-15 07:46:27) >> 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 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. > Sounds resonable to me. > > >> 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 | 9 +++ >> src/libcamera/sensor/camera_sensor.cpp | 13 +++ >> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++ >> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++ >> 5 files changed, 136 insertions(+) >> >> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h >> index 8aafd82e..a9b2d494 100644 >> --- a/include/libcamera/internal/camera_sensor.h >> +++ b/include/libcamera/internal/camera_sensor.h >> @@ -73,6 +73,8 @@ public: >> virtual const std::vector<controls::draft::TestPatternModeEnum> & >> testPatternModes() const = 0; >> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0; >> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, >> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0; > Could/Should this return a const pointer to a struct SensorDelays now ? Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a Factory class now) - I don't really have any strong feelings either way, so as long as that's fine then I'll make the change. > >> }; >> >> class CameraSensorFactoryBase >> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h >> index 480ac121..56d5c15d 100644 >> --- a/include/libcamera/internal/camera_sensor_properties.h >> +++ b/include/libcamera/internal/camera_sensor_properties.h >> @@ -10,6 +10,8 @@ >> #include <map> >> #include <string> >> >> +#include <stdint.h> >> + >> #include <libcamera/control_ids.h> >> #include <libcamera/geometry.h> >> >> @@ -20,6 +22,13 @@ struct CameraSensorProperties { >> >> Size unitCellSize; >> std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; >> + >> + struct { >> + uint8_t exposureDelay; >> + uint8_t gainDelay; >> + uint8_t vblankDelay; >> + uint8_t hblankDelay; >> + } sensorDelays; >> }; >> >> } /* namespace libcamera */ >> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp >> index 54cf98b2..61420873 100644 >> --- a/src/libcamera/sensor/camera_sensor.cpp >> +++ b/src/libcamera/sensor/camera_sensor.cpp >> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default; >> * pattern mode for every frame thus incurs no performance penalty. >> */ >> >> +/** >> + * \fn CameraSensor::getSensorDelays() >> + * \brief Fetch the sensor delay values >> + * \param[out] exposureDelay The exposure delay >> + * \param[out] gainDelay The analogue gain delay >> + * \param[out] vblankDelay The vblank delay >> + * \param[out] hblankDelay The hblank delay >> + * >> + * This function fills in sensor control delays for pipeline handlers to use to >> + * inform the DelayedControls. If no static properties are available it fills in >> + * some widely applicable default 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..84972f02 100644 >> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp >> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp >> @@ -95,6 +95,8 @@ public: >> const std::vector<controls::draft::TestPatternModeEnum> & >> testPatternModes() const override { return testPatternModes_; } >> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override; >> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, >> + uint8_t &vblankDelay, uint8_t &hblankDelay) override; >> >> protected: >> std::string logPrefix() const override; >> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties() >> initTestPatternModes(); >> } >> >> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, >> + uint8_t &vblankDelay, uint8_t &hblankDelay) >> +{ >> + >> + /* >> + * These defaults are applicable to many sensors, however more specific >> + * values can be added to the CameraSensorProperties for a sensor if >> + * required. >> + */ >> + 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."; >> + >> + exposureDelay = 2; >> + gainDelay = 1; >> + vblankDelay = 2; >> + hblankDelay = 2; >> + return; >> + } >> + >> + exposureDelay = staticProps_->sensorDelays.exposureDelay; >> + gainDelay = staticProps_->sensorDelays.gainDelay; >> + vblankDelay = staticProps_->sensorDelays.vblankDelay; >> + hblankDelay = staticProps_->sensorDelays.hblankDelay; > Which would make this just > > return staticProps_->sensorDelays; > >> +} > I'm glad this function prints a warning! > >> + >> 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..6dda7ba9 100644 >> --- a/src/libcamera/sensor/camera_sensor_properties.cpp >> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp >> @@ -41,6 +41,13 @@ 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 Holds the delays, expressed in number of frames, between the time a >> + * control is applied to the sensor and the time it actually takes effect. >> + * Delays are recorded for the exposure time, analogue gain, vertical and >> + * horizontal blanking. These values may be defined as empty, in which case the >> + * CameraSensor derivative should provide default values. >> */ >> >> /** >> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> { controls::draft::TestPatternModeColorBars, 2 }, >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, >> }, >> + .sensorDelays = { }, >> } }, >> { "ar0521", { >> .unitCellSize = { 2200, 2200 }, >> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> { controls::draft::TestPatternModeColorBars, 2 }, >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, >> }, >> + .sensorDelays = { }, >> } }, >> { "hi846", { >> .unitCellSize = { 1120, 1120 }, >> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> * 9: "Resolution Pattern" >> */ >> }, >> + .sensorDelays = { }, >> } }, >> { "imx214", { >> .unitCellSize = { 1120, 1120 }, >> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, >> { controls::draft::TestPatternModePn9, 4 }, >> }, >> + .sensorDelays = { }, >> } }, >> { "imx219", { >> .unitCellSize = { 1120, 1120 }, >> @@ -107,6 +118,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 +134,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 +202,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 +218,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 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> * 5: "Color Square" >> */ >> }, >> + .sensorDelays = { }, >> } }, >> { "ov2740", { >> .unitCellSize = { 1400, 1400 }, >> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> { controls::draft::TestPatternModeOff, 0 }, >> { controls::draft::TestPatternModeColorBars, 1}, >> }, >> + .sensorDelays = { }, >> } }, >> { "ov4689", { >> .unitCellSize = { 2000, 2000 }, >> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> * colorBarType2 and colorBarType3. >> */ >> }, >> + .sensorDelays = { }, >> } }, >> { "ov5640", { >> .unitCellSize = { 1400, 1400 }, >> @@ -208,10 +268,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 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> { controls::draft::TestPatternModeOff, 0 }, >> { controls::draft::TestPatternModeColorBars, 1 }, >> }, >> + .sensorDelays = { }, >> } }, >> { "ov5675", { >> .unitCellSize = { 1120, 1120 }, >> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> { controls::draft::TestPatternModeOff, 0 }, >> { controls::draft::TestPatternModeColorBars, 1 }, >> }, >> + .sensorDelays = { }, >> } }, >> { "ov5693", { >> .unitCellSize = { 1400, 1400 }, >> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> * Rolling Bar". >> */ >> }, >> + .sensorDelays = { }, >> } }, >> { "ov64a40", { >> .unitCellSize = { 1008, 1008 }, >> @@ -251,6 +321,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 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> * 4: "Vertical Color Bar Type 4" >> */ >> }, >> + .sensorDelays = { }, >> } }, >> { "ov8865", { >> .unitCellSize = { 1400, 1400 }, >> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> * 5: "Color squares with rolling bar" >> */ >> }, >> + .sensorDelays = { }, >> } }, >> { "ov13858", { >> .unitCellSize = { 1120, 1120 }, >> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >> { controls::draft::TestPatternModeOff, 0 }, >> { controls::draft::TestPatternModeColorBars, 1 }, >> }, >> + .sensorDelays = { }, >> } }, >> }; >> >> -- >> 2.30.2 >>
Quoting Dan Scally (2024-11-15 13:35:54) > Hi Kieran > > On 15/11/2024 12:10, Kieran Bingham wrote: > > Quoting Daniel Scally (2024-11-15 07:46:27) > >> 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 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. > > Sounds resonable to me. > > > > > >> 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 | 9 +++ > >> src/libcamera/sensor/camera_sensor.cpp | 13 +++ > >> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++ > >> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++ > >> 5 files changed, 136 insertions(+) > >> > >> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > >> index 8aafd82e..a9b2d494 100644 > >> --- a/include/libcamera/internal/camera_sensor.h > >> +++ b/include/libcamera/internal/camera_sensor.h > >> @@ -73,6 +73,8 @@ public: > >> virtual const std::vector<controls::draft::TestPatternModeEnum> & > >> testPatternModes() const = 0; > >> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0; > >> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > >> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0; > > Could/Should this return a const pointer to a struct SensorDelays now ? > > > Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a > Factory class now) - I don't really have any strong feelings either way, so as long as that's fine > then I'll make the change. I think it's reasonable.... -- Kieran
Hi Dan, On Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote: > On 15/11/2024 12:10, Kieran Bingham wrote: > > Quoting Daniel Scally (2024-11-15 07:46:27) > >> 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 I had never seen "that're" before. > >> taken from Raspberry Pi's CamHelper class definitions. > >> > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >> --- > >> 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. > > > > Sounds resonable to me. > > > >> 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 | 9 +++ > >> src/libcamera/sensor/camera_sensor.cpp | 13 +++ > >> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++ > >> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++ > >> 5 files changed, 136 insertions(+) > >> > >> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > >> index 8aafd82e..a9b2d494 100644 > >> --- a/include/libcamera/internal/camera_sensor.h > >> +++ b/include/libcamera/internal/camera_sensor.h > >> @@ -73,6 +73,8 @@ public: > >> virtual const std::vector<controls::draft::TestPatternModeEnum> & > >> testPatternModes() const = 0; > >> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0; > >> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > >> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0; > > > > Could/Should this return a const pointer to a struct SensorDelays now ? > > Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a > Factory class now) - I don't really have any strong feelings either way, so as long as that's fine > then I'll make the change. I think it would make the API nicer. > >> }; > >> > >> class CameraSensorFactoryBase > >> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h > >> index 480ac121..56d5c15d 100644 > >> --- a/include/libcamera/internal/camera_sensor_properties.h > >> +++ b/include/libcamera/internal/camera_sensor_properties.h > >> @@ -10,6 +10,8 @@ > >> #include <map> > >> #include <string> > >> > >> +#include <stdint.h> stdint.h should go just before string, we mix the C and C++ headers. > >> + > >> #include <libcamera/control_ids.h> > >> #include <libcamera/geometry.h> > >> > >> @@ -20,6 +22,13 @@ struct CameraSensorProperties { > >> > >> Size unitCellSize; > >> std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; > >> + > >> + struct { > >> + uint8_t exposureDelay; > >> + uint8_t gainDelay; > >> + uint8_t vblankDelay; > >> + uint8_t hblankDelay; > >> + } sensorDelays; > >> }; > >> > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > >> index 54cf98b2..61420873 100644 > >> --- a/src/libcamera/sensor/camera_sensor.cpp > >> +++ b/src/libcamera/sensor/camera_sensor.cpp > >> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default; > >> * pattern mode for every frame thus incurs no performance penalty. > >> */ > >> > >> +/** > >> + * \fn CameraSensor::getSensorDelays() We don't usually prefix getters with "get". > >> + * \brief Fetch the sensor delay values > >> + * \param[out] exposureDelay The exposure delay > >> + * \param[out] gainDelay The analogue gain delay > >> + * \param[out] vblankDelay The vblank delay > >> + * \param[out] hblankDelay The hblank delay > >> + * > >> + * This function fills in sensor control delays for pipeline handlers to use to > >> + * inform the DelayedControls. If no static properties are available it fills in > >> + * some widely applicable default values. We need to return some default values until all sensors provide the information we need, but I wouldn't call those "widely applicable default values". It makes it sound those defaults will have a high chance of working, while that's not specifically the case. > >> + */ > >> + > >> /** > >> * \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..84972f02 100644 > >> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > >> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > >> @@ -95,6 +95,8 @@ public: > >> const std::vector<controls::draft::TestPatternModeEnum> & > >> testPatternModes() const override { return testPatternModes_; } > >> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override; > >> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > >> + uint8_t &vblankDelay, uint8_t &hblankDelay) override; > >> > >> protected: > >> std::string logPrefix() const override; > >> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties() > >> initTestPatternModes(); > >> } > >> > >> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > >> + uint8_t &vblankDelay, uint8_t &hblankDelay) > >> +{ > >> + > >> + /* > >> + * These defaults are applicable to many sensors, however more specific > >> + * values can be added to the CameraSensorProperties for a sensor if > >> + * required. > >> + */ > >> + 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."; Here the lack of sensor-specific values is reported with a more appropriate seriousness. Let's update the comments to match. > >> + > >> + exposureDelay = 2; > >> + gainDelay = 1; > >> + vblankDelay = 2; > >> + hblankDelay = 2; > >> + return; > >> + } > >> + > >> + exposureDelay = staticProps_->sensorDelays.exposureDelay; > >> + gainDelay = staticProps_->sensorDelays.gainDelay; > >> + vblankDelay = staticProps_->sensorDelays.vblankDelay; > >> + hblankDelay = staticProps_->sensorDelays.hblankDelay; > > > > Which would make this just > > > > return staticProps_->sensorDelays; > > > >> +} > > > > I'm glad this function prints a warning! > > > >> + > >> 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..6dda7ba9 100644 > >> --- a/src/libcamera/sensor/camera_sensor_properties.cpp > >> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp > >> @@ -41,6 +41,13 @@ 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 Holds the delays, expressed in number of frames, between the time a > >> + * control is applied to the sensor and the time it actually takes effect. > >> + * Delays are recorded for the exposure time, analogue gain, vertical and > >> + * horizontal blanking. These values may be defined as empty, in which case the > >> + * CameraSensor derivative should provide default values. A \brief should be brief :-) Make it a single short sentence, and you can add more information in the body of the documentation. > >> */ > >> > >> /** > >> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> { controls::draft::TestPatternModeColorBars, 2 }, > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > >> }, > >> + .sensorDelays = { }, The AR0144 has a 2 frames delay for all parameters. Assuming that "2 frames delay" means that parameters set during frame N will take effect for frame N+2 (so a delay of 0 means the parameter is applied immediately to the current frame). Maybe this should be documented more clearly above. Note that the sensor can also be programmed to apply a one frame delay instead of two frames for the analog gain. The value is therefore driver-dependent, and could even be changed dynamically. That's something to worry about later. > >> } }, > >> { "ar0521", { > >> .unitCellSize = { 2200, 2200 }, > >> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> { controls::draft::TestPatternModeColorBars, 2 }, > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > >> }, > >> + .sensorDelays = { }, The AR0521 will be interesting. The exposure time has a 2 frames delay, while the analog gain will have a 1 frame delay if it is set alone, and a 2 frames delay if the exposure time is also set during the same frame. This is something we should consider when we'll develop a tool to measure delays. The sensor can be configured to have a fixed 2 frames delay for the analog gain, but that's not how the driver currently configures it. The delay for the blanking values doesn't seem to be documented. > >> } }, > >> { "hi846", { > >> .unitCellSize = { 1120, 1120 }, > >> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> * 9: "Resolution Pattern" > >> */ > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "imx214", { > >> .unitCellSize = { 1120, 1120 }, > >> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > >> { controls::draft::TestPatternModePn9, 4 }, > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "imx219", { > >> .unitCellSize = { 1120, 1120 }, > >> @@ -107,6 +118,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 +134,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 +202,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 +218,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 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> * 5: "Color Square" > >> */ > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "ov2740", { > >> .unitCellSize = { 1400, 1400 }, > >> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> { controls::draft::TestPatternModeOff, 0 }, > >> { controls::draft::TestPatternModeColorBars, 1}, > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "ov4689", { > >> .unitCellSize = { 2000, 2000 }, > >> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> * colorBarType2 and colorBarType3. > >> */ > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "ov5640", { > >> .unitCellSize = { 1400, 1400 }, > >> @@ -208,10 +268,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 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> { controls::draft::TestPatternModeOff, 0 }, > >> { controls::draft::TestPatternModeColorBars, 1 }, > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "ov5675", { > >> .unitCellSize = { 1120, 1120 }, > >> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> { controls::draft::TestPatternModeOff, 0 }, > >> { controls::draft::TestPatternModeColorBars, 1 }, > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "ov5693", { > >> .unitCellSize = { 1400, 1400 }, > >> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> * Rolling Bar". > >> */ > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "ov64a40", { > >> .unitCellSize = { 1008, 1008 }, > >> @@ -251,6 +321,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 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> * 4: "Vertical Color Bar Type 4" > >> */ > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "ov8865", { > >> .unitCellSize = { 1400, 1400 }, > >> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> * 5: "Color squares with rolling bar" > >> */ > >> }, > >> + .sensorDelays = { }, > >> } }, > >> { "ov13858", { > >> .unitCellSize = { 1120, 1120 }, > >> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >> { controls::draft::TestPatternModeOff, 0 }, > >> { controls::draft::TestPatternModeColorBars, 1 }, > >> }, > >> + .sensorDelays = { }, > >> } }, > >> }; > >>
On Mon, Nov 18, 2024 at 03:36:53AM +0200, Laurent Pinchart wrote: > Hi Dan, > > On Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote: > > On 15/11/2024 12:10, Kieran Bingham wrote: > > > Quoting Daniel Scally (2024-11-15 07:46:27) > > >> 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 > > I had never seen "that're" before. > > > >> taken from Raspberry Pi's CamHelper class definitions. > > >> > > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > > >> --- > > >> 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. > > > > > > Sounds resonable to me. > > > > > >> 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 | 9 +++ > > >> src/libcamera/sensor/camera_sensor.cpp | 13 +++ > > >> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++ > > >> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++ > > >> 5 files changed, 136 insertions(+) > > >> > > >> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > >> index 8aafd82e..a9b2d494 100644 > > >> --- a/include/libcamera/internal/camera_sensor.h > > >> +++ b/include/libcamera/internal/camera_sensor.h > > >> @@ -73,6 +73,8 @@ public: > > >> virtual const std::vector<controls::draft::TestPatternModeEnum> & > > >> testPatternModes() const = 0; > > >> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0; > > >> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > > >> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0; > > > > > > Could/Should this return a const pointer to a struct SensorDelays now ? > > > > Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a > > Factory class now) - I don't really have any strong feelings either way, so as long as that's fine > > then I'll make the change. > > I think it would make the API nicer. > > > >> }; > > >> > > >> class CameraSensorFactoryBase > > >> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h > > >> index 480ac121..56d5c15d 100644 > > >> --- a/include/libcamera/internal/camera_sensor_properties.h > > >> +++ b/include/libcamera/internal/camera_sensor_properties.h > > >> @@ -10,6 +10,8 @@ > > >> #include <map> > > >> #include <string> > > >> > > >> +#include <stdint.h> > > stdint.h should go just before string, we mix the C and C++ headers. > > > >> + > > >> #include <libcamera/control_ids.h> > > >> #include <libcamera/geometry.h> > > >> > > >> @@ -20,6 +22,13 @@ struct CameraSensorProperties { > > >> > > >> Size unitCellSize; > > >> std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; > > >> + > > >> + struct { > > >> + uint8_t exposureDelay; > > >> + uint8_t gainDelay; > > >> + uint8_t vblankDelay; > > >> + uint8_t hblankDelay; > > >> + } sensorDelays; > > >> }; > > >> > > >> } /* namespace libcamera */ > > >> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > > >> index 54cf98b2..61420873 100644 > > >> --- a/src/libcamera/sensor/camera_sensor.cpp > > >> +++ b/src/libcamera/sensor/camera_sensor.cpp > > >> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default; > > >> * pattern mode for every frame thus incurs no performance penalty. > > >> */ > > >> > > >> +/** > > >> + * \fn CameraSensor::getSensorDelays() > > We don't usually prefix getters with "get". > > > >> + * \brief Fetch the sensor delay values > > >> + * \param[out] exposureDelay The exposure delay > > >> + * \param[out] gainDelay The analogue gain delay > > >> + * \param[out] vblankDelay The vblank delay > > >> + * \param[out] hblankDelay The hblank delay > > >> + * > > >> + * This function fills in sensor control delays for pipeline handlers to use to > > >> + * inform the DelayedControls. If no static properties are available it fills in > > >> + * some widely applicable default values. > > We need to return some default values until all sensors provide the > information we need, but I wouldn't call those "widely applicable > default values". It makes it sound those defaults will have a high > chance of working, while that's not specifically the case. > > > >> + */ > > >> + > > >> /** > > >> * \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..84972f02 100644 > > >> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > > >> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > > >> @@ -95,6 +95,8 @@ public: > > >> const std::vector<controls::draft::TestPatternModeEnum> & > > >> testPatternModes() const override { return testPatternModes_; } > > >> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override; > > >> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > > >> + uint8_t &vblankDelay, uint8_t &hblankDelay) override; > > >> > > >> protected: > > >> std::string logPrefix() const override; > > >> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties() > > >> initTestPatternModes(); > > >> } > > >> > > >> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > > >> + uint8_t &vblankDelay, uint8_t &hblankDelay) > > >> +{ > > >> + > > >> + /* > > >> + * These defaults are applicable to many sensors, however more specific > > >> + * values can be added to the CameraSensorProperties for a sensor if > > >> + * required. > > >> + */ > > >> + 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."; > > Here the lack of sensor-specific values is reported with a more > appropriate seriousness. Let's update the comments to match. > > > >> + > > >> + exposureDelay = 2; > > >> + gainDelay = 1; > > >> + vblankDelay = 2; > > >> + hblankDelay = 2; > > >> + return; > > >> + } > > >> + > > >> + exposureDelay = staticProps_->sensorDelays.exposureDelay; > > >> + gainDelay = staticProps_->sensorDelays.gainDelay; > > >> + vblankDelay = staticProps_->sensorDelays.vblankDelay; > > >> + hblankDelay = staticProps_->sensorDelays.hblankDelay; > > > > > > Which would make this just > > > > > > return staticProps_->sensorDelays; > > > > > >> +} > > > > > > I'm glad this function prints a warning! > > > > > >> + > > >> 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..6dda7ba9 100644 > > >> --- a/src/libcamera/sensor/camera_sensor_properties.cpp > > >> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp > > >> @@ -41,6 +41,13 @@ 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 Holds the delays, expressed in number of frames, between the time a > > >> + * control is applied to the sensor and the time it actually takes effect. > > >> + * Delays are recorded for the exposure time, analogue gain, vertical and > > >> + * horizontal blanking. These values may be defined as empty, in which case the > > >> + * CameraSensor derivative should provide default values. > > A \brief should be brief :-) Make it a single short sentence, and you > can add more information in the body of the documentation. > > > >> */ > > >> > > >> /** > > >> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> { controls::draft::TestPatternModeColorBars, 2 }, > > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > > >> }, > > >> + .sensorDelays = { }, > > The AR0144 has a 2 frames delay for all parameters. Assuming that "2 > frames delay" means that parameters set during frame N will take effect > for frame N+2 (so a delay of 0 means the parameter is applied > immediately to the current frame). Maybe this should be documented more > clearly above. > > Note that the sensor can also be programmed to apply a one frame delay > instead of two frames for the analog gain. The value is therefore > driver-dependent, and could even be changed dynamically. That's > something to worry about later. > > > >> } }, > > >> { "ar0521", { > > >> .unitCellSize = { 2200, 2200 }, > > >> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> { controls::draft::TestPatternModeColorBars, 2 }, > > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > > >> }, > > >> + .sensorDelays = { }, > > The AR0521 will be interesting. The exposure time has a 2 frames delay, > while the analog gain will have a 1 frame delay if it is set alone, and > a 2 frames delay if the exposure time is also set during the same frame. > This is something we should consider when we'll develop a tool to > measure delays. > > The sensor can be configured to have a fixed 2 frames delay for the > analog gain, but that's not how the driver currently configures it. > > The delay for the blanking values doesn't seem to be documented. > > > >> } }, > > >> { "hi846", { > > >> .unitCellSize = { 1120, 1120 }, > > >> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> * 9: "Resolution Pattern" > > >> */ > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "imx214", { > > >> .unitCellSize = { 1120, 1120 }, > > >> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > > >> { controls::draft::TestPatternModePn9, 4 }, > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "imx219", { > > >> .unitCellSize = { 1120, 1120 }, > > >> @@ -107,6 +118,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 +134,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 I forgot to mention that this will be "interesting" to handle. The exposure time is bound by the frame time (with a sensor-specific margin), different delays for the exposure time and the blanking values will probably require careful handling. > > >> + }, > > >> } }, > > >> { "imx519", { > > >> .unitCellSize = { 1220, 1220 }, > > >> @@ -157,6 +202,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 +218,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 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> * 5: "Color Square" > > >> */ > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "ov2740", { > > >> .unitCellSize = { 1400, 1400 }, > > >> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> { controls::draft::TestPatternModeOff, 0 }, > > >> { controls::draft::TestPatternModeColorBars, 1}, > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "ov4689", { > > >> .unitCellSize = { 2000, 2000 }, > > >> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> * colorBarType2 and colorBarType3. > > >> */ > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "ov5640", { > > >> .unitCellSize = { 1400, 1400 }, > > >> @@ -208,10 +268,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 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> { controls::draft::TestPatternModeOff, 0 }, > > >> { controls::draft::TestPatternModeColorBars, 1 }, > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "ov5675", { > > >> .unitCellSize = { 1120, 1120 }, > > >> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> { controls::draft::TestPatternModeOff, 0 }, > > >> { controls::draft::TestPatternModeColorBars, 1 }, > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "ov5693", { > > >> .unitCellSize = { 1400, 1400 }, > > >> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> * Rolling Bar". > > >> */ > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "ov64a40", { > > >> .unitCellSize = { 1008, 1008 }, > > >> @@ -251,6 +321,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 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> * 4: "Vertical Color Bar Type 4" > > >> */ > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "ov8865", { > > >> .unitCellSize = { 1400, 1400 }, > > >> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> * 5: "Color squares with rolling bar" > > >> */ > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> { "ov13858", { > > >> .unitCellSize = { 1120, 1120 }, > > >> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > >> { controls::draft::TestPatternModeOff, 0 }, > > >> { controls::draft::TestPatternModeColorBars, 1 }, > > >> }, > > >> + .sensorDelays = { }, > > >> } }, > > >> }; > > >>
Hi Laurent On 18/11/2024 01:36, Laurent Pinchart wrote: > Hi Dan, > > On Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote: >> On 15/11/2024 12:10, Kieran Bingham wrote: >>> Quoting Daniel Scally (2024-11-15 07:46:27) >>>> 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 > I had never seen "that're" before. English is ever surprising (I'm not even sure if it's proper usage tbh...) > >>>> taken from Raspberry Pi's CamHelper class definitions. >>>> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>> --- >>>> 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. >>> Sounds resonable to me. >>> >>>> 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 | 9 +++ >>>> src/libcamera/sensor/camera_sensor.cpp | 13 +++ >>>> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++ >>>> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++ >>>> 5 files changed, 136 insertions(+) >>>> >>>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h >>>> index 8aafd82e..a9b2d494 100644 >>>> --- a/include/libcamera/internal/camera_sensor.h >>>> +++ b/include/libcamera/internal/camera_sensor.h >>>> @@ -73,6 +73,8 @@ public: >>>> virtual const std::vector<controls::draft::TestPatternModeEnum> & >>>> testPatternModes() const = 0; >>>> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0; >>>> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0; >>> Could/Should this return a const pointer to a struct SensorDelays now ? >> Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a >> Factory class now) - I don't really have any strong feelings either way, so as long as that's fine >> then I'll make the change. > I think it would make the API nicer. > >>>> }; >>>> >>>> class CameraSensorFactoryBase >>>> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h >>>> index 480ac121..56d5c15d 100644 >>>> --- a/include/libcamera/internal/camera_sensor_properties.h >>>> +++ b/include/libcamera/internal/camera_sensor_properties.h >>>> @@ -10,6 +10,8 @@ >>>> #include <map> >>>> #include <string> >>>> >>>> +#include <stdint.h> > stdint.h should go just before string, we mix the C and C++ headers. > >>>> + >>>> #include <libcamera/control_ids.h> >>>> #include <libcamera/geometry.h> >>>> >>>> @@ -20,6 +22,13 @@ struct CameraSensorProperties { >>>> >>>> Size unitCellSize; >>>> std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; >>>> + >>>> + struct { >>>> + uint8_t exposureDelay; >>>> + uint8_t gainDelay; >>>> + uint8_t vblankDelay; >>>> + uint8_t hblankDelay; >>>> + } sensorDelays; >>>> }; >>>> >>>> } /* namespace libcamera */ >>>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp >>>> index 54cf98b2..61420873 100644 >>>> --- a/src/libcamera/sensor/camera_sensor.cpp >>>> +++ b/src/libcamera/sensor/camera_sensor.cpp >>>> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default; >>>> * pattern mode for every frame thus incurs no performance penalty. >>>> */ >>>> >>>> +/** >>>> + * \fn CameraSensor::getSensorDelays() > We don't usually prefix getters with "get". > >>>> + * \brief Fetch the sensor delay values >>>> + * \param[out] exposureDelay The exposure delay >>>> + * \param[out] gainDelay The analogue gain delay >>>> + * \param[out] vblankDelay The vblank delay >>>> + * \param[out] hblankDelay The hblank delay >>>> + * >>>> + * This function fills in sensor control delays for pipeline handlers to use to >>>> + * inform the DelayedControls. If no static properties are available it fills in >>>> + * some widely applicable default values. > We need to return some default values until all sensors provide the > information we need, but I wouldn't call those "widely applicable > default values". It makes it sound those defaults will have a high > chance of working, while that's not specifically the case. Ack > >>>> + */ >>>> + >>>> /** >>>> * \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..84972f02 100644 >>>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp >>>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp >>>> @@ -95,6 +95,8 @@ public: >>>> const std::vector<controls::draft::TestPatternModeEnum> & >>>> testPatternModes() const override { return testPatternModes_; } >>>> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override; >>>> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) override; >>>> >>>> protected: >>>> std::string logPrefix() const override; >>>> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties() >>>> initTestPatternModes(); >>>> } >>>> >>>> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) >>>> +{ >>>> + >>>> + /* >>>> + * These defaults are applicable to many sensors, however more specific >>>> + * values can be added to the CameraSensorProperties for a sensor if >>>> + * required. >>>> + */ >>>> + 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."; > Here the lack of sensor-specific values is reported with a more > appropriate seriousness. Let's update the comments to match. > >>>> + >>>> + exposureDelay = 2; >>>> + gainDelay = 1; >>>> + vblankDelay = 2; >>>> + hblankDelay = 2; >>>> + return; >>>> + } >>>> + >>>> + exposureDelay = staticProps_->sensorDelays.exposureDelay; >>>> + gainDelay = staticProps_->sensorDelays.gainDelay; >>>> + vblankDelay = staticProps_->sensorDelays.vblankDelay; >>>> + hblankDelay = staticProps_->sensorDelays.hblankDelay; >>> Which would make this just >>> >>> return staticProps_->sensorDelays; >>> >>>> +} >>> I'm glad this function prints a warning! >>> >>>> + >>>> 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..6dda7ba9 100644 >>>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp >>>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp >>>> @@ -41,6 +41,13 @@ 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 Holds the delays, expressed in number of frames, between the time a >>>> + * control is applied to the sensor and the time it actually takes effect. >>>> + * Delays are recorded for the exposure time, analogue gain, vertical and >>>> + * horizontal blanking. These values may be defined as empty, in which case the >>>> + * CameraSensor derivative should provide default values. > A \brief should be brief :-) Make it a single short sentence, and you > can add more information in the body of the documentation. > >>>> */ >>>> >>>> /** >>>> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeColorBars, 2 }, >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, >>>> }, >>>> + .sensorDelays = { }, > The AR0144 has a 2 frames delay for all parameters. Assuming that "2 > frames delay" means that parameters set during frame N will take effect > for frame N+2 (so a delay of 0 means the parameter is applied > immediately to the current frame). Maybe this should be documented more > clearly above. > > Note that the sensor can also be programmed to apply a one frame delay > instead of two frames for the analog gain. The value is therefore > driver-dependent, and could even be changed dynamically. That's > something to worry about later. Oof, in that case we probably need sensor controls in the drivers to report those values > >>>> } }, >>>> { "ar0521", { >>>> .unitCellSize = { 2200, 2200 }, >>>> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeColorBars, 2 }, >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, >>>> }, >>>> + .sensorDelays = { }, > The AR0521 will be interesting. The exposure time has a 2 frames delay, > while the analog gain will have a 1 frame delay if it is set alone, and > a 2 frames delay if the exposure time is also set during the same frame. > This is something we should consider when we'll develop a tool to > measure delays. > > The sensor can be configured to have a fixed 2 frames delay for the > analog gain, but that's not how the driver currently configures it. > > The delay for the blanking values doesn't seem to be documented. > >>>> } }, >>>> { "hi846", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * 9: "Resolution Pattern" >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "imx214", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, >>>> { controls::draft::TestPatternModePn9, 4 }, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "imx219", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -107,6 +118,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 +134,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 +202,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 +218,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 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * 5: "Color Square" >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov2740", { >>>> .unitCellSize = { 1400, 1400 }, >>>> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeOff, 0 }, >>>> { controls::draft::TestPatternModeColorBars, 1}, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov4689", { >>>> .unitCellSize = { 2000, 2000 }, >>>> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * colorBarType2 and colorBarType3. >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov5640", { >>>> .unitCellSize = { 1400, 1400 }, >>>> @@ -208,10 +268,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 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeOff, 0 }, >>>> { controls::draft::TestPatternModeColorBars, 1 }, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov5675", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeOff, 0 }, >>>> { controls::draft::TestPatternModeColorBars, 1 }, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov5693", { >>>> .unitCellSize = { 1400, 1400 }, >>>> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * Rolling Bar". >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov64a40", { >>>> .unitCellSize = { 1008, 1008 }, >>>> @@ -251,6 +321,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 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * 4: "Vertical Color Bar Type 4" >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov8865", { >>>> .unitCellSize = { 1400, 1400 }, >>>> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * 5: "Color squares with rolling bar" >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov13858", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeOff, 0 }, >>>> { controls::draft::TestPatternModeColorBars, 1 }, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> }; >>>>
Hi Laurent, Kieran On 18/11/2024 01:36, Laurent Pinchart wrote: > Hi Dan, > > On Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote: >> On 15/11/2024 12:10, Kieran Bingham wrote: >>> Quoting Daniel Scally (2024-11-15 07:46:27) >>>> 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 > I had never seen "that're" before. > >>>> taken from Raspberry Pi's CamHelper class definitions. >>>> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>> --- >>>> 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. >>> Sounds resonable to me. >>> >>>> 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 | 9 +++ >>>> src/libcamera/sensor/camera_sensor.cpp | 13 +++ >>>> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++ >>>> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++ >>>> 5 files changed, 136 insertions(+) >>>> >>>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h >>>> index 8aafd82e..a9b2d494 100644 >>>> --- a/include/libcamera/internal/camera_sensor.h >>>> +++ b/include/libcamera/internal/camera_sensor.h >>>> @@ -73,6 +73,8 @@ public: >>>> virtual const std::vector<controls::draft::TestPatternModeEnum> & >>>> testPatternModes() const = 0; >>>> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0; >>>> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0; >>> Could/Should this return a const pointer to a struct SensorDelays now ? >> Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a >> Factory class now) - I don't really have any strong feelings either way, so as long as that's fine >> then I'll make the change. > I think it would make the API nicer. > Hm, how do we represent the defaults then? A global instance of the struct holding defaults? >>>> }; >>>> >>>> class CameraSensorFactoryBase >>>> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h >>>> index 480ac121..56d5c15d 100644 >>>> --- a/include/libcamera/internal/camera_sensor_properties.h >>>> +++ b/include/libcamera/internal/camera_sensor_properties.h >>>> @@ -10,6 +10,8 @@ >>>> #include <map> >>>> #include <string> >>>> >>>> +#include <stdint.h> > stdint.h should go just before string, we mix the C and C++ headers. > >>>> + >>>> #include <libcamera/control_ids.h> >>>> #include <libcamera/geometry.h> >>>> >>>> @@ -20,6 +22,13 @@ struct CameraSensorProperties { >>>> >>>> Size unitCellSize; >>>> std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; >>>> + >>>> + struct { >>>> + uint8_t exposureDelay; >>>> + uint8_t gainDelay; >>>> + uint8_t vblankDelay; >>>> + uint8_t hblankDelay; >>>> + } sensorDelays; >>>> }; >>>> >>>> } /* namespace libcamera */ >>>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp >>>> index 54cf98b2..61420873 100644 >>>> --- a/src/libcamera/sensor/camera_sensor.cpp >>>> +++ b/src/libcamera/sensor/camera_sensor.cpp >>>> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default; >>>> * pattern mode for every frame thus incurs no performance penalty. >>>> */ >>>> >>>> +/** >>>> + * \fn CameraSensor::getSensorDelays() > We don't usually prefix getters with "get". > >>>> + * \brief Fetch the sensor delay values >>>> + * \param[out] exposureDelay The exposure delay >>>> + * \param[out] gainDelay The analogue gain delay >>>> + * \param[out] vblankDelay The vblank delay >>>> + * \param[out] hblankDelay The hblank delay >>>> + * >>>> + * This function fills in sensor control delays for pipeline handlers to use to >>>> + * inform the DelayedControls. If no static properties are available it fills in >>>> + * some widely applicable default values. > We need to return some default values until all sensors provide the > information we need, but I wouldn't call those "widely applicable > default values". It makes it sound those defaults will have a high > chance of working, while that's not specifically the case. > >>>> + */ >>>> + >>>> /** >>>> * \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..84972f02 100644 >>>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp >>>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp >>>> @@ -95,6 +95,8 @@ public: >>>> const std::vector<controls::draft::TestPatternModeEnum> & >>>> testPatternModes() const override { return testPatternModes_; } >>>> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override; >>>> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) override; >>>> >>>> protected: >>>> std::string logPrefix() const override; >>>> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties() >>>> initTestPatternModes(); >>>> } >>>> >>>> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) >>>> +{ >>>> + >>>> + /* >>>> + * These defaults are applicable to many sensors, however more specific >>>> + * values can be added to the CameraSensorProperties for a sensor if >>>> + * required. >>>> + */ >>>> + 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."; > Here the lack of sensor-specific values is reported with a more > appropriate seriousness. Let's update the comments to match. > >>>> + >>>> + exposureDelay = 2; >>>> + gainDelay = 1; >>>> + vblankDelay = 2; >>>> + hblankDelay = 2; >>>> + return; >>>> + } >>>> + >>>> + exposureDelay = staticProps_->sensorDelays.exposureDelay; >>>> + gainDelay = staticProps_->sensorDelays.gainDelay; >>>> + vblankDelay = staticProps_->sensorDelays.vblankDelay; >>>> + hblankDelay = staticProps_->sensorDelays.hblankDelay; >>> Which would make this just >>> >>> return staticProps_->sensorDelays; >>> >>>> +} >>> I'm glad this function prints a warning! >>> >>>> + >>>> 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..6dda7ba9 100644 >>>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp >>>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp >>>> @@ -41,6 +41,13 @@ 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 Holds the delays, expressed in number of frames, between the time a >>>> + * control is applied to the sensor and the time it actually takes effect. >>>> + * Delays are recorded for the exposure time, analogue gain, vertical and >>>> + * horizontal blanking. These values may be defined as empty, in which case the >>>> + * CameraSensor derivative should provide default values. > A \brief should be brief :-) Make it a single short sentence, and you > can add more information in the body of the documentation. > >>>> */ >>>> >>>> /** >>>> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeColorBars, 2 }, >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, >>>> }, >>>> + .sensorDelays = { }, > The AR0144 has a 2 frames delay for all parameters. Assuming that "2 > frames delay" means that parameters set during frame N will take effect > for frame N+2 (so a delay of 0 means the parameter is applied > immediately to the current frame). Maybe this should be documented more > clearly above. > > Note that the sensor can also be programmed to apply a one frame delay > instead of two frames for the analog gain. The value is therefore > driver-dependent, and could even be changed dynamically. That's > something to worry about later. > >>>> } }, >>>> { "ar0521", { >>>> .unitCellSize = { 2200, 2200 }, >>>> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeColorBars, 2 }, >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, >>>> }, >>>> + .sensorDelays = { }, > The AR0521 will be interesting. The exposure time has a 2 frames delay, > while the analog gain will have a 1 frame delay if it is set alone, and > a 2 frames delay if the exposure time is also set during the same frame. > This is something we should consider when we'll develop a tool to > measure delays. > > The sensor can be configured to have a fixed 2 frames delay for the > analog gain, but that's not how the driver currently configures it. > > The delay for the blanking values doesn't seem to be documented. > >>>> } }, >>>> { "hi846", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * 9: "Resolution Pattern" >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "imx214", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, >>>> { controls::draft::TestPatternModePn9, 4 }, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "imx219", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -107,6 +118,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 +134,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 +202,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 +218,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 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * 5: "Color Square" >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov2740", { >>>> .unitCellSize = { 1400, 1400 }, >>>> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeOff, 0 }, >>>> { controls::draft::TestPatternModeColorBars, 1}, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov4689", { >>>> .unitCellSize = { 2000, 2000 }, >>>> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * colorBarType2 and colorBarType3. >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov5640", { >>>> .unitCellSize = { 1400, 1400 }, >>>> @@ -208,10 +268,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 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeOff, 0 }, >>>> { controls::draft::TestPatternModeColorBars, 1 }, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov5675", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeOff, 0 }, >>>> { controls::draft::TestPatternModeColorBars, 1 }, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov5693", { >>>> .unitCellSize = { 1400, 1400 }, >>>> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * Rolling Bar". >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov64a40", { >>>> .unitCellSize = { 1008, 1008 }, >>>> @@ -251,6 +321,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 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * 4: "Vertical Color Bar Type 4" >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov8865", { >>>> .unitCellSize = { 1400, 1400 }, >>>> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> * 5: "Color squares with rolling bar" >>>> */ >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> { "ov13858", { >>>> .unitCellSize = { 1120, 1120 }, >>>> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen >>>> { controls::draft::TestPatternModeOff, 0 }, >>>> { controls::draft::TestPatternModeColorBars, 1 }, >>>> }, >>>> + .sensorDelays = { }, >>>> } }, >>>> }; >>>>
Hi Dan, On Tue, Nov 19, 2024 at 10:25:00AM +0000, Daniel Scally wrote: > On 18/11/2024 01:36, Laurent Pinchart wrote: > > On Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote: > >> On 15/11/2024 12:10, Kieran Bingham wrote: > >>> Quoting Daniel Scally (2024-11-15 07:46:27) > >>>> 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 > > > > I had never seen "that're" before. > > > >>>> taken from Raspberry Pi's CamHelper class definitions. > >>>> > >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >>>> --- > >>>> 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. > >>> Sounds resonable to me. > >>> > >>>> 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 | 9 +++ > >>>> src/libcamera/sensor/camera_sensor.cpp | 13 +++ > >>>> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++ > >>>> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++ > >>>> 5 files changed, 136 insertions(+) > >>>> > >>>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > >>>> index 8aafd82e..a9b2d494 100644 > >>>> --- a/include/libcamera/internal/camera_sensor.h > >>>> +++ b/include/libcamera/internal/camera_sensor.h > >>>> @@ -73,6 +73,8 @@ public: > >>>> virtual const std::vector<controls::draft::TestPatternModeEnum> & > >>>> testPatternModes() const = 0; > >>>> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0; > >>>> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0; > >>> > >>> Could/Should this return a const pointer to a struct SensorDelays now ? > >> > >> Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a > >> Factory class now) - I don't really have any strong feelings either way, so as long as that's fine > >> then I'll make the change. > > > > I think it would make the API nicer. > > Hm, how do we represent the defaults then? A global instance of the > struct holding defaults? You can make it a static const variable, yes. > >>>> }; > >>>> > >>>> class CameraSensorFactoryBase > >>>> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h > >>>> index 480ac121..56d5c15d 100644 > >>>> --- a/include/libcamera/internal/camera_sensor_properties.h > >>>> +++ b/include/libcamera/internal/camera_sensor_properties.h > >>>> @@ -10,6 +10,8 @@ > >>>> #include <map> > >>>> #include <string> > >>>> > >>>> +#include <stdint.h> > > > > stdint.h should go just before string, we mix the C and C++ headers. > > > >>>> + > >>>> #include <libcamera/control_ids.h> > >>>> #include <libcamera/geometry.h> > >>>> > >>>> @@ -20,6 +22,13 @@ struct CameraSensorProperties { > >>>> > >>>> Size unitCellSize; > >>>> std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; > >>>> + > >>>> + struct { > >>>> + uint8_t exposureDelay; > >>>> + uint8_t gainDelay; > >>>> + uint8_t vblankDelay; > >>>> + uint8_t hblankDelay; > >>>> + } sensorDelays; > >>>> }; > >>>> > >>>> } /* namespace libcamera */ > >>>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > >>>> index 54cf98b2..61420873 100644 > >>>> --- a/src/libcamera/sensor/camera_sensor.cpp > >>>> +++ b/src/libcamera/sensor/camera_sensor.cpp > >>>> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default; > >>>> * pattern mode for every frame thus incurs no performance penalty. > >>>> */ > >>>> > >>>> +/** > >>>> + * \fn CameraSensor::getSensorDelays() > > > > We don't usually prefix getters with "get". > > > >>>> + * \brief Fetch the sensor delay values > >>>> + * \param[out] exposureDelay The exposure delay > >>>> + * \param[out] gainDelay The analogue gain delay > >>>> + * \param[out] vblankDelay The vblank delay > >>>> + * \param[out] hblankDelay The hblank delay > >>>> + * > >>>> + * This function fills in sensor control delays for pipeline handlers to use to > >>>> + * inform the DelayedControls. If no static properties are available it fills in > >>>> + * some widely applicable default values. > > > > We need to return some default values until all sensors provide the > > information we need, but I wouldn't call those "widely applicable > > default values". It makes it sound those defaults will have a high > > chance of working, while that's not specifically the case. > > > >>>> + */ > >>>> + > >>>> /** > >>>> * \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..84972f02 100644 > >>>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > >>>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > >>>> @@ -95,6 +95,8 @@ public: > >>>> const std::vector<controls::draft::TestPatternModeEnum> & > >>>> testPatternModes() const override { return testPatternModes_; } > >>>> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override; > >>>> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) override; > >>>> > >>>> protected: > >>>> std::string logPrefix() const override; > >>>> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties() > >>>> initTestPatternModes(); > >>>> } > >>>> > >>>> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, > >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) > >>>> +{ > >>>> + > >>>> + /* > >>>> + * These defaults are applicable to many sensors, however more specific > >>>> + * values can be added to the CameraSensorProperties for a sensor if > >>>> + * required. > >>>> + */ > >>>> + 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."; > > > > Here the lack of sensor-specific values is reported with a more > > appropriate seriousness. Let's update the comments to match. > > > >>>> + > >>>> + exposureDelay = 2; > >>>> + gainDelay = 1; > >>>> + vblankDelay = 2; > >>>> + hblankDelay = 2; > >>>> + return; > >>>> + } > >>>> + > >>>> + exposureDelay = staticProps_->sensorDelays.exposureDelay; > >>>> + gainDelay = staticProps_->sensorDelays.gainDelay; > >>>> + vblankDelay = staticProps_->sensorDelays.vblankDelay; > >>>> + hblankDelay = staticProps_->sensorDelays.hblankDelay; > >>> > >>> Which would make this just > >>> > >>> return staticProps_->sensorDelays; > >>> > >>>> +} > >>> > >>> I'm glad this function prints a warning! > >>> > >>>> + > >>>> 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..6dda7ba9 100644 > >>>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp > >>>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp > >>>> @@ -41,6 +41,13 @@ 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 Holds the delays, expressed in number of frames, between the time a > >>>> + * control is applied to the sensor and the time it actually takes effect. > >>>> + * Delays are recorded for the exposure time, analogue gain, vertical and > >>>> + * horizontal blanking. These values may be defined as empty, in which case the > >>>> + * CameraSensor derivative should provide default values. > > > > A \brief should be brief :-) Make it a single short sentence, and you > > can add more information in the body of the documentation. > > > >>>> */ > >>>> > >>>> /** > >>>> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> { controls::draft::TestPatternModeColorBars, 2 }, > >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > >>>> }, > >>>> + .sensorDelays = { }, > > > > The AR0144 has a 2 frames delay for all parameters. Assuming that "2 > > frames delay" means that parameters set during frame N will take effect > > for frame N+2 (so a delay of 0 means the parameter is applied > > immediately to the current frame). Maybe this should be documented more > > clearly above. > > > > Note that the sensor can also be programmed to apply a one frame delay > > instead of two frames for the analog gain. The value is therefore > > driver-dependent, and could even be changed dynamically. That's > > something to worry about later. > > > >>>> } }, > >>>> { "ar0521", { > >>>> .unitCellSize = { 2200, 2200 }, > >>>> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> { controls::draft::TestPatternModeColorBars, 2 }, > >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > >>>> }, > >>>> + .sensorDelays = { }, > > > > The AR0521 will be interesting. The exposure time has a 2 frames delay, > > while the analog gain will have a 1 frame delay if it is set alone, and > > a 2 frames delay if the exposure time is also set during the same frame. > > This is something we should consider when we'll develop a tool to > > measure delays. > > > > The sensor can be configured to have a fixed 2 frames delay for the > > analog gain, but that's not how the driver currently configures it. > > > > The delay for the blanking values doesn't seem to be documented. > > > >>>> } }, > >>>> { "hi846", { > >>>> .unitCellSize = { 1120, 1120 }, > >>>> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> * 9: "Resolution Pattern" > >>>> */ > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "imx214", { > >>>> .unitCellSize = { 1120, 1120 }, > >>>> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > >>>> { controls::draft::TestPatternModePn9, 4 }, > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "imx219", { > >>>> .unitCellSize = { 1120, 1120 }, > >>>> @@ -107,6 +118,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 +134,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 +202,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 +218,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 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> * 5: "Color Square" > >>>> */ > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "ov2740", { > >>>> .unitCellSize = { 1400, 1400 }, > >>>> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> { controls::draft::TestPatternModeOff, 0 }, > >>>> { controls::draft::TestPatternModeColorBars, 1}, > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "ov4689", { > >>>> .unitCellSize = { 2000, 2000 }, > >>>> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> * colorBarType2 and colorBarType3. > >>>> */ > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "ov5640", { > >>>> .unitCellSize = { 1400, 1400 }, > >>>> @@ -208,10 +268,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 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> { controls::draft::TestPatternModeOff, 0 }, > >>>> { controls::draft::TestPatternModeColorBars, 1 }, > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "ov5675", { > >>>> .unitCellSize = { 1120, 1120 }, > >>>> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> { controls::draft::TestPatternModeOff, 0 }, > >>>> { controls::draft::TestPatternModeColorBars, 1 }, > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "ov5693", { > >>>> .unitCellSize = { 1400, 1400 }, > >>>> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> * Rolling Bar". > >>>> */ > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "ov64a40", { > >>>> .unitCellSize = { 1008, 1008 }, > >>>> @@ -251,6 +321,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 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> * 4: "Vertical Color Bar Type 4" > >>>> */ > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "ov8865", { > >>>> .unitCellSize = { 1400, 1400 }, > >>>> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> * 5: "Color squares with rolling bar" > >>>> */ > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> { "ov13858", { > >>>> .unitCellSize = { 1120, 1120 }, > >>>> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > >>>> { controls::draft::TestPatternModeOff, 0 }, > >>>> { controls::draft::TestPatternModeColorBars, 1 }, > >>>> }, > >>>> + .sensorDelays = { }, > >>>> } }, > >>>> }; > >>>>
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index 8aafd82e..a9b2d494 100644 --- a/include/libcamera/internal/camera_sensor.h +++ b/include/libcamera/internal/camera_sensor.h @@ -73,6 +73,8 @@ public: virtual const std::vector<controls::draft::TestPatternModeEnum> & testPatternModes() const = 0; virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0; + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0; }; class CameraSensorFactoryBase diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h index 480ac121..56d5c15d 100644 --- a/include/libcamera/internal/camera_sensor_properties.h +++ b/include/libcamera/internal/camera_sensor_properties.h @@ -10,6 +10,8 @@ #include <map> #include <string> +#include <stdint.h> + #include <libcamera/control_ids.h> #include <libcamera/geometry.h> @@ -20,6 +22,13 @@ struct CameraSensorProperties { Size unitCellSize; std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; + + struct { + uint8_t exposureDelay; + uint8_t gainDelay; + uint8_t vblankDelay; + uint8_t hblankDelay; + } sensorDelays; }; } /* namespace libcamera */ diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp index 54cf98b2..61420873 100644 --- a/src/libcamera/sensor/camera_sensor.cpp +++ b/src/libcamera/sensor/camera_sensor.cpp @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default; * pattern mode for every frame thus incurs no performance penalty. */ +/** + * \fn CameraSensor::getSensorDelays() + * \brief Fetch the sensor delay values + * \param[out] exposureDelay The exposure delay + * \param[out] gainDelay The analogue gain delay + * \param[out] vblankDelay The vblank delay + * \param[out] hblankDelay The hblank delay + * + * This function fills in sensor control delays for pipeline handlers to use to + * inform the DelayedControls. If no static properties are available it fills in + * some widely applicable default 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..84972f02 100644 --- a/src/libcamera/sensor/camera_sensor_legacy.cpp +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp @@ -95,6 +95,8 @@ public: const std::vector<controls::draft::TestPatternModeEnum> & testPatternModes() const override { return testPatternModes_; } int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override; + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, + uint8_t &vblankDelay, uint8_t &hblankDelay) override; protected: std::string logPrefix() const override; @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties() initTestPatternModes(); } +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay, + uint8_t &vblankDelay, uint8_t &hblankDelay) +{ + + /* + * These defaults are applicable to many sensors, however more specific + * values can be added to the CameraSensorProperties for a sensor if + * required. + */ + 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."; + + exposureDelay = 2; + gainDelay = 1; + vblankDelay = 2; + hblankDelay = 2; + return; + } + + exposureDelay = staticProps_->sensorDelays.exposureDelay; + gainDelay = staticProps_->sensorDelays.gainDelay; + vblankDelay = staticProps_->sensorDelays.vblankDelay; + hblankDelay = staticProps_->sensorDelays.hblankDelay; +} + 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..6dda7ba9 100644 --- a/src/libcamera/sensor/camera_sensor_properties.cpp +++ b/src/libcamera/sensor/camera_sensor_properties.cpp @@ -41,6 +41,13 @@ 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 Holds the delays, expressed in number of frames, between the time a + * control is applied to the sensor and the time it actually takes effect. + * Delays are recorded for the exposure time, analogue gain, vertical and + * horizontal blanking. These values may be defined as empty, in which case the + * CameraSensor derivative should provide default values. */ /** @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeColorBars, 2 }, { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, }, + .sensorDelays = { }, } }, { "ar0521", { .unitCellSize = { 2200, 2200 }, @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeColorBars, 2 }, { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, }, + .sensorDelays = { }, } }, { "hi846", { .unitCellSize = { 1120, 1120 }, @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen * 9: "Resolution Pattern" */ }, + .sensorDelays = { }, } }, { "imx214", { .unitCellSize = { 1120, 1120 }, @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, { controls::draft::TestPatternModePn9, 4 }, }, + .sensorDelays = { }, } }, { "imx219", { .unitCellSize = { 1120, 1120 }, @@ -107,6 +118,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 +134,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 +202,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 +218,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 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen * 5: "Color Square" */ }, + .sensorDelays = { }, } }, { "ov2740", { .unitCellSize = { 1400, 1400 }, @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeOff, 0 }, { controls::draft::TestPatternModeColorBars, 1}, }, + .sensorDelays = { }, } }, { "ov4689", { .unitCellSize = { 2000, 2000 }, @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen * colorBarType2 and colorBarType3. */ }, + .sensorDelays = { }, } }, { "ov5640", { .unitCellSize = { 1400, 1400 }, @@ -208,10 +268,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 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeOff, 0 }, { controls::draft::TestPatternModeColorBars, 1 }, }, + .sensorDelays = { }, } }, { "ov5675", { .unitCellSize = { 1120, 1120 }, @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeOff, 0 }, { controls::draft::TestPatternModeColorBars, 1 }, }, + .sensorDelays = { }, } }, { "ov5693", { .unitCellSize = { 1400, 1400 }, @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen * Rolling Bar". */ }, + .sensorDelays = { }, } }, { "ov64a40", { .unitCellSize = { 1008, 1008 }, @@ -251,6 +321,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 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen * 4: "Vertical Color Bar Type 4" */ }, + .sensorDelays = { }, } }, { "ov8865", { .unitCellSize = { 1400, 1400 }, @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen * 5: "Color squares with rolling bar" */ }, + .sensorDelays = { }, } }, { "ov13858", { .unitCellSize = { 1120, 1120 }, @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeOff, 0 }, { controls::draft::TestPatternModeColorBars, 1 }, }, + .sensorDelays = { }, } }, };
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 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 | 9 +++ src/libcamera/sensor/camera_sensor.cpp | 13 +++ src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++ .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++ 5 files changed, 136 insertions(+)