| Message ID | 20240701144122.3418955-2-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote: > For a proper tuning process we need to know the sensor black levels. In > most cases these are fixed and not reported by the kernel driver. Store > them inside the sensor helpers for later retrieval by the algorithms. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++ > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++ > 2 files changed, 24 insertions(+) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index 782ff9904e81..6d8850eb25a5 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -47,6 +47,21 @@ namespace ipa { > * function. > */ > > +/** > + * \brief Fetch the black levels of the sensor > + * > + * This function returns the black levels of the sensor with respect to a 16bit > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/ > + * returned. > + * > + * \return The black levels of the sensor , or std::nullopt if not initialized > + */ > +std::optional<CameraSensorHelper::BlackLevels> > +CameraSensorHelper::blackLevels() const > +{ > + return blackLevels_; > +} > + > /** > * \brief Compute gain code from the analogue gain absolute value > * \param[in] gain The real gain to pass > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper > public: > CameraSensorHelperImx219() > { > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > gainType_ = AnalogueGainLinear; > gainConstants_.linear = { 0, 256, -1, 256 }; > } > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper > public: > CameraSensorHelperImx258() > { > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > gainType_ = AnalogueGainLinear; > gainConstants_.linear = { 0, 512, -1, 512 }; > } > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper > public: > CameraSensorHelperImx335() > { > + blackLevels_ = { 3200, 3200, 3200, 3200 }; I trust you on these values, which I presume come from the sensor's datasheets ? If you have measured them instead, I would add them in a separate commit with the commit message reporting how these have been measured > gainType_ = AnalogueGainExponential; > gainConstants_.exp = { 1.0, expGainDb(0.3) }; > } > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > index 0d99073bea82..f025727c06ee 100644 > --- a/src/ipa/libipa/camera_sensor_helper.h > +++ b/src/ipa/libipa/camera_sensor_helper.h > @@ -9,7 +9,9 @@ > > #include <stdint.h> > > +#include <array> > #include <memory> > +#include <optional> > #include <string> > #include <vector> > > @@ -22,9 +24,12 @@ namespace ipa { > class CameraSensorHelper > { > public: > + typedef std::array<int32_t, 4> BlackLevels; stupid question: are we sure we will always have 4 values only ? We tried not to assume a Bayer pattern in the design of the camera sensor helper classes. > + > CameraSensorHelper() = default; > virtual ~CameraSensorHelper() = default; > > + virtual std::optional<BlackLevels> blackLevels() const; Should this be virtual ? Is there any case when a derived class will have to override this instead of just initializing BlackLevels to the desired values ? Thanks j > virtual uint32_t gainCode(double gain) const; > virtual double gain(uint32_t gainCode) const; > > @@ -51,6 +56,7 @@ protected: > AnalogueGainExpConstants exp; > }; > > + std::optional<BlackLevels> blackLevels_; > AnalogueGainType gainType_; > AnalogueGainConstants gainConstants_; > > -- > 2.43.0 >
Hi Stefan On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote: > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote: > > For a proper tuning process we need to know the sensor black levels. In > > most cases these are fixed and not reported by the kernel driver. Store > > them inside the sensor helpers for later retrieval by the algorithms. Add something like ---- Add black level values corresponding to the data pedestal for three initial sensors. More should be added, eventually filling the gaps for all supported sensors. ---- > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++ > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > index 782ff9904e81..6d8850eb25a5 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > @@ -47,6 +47,21 @@ namespace ipa { > > * function. > > */ > > > > +/** > > + * \brief Fetch the black levels of the sensor > > + * > > + * This function returns the black levels of the sensor with respect to a 16bit > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is > > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/ > > > + * returned. > > + * > > + * \return The black levels of the sensor > > , or std::nullopt if not initialized s/initialized/known/ :-) > > + */ > > +std::optional<CameraSensorHelper::BlackLevels> > > +CameraSensorHelper::blackLevels() const > > +{ > > + return blackLevels_; > > +} > > + > > /** > > * \brief Compute gain code from the analogue gain absolute value > > * \param[in] gain The real gain to pass > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper > > public: > > CameraSensorHelperImx219() > > { > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > gainType_ = AnalogueGainLinear; > > gainConstants_.linear = { 0, 256, -1, 256 }; > > } > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper > > public: > > CameraSensorHelperImx258() > > { > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > gainType_ = AnalogueGainLinear; > > gainConstants_.linear = { 0, 512, -1, 512 }; > > } > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper > > public: > > CameraSensorHelperImx335() > > { > > + blackLevels_ = { 3200, 3200, 3200, 3200 }; > > I trust you on these values, which I presume come from the sensor's > datasheets ? If you have measured them instead, I would add them in a > separate commit with the commit message reporting how these have been > measured > > > gainType_ = AnalogueGainExponential; > > gainConstants_.exp = { 1.0, expGainDb(0.3) }; > > } > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > > index 0d99073bea82..f025727c06ee 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.h > > +++ b/src/ipa/libipa/camera_sensor_helper.h > > @@ -9,7 +9,9 @@ > > > > #include <stdint.h> > > > > +#include <array> > > #include <memory> > > +#include <optional> > > #include <string> > > #include <vector> > > > > @@ -22,9 +24,12 @@ namespace ipa { > > class CameraSensorHelper > > { > > public: > > + typedef std::array<int32_t, 4> BlackLevels; using BlackLevels = std::array<int32_t, 4>; > > stupid question: are we sure we will always have 4 values only ? We > tried not to assume a Bayer pattern in the design of the camera sensor > helper classes. I would also have used 4 levels, but that's an interesting point. I would be fine starting with 4, considering that sensors with less than 4 colour channels (the main case today is monochrome sensors) would use the first elements, and figure out later what to do for sensors with more than 4 channels. Or, given that we only deal with the pedestal today, we could replace this with a single black level value. I'm slightly tempted to go for the latter, as it's simpler. > > + > > CameraSensorHelper() = default; > > virtual ~CameraSensorHelper() = default; > > > > + virtual std::optional<BlackLevels> blackLevels() const; > > Should this be virtual ? Is there any case when a derived class will > have to override this instead of just initializing BlackLevels to the > desired values ? Possibly, for instance the dark currents could be estimated based on the sensor temperature. There's a high chance we'll never get there though, so I'd start with a non-virtual function, and I would even make it inline. > > virtual uint32_t gainCode(double gain) const; > > virtual double gain(uint32_t gainCode) const; > > > > @@ -51,6 +56,7 @@ protected: > > AnalogueGainExpConstants exp; > > }; > > > > + std::optional<BlackLevels> blackLevels_; > > AnalogueGainType gainType_; > > AnalogueGainConstants gainConstants_; > >
On Tue, Jul 02, 2024 at 03:51:02PM GMT, Laurent Pinchart wrote: > Hi Stefan > > On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote: > > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote: > > > For a proper tuning process we need to know the sensor black levels. In > > > most cases these are fixed and not reported by the kernel driver. Store > > > them inside the sensor helpers for later retrieval by the algorithms. > > Add something like > > ---- > Add black level values corresponding to the data pedestal for three > initial sensors. More should be added, eventually filling the gaps for > all supported sensors. > ---- > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++ > > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++ > > > 2 files changed, 24 insertions(+) > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > index 782ff9904e81..6d8850eb25a5 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > @@ -47,6 +47,21 @@ namespace ipa { > > > * function. > > > */ > > > > > > +/** > > > + * \brief Fetch the black levels of the sensor > > > + * > > > + * This function returns the black levels of the sensor with respect to a 16bit > > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is > > > > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/ > > > > > + * returned. > > > + * > > > + * \return The black levels of the sensor > > > > , or std::nullopt if not initialized > > s/initialized/known/ > > :-) > > > > + */ > > > +std::optional<CameraSensorHelper::BlackLevels> > > > +CameraSensorHelper::blackLevels() const > > > +{ > > > + return blackLevels_; > > > +} > > > + > > > /** > > > * \brief Compute gain code from the analogue gain absolute value > > > * \param[in] gain The real gain to pass > > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper > > > public: > > > CameraSensorHelperImx219() > > > { > > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > > gainType_ = AnalogueGainLinear; > > > gainConstants_.linear = { 0, 256, -1, 256 }; > > > } > > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper > > > public: > > > CameraSensorHelperImx258() > > > { > > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > > gainType_ = AnalogueGainLinear; > > > gainConstants_.linear = { 0, 512, -1, 512 }; > > > } > > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper > > > public: > > > CameraSensorHelperImx335() > > > { > > > + blackLevels_ = { 3200, 3200, 3200, 3200 }; > > > > I trust you on these values, which I presume come from the sensor's > > datasheets ? If you have measured them instead, I would add them in a > > separate commit with the commit message reporting how these have been > > measured > > > > > gainType_ = AnalogueGainExponential; > > > gainConstants_.exp = { 1.0, expGainDb(0.3) }; > > > } > > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > > > index 0d99073bea82..f025727c06ee 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.h > > > +++ b/src/ipa/libipa/camera_sensor_helper.h > > > @@ -9,7 +9,9 @@ > > > > > > #include <stdint.h> > > > > > > +#include <array> > > > #include <memory> > > > +#include <optional> > > > #include <string> > > > #include <vector> > > > > > > @@ -22,9 +24,12 @@ namespace ipa { > > > class CameraSensorHelper > > > { > > > public: > > > + typedef std::array<int32_t, 4> BlackLevels; > > using BlackLevels = std::array<int32_t, 4>; > > > > > stupid question: are we sure we will always have 4 values only ? We > > tried not to assume a Bayer pattern in the design of the camera sensor > > helper classes. > > I would also have used 4 levels, but that's an interesting point. I > would be fine starting with 4, considering that sensors with less than 4 > colour channels (the main case today is monochrome sensors) would use > the first elements, and figure out later what to do for sensors with > more than 4 channels. Or, given that we only deal with the pedestal > today, we could replace this with a single black level value. I'm > slightly tempted to go for the latter, as it's simpler. > Actually, from a sensor perspective, do we actually have one pedestal value for each color channel, or is the ISP that requires to express one value per channel ? > > > + > > > CameraSensorHelper() = default; > > > virtual ~CameraSensorHelper() = default; > > > > > > + virtual std::optional<BlackLevels> blackLevels() const; > > > > Should this be virtual ? Is there any case when a derived class will > > have to override this instead of just initializing BlackLevels to the > > desired values ? > > Possibly, for instance the dark currents could be estimated based on the > sensor temperature. There's a high chance we'll never get there though, > so I'd start with a non-virtual function, and I would even make it > inline. > > > > virtual uint32_t gainCode(double gain) const; > > > virtual double gain(uint32_t gainCode) const; > > > > > > @@ -51,6 +56,7 @@ protected: > > > AnalogueGainExpConstants exp; > > > }; > > > > > > + std::optional<BlackLevels> blackLevels_; > > > AnalogueGainType gainType_; > > > AnalogueGainConstants gainConstants_; > > > > > -- > Regards, > > Laurent Pinchart
On Tue, Jul 02, 2024 at 02:59:32PM +0200, Jacopo Mondi wrote: > On Tue, Jul 02, 2024 at 03:51:02PM GMT, Laurent Pinchart wrote: > > Hi Stefan > > > > On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote: > > > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote: > > > > For a proper tuning process we need to know the sensor black levels. In > > > > most cases these are fixed and not reported by the kernel driver. Store > > > > them inside the sensor helpers for later retrieval by the algorithms. > > > > Add something like > > > > ---- > > Add black level values corresponding to the data pedestal for three > > initial sensors. More should be added, eventually filling the gaps for > > all supported sensors. > > ---- > > > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++ > > > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++ > > > > 2 files changed, 24 insertions(+) > > > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > > index 782ff9904e81..6d8850eb25a5 100644 > > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > > @@ -47,6 +47,21 @@ namespace ipa { > > > > * function. > > > > */ > > > > > > > > +/** > > > > + * \brief Fetch the black levels of the sensor > > > > + * > > > > + * This function returns the black levels of the sensor with respect to a 16bit > > > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is > > > > > > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/ > > > > > > > + * returned. > > > > + * > > > > + * \return The black levels of the sensor > > > > > > , or std::nullopt if not initialized > > > > s/initialized/known/ > > > > :-) > > > > > > + */ > > > > +std::optional<CameraSensorHelper::BlackLevels> > > > > +CameraSensorHelper::blackLevels() const > > > > +{ > > > > + return blackLevels_; > > > > +} > > > > + > > > > /** > > > > * \brief Compute gain code from the analogue gain absolute value > > > > * \param[in] gain The real gain to pass > > > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperImx219() > > > > { > > > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > > > gainType_ = AnalogueGainLinear; > > > > gainConstants_.linear = { 0, 256, -1, 256 }; > > > > } > > > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperImx258() > > > > { > > > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > > > gainType_ = AnalogueGainLinear; > > > > gainConstants_.linear = { 0, 512, -1, 512 }; > > > > } > > > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperImx335() > > > > { > > > > + blackLevels_ = { 3200, 3200, 3200, 3200 }; > > > > > > I trust you on these values, which I presume come from the sensor's > > > datasheets ? If you have measured them instead, I would add them in a > > > separate commit with the commit message reporting how these have been > > > measured > > > > > > > gainType_ = AnalogueGainExponential; > > > > gainConstants_.exp = { 1.0, expGainDb(0.3) }; > > > > } > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > > > > index 0d99073bea82..f025727c06ee 100644 > > > > --- a/src/ipa/libipa/camera_sensor_helper.h > > > > +++ b/src/ipa/libipa/camera_sensor_helper.h > > > > @@ -9,7 +9,9 @@ > > > > > > > > #include <stdint.h> > > > > > > > > +#include <array> > > > > #include <memory> > > > > +#include <optional> > > > > #include <string> > > > > #include <vector> > > > > > > > > @@ -22,9 +24,12 @@ namespace ipa { > > > > class CameraSensorHelper > > > > { > > > > public: > > > > + typedef std::array<int32_t, 4> BlackLevels; > > > > using BlackLevels = std::array<int32_t, 4>; > > > > > > > > stupid question: are we sure we will always have 4 values only ? We > > > tried not to assume a Bayer pattern in the design of the camera sensor > > > helper classes. > > > > I would also have used 4 levels, but that's an interesting point. I > > would be fine starting with 4, considering that sensors with less than 4 > > colour channels (the main case today is monochrome sensors) would use > > the first elements, and figure out later what to do for sensors with > > more than 4 channels. Or, given that we only deal with the pedestal > > today, we could replace this with a single black level value. I'm > > slightly tempted to go for the latter, as it's simpler. > > Actually, from a sensor perspective, do we actually have one pedestal > value for each color channel, or is the ISP that requires to express one > value per channel ? I suppose it's sensor-dependent, but in practice I don't really see a reason why one would need per-channel values. There are certainly sensors that program the data pedestal using a single register. > > > > + > > > > CameraSensorHelper() = default; > > > > virtual ~CameraSensorHelper() = default; > > > > > > > > + virtual std::optional<BlackLevels> blackLevels() const; > > > > > > Should this be virtual ? Is there any case when a derived class will > > > have to override this instead of just initializing BlackLevels to the > > > desired values ? > > > > Possibly, for instance the dark currents could be estimated based on the > > sensor temperature. There's a high chance we'll never get there though, > > so I'd start with a non-virtual function, and I would even make it > > inline. > > > > > > virtual uint32_t gainCode(double gain) const; > > > > virtual double gain(uint32_t gainCode) const; > > > > > > > > @@ -51,6 +56,7 @@ protected: > > > > AnalogueGainExpConstants exp; > > > > }; > > > > > > > > + std::optional<BlackLevels> blackLevels_; > > > > AnalogueGainType gainType_; > > > > AnalogueGainConstants gainConstants_; > > > >
Hi Jacopo, On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote: > Hi Stefan > > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote: > > For a proper tuning process we need to know the sensor black levels. In > > most cases these are fixed and not reported by the kernel driver. Store > > them inside the sensor helpers for later retrieval by the algorithms. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++ > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > index 782ff9904e81..6d8850eb25a5 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > @@ -47,6 +47,21 @@ namespace ipa { > > * function. > > */ > > > > +/** > > + * \brief Fetch the black levels of the sensor > > + * > > + * This function returns the black levels of the sensor with respect to a 16bit > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is > > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/ Does that really improve the sentence? Because the values used are actually 32 bit, to be the same as the metadata type. > > + * returned. > > + * > > + * \return The black levels of the sensor > , or std::nullopt if not initialized > > > + */ > > +std::optional<CameraSensorHelper::BlackLevels> > > +CameraSensorHelper::blackLevels() const > > +{ > > + return blackLevels_; > > +} > > + > > /** > > * \brief Compute gain code from the analogue gain absolute value > > * \param[in] gain The real gain to pass > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper > > public: > > CameraSensorHelperImx219() > > { > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > gainType_ = AnalogueGainLinear; > > gainConstants_.linear = { 0, 256, -1, 256 }; > > } > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper > > public: > > CameraSensorHelperImx258() > > { > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > gainType_ = AnalogueGainLinear; > > gainConstants_.linear = { 0, 512, -1, 512 }; > > } > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper > > public: > > CameraSensorHelperImx335() > > { > > + blackLevels_ = { 3200, 3200, 3200, 3200 }; > > I trust you on these values, which I presume come from the sensor's > datasheets ? If you have measured them instead, I would add them in a > separate commit with the commit message reporting how these have been > measured Yes, these are all from datsheets. I'll add a comment. All the other commets were already answered by Lauerent and will be fixed where needed. Cheers, Stefan > > > gainType_ = AnalogueGainExponential; > > gainConstants_.exp = { 1.0, expGainDb(0.3) }; > > } > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > > index 0d99073bea82..f025727c06ee 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.h > > +++ b/src/ipa/libipa/camera_sensor_helper.h > > @@ -9,7 +9,9 @@ > > > > #include <stdint.h> > > > > +#include <array> > > #include <memory> > > +#include <optional> > > #include <string> > > #include <vector> > > > > @@ -22,9 +24,12 @@ namespace ipa { > > class CameraSensorHelper > > { > > public: > > + typedef std::array<int32_t, 4> BlackLevels; > > stupid question: are we sure we will always have 4 values only ? We > tried not to assume a Bayer pattern in the design of the camera sensor > helper classes. > > > + > > CameraSensorHelper() = default; > > virtual ~CameraSensorHelper() = default; > > > > + virtual std::optional<BlackLevels> blackLevels() const; > > Should this be virtual ? Is there any case when a derived class will > have to override this instead of just initializing BlackLevels to the > desired values ? > > Thanks > j > > > virtual uint32_t gainCode(double gain) const; > > virtual double gain(uint32_t gainCode) const; > > > > @@ -51,6 +56,7 @@ protected: > > AnalogueGainExpConstants exp; > > }; > > > > + std::optional<BlackLevels> blackLevels_; > > AnalogueGainType gainType_; > > AnalogueGainConstants gainConstants_; > > > > -- > > 2.43.0 > >
On Tue, Jul 02, 2024 at 04:25:23PM GMT, Stefan Klug wrote: > Hi Jacopo, > > On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote: > > Hi Stefan > > > > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote: > > > For a proper tuning process we need to know the sensor black levels. In > > > most cases these are fixed and not reported by the kernel driver. Store > > > them inside the sensor helpers for later retrieval by the algorithms. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++ > > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++ > > > 2 files changed, 24 insertions(+) > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > index 782ff9904e81..6d8850eb25a5 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > @@ -47,6 +47,21 @@ namespace ipa { > > > * function. > > > */ > > > > > > +/** > > > + * \brief Fetch the black levels of the sensor > > > + * > > > + * This function returns the black levels of the sensor with respect to a 16bit > > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is > > > > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/ > > Does that really improve the sentence? Because the values used are > actually 32 bit, to be the same as the metadata type. "with respect to" doesn't sound right to me in English, but, not a native speaker, so please wait for someone else opinion > > > > + * returned. > > > + * > > > + * \return The black levels of the sensor > > , or std::nullopt if not initialized > > > > > + */ > > > +std::optional<CameraSensorHelper::BlackLevels> > > > +CameraSensorHelper::blackLevels() const > > > +{ > > > + return blackLevels_; > > > +} > > > + > > > /** > > > * \brief Compute gain code from the analogue gain absolute value > > > * \param[in] gain The real gain to pass > > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper > > > public: > > > CameraSensorHelperImx219() > > > { > > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > > gainType_ = AnalogueGainLinear; > > > gainConstants_.linear = { 0, 256, -1, 256 }; > > > } > > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper > > > public: > > > CameraSensorHelperImx258() > > > { > > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > > gainType_ = AnalogueGainLinear; > > > gainConstants_.linear = { 0, 512, -1, 512 }; > > > } > > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper > > > public: > > > CameraSensorHelperImx335() > > > { > > > + blackLevels_ = { 3200, 3200, 3200, 3200 }; > > > > I trust you on these values, which I presume come from the sensor's > > datasheets ? If you have measured them instead, I would add them in a > > separate commit with the commit message reporting how these have been > > measured > > Yes, these are all from datsheets. I'll add a comment. > > All the other commets were already answered by Lauerent and will be > fixed where needed. > > Cheers, > Stefan > > > > > > gainType_ = AnalogueGainExponential; > > > gainConstants_.exp = { 1.0, expGainDb(0.3) }; > > > } > > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > > > index 0d99073bea82..f025727c06ee 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.h > > > +++ b/src/ipa/libipa/camera_sensor_helper.h > > > @@ -9,7 +9,9 @@ > > > > > > #include <stdint.h> > > > > > > +#include <array> > > > #include <memory> > > > +#include <optional> > > > #include <string> > > > #include <vector> > > > > > > @@ -22,9 +24,12 @@ namespace ipa { > > > class CameraSensorHelper > > > { > > > public: > > > + typedef std::array<int32_t, 4> BlackLevels; > > > > stupid question: are we sure we will always have 4 values only ? We > > tried not to assume a Bayer pattern in the design of the camera sensor > > helper classes. > > > > > + > > > CameraSensorHelper() = default; > > > virtual ~CameraSensorHelper() = default; > > > > > > + virtual std::optional<BlackLevels> blackLevels() const; > > > > Should this be virtual ? Is there any case when a derived class will > > have to override this instead of just initializing BlackLevels to the > > desired values ? > > > > Thanks > > j > > > > > virtual uint32_t gainCode(double gain) const; > > > virtual double gain(uint32_t gainCode) const; > > > > > > @@ -51,6 +56,7 @@ protected: > > > AnalogueGainExpConstants exp; > > > }; > > > > > > + std::optional<BlackLevels> blackLevels_; > > > AnalogueGainType gainType_; > > > AnalogueGainConstants gainConstants_; > > > > > > -- > > > 2.43.0 > > >
On Tue, Jul 02, 2024 at 04:42:38PM +0200, Jacopo Mondi wrote: > On Tue, Jul 02, 2024 at 04:25:23PM GMT, Stefan Klug wrote: > > Hi Jacopo, > > > > On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote: > > > Hi Stefan > > > > > > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote: > > > > For a proper tuning process we need to know the sensor black levels. In > > > > most cases these are fixed and not reported by the kernel driver. Store > > > > them inside the sensor helpers for later retrieval by the algorithms. > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++ > > > > src/ipa/libipa/camera_sensor_helper.h | 6 ++++++ > > > > 2 files changed, 24 insertions(+) > > > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > > index 782ff9904e81..6d8850eb25a5 100644 > > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > > @@ -47,6 +47,21 @@ namespace ipa { > > > > * function. > > > > */ > > > > > > > > +/** > > > > + * \brief Fetch the black levels of the sensor > > > > + * > > > > + * This function returns the black levels of the sensor with respect to a 16bit > > > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is > > > > > > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/ > > > > Does that really improve the sentence? Because the values used are > > actually 32 bit, to be the same as the metadata type. > > "with respect to" doesn't sound right to me in English, but, not a > native speaker, so please wait for someone else opinion Yeah it's not great in this context. Paul > > > > > > > + * returned. > > > > + * > > > > + * \return The black levels of the sensor > > > , or std::nullopt if not initialized > > > > > > > + */ > > > > +std::optional<CameraSensorHelper::BlackLevels> > > > > +CameraSensorHelper::blackLevels() const > > > > +{ > > > > + return blackLevels_; > > > > +} > > > > + > > > > /** > > > > * \brief Compute gain code from the analogue gain absolute value > > > > * \param[in] gain The real gain to pass > > > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperImx219() > > > > { > > > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > > > gainType_ = AnalogueGainLinear; > > > > gainConstants_.linear = { 0, 256, -1, 256 }; > > > > } > > > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperImx258() > > > > { > > > > + blackLevels_ = { 4096, 4096, 4096, 4096 }; > > > > gainType_ = AnalogueGainLinear; > > > > gainConstants_.linear = { 0, 512, -1, 512 }; > > > > } > > > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper > > > > public: > > > > CameraSensorHelperImx335() > > > > { > > > > + blackLevels_ = { 3200, 3200, 3200, 3200 }; > > > > > > I trust you on these values, which I presume come from the sensor's > > > datasheets ? If you have measured them instead, I would add them in a > > > separate commit with the commit message reporting how these have been > > > measured > > > > Yes, these are all from datsheets. I'll add a comment. > > > > All the other commets were already answered by Lauerent and will be > > fixed where needed. > > > > Cheers, > > Stefan > > > > > > > > > gainType_ = AnalogueGainExponential; > > > > gainConstants_.exp = { 1.0, expGainDb(0.3) }; > > > > } > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h > > > > index 0d99073bea82..f025727c06ee 100644 > > > > --- a/src/ipa/libipa/camera_sensor_helper.h > > > > +++ b/src/ipa/libipa/camera_sensor_helper.h > > > > @@ -9,7 +9,9 @@ > > > > > > > > #include <stdint.h> > > > > > > > > +#include <array> > > > > #include <memory> > > > > +#include <optional> > > > > #include <string> > > > > #include <vector> > > > > > > > > @@ -22,9 +24,12 @@ namespace ipa { > > > > class CameraSensorHelper > > > > { > > > > public: > > > > + typedef std::array<int32_t, 4> BlackLevels; > > > > > > stupid question: are we sure we will always have 4 values only ? We > > > tried not to assume a Bayer pattern in the design of the camera sensor > > > helper classes. > > > > > > > + > > > > CameraSensorHelper() = default; > > > > virtual ~CameraSensorHelper() = default; > > > > > > > > + virtual std::optional<BlackLevels> blackLevels() const; > > > > > > Should this be virtual ? Is there any case when a derived class will > > > have to override this instead of just initializing BlackLevels to the > > > desired values ? > > > > > > Thanks > > > j > > > > > > > virtual uint32_t gainCode(double gain) const; > > > > virtual double gain(uint32_t gainCode) const; > > > > > > > > @@ -51,6 +56,7 @@ protected: > > > > AnalogueGainExpConstants exp; > > > > }; > > > > > > > > + std::optional<BlackLevels> blackLevels_; > > > > AnalogueGainType gainType_; > > > > AnalogueGainConstants gainConstants_; > > > > > > > > -- > > > > 2.43.0 > > > >
diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp index 782ff9904e81..6d8850eb25a5 100644 --- a/src/ipa/libipa/camera_sensor_helper.cpp +++ b/src/ipa/libipa/camera_sensor_helper.cpp @@ -47,6 +47,21 @@ namespace ipa { * function. */ +/** + * \brief Fetch the black levels of the sensor + * + * This function returns the black levels of the sensor with respect to a 16bit + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is + * returned. + * + * \return The black levels of the sensor + */ +std::optional<CameraSensorHelper::BlackLevels> +CameraSensorHelper::blackLevels() const +{ + return blackLevels_; +} + /** * \brief Compute gain code from the analogue gain absolute value * \param[in] gain The real gain to pass @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper public: CameraSensorHelperImx219() { + blackLevels_ = { 4096, 4096, 4096, 4096 }; gainType_ = AnalogueGainLinear; gainConstants_.linear = { 0, 256, -1, 256 }; } @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper public: CameraSensorHelperImx258() { + blackLevels_ = { 4096, 4096, 4096, 4096 }; gainType_ = AnalogueGainLinear; gainConstants_.linear = { 0, 512, -1, 512 }; } @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper public: CameraSensorHelperImx335() { + blackLevels_ = { 3200, 3200, 3200, 3200 }; gainType_ = AnalogueGainExponential; gainConstants_.exp = { 1.0, expGainDb(0.3) }; } diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h index 0d99073bea82..f025727c06ee 100644 --- a/src/ipa/libipa/camera_sensor_helper.h +++ b/src/ipa/libipa/camera_sensor_helper.h @@ -9,7 +9,9 @@ #include <stdint.h> +#include <array> #include <memory> +#include <optional> #include <string> #include <vector> @@ -22,9 +24,12 @@ namespace ipa { class CameraSensorHelper { public: + typedef std::array<int32_t, 4> BlackLevels; + CameraSensorHelper() = default; virtual ~CameraSensorHelper() = default; + virtual std::optional<BlackLevels> blackLevels() const; virtual uint32_t gainCode(double gain) const; virtual double gain(uint32_t gainCode) const; @@ -51,6 +56,7 @@ protected: AnalogueGainExpConstants exp; }; + std::optional<BlackLevels> blackLevels_; AnalogueGainType gainType_; AnalogueGainConstants gainConstants_;
For a proper tuning process we need to know the sensor black levels. In most cases these are fixed and not reported by the kernel driver. Store them inside the sensor helpers for later retrieval by the algorithms. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/libipa/camera_sensor_helper.cpp | 18 ++++++++++++++++++ src/ipa/libipa/camera_sensor_helper.h | 6 ++++++ 2 files changed, 24 insertions(+)