[PATCH v4 1/2] libcamera: camera_sensor_properties: Add sensor control delays
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Nov 19 17:50:43 CET 2024
Hi Dan
On Tue, Nov 19, 2024 at 04:20:16PM +0000, Dan Scally wrote:
> Hi Jacopo - thank you for the review
>
> On 19/11/2024 15:57, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Tue, Nov 19, 2024 at 01:03:00PM +0000, Daniel Scally wrote:
> > > Add properties covering the sensor control application delays to both
> > > the static CameraSensorProperties definitions. The values used are the
> > > defaults that're in use across the library, with deviations from that
> > This probably needs to be updated
>
> Indeed
>
> >
> > > taken from Raspberry Pi's CamHelper class definitions.
> > >
> > > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > > ---
> > > Changes in v4:
> > >
> > > - getSensorDelays() renamed to sensorDelays()
> > > - sensorDelays() returns a reference to the SensorDelays struct rather
> > > than each of the values.
> > > - Reworded the comments to make the default values seem less acceptable
> > > - Added delay values for AR0144
> > >
> > > Changes in v3:
> > >
> > > - Rebased on top of the CameraSensorFactory introduction
> > > - Some rephrasing
> > > - Defined the sensorDelays member as empty where Raspberry Pi didn't
> > > have any specific values. Check for the empty struct in
> > > getSensorDelays() and return the defaults from there with a warning.
> > >
> > > Changes in v2:
> > >
> > > - Rather than adding the delays to the properties ControlList, added a
> > > new function in CameraSensor that allows PipelineHandlers to retreive
> > > the delay values.
> > >
> > > include/libcamera/internal/camera_sensor.h | 2 +
> > > .../internal/camera_sensor_properties.h | 10 ++
> > > src/libcamera/sensor/camera_sensor.cpp | 12 ++
> > > src/libcamera/sensor/camera_sensor_legacy.cpp | 18 +++
> > > .../sensor/camera_sensor_properties.cpp | 121 ++++++++++++++++++
> > > 5 files changed, 163 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index 8aafd82e..bbdb83a1 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -20,6 +20,7 @@
> > > #include <libcamera/orientation.h>
> > > #include <libcamera/transform.h>
> > >
> > > +#include "libcamera/internal/camera_sensor_properties.h"
> > > #include "libcamera/internal/bayer_format.h"
> > > #include "libcamera/internal/v4l2_subdevice.h"
> > >
> > > @@ -73,6 +74,7 @@ public:
> > > virtual const std::vector<controls::draft::TestPatternModeEnum> &
> > > testPatternModes() const = 0;
> > > virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;
> > > + virtual const CameraSensorProperties::SensorDelays &sensorDelays() = 0;
> > > };
> > >
> > > class CameraSensorFactoryBase
> > > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
> > > index 480ac121..fe40bd7f 100644
> > > --- a/include/libcamera/internal/camera_sensor_properties.h
> > > +++ b/include/libcamera/internal/camera_sensor_properties.h
> > > @@ -8,6 +8,7 @@
> > > #pragma once
> > >
> > > #include <map>
> > > +#include <stdint.h>
> > > #include <string>
> > >
> > > #include <libcamera/control_ids.h>
> > > @@ -20,6 +21,15 @@ struct CameraSensorProperties {
> > >
> > > Size unitCellSize;
> > > std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
> > > +
> > > + struct SensorDelays {
> > > + uint8_t exposureDelay;
> > > + uint8_t gainDelay;
> > > + uint8_t vblankDelay;
> > > + uint8_t hblankDelay;
> > > + } sensorDelays;
> > > };
> > >
> > > +extern const CameraSensorProperties::SensorDelays defaultSensorDelays;
> > > +
> > Fine. I thought we could have defined this inside
> > CameraSensor::sensorDelays() as
> >
> > const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
> > {
> > static constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
> > .exposureDelay = 2,
> > .gainDelay = 1,
> > .vblankDelay = 2,
> > .hblankDelay = 2,
> > };
> Are references to that safe from outside the function scope? If so then sure we could do that instead...
I don't see any code reference outside of this, but indeed
documentation links would be broken.
> > if (!staticProps_ ||
> > (!staticProps_->sensorDelays.exposureDelay &&
> > !staticProps_->sensorDelays.gainDelay &&
> > !staticProps_->sensorDelays.vblankDelay &&
> > !staticProps_->sensorDelays.hblankDelay)) {
> > LOG(CameraSensor, Warning)
> > << "No sensor delays found in static properties. "
> > "Assuming unverified defaults.";
> >
> > return defaultSensorDelays;
> > }
> >
> > return staticProps_->sensorDelays;
> > }
> >
> > But then I realized that if we introduced a new derived CameraSensor
> > class this will have to be replicated in two places. Btw, the
> > function will have to replicated anyhow as we don't have any concrete
> > base class between the abstract CameraSensor and
> > CameraSensorLegacy/CameraSensorRaw.
> I don't know that that's such a problem...I'm not following the plans for
> CameraSensorRaw but I wondered about a CameraSensorTPG or something and we'd
> presumably want to define the delays differently there anyway.
Fair point
> >
> > We might want to consider
> >
> > +----------------------------+
> > | Abstract |
> > | CameraSensorInterface |
> > +----------------------------+
> > ^
> > |
> > +----------------------------+
> > | CameraSensorBase |
> > +----------------------------+
> > ^
> > --------------|--------------
> > | |
> > | |
> > +-------------+ +-------------+
> > | Legacy | | RAW |
> > +-------------+ +-------------+
> >
> > Where to group common functions like this one. Let's consider it when
> > adding the new RAW class. I wonder that for the time being defaultSensorDelays
> > shouldn't be moved inside CameraSensorLegacy::sensorDelays().
> >
> >
> >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> > > index 54cf98b2..e9ca291c 100644
> > > --- a/src/libcamera/sensor/camera_sensor.cpp
> > > +++ b/src/libcamera/sensor/camera_sensor.cpp
> > > @@ -336,6 +336,18 @@ CameraSensor::~CameraSensor() = default;
> > > * pattern mode for every frame thus incurs no performance penalty.
> > > */
> > >
> > > +/**
> > > + * \fn CameraSensor::sensorDelays()
> > > + * \brief Fetch the sensor delay values
> > > + *
> > > + * This function fills in sensor control delays for pipeline handlers to use to
> > s/fills in/retreieves the/
> ack
> >
> > > + * inform the DelayedControls. If no static properties are available it returns
> > > + * a reference to the default sensor delays.
> > What are default ? How have they been measured and how accurate they
> > are ? I would
> >
> > * inform the DelayedControls. If controls delays are not specied in the static
> > * sensor properties database, this function returns a reference to a
> > * set of default sensor delays provided as best-effort place-holders
> > * for the actual sensor-specific delays.
> > *
> > * \sa defaultSensorDelays
>
>
> Sure that sounds fine to me
>
> >
> > > + *
> > > + * \return A reference to a struct CameraSensorProperties::SensorDelays holding
> > > + * the delay values.
> > remove the full stop please
>
>
> Okedokey
>
> >
> > > + */
> > > +
> > > /**
> > > * \class CameraSensorFactoryBase
> > > * \brief Base class for camera sensor factories
> > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > index a9b15c03..f9202606 100644
> > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > @@ -95,6 +95,7 @@ public:
> > > const std::vector<controls::draft::TestPatternModeEnum> &
> > > testPatternModes() const override { return testPatternModes_; }
> > > int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;
> > > + const CameraSensorProperties::SensorDelays &sensorDelays() override;
> > >
> > > protected:
> > > std::string logPrefix() const override;
> > > @@ -482,6 +483,23 @@ void CameraSensorLegacy::initStaticProperties()
> > > initTestPatternModes();
> > > }
> > >
> > > +const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
> > > +{
> > > + if (!staticProps_ ||
> > > + (!staticProps_->sensorDelays.exposureDelay &&
> > > + !staticProps_->sensorDelays.gainDelay &&
> > > + !staticProps_->sensorDelays.vblankDelay &&
> > > + !staticProps_->sensorDelays.hblankDelay)) {
> > > + LOG(CameraSensor, Warning)
> > > + << "No sensor delays found in static properties. "
> > > + "Assuming unverified defaults.";
> > > +
> > > + return defaultSensorDelays;
> > > + }
> > > +
> > > + return staticProps_->sensorDelays;
> > > +}
> > > +
> > > void CameraSensorLegacy::initTestPatternModes()
> > > {
> > > const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > index 6d4136d0..97202bee 100644
> > > --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> > > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > @@ -41,7 +41,51 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)
> > > * \brief Map that associates the TestPattern control value with the indexes of
> > > * the corresponding sensor test pattern modes as returned by
> > > * V4L2_CID_TEST_PATTERN.
> > > + *
> > > + * \var CameraSensorProperties::sensorDelays
> > > + * \brief Sensor control application delays.
> > > + *
> > > + * This struct may be defined as empty, in which case the CameraSensor
> > > + * derivative should provide some appropriate default values.
> > True this is internal stuff, but this seems an implementation detail I
> > would
> >
> > * This struct may be defined as empty if the actual sensor delays are
> > * not available or have not been measured. Best-effort default values
> > * are returned in this case.
> > *
> > * \sa defaultSensorDelays
>
>
> Okedokey
>
> >
> > > + */
> > > +
> > > +/**
> > > + * \struct CameraSensorProperties::SensorDelays
> > > + * \brief Sensor control application delay values
> > > + *
> > > + * This struct holds delay values, expressed in number of frames, between the
> > > + * time a control value is applied to the sensor and the time that value is
> > > + * reflected in the output. For example "2 frames delay" means that parameters
> > > + * set during frame N will take effect for frame N+2 (and by extension a delay
> > > + * of 0 would mean the parameter is applied immediately to the current frame).
> > "to the current frame", if you mean the one currently being exposed, seems
> > unlikely to me, as I presume changes take effect at least in the next frame.
>
> Me too, I just took Laurent's wording as I thought it made it very clear.
>
Ah, let's wait for him to comment then
> >
> > > + *
> > > + * \var CameraSensorProperties::SensorDelays::exposureDelay
> > > + * \brief Number of frames between application of exposure control and effect
> > > + *
> > > + * \var CameraSensorProperties::SensorDelays::gainDelay
> > > + * \brief Number of frames between application of analogue gain control and effect
> > Will we ever have digital gain here ?
> Possibly - it's a topic for the discussion with Anthony tomorrow in fact.
> >
> > > + *
> > > + * \var CameraSensorProperties::SensorDelays::vblankDelay
> > > + * \brief Number of frames between application of vblank control and effect
> > > + *
> > > + * \var CameraSensorProperties::SensorDelays::hblankDelay
> > > + * \brief Number of frames between application of hblank control and effect
> > > + */
> > > +
> > > +/**
> > > + * \brief Default sensor control application delay values
> > "application" sounds confusing to me, I would drop it.
> Hm, application in this sense meaning "the action of putting something into
> operation", does just "sensor control delay values" convey that meaning
> already? If so it's fine to drop.
Maybe it's just me being confused by what "application" usually means
then :)
Up to you, really
Thanks
j
> >
> > > + *
> > > + * These sensor control delays are intended to be used where no specific values
> > > + * are defined for a particular sensor in its CameraSensorProperties. These are
> > > + * not verified for use with any particular sensor and so should be used with
> > > + * caution.
> > > */
> > > +constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 1,
> > > + .vblankDelay = 2,
> > > + .hblankDelay = 2,
> > > +};
> > >
> > > /**
> > > * \brief Retrieve the properties associated with a sensor
> > > @@ -60,6 +104,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeColorBars, 2 },
> > > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > > },
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 2,
> > > + .vblankDelay = 2,
> > > + .hblankDelay = 2
> > > + },
> > > } },
> > > { "ar0521", {
> > > .unitCellSize = { 2200, 2200 },
> > > @@ -69,6 +119,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeColorBars, 2 },
> > > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "hi846", {
> > > .unitCellSize = { 1120, 1120 },
> > > @@ -87,6 +138,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > * 9: "Resolution Pattern"
> > > */
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "imx214", {
> > > .unitCellSize = { 1120, 1120 },
> > > @@ -97,6 +149,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > > { controls::draft::TestPatternModePn9, 4 },
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "imx219", {
> > > .unitCellSize = { 1120, 1120 },
> > > @@ -107,6 +160,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > > { controls::draft::TestPatternModePn9, 4 },
> > > },
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 1,
> > > + .vblankDelay = 2,
> > > + .hblankDelay = 2
> > > + },
> > > } },
> > > { "imx258", {
> > > .unitCellSize = { 1120, 1120 },
> > > @@ -117,34 +176,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > > { controls::draft::TestPatternModePn9, 4 },
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "imx283", {
> > > .unitCellSize = { 2400, 2400 },
> > > .testPatternModes = {},
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 2,
> > > + .vblankDelay = 2,
> > > + .hblankDelay = 2
> > > + },
> > > } },
> > > { "imx290", {
> > > .unitCellSize = { 2900, 2900 },
> > > .testPatternModes = {},
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 2,
> > > + .vblankDelay = 2,
> > > + .hblankDelay = 2
> > > + },
> > > } },
> > > { "imx296", {
> > > .unitCellSize = { 3450, 3450 },
> > > .testPatternModes = {},
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 2,
> > > + .vblankDelay = 2,
> > > + .hblankDelay = 2
> > > + },
> > > } },
> > > { "imx327", {
> > > .unitCellSize = { 2900, 2900 },
> > > .testPatternModes = {},
> > > + .sensorDelays = { },
> > > } },
> > > { "imx335", {
> > > .unitCellSize = { 2000, 2000 },
> > > .testPatternModes = {},
> > > + .sensorDelays = { },
> > > } },
> > > { "imx415", {
> > > .unitCellSize = { 1450, 1450 },
> > > .testPatternModes = {},
> > > + .sensorDelays = { },
> > > } },
> > > { "imx477", {
> > > .unitCellSize = { 1550, 1550 },
> > > .testPatternModes = {},
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 2,
> > > + .vblankDelay = 3,
> > > + .hblankDelay = 3
> > > + },
> > > } },
> > > { "imx519", {
> > > .unitCellSize = { 1220, 1220 },
> > > @@ -157,6 +244,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).
> > > */
> > > },
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 2,
> > > + .vblankDelay = 3,
> > > + .hblankDelay = 3
> > > + },
> > > } },
> > > { "imx708", {
> > > .unitCellSize = { 1400, 1400 },
> > > @@ -167,6 +260,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > > { controls::draft::TestPatternModePn9, 4 },
> > > },
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 2,
> > > + .vblankDelay = 3,
> > > + .hblankDelay = 3
> > > + },
> > > } },
> > > { "ov2685", {
> > > .unitCellSize = { 1750, 1750 },
> > > @@ -181,6 +280,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > * 5: "Color Square"
> > > */
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "ov2740", {
> > > .unitCellSize = { 1400, 1400 },
> > > @@ -188,6 +288,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeOff, 0 },
> > > { controls::draft::TestPatternModeColorBars, 1},
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "ov4689", {
> > > .unitCellSize = { 2000, 2000 },
> > > @@ -201,6 +302,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > * colorBarType2 and colorBarType3.
> > > */
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "ov5640", {
> > > .unitCellSize = { 1400, 1400 },
> > > @@ -208,10 +310,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeOff, 0 },
> > > { controls::draft::TestPatternModeColorBars, 1 },
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "ov5647", {
> > > .unitCellSize = { 1400, 1400 },
> > > .testPatternModes = {},
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 2,
> > > + .vblankDelay = 2,
> > > + .hblankDelay = 2
> > > + },
> > > } },
> > > { "ov5670", {
> > > .unitCellSize = { 1120, 1120 },
> > > @@ -219,6 +328,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeOff, 0 },
> > > { controls::draft::TestPatternModeColorBars, 1 },
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "ov5675", {
> > > .unitCellSize = { 1120, 1120 },
> > > @@ -226,6 +336,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeOff, 0 },
> > > { controls::draft::TestPatternModeColorBars, 1 },
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "ov5693", {
> > > .unitCellSize = { 1400, 1400 },
> > > @@ -238,6 +349,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > * Rolling Bar".
> > > */
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "ov64a40", {
> > > .unitCellSize = { 1008, 1008 },
> > > @@ -251,6 +363,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > * 4: "Vertical Color Bar Type 4"
> > > */
> > > },
> > > + .sensorDelays = {
> > > + .exposureDelay = 2,
> > > + .gainDelay = 2,
> > > + .vblankDelay = 2,
> > > + .hblankDelay = 2
> > > + },
> > > } },
> > > { "ov8858", {
> > > .unitCellSize = { 1120, 1120 },
> > > @@ -264,6 +382,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > * 4: "Vertical Color Bar Type 4"
> > > */
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "ov8865", {
> > > .unitCellSize = { 1400, 1400 },
> > > @@ -278,6 +397,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > * 5: "Color squares with rolling bar"
> > > */
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > { "ov13858", {
> > > .unitCellSize = { 1120, 1120 },
> > > @@ -285,6 +405,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > { controls::draft::TestPatternModeOff, 0 },
> > > { controls::draft::TestPatternModeColorBars, 1 },
> > > },
> > > + .sensorDelays = { },
> > > } },
> > > };
> > >
> > > --
> > > 2.34.1
> > >
More information about the libcamera-devel
mailing list