[PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor control delays

Laurent Pinchart laurent.pinchart at ideasonboard.com
Tue Nov 19 11:48:59 CET 2024


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 at 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 = { },
> >>>>                   } },
> >>>>           };
> >>>>    

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list