| Message ID | 20220630133902.321099-10-jacopo@jmondi.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:48) > Add to the camera sensor properties a map of control identifiers to > their latencies. > > Some camera sensor controls take a variable number of frames to take > effect, hence they should be set in advance. The newly introduced map > records that information as a property of the camera sensor class. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../internal/camera_sensor_properties.h | 4 ++ > .../camera_sensor_properties.cpp | 40 +++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h > index 1ee3cb994106..2404b0674323 100644 > --- a/include/libcamera/internal/camera_sensor_properties.h > +++ b/include/libcamera/internal/camera_sensor_properties.h > @@ -9,6 +9,7 @@ > > #include <map> > #include <string> > +#include <unordered_map> > > #include <libcamera/control_ids.h> > #include <libcamera/geometry.h> > @@ -16,10 +17,13 @@ > namespace libcamera { > > struct CameraSensorProperties { > + using SensorDelays = std::unordered_map<const ControlId *, unsigned int>; > + > static const CameraSensorProperties *get(const std::string &sensor); > > Size unitCellSize; > std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; > + SensorDelays sensorDelays; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera_sensor/camera_sensor_properties.cpp b/src/libcamera/camera_sensor/camera_sensor_properties.cpp > index e5f27f06eb1d..4b5ab716abe2 100644 > --- a/src/libcamera/camera_sensor/camera_sensor_properties.cpp > +++ b/src/libcamera/camera_sensor/camera_sensor_properties.cpp > @@ -13,6 +13,8 @@ > > #include <libcamera/control_ids.h> > > +#include <libcamera/internal/control_ids.h> > + > /** > * \file camera_sensor_properties.h > * \brief Database of camera sensor properties > @@ -41,6 +43,18 @@ 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 Map that associates control identifiers with their latencies, > + * expressed in frame periods. > + */ > + > +/** > + * \typedef CameraSensorProperties::SensorDelays > + * \brief Camera sensor controls delays > + * > + * Type to represent a map of camera sensor controls associated with their > + * latencies, expressed in frame periods. > */ > I wonder if defining a 'common' struct of sensor delays here could ease referencing it in the default case. static const SensorDelays commonDelays = { { &controls::internal::FrameDuration, 2 }, { &controls::internal::ExposureTime, 2 }, { &controls::internal::AnalogueGain, 1 }, }; Or ideally, if we knew 'all ccs' cameras or such used these delays we could have a better name ? defaultDelays ? I know I commented on this before saying if we don't have the numbers we should leave the the delays as zero, but I think there I meant on an individual entry. But perhaps now I see this I'm changing my mind too - and leaning towards a default set can easily be defined and referenced, and overridden in the case of a difference? > /** > @@ -69,6 +83,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > * 9: "Resolution Pattern" > */ > }, > + .sensorDelays = { }, then .sensorDelays = &commonDelays; ? Though this is the hi846, so we should identify what the delays really are there. > } }, > { "imx219", { > .unitCellSize = { 1120, 1120 }, > @@ -79,6 +94,11 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > { controls::draft::TestPatternModePn9, 4 }, > }, > + .sensorDelays = { > + { &controls::internal::FrameDuration, 2 }, > + { &controls::internal::ExposureTime, 2 }, > + { &controls::internal::AnalogueGain, 1 }, > + }, But this one could be: .sensorDelays = &commonDelays; > } }, > { "imx258", { > .unitCellSize = { 1120, 1120 }, > @@ -89,18 +109,26 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > { controls::draft::TestPatternModePn9, 4 }, > }, > + .sensorDelays = { }, > } }, > { "imx290", { > .unitCellSize = { 2900, 2900 }, > .testPatternModes = {}, > + .sensorDelays = { > + { &controls::internal::FrameDuration, 2 }, > + { &controls::internal::ExposureTime, 2 }, > + { &controls::internal::AnalogueGain, 2 }, > + }, > } }, > { "imx296", { > .unitCellSize = { 3450, 3450 }, > .testPatternModes = {}, > + .sensorDelays = { }, The IMX296 is listed in RPi's cam_helper_imx296 without an override. That means in their implementation that this is expected to use .sensorDelays = &commonDelays; > } }, > { "imx477", { > .unitCellSize = { 1550, 1550 }, > .testPatternModes = {}, > + .sensorDelays = { }, cam_helper_imx477 has the following overrides: exposure_delay = 2; gain_delay = 2; vblank_delay = 3; > } }, > { "ov2740", { > .unitCellSize = { 1400, 1400 }, > @@ -108,6 +136,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeOff, 0 }, > { controls::draft::TestPatternModeColorBars, 1}, > }, > + .sensorDelays = { }, > } }, > { "ov5640", { > .unitCellSize = { 1400, 1400 }, > @@ -115,10 +144,16 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeOff, 0 }, > { controls::draft::TestPatternModeColorBars, 1 }, > }, > + .sensorDelays = { }, > } }, > { "ov5647", { > .unitCellSize = { 1400, 1400 }, > .testPatternModes = {}, > + .sensorDelays = { > + { &controls::internal::FrameDuration, 2 }, > + { &controls::internal::ExposureTime, 2 }, > + { &controls::internal::AnalogueGain, 2 }, > + }, > } }, > { "ov5670", { > .unitCellSize = { 1120, 1120 }, > @@ -126,6 +161,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeOff, 0 }, > { controls::draft::TestPatternModeColorBars, 1 }, > }, > + .sensorDelays = { }, > } }, > { "ov5675", { > .unitCellSize = { 1120, 1120 }, > @@ -133,6 +169,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeOff, 0 }, > { controls::draft::TestPatternModeColorBars, 1 }, > }, > + .sensorDelays = { }, > } }, > { "ov5693", { > .unitCellSize = { 1400, 1400 }, > @@ -145,6 +182,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > * Rolling Bar". > */ > }, > + .sensorDelays = { }, > } }, > { "ov8865", { > .unitCellSize = { 1400, 1400 }, > @@ -159,6 +197,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > * 5: "Color squares with rolling bar" > */ > }, > + .sensorDelays = { }, > } }, > { "ov13858", { > .unitCellSize = { 1120, 1120 }, > @@ -166,6 +205,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > { controls::draft::TestPatternModeOff, 0 }, > { controls::draft::TestPatternModeColorBars, 1 }, > }, > + .sensorDelays = { }, > } }, > }; > > -- > 2.36.1 >
Hi Kieran On Thu, Jun 30, 2022 at 11:37:32PM +0100, Kieran Bingham wrote: > Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:48) > > Add to the camera sensor properties a map of control identifiers to > > their latencies. > > > > Some camera sensor controls take a variable number of frames to take > > effect, hence they should be set in advance. The newly introduced map > > records that information as a property of the camera sensor class. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > .../internal/camera_sensor_properties.h | 4 ++ > > .../camera_sensor_properties.cpp | 40 +++++++++++++++++++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h > > index 1ee3cb994106..2404b0674323 100644 > > --- a/include/libcamera/internal/camera_sensor_properties.h > > +++ b/include/libcamera/internal/camera_sensor_properties.h > > @@ -9,6 +9,7 @@ > > > > #include <map> > > #include <string> > > +#include <unordered_map> > > > > #include <libcamera/control_ids.h> > > #include <libcamera/geometry.h> > > @@ -16,10 +17,13 @@ > > namespace libcamera { > > > > struct CameraSensorProperties { > > + using SensorDelays = std::unordered_map<const ControlId *, unsigned int>; > > + > > static const CameraSensorProperties *get(const std::string &sensor); > > > > Size unitCellSize; > > std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; > > + SensorDelays sensorDelays; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/camera_sensor/camera_sensor_properties.cpp b/src/libcamera/camera_sensor/camera_sensor_properties.cpp > > index e5f27f06eb1d..4b5ab716abe2 100644 > > --- a/src/libcamera/camera_sensor/camera_sensor_properties.cpp > > +++ b/src/libcamera/camera_sensor/camera_sensor_properties.cpp > > @@ -13,6 +13,8 @@ > > > > #include <libcamera/control_ids.h> > > > > +#include <libcamera/internal/control_ids.h> > > + > > /** > > * \file camera_sensor_properties.h > > * \brief Database of camera sensor properties > > @@ -41,6 +43,18 @@ 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 Map that associates control identifiers with their latencies, > > + * expressed in frame periods. > > + */ > > + > > +/** > > + * \typedef CameraSensorProperties::SensorDelays > > + * \brief Camera sensor controls delays > > + * > > + * Type to represent a map of camera sensor controls associated with their > > + * latencies, expressed in frame periods. > > */ > > > > I wonder if defining a 'common' struct of sensor delays here could ease > referencing it in the default case. > > static const SensorDelays commonDelays = { > { &controls::internal::FrameDuration, 2 }, > { &controls::internal::ExposureTime, 2 }, > { &controls::internal::AnalogueGain, 1 }, > }; > If we want something like this, I think it should not be here but rather in CameraSensor. Have a look at [PATCH v3 10/23] libcamera: camera_sensor: Initialize delayed controls +int CameraSensor::initDelayedControls() +{ + /* If no static priorities are available, all controls have delay 0. */ + static const CameraSensorProperties::SensorDelays noDelays{}; + const CameraSensorProperties::SensorDelays *delays = staticProps_ + ? &staticProps_->sensorDelays + : &noDelays; The noDelays{} there can be made a defaultDelays map. I actually had it but I had no idea how "default" those defaults actually are.. > Or ideally, if we knew 'all ccs' cameras or such used these delays we > could have a better name ? > > defaultDelays ? > > I know I commented on this before saying if we don't have the numbers we > should leave the the delays as zero, but I think there I meant on an > individual entry. > > But perhaps now I see this I'm changing my mind too - and leaning > towards a default set can easily be defined and referenced, and > overridden in the case of a difference? > > > > /** > > @@ -69,6 +83,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > * 9: "Resolution Pattern" > > */ > > }, > > + .sensorDelays = { }, > > then > .sensorDelays = &commonDelays; > ? > > Though this is the hi846, so we should identify what the delays really > are there. > > > } }, > > { "imx219", { > > .unitCellSize = { 1120, 1120 }, > > @@ -79,6 +94,11 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > > { controls::draft::TestPatternModePn9, 4 }, > > }, > > + .sensorDelays = { > > + { &controls::internal::FrameDuration, 2 }, > > + { &controls::internal::ExposureTime, 2 }, > > + { &controls::internal::AnalogueGain, 1 }, > > + }, > > But this one could be: > .sensorDelays = &commonDelays; > > > } }, > > { "imx258", { > > .unitCellSize = { 1120, 1120 }, > > @@ -89,18 +109,26 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, > > { controls::draft::TestPatternModePn9, 4 }, > > }, > > + .sensorDelays = { }, > > } }, > > { "imx290", { > > .unitCellSize = { 2900, 2900 }, > > .testPatternModes = {}, > > + .sensorDelays = { > > + { &controls::internal::FrameDuration, 2 }, > > + { &controls::internal::ExposureTime, 2 }, > > + { &controls::internal::AnalogueGain, 2 }, > > + }, > > } }, > > { "imx296", { > > .unitCellSize = { 3450, 3450 }, > > .testPatternModes = {}, > > + .sensorDelays = { }, > > The IMX296 is listed in RPi's cam_helper_imx296 without an override. > That means in their implementation that this is expected to use > > .sensorDelays = &commonDelays; Ah ok, missed that ! > > > } }, > > { "imx477", { > > .unitCellSize = { 1550, 1550 }, > > .testPatternModes = {}, > > + .sensorDelays = { }, > > cam_helper_imx477 has the following overrides: > > exposure_delay = 2; > gain_delay = 2; > vblank_delay = 3; > Ah thanks, I'll fix > > } }, > > { "ov2740", { > > .unitCellSize = { 1400, 1400 }, > > @@ -108,6 +136,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > { controls::draft::TestPatternModeOff, 0 }, > > { controls::draft::TestPatternModeColorBars, 1}, > > }, > > + .sensorDelays = { }, > > } }, > > { "ov5640", { > > .unitCellSize = { 1400, 1400 }, > > @@ -115,10 +144,16 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > { controls::draft::TestPatternModeOff, 0 }, > > { controls::draft::TestPatternModeColorBars, 1 }, > > }, > > + .sensorDelays = { }, > > } }, > > { "ov5647", { > > .unitCellSize = { 1400, 1400 }, > > .testPatternModes = {}, > > + .sensorDelays = { > > + { &controls::internal::FrameDuration, 2 }, > > + { &controls::internal::ExposureTime, 2 }, > > + { &controls::internal::AnalogueGain, 2 }, > > + }, > > } }, > > { "ov5670", { > > .unitCellSize = { 1120, 1120 }, > > @@ -126,6 +161,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > { controls::draft::TestPatternModeOff, 0 }, > > { controls::draft::TestPatternModeColorBars, 1 }, > > }, > > + .sensorDelays = { }, > > } }, > > { "ov5675", { > > .unitCellSize = { 1120, 1120 }, > > @@ -133,6 +169,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > { controls::draft::TestPatternModeOff, 0 }, > > { controls::draft::TestPatternModeColorBars, 1 }, > > }, > > + .sensorDelays = { }, > > } }, > > { "ov5693", { > > .unitCellSize = { 1400, 1400 }, > > @@ -145,6 +182,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > * Rolling Bar". > > */ > > }, > > + .sensorDelays = { }, > > } }, > > { "ov8865", { > > .unitCellSize = { 1400, 1400 }, > > @@ -159,6 +197,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > * 5: "Color squares with rolling bar" > > */ > > }, > > + .sensorDelays = { }, > > } }, > > { "ov13858", { > > .unitCellSize = { 1120, 1120 }, > > @@ -166,6 +205,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > > { controls::draft::TestPatternModeOff, 0 }, > > { controls::draft::TestPatternModeColorBars, 1 }, > > }, > > + .sensorDelays = { }, > > } }, > > }; > > > > -- > > 2.36.1 > >
diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h index 1ee3cb994106..2404b0674323 100644 --- a/include/libcamera/internal/camera_sensor_properties.h +++ b/include/libcamera/internal/camera_sensor_properties.h @@ -9,6 +9,7 @@ #include <map> #include <string> +#include <unordered_map> #include <libcamera/control_ids.h> #include <libcamera/geometry.h> @@ -16,10 +17,13 @@ namespace libcamera { struct CameraSensorProperties { + using SensorDelays = std::unordered_map<const ControlId *, unsigned int>; + static const CameraSensorProperties *get(const std::string &sensor); Size unitCellSize; std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes; + SensorDelays sensorDelays; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera_sensor/camera_sensor_properties.cpp b/src/libcamera/camera_sensor/camera_sensor_properties.cpp index e5f27f06eb1d..4b5ab716abe2 100644 --- a/src/libcamera/camera_sensor/camera_sensor_properties.cpp +++ b/src/libcamera/camera_sensor/camera_sensor_properties.cpp @@ -13,6 +13,8 @@ #include <libcamera/control_ids.h> +#include <libcamera/internal/control_ids.h> + /** * \file camera_sensor_properties.h * \brief Database of camera sensor properties @@ -41,6 +43,18 @@ 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 Map that associates control identifiers with their latencies, + * expressed in frame periods. + */ + +/** + * \typedef CameraSensorProperties::SensorDelays + * \brief Camera sensor controls delays + * + * Type to represent a map of camera sensor controls associated with their + * latencies, expressed in frame periods. */ /** @@ -69,6 +83,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen * 9: "Resolution Pattern" */ }, + .sensorDelays = { }, } }, { "imx219", { .unitCellSize = { 1120, 1120 }, @@ -79,6 +94,11 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, { controls::draft::TestPatternModePn9, 4 }, }, + .sensorDelays = { + { &controls::internal::FrameDuration, 2 }, + { &controls::internal::ExposureTime, 2 }, + { &controls::internal::AnalogueGain, 1 }, + }, } }, { "imx258", { .unitCellSize = { 1120, 1120 }, @@ -89,18 +109,26 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeColorBarsFadeToGray, 3 }, { controls::draft::TestPatternModePn9, 4 }, }, + .sensorDelays = { }, } }, { "imx290", { .unitCellSize = { 2900, 2900 }, .testPatternModes = {}, + .sensorDelays = { + { &controls::internal::FrameDuration, 2 }, + { &controls::internal::ExposureTime, 2 }, + { &controls::internal::AnalogueGain, 2 }, + }, } }, { "imx296", { .unitCellSize = { 3450, 3450 }, .testPatternModes = {}, + .sensorDelays = { }, } }, { "imx477", { .unitCellSize = { 1550, 1550 }, .testPatternModes = {}, + .sensorDelays = { }, } }, { "ov2740", { .unitCellSize = { 1400, 1400 }, @@ -108,6 +136,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeOff, 0 }, { controls::draft::TestPatternModeColorBars, 1}, }, + .sensorDelays = { }, } }, { "ov5640", { .unitCellSize = { 1400, 1400 }, @@ -115,10 +144,16 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeOff, 0 }, { controls::draft::TestPatternModeColorBars, 1 }, }, + .sensorDelays = { }, } }, { "ov5647", { .unitCellSize = { 1400, 1400 }, .testPatternModes = {}, + .sensorDelays = { + { &controls::internal::FrameDuration, 2 }, + { &controls::internal::ExposureTime, 2 }, + { &controls::internal::AnalogueGain, 2 }, + }, } }, { "ov5670", { .unitCellSize = { 1120, 1120 }, @@ -126,6 +161,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeOff, 0 }, { controls::draft::TestPatternModeColorBars, 1 }, }, + .sensorDelays = { }, } }, { "ov5675", { .unitCellSize = { 1120, 1120 }, @@ -133,6 +169,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeOff, 0 }, { controls::draft::TestPatternModeColorBars, 1 }, }, + .sensorDelays = { }, } }, { "ov5693", { .unitCellSize = { 1400, 1400 }, @@ -145,6 +182,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen * Rolling Bar". */ }, + .sensorDelays = { }, } }, { "ov8865", { .unitCellSize = { 1400, 1400 }, @@ -159,6 +197,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen * 5: "Color squares with rolling bar" */ }, + .sensorDelays = { }, } }, { "ov13858", { .unitCellSize = { 1120, 1120 }, @@ -166,6 +205,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen { controls::draft::TestPatternModeOff, 0 }, { controls::draft::TestPatternModeColorBars, 1 }, }, + .sensorDelays = { }, } }, };
Add to the camera sensor properties a map of control identifiers to their latencies. Some camera sensor controls take a variable number of frames to take effect, hence they should be set in advance. The newly introduced map records that information as a property of the camera sensor class. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- .../internal/camera_sensor_properties.h | 4 ++ .../camera_sensor_properties.cpp | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+)