| Message ID | 20220328120336.10834-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Mar 28, 2022 at 03:03:35PM +0300, Laurent Pinchart via libcamera-devel wrote: > The IMX290 is a Sony sensor that expresses its gain in 0.3dB units. It > thus maps to the exponential gain model. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/libipa/camera_sensor_helper.cpp | 31 +++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index 7bb999e19102..136b9f6bc3c5 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -332,6 +332,26 @@ std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories() > > #ifndef __DOXYGEN__ > > +/* > + * Helper function to compute the m parameter of the exponential gain model > + * when the gain code is expressed in dB. > + */ > +static constexpr double expGainDb(double step) > +{ > + constexpr double log2_10 = 3.321928094887362; > + > + /* > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps): > + * > + * G_code = G_dB/step = 20/step*log10(G_linear) Why was I expecting 10/step*log10(G_linear) As my understanding is that G_db = 10*log10(G_linear) ? (nit: isn't it more readable with spaces between operands ?) Thanks j > + * > + * Inverting the formula, we get > + * > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code) > + */ > + return log2_10 * step / 20; > +} > + > class CameraSensorHelperImx219 : public CameraSensorHelper > { > public: > @@ -354,6 +374,17 @@ public: > }; > REGISTER_CAMERA_SENSOR_HELPER("imx258", CameraSensorHelperImx258) > > +class CameraSensorHelperImx290 : public CameraSensorHelper > +{ > +public: > + CameraSensorHelperImx290() > + { > + gainType_ = AnalogueGainExponential; > + gainConstants_.exp = { 1.0, expGainDb(0.3) }; > + } > +}; > +REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290) > + > class CameraSensorHelperOv2740 : public CameraSensorHelper > { > public: > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Mon, Mar 28, 2022 at 03:52:18PM +0200, Jacopo Mondi wrote: > On Mon, Mar 28, 2022 at 03:03:35PM +0300, Laurent Pinchart via libcamera-devel wrote: > > The IMX290 is a Sony sensor that expresses its gain in 0.3dB units. It > > thus maps to the exponential gain model. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 31 +++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > index 7bb999e19102..136b9f6bc3c5 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > @@ -332,6 +332,26 @@ std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories() > > > > #ifndef __DOXYGEN__ > > > > +/* > > + * Helper function to compute the m parameter of the exponential gain model > > + * when the gain code is expressed in dB. > > + */ > > +static constexpr double expGainDb(double step) > > +{ > > + constexpr double log2_10 = 3.321928094887362; > > + > > + /* > > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps): > > + * > > + * G_code = G_dB/step = 20/step*log10(G_linear) > > Why was I expecting > 10/step*log10(G_linear) > > As my understanding is that > > G_db = 10*log10(G_linear) ? The joys of dB as a power ratio or an amplitude ratio :-) Here the linear gain is an amplitude gain, so G_dB = 20*log10(G_linear). Quoting https://en.wikipedia.org/wiki/Decibel, "Two principal types of scaling of the decibel are in common use. When expressing a power ratio, it is defined as ten times the logarithm in base 10.[5] That is, a change in power by a factor of 10 corresponds to a 10 dB change in level. When expressing root-power quantities, a change in amplitude by a factor of 10 corresponds to a 20 dB change in level. The decibel scales differ by a factor of two, so that the related power and root-power levels change by the same value in linear systems, where power is proportional to the square of amplitude." A 20dB increment will multiple the amplitude by 10 and the power by 100. > (nit: isn't it more readable with spaces between operands ?) I can add spaces. The line below may get long though: * G_linear = 10 ^ (step / 20 * G_code) = 2 ^ (log2(10) * step / 20 * G_code) What do you prefer ? I can also switch to Latex notation: * G_{linear} = 10^{frac{step*G_{code}}{20}} = 2^{log_{2}(10) * frac{step*G_{code}}{20}} but that's likely not better :-) > > + * > > + * Inverting the formula, we get > > + * > > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code) > > + */ > > + return log2_10 * step / 20; > > +} > > + > > class CameraSensorHelperImx219 : public CameraSensorHelper > > { > > public: > > @@ -354,6 +374,17 @@ public: > > }; > > REGISTER_CAMERA_SENSOR_HELPER("imx258", CameraSensorHelperImx258) > > > > +class CameraSensorHelperImx290 : public CameraSensorHelper > > +{ > > +public: > > + CameraSensorHelperImx290() > > + { > > + gainType_ = AnalogueGainExponential; > > + gainConstants_.exp = { 1.0, expGainDb(0.3) }; > > + } > > +}; > > +REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290) > > + > > class CameraSensorHelperOv2740 : public CameraSensorHelper > > { > > public:
Hi Laurent On Mon, Mar 28, 2022 at 07:18:27PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Mar 28, 2022 at 03:52:18PM +0200, Jacopo Mondi wrote: > > On Mon, Mar 28, 2022 at 03:03:35PM +0300, Laurent Pinchart via libcamera-devel wrote: > > > The IMX290 is a Sony sensor that expresses its gain in 0.3dB units. It > > > thus maps to the exponential gain model. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/ipa/libipa/camera_sensor_helper.cpp | 31 +++++++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > index 7bb999e19102..136b9f6bc3c5 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > @@ -332,6 +332,26 @@ std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories() > > > > > > #ifndef __DOXYGEN__ > > > > > > +/* > > > + * Helper function to compute the m parameter of the exponential gain model > > > + * when the gain code is expressed in dB. > > > + */ > > > +static constexpr double expGainDb(double step) > > > +{ > > > + constexpr double log2_10 = 3.321928094887362; > > > + > > > + /* > > > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps): > > > + * > > > + * G_code = G_dB/step = 20/step*log10(G_linear) > > > > Why was I expecting > > 10/step*log10(G_linear) > > > > As my understanding is that > > > > G_db = 10*log10(G_linear) ? > > The joys of dB as a power ratio or an amplitude ratio :-) Here the > linear gain is an amplitude gain, so G_dB = 20*log10(G_linear). Quoting > https://en.wikipedia.org/wiki/Decibel, > > "Two principal types of scaling of the decibel are in common use. When > expressing a power ratio, it is defined as ten times the logarithm in > base 10.[5] That is, a change in power by a factor of 10 corresponds to > a 10 dB change in level. When expressing root-power quantities, a change > in amplitude by a factor of 10 corresponds to a 20 dB change in level. > The decibel scales differ by a factor of two, so that the related power > and root-power levels change by the same value in linear systems, where > power is proportional to the square of amplitude." > > A 20dB increment will multiple the amplitude by 10 and the power by 100. > Thanks, I was missing that part! > > (nit: isn't it more readable with spaces between operands ?) > > I can add spaces. The line below may get long though: > > * G_linear = 10 ^ (step / 20 * G_code) = 2 ^ (log2(10) * step / 20 * G_code) > > What do you prefer ? I can also switch to Latex notation: > > * G_{linear} = 10^{frac{step*G_{code}}{20}} = 2^{log_{2}(10) * frac{step*G_{code}}{20}} > > but that's likely not better :-) Likely not, yeah :) It gets a bit longer you're right. Whatever works Thanks for the clarification Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > > > + * > > > + * Inverting the formula, we get > > > + * > > > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code) > > > + */ > > > + return log2_10 * step / 20; > > > +} > > > + > > > class CameraSensorHelperImx219 : public CameraSensorHelper > > > { > > > public: > > > @@ -354,6 +374,17 @@ public: > > > }; > > > REGISTER_CAMERA_SENSOR_HELPER("imx258", CameraSensorHelperImx258) > > > > > > +class CameraSensorHelperImx290 : public CameraSensorHelper > > > +{ > > > +public: > > > + CameraSensorHelperImx290() > > > + { > > > + gainType_ = AnalogueGainExponential; > > > + gainConstants_.exp = { 1.0, expGainDb(0.3) }; > > > + } > > > +}; > > > +REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290) > > > + > > > class CameraSensorHelperOv2740 : public CameraSensorHelper > > > { > > > public: > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On Mon, Mar 28, 2022 at 03:03:35PM +0300, Laurent Pinchart via libcamera-devel wrote: > The IMX290 is a Sony sensor that expresses its gain in 0.3dB units. It > thus maps to the exponential gain model. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/libipa/camera_sensor_helper.cpp | 31 +++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index 7bb999e19102..136b9f6bc3c5 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -332,6 +332,26 @@ std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories() > > #ifndef __DOXYGEN__ > > +/* > + * Helper function to compute the m parameter of the exponential gain model > + * when the gain code is expressed in dB. > + */ > +static constexpr double expGainDb(double step) > +{ > + constexpr double log2_10 = 3.321928094887362; > + > + /* > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps): > + * > + * G_code = G_dB/step = 20/step*log10(G_linear) > + * > + * Inverting the formula, we get > + * > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code) > + */ > + return log2_10 * step / 20; > +} > + Would this be better in a separate patch? > class CameraSensorHelperImx219 : public CameraSensorHelper > { > public: > @@ -354,6 +374,17 @@ public: > }; > REGISTER_CAMERA_SENSOR_HELPER("imx258", CameraSensorHelperImx258) > > +class CameraSensorHelperImx290 : public CameraSensorHelper > +{ > +public: > + CameraSensorHelperImx290() > + { > + gainType_ = AnalogueGainExponential; > + gainConstants_.exp = { 1.0, expGainDb(0.3) }; > + } > +}; > +REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290) > + > class CameraSensorHelperOv2740 : public CameraSensorHelper > { > public: > -- The rest looks good. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Hello, On 3/28/22 17:33, Laurent Pinchart via libcamera-devel wrote: > The IMX290 is a Sony sensor that expresses its gain in 0.3dB units. It > thus maps to the exponential gain model. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/ipa/libipa/camera_sensor_helper.cpp | 31 +++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index 7bb999e19102..136b9f6bc3c5 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -332,6 +332,26 @@ std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories() > > #ifndef __DOXYGEN__ > > +/* > + * Helper function to compute the m parameter of the exponential gain model > + * when the gain code is expressed in dB. > + */ > +static constexpr double expGainDb(double step) > +{ > + constexpr double log2_10 = 3.321928094887362; > + > + /* > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps): > + * > + * G_code = G_dB/step = 20/step*log10(G_linear) > + * > + * Inverting the formula, we get > + * > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code) > + */ > + return log2_10 * step / 20; > +} > + > class CameraSensorHelperImx219 : public CameraSensorHelper > { > public: > @@ -354,6 +374,17 @@ public: > }; > REGISTER_CAMERA_SENSOR_HELPER("imx258", CameraSensorHelperImx258) > > +class CameraSensorHelperImx290 : public CameraSensorHelper > +{ > +public: > + CameraSensorHelperImx290() > + { > + gainType_ = AnalogueGainExponential; > + gainConstants_.exp = { 1.0, expGainDb(0.3) }; > + } > +}; > +REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290) > + > class CameraSensorHelperOv2740 : public CameraSensorHelper > { > public:
Hi Paul, On Wed, Mar 30, 2022 at 05:50:03PM +0900, paul.elder@ideasonboard.com wrote: > On Mon, Mar 28, 2022 at 03:03:35PM +0300, Laurent Pinchart via libcamera-devel wrote: > > The IMX290 is a Sony sensor that expresses its gain in 0.3dB units. It > > thus maps to the exponential gain model. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 31 +++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > index 7bb999e19102..136b9f6bc3c5 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > @@ -332,6 +332,26 @@ std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories() > > > > #ifndef __DOXYGEN__ > > > > +/* > > + * Helper function to compute the m parameter of the exponential gain model > > + * when the gain code is expressed in dB. > > + */ > > +static constexpr double expGainDb(double step) > > +{ > > + constexpr double log2_10 = 3.321928094887362; > > + > > + /* > > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps): > > + * > > + * G_code = G_dB/step = 20/step*log10(G_linear) > > + * > > + * Inverting the formula, we get > > + * > > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code) > > + */ > > + return log2_10 * step / 20; > > +} > > + > > Would this be better in a separate patch? I've thought about it, but the compiler then warns that the function is unused. It could be worked around by adding a [[maybe_unused]] and then dropping it in the next patch, but I didn't think it was worth it. > > class CameraSensorHelperImx219 : public CameraSensorHelper > > { > > public: > > @@ -354,6 +374,17 @@ public: > > }; > > REGISTER_CAMERA_SENSOR_HELPER("imx258", CameraSensorHelperImx258) > > > > +class CameraSensorHelperImx290 : public CameraSensorHelper > > +{ > > +public: > > + CameraSensorHelperImx290() > > + { > > + gainType_ = AnalogueGainExponential; > > + gainConstants_.exp = { 1.0, expGainDb(0.3) }; > > + } > > +}; > > +REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290) > > + > > class CameraSensorHelperOv2740 : public CameraSensorHelper > > { > > public: > > -- > > The rest looks good. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp index 7bb999e19102..136b9f6bc3c5 100644 --- a/src/ipa/libipa/camera_sensor_helper.cpp +++ b/src/ipa/libipa/camera_sensor_helper.cpp @@ -332,6 +332,26 @@ std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories() #ifndef __DOXYGEN__ +/* + * Helper function to compute the m parameter of the exponential gain model + * when the gain code is expressed in dB. + */ +static constexpr double expGainDb(double step) +{ + constexpr double log2_10 = 3.321928094887362; + + /* + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps): + * + * G_code = G_dB/step = 20/step*log10(G_linear) + * + * Inverting the formula, we get + * + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code) + */ + return log2_10 * step / 20; +} + class CameraSensorHelperImx219 : public CameraSensorHelper { public: @@ -354,6 +374,17 @@ public: }; REGISTER_CAMERA_SENSOR_HELPER("imx258", CameraSensorHelperImx258) +class CameraSensorHelperImx290 : public CameraSensorHelper +{ +public: + CameraSensorHelperImx290() + { + gainType_ = AnalogueGainExponential; + gainConstants_.exp = { 1.0, expGainDb(0.3) }; + } +}; +REGISTER_CAMERA_SENSOR_HELPER("imx290", CameraSensorHelperImx290) + class CameraSensorHelperOv2740 : public CameraSensorHelper { public:
The IMX290 is a Sony sensor that expresses its gain in 0.3dB units. It thus maps to the exponential gain model. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/libipa/camera_sensor_helper.cpp | 31 +++++++++++++++++++++++++ 1 file changed, 31 insertions(+)