| Message ID | 20260114113016.25162-10-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: > The gamma value is fixed in software ISP. Let's make it adjustable, > similarly to contrast and saturation. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/adjust.cpp | 8 ++++++++ > src/ipa/simple/algorithms/adjust.h | 2 ++ > src/ipa/simple/algorithms/lut.cpp | 12 +++++++----- > src/ipa/simple/ipa_context.h | 4 +++- > 4 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp > index 27ae2a53a..cfbc39610 100644 > --- a/src/ipa/simple/algorithms/adjust.cpp > +++ b/src/ipa/simple/algorithms/adjust.cpp > @@ -23,6 +23,7 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust) > > int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) > { > + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma); > context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f); > if (context.ccmEnabled) > context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); > @@ -32,6 +33,7 @@ int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningD > int Adjust::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > + context.activeState.knobs.gamma = std::optional<double>(); Is the `std::optional` necessary? I know that the others already use it, but it seems to me that doing `... = kDefaultGamma` would work just as well, no? > context.activeState.knobs.contrast = std::optional<double>(); > context.activeState.knobs.saturation = std::optional<double>(); > > @@ -43,6 +45,12 @@ void Adjust::queueRequest(typename Module::Context &context, > [[maybe_unused]] typename Module::FrameContext &frameContext, > const ControlList &controls) > { > + const auto &gamma = controls.get(controls::Gamma); > + if (gamma.has_value()) { > + context.activeState.knobs.gamma = gamma; > + LOG(IPASoftAdjust, Debug) << "Setting gamma to " << gamma.value(); > + } > + > const auto &contrast = controls.get(controls::Contrast); > if (contrast.has_value()) { > context.activeState.knobs.contrast = contrast; > [...]
Hi Barnabás, thank you for review. Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: >> The gamma value is fixed in software ISP. Let's make it adjustable, >> similarly to contrast and saturation. >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/adjust.cpp | 8 ++++++++ >> src/ipa/simple/algorithms/adjust.h | 2 ++ >> src/ipa/simple/algorithms/lut.cpp | 12 +++++++----- >> src/ipa/simple/ipa_context.h | 4 +++- >> 4 files changed, 20 insertions(+), 6 deletions(-) >> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp >> index 27ae2a53a..cfbc39610 100644 >> --- a/src/ipa/simple/algorithms/adjust.cpp >> +++ b/src/ipa/simple/algorithms/adjust.cpp >> @@ -23,6 +23,7 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust) >> int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) >> { >> + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma); >> context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f); >> if (context.ccmEnabled) >> context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); >> @@ -32,6 +33,7 @@ int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningD >> int Adjust::configure(IPAContext &context, >> [[maybe_unused]] const IPAConfigInfo &configInfo) >> { >> + context.activeState.knobs.gamma = std::optional<double>(); > > Is the `std::optional` necessary? I know that the others already use it, > but it seems to me that doing `... = kDefaultGamma` would work just > as well, no? Maybe there is a reason why it should be optional, see below. >> context.activeState.knobs.contrast = std::optional<double>(); >> context.activeState.knobs.saturation = std::optional<double>(); >> @@ -43,6 +45,12 @@ void Adjust::queueRequest(typename Module::Context &context, >> [[maybe_unused]] typename Module::FrameContext &frameContext, >> const ControlList &controls) >> { >> + const auto &gamma = controls.get(controls::Gamma); >> + if (gamma.has_value()) { >> + context.activeState.knobs.gamma = gamma; I wonder whether it should actually be: context.activeState.knobs.gamma = controls.get(controls::Gamma); With the rationale that if the control stops providing a value (possible?), the activeState value should get reset to the default value. The same for the other controls here. >> + LOG(IPASoftAdjust, Debug) << "Setting gamma to " << gamma.value(); >> + } >> + >> const auto &contrast = controls.get(controls::Contrast); >> if (contrast.has_value()) { >> context.activeState.knobs.contrast = contrast; >> [...] A missing piece: The gamma value should be reported in metadata.
2026. 01. 16. 20:22 keltezéssel, Milan Zamazal írta: > Hi Barnabás, > > thank you for review. > > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> 2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: >>> The gamma value is fixed in software ISP. Let's make it adjustable, >>> similarly to contrast and saturation. >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/ipa/simple/algorithms/adjust.cpp | 8 ++++++++ >>> src/ipa/simple/algorithms/adjust.h | 2 ++ >>> src/ipa/simple/algorithms/lut.cpp | 12 +++++++----- >>> src/ipa/simple/ipa_context.h | 4 +++- >>> 4 files changed, 20 insertions(+), 6 deletions(-) >>> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp >>> index 27ae2a53a..cfbc39610 100644 >>> --- a/src/ipa/simple/algorithms/adjust.cpp >>> +++ b/src/ipa/simple/algorithms/adjust.cpp >>> @@ -23,6 +23,7 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust) >>> int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) >>> { >>> + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma); >>> context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f); >>> if (context.ccmEnabled) >>> context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); >>> @@ -32,6 +33,7 @@ int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningD >>> int Adjust::configure(IPAContext &context, >>> [[maybe_unused]] const IPAConfigInfo &configInfo) >>> { >>> + context.activeState.knobs.gamma = std::optional<double>(); >> >> Is the `std::optional` necessary? I know that the others already use it, >> but it seems to me that doing `... = kDefaultGamma` would work just >> as well, no? > > Maybe there is a reason why it should be optional, see below. > >>> context.activeState.knobs.contrast = std::optional<double>(); >>> context.activeState.knobs.saturation = std::optional<double>(); >>> @@ -43,6 +45,12 @@ void Adjust::queueRequest(typename Module::Context &context, >>> [[maybe_unused]] typename Module::FrameContext &frameContext, >>> const ControlList &controls) >>> { >>> + const auto &gamma = controls.get(controls::Gamma); >>> + if (gamma.has_value()) { >>> + context.activeState.knobs.gamma = gamma; > > I wonder whether it should actually be: > > context.activeState.knobs.gamma = controls.get(controls::Gamma); > > With the rationale that if the control stops providing a value > (possible?), the activeState value should get reset to the default > value. > > The same for the other controls here. Doesn't that mean that an application would have to continuously provide e.g. the Gamma control otherwise it will go back to its default? If that is the case, that does not match the current expectations, I think; which is that most controls are "stateful", it has to keep its last set value until the camera is stopped or the control is modified, I believe. > >>> + LOG(IPASoftAdjust, Debug) << "Setting gamma to " << gamma.value(); >>> + } >>> + >>> const auto &contrast = controls.get(controls::Contrast); >>> if (contrast.has_value()) { >>> context.activeState.knobs.contrast = contrast; >>> [...] > > A missing piece: The gamma value should be reported in metadata. >
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 01. 16. 20:22 keltezéssel, Milan Zamazal írta: >> Hi Barnabás, >> thank you for review. >> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >> >>> 2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: >>>> The gamma value is fixed in software ISP. Let's make it adjustable, >>>> similarly to contrast and saturation. >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/ipa/simple/algorithms/adjust.cpp | 8 ++++++++ >>>> src/ipa/simple/algorithms/adjust.h | 2 ++ >>>> src/ipa/simple/algorithms/lut.cpp | 12 +++++++----- >>>> src/ipa/simple/ipa_context.h | 4 +++- >>>> 4 files changed, 20 insertions(+), 6 deletions(-) >>>> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp >>>> index 27ae2a53a..cfbc39610 100644 >>>> --- a/src/ipa/simple/algorithms/adjust.cpp >>>> +++ b/src/ipa/simple/algorithms/adjust.cpp >>>> @@ -23,6 +23,7 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust) >>>> int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) >>>> { >>>> + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma); >>>> context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f); >>>> if (context.ccmEnabled) >>>> context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); >>>> @@ -32,6 +33,7 @@ int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningD >>>> int Adjust::configure(IPAContext &context, >>>> [[maybe_unused]] const IPAConfigInfo &configInfo) >>>> { >>>> + context.activeState.knobs.gamma = std::optional<double>(); >>> >>> Is the `std::optional` necessary? I know that the others already use it, >>> but it seems to me that doing `... = kDefaultGamma` would work just >>> as well, no? >> Maybe there is a reason why it should be optional, see below. >> >>>> context.activeState.knobs.contrast = std::optional<double>(); >>>> context.activeState.knobs.saturation = std::optional<double>(); >>>> @@ -43,6 +45,12 @@ void Adjust::queueRequest(typename Module::Context &context, >>>> [[maybe_unused]] typename Module::FrameContext &frameContext, >>>> const ControlList &controls) >>>> { >>>> + const auto &gamma = controls.get(controls::Gamma); >>>> + if (gamma.has_value()) { >>>> + context.activeState.knobs.gamma = gamma; >> I wonder whether it should actually be: >> context.activeState.knobs.gamma = controls.get(controls::Gamma); >> With the rationale that if the control stops providing a value >> (possible?), the activeState value should get reset to the default >> value. >> The same for the other controls here. > > Doesn't that mean that an application would have to continuously provide e.g. > the Gamma control otherwise it will go back to its default? If that is the case, > that does not match the current expectations, I think; which is that most controls > are "stateful", it has to keep its last set value until the camera is stopped or > the control is modified, I believe. Thank you for clarification. Then indeed, I can't see a reason to have it optional. >> >>>> + LOG(IPASoftAdjust, Debug) << "Setting gamma to " << gamma.value(); >>>> + } >>>> + >>>> const auto &contrast = controls.get(controls::Contrast); >>>> if (contrast.has_value()) { >>>> context.activeState.knobs.contrast = contrast; >>>> [...] >> A missing piece: The gamma value should be reported in metadata. >>
2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: > The gamma value is fixed in software ISP. Let's make it adjustable, > similarly to contrast and saturation. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/adjust.cpp | 8 ++++++++ > src/ipa/simple/algorithms/adjust.h | 2 ++ > src/ipa/simple/algorithms/lut.cpp | 12 +++++++----- > src/ipa/simple/ipa_context.h | 4 +++- > 4 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp > index 27ae2a53a..cfbc39610 100644 > --- a/src/ipa/simple/algorithms/adjust.cpp > +++ b/src/ipa/simple/algorithms/adjust.cpp > @@ -23,6 +23,7 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust) > > int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) > { > + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma); > context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f); > if (context.ccmEnabled) > context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); > @@ -32,6 +33,7 @@ int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningD > int Adjust::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > + context.activeState.knobs.gamma = std::optional<double>(); > context.activeState.knobs.contrast = std::optional<double>(); > context.activeState.knobs.saturation = std::optional<double>(); > > @@ -43,6 +45,12 @@ void Adjust::queueRequest(typename Module::Context &context, > [[maybe_unused]] typename Module::FrameContext &frameContext, > const ControlList &controls) > { > + const auto &gamma = controls.get(controls::Gamma); > + if (gamma.has_value()) { > + context.activeState.knobs.gamma = gamma; > + LOG(IPASoftAdjust, Debug) << "Setting gamma to " << gamma.value(); > + } > + > const auto &contrast = controls.get(controls::Contrast); > if (contrast.has_value()) { > context.activeState.knobs.contrast = contrast; I believe this is missing the reporting of `controls::Gamma` in the metadata. > diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h > index c4baa2503..190d2079f 100644 > --- a/src/ipa/simple/algorithms/adjust.h > +++ b/src/ipa/simple/algorithms/adjust.h > @@ -19,6 +19,8 @@ namespace libcamera { > > namespace ipa::soft::algorithms { > > +const float kDefaultGamma = 2.2f; > + > class Adjust : public Algorithm > { > public: > diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp > index 3a00bf4ee..25b188b91 100644 > --- a/src/ipa/simple/algorithms/lut.cpp > +++ b/src/ipa/simple/algorithms/lut.cpp > @@ -18,6 +18,8 @@ > > #include "simple/ipa_context.h" > > +#include "adjust.h" > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(IPASoftLut) > @@ -27,8 +29,6 @@ namespace ipa::soft::algorithms { > int Lut::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > - /* Gamma value is fixed */ > - context.configuration.gamma = 1.0 / 2.2; > updateGammaTable(context); > > return 0; > @@ -37,6 +37,8 @@ int Lut::configure(IPAContext &context, > void Lut::updateGammaTable(IPAContext &context) > { > const auto blackLevel = context.activeState.blc.level; > + const auto gamma = > + 1.0 / context.activeState.knobs.gamma.value_or(kDefaultGamma); > const auto contrast = context.activeState.knobs.contrast.value_or(1.0); > /* Convert 0..2 to 0..infinity; avoid actual inifinity at tan(pi/2) */ > double contrastExp = tan(std::clamp(contrast * M_PI_4, 0.0, M_PI_2 - 0.00001)); > @@ -52,8 +54,7 @@ void Lut::updateGammaTable(IPAContext &context) > normalized = 0.5 * std::pow(normalized / 0.5, contrastExp); > else > normalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrastExp); > - gammaTable[i] = UINT8_MAX * > - std::pow(normalized, context.configuration.gamma); > + gammaTable[i] = UINT8_MAX * std::pow(normalized, gamma); > } > /* > * Due to CCM operations, the table lookup may reach indices below the black > @@ -65,6 +66,7 @@ void Lut::updateGammaTable(IPAContext &context) > gammaTable[blackIndex]); > } > > + context.activeState.gamma.gamma = gamma; > context.activeState.gamma.blackLevel = blackLevel; > context.activeState.gamma.contrastExp = contrastExp; > } > @@ -131,7 +133,7 @@ void Lut::prepare(IPAContext &context, > context.activeState.matrixChanged = false; > } > > - params->gamma = context.configuration.gamma; > + params->gamma = context.activeState.gamma.gamma; > params->contrastExp = context.activeState.gamma.contrastExp; > } > > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index 58dcad290..680b72eb9 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -25,7 +25,6 @@ namespace libcamera { > namespace ipa::soft { > > struct IPASessionConfiguration { > - float gamma; > struct { > int32_t exposureMin, exposureMax; > double againMin, againMax, again10, againMinStep; > @@ -58,6 +57,7 @@ struct IPAActiveState { > struct { > std::array<double, kGammaLookupSize> gammaTable; > uint8_t blackLevel; > + float gamma; > double contrast; > double contrastExp; > } gamma; > @@ -67,6 +67,7 @@ struct IPAActiveState { > bool matrixChanged = false; > > struct { > + std::optional<float> gamma; > /* 0..2 range, 1.0 = normal */ > std::optional<double> contrast; > std::optional<float> saturation; > @@ -86,6 +87,7 @@ struct IPAFrameContext : public FrameContext { > double blue; > } gains; > > + std::optional<float> gamma; > std::optional<double> contrast; > std::optional<float> saturation; > };
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: >> The gamma value is fixed in software ISP. Let's make it adjustable, >> similarly to contrast and saturation. >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/adjust.cpp | 8 ++++++++ >> src/ipa/simple/algorithms/adjust.h | 2 ++ >> src/ipa/simple/algorithms/lut.cpp | 12 +++++++----- >> src/ipa/simple/ipa_context.h | 4 +++- >> 4 files changed, 20 insertions(+), 6 deletions(-) >> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp >> index 27ae2a53a..cfbc39610 100644 >> --- a/src/ipa/simple/algorithms/adjust.cpp >> +++ b/src/ipa/simple/algorithms/adjust.cpp >> @@ -23,6 +23,7 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust) >> int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) >> { >> + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma); >> context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f); >> if (context.ccmEnabled) >> context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); >> @@ -32,6 +33,7 @@ int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningD >> int Adjust::configure(IPAContext &context, >> [[maybe_unused]] const IPAConfigInfo &configInfo) >> { >> + context.activeState.knobs.gamma = std::optional<double>(); >> context.activeState.knobs.contrast = std::optional<double>(); >> context.activeState.knobs.saturation = std::optional<double>(); >> @@ -43,6 +45,12 @@ void Adjust::queueRequest(typename Module::Context &context, >> [[maybe_unused]] typename Module::FrameContext &frameContext, >> const ControlList &controls) >> { >> + const auto &gamma = controls.get(controls::Gamma); >> + if (gamma.has_value()) { >> + context.activeState.knobs.gamma = gamma; >> + LOG(IPASoftAdjust, Debug) << "Setting gamma to " << gamma.value(); >> + } >> + >> const auto &contrast = controls.get(controls::Contrast); >> if (contrast.has_value()) { >> context.activeState.knobs.contrast = contrast; > > I believe this is missing the reporting of `controls::Gamma` in the metadata. Yes, I've already mentioned that and I'll add it. >> diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h >> index c4baa2503..190d2079f 100644 >> --- a/src/ipa/simple/algorithms/adjust.h >> +++ b/src/ipa/simple/algorithms/adjust.h >> @@ -19,6 +19,8 @@ namespace libcamera { >> namespace ipa::soft::algorithms { >> +const float kDefaultGamma = 2.2f; >> + >> class Adjust : public Algorithm >> { >> public: >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp >> index 3a00bf4ee..25b188b91 100644 >> --- a/src/ipa/simple/algorithms/lut.cpp >> +++ b/src/ipa/simple/algorithms/lut.cpp >> @@ -18,6 +18,8 @@ >> #include "simple/ipa_context.h" >> +#include "adjust.h" >> + >> namespace libcamera { >> LOG_DEFINE_CATEGORY(IPASoftLut) >> @@ -27,8 +29,6 @@ namespace ipa::soft::algorithms { >> int Lut::configure(IPAContext &context, >> [[maybe_unused]] const IPAConfigInfo &configInfo) >> { >> - /* Gamma value is fixed */ >> - context.configuration.gamma = 1.0 / 2.2; >> updateGammaTable(context); >> return 0; >> @@ -37,6 +37,8 @@ int Lut::configure(IPAContext &context, >> void Lut::updateGammaTable(IPAContext &context) >> { >> const auto blackLevel = context.activeState.blc.level; >> + const auto gamma = >> + 1.0 / context.activeState.knobs.gamma.value_or(kDefaultGamma); >> const auto contrast = context.activeState.knobs.contrast.value_or(1.0); >> /* Convert 0..2 to 0..infinity; avoid actual inifinity at tan(pi/2) */ >> double contrastExp = tan(std::clamp(contrast * M_PI_4, 0.0, M_PI_2 - 0.00001)); >> @@ -52,8 +54,7 @@ void Lut::updateGammaTable(IPAContext &context) >> normalized = 0.5 * std::pow(normalized / 0.5, contrastExp); >> else >> normalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrastExp); >> - gammaTable[i] = UINT8_MAX * >> - std::pow(normalized, context.configuration.gamma); >> + gammaTable[i] = UINT8_MAX * std::pow(normalized, gamma); >> } >> /* >> * Due to CCM operations, the table lookup may reach indices below the black >> @@ -65,6 +66,7 @@ void Lut::updateGammaTable(IPAContext &context) >> gammaTable[blackIndex]); >> } >> + context.activeState.gamma.gamma = gamma; >> context.activeState.gamma.blackLevel = blackLevel; >> context.activeState.gamma.contrastExp = contrastExp; >> } >> @@ -131,7 +133,7 @@ void Lut::prepare(IPAContext &context, >> context.activeState.matrixChanged = false; >> } >> - params->gamma = context.configuration.gamma; >> + params->gamma = context.activeState.gamma.gamma; >> params->contrastExp = context.activeState.gamma.contrastExp; >> } >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> index 58dcad290..680b72eb9 100644 >> --- a/src/ipa/simple/ipa_context.h >> +++ b/src/ipa/simple/ipa_context.h >> @@ -25,7 +25,6 @@ namespace libcamera { >> namespace ipa::soft { >> struct IPASessionConfiguration { >> - float gamma; >> struct { >> int32_t exposureMin, exposureMax; >> double againMin, againMax, again10, againMinStep; >> @@ -58,6 +57,7 @@ struct IPAActiveState { >> struct { >> std::array<double, kGammaLookupSize> gammaTable; >> uint8_t blackLevel; >> + float gamma; >> double contrast; >> double contrastExp; >> } gamma; >> @@ -67,6 +67,7 @@ struct IPAActiveState { >> bool matrixChanged = false; >> struct { >> + std::optional<float> gamma; >> /* 0..2 range, 1.0 = normal */ >> std::optional<double> contrast; >> std::optional<float> saturation; >> @@ -86,6 +87,7 @@ struct IPAFrameContext : public FrameContext { >> double blue; >> } gains; >> + std::optional<float> gamma; >> std::optional<double> contrast; >> std::optional<float> saturation; >> };
2026. 01. 20. 18:03 keltezéssel, Milan Zamazal írta: > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> 2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta: >>> The gamma value is fixed in software ISP. Let's make it adjustable, >>> similarly to contrast and saturation. >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/ipa/simple/algorithms/adjust.cpp | 8 ++++++++ >>> src/ipa/simple/algorithms/adjust.h | 2 ++ >>> src/ipa/simple/algorithms/lut.cpp | 12 +++++++----- >>> src/ipa/simple/ipa_context.h | 4 +++- >>> 4 files changed, 20 insertions(+), 6 deletions(-) >>> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp >>> index 27ae2a53a..cfbc39610 100644 >>> --- a/src/ipa/simple/algorithms/adjust.cpp >>> +++ b/src/ipa/simple/algorithms/adjust.cpp >>> @@ -23,6 +23,7 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust) > [...] >>> @@ -43,6 +45,12 @@ void Adjust::queueRequest(typename Module::Context &context, >>> [[maybe_unused]] typename Module::FrameContext &frameContext, >>> const ControlList &controls) >>> { >>> + const auto &gamma = controls.get(controls::Gamma); >>> + if (gamma.has_value()) { >>> + context.activeState.knobs.gamma = gamma; >>> + LOG(IPASoftAdjust, Debug) << "Setting gamma to " << gamma.value(); >>> + } >>> + >>> const auto &contrast = controls.get(controls::Contrast); >>> if (contrast.has_value()) { >>> context.activeState.knobs.contrast = contrast; >> >> I believe this is missing the reporting of `controls::Gamma` in the metadata. > > Yes, I've already mentioned that and I'll add it. Of course, you're right, sorry. I did not notice it in your other reply.
diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp index 27ae2a53a..cfbc39610 100644 --- a/src/ipa/simple/algorithms/adjust.cpp +++ b/src/ipa/simple/algorithms/adjust.cpp @@ -23,6 +23,7 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust) int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData) { + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma); context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f); if (context.ccmEnabled) context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); @@ -32,6 +33,7 @@ int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningD int Adjust::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { + context.activeState.knobs.gamma = std::optional<double>(); context.activeState.knobs.contrast = std::optional<double>(); context.activeState.knobs.saturation = std::optional<double>(); @@ -43,6 +45,12 @@ void Adjust::queueRequest(typename Module::Context &context, [[maybe_unused]] typename Module::FrameContext &frameContext, const ControlList &controls) { + const auto &gamma = controls.get(controls::Gamma); + if (gamma.has_value()) { + context.activeState.knobs.gamma = gamma; + LOG(IPASoftAdjust, Debug) << "Setting gamma to " << gamma.value(); + } + const auto &contrast = controls.get(controls::Contrast); if (contrast.has_value()) { context.activeState.knobs.contrast = contrast; diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h index c4baa2503..190d2079f 100644 --- a/src/ipa/simple/algorithms/adjust.h +++ b/src/ipa/simple/algorithms/adjust.h @@ -19,6 +19,8 @@ namespace libcamera { namespace ipa::soft::algorithms { +const float kDefaultGamma = 2.2f; + class Adjust : public Algorithm { public: diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp index 3a00bf4ee..25b188b91 100644 --- a/src/ipa/simple/algorithms/lut.cpp +++ b/src/ipa/simple/algorithms/lut.cpp @@ -18,6 +18,8 @@ #include "simple/ipa_context.h" +#include "adjust.h" + namespace libcamera { LOG_DEFINE_CATEGORY(IPASoftLut) @@ -27,8 +29,6 @@ namespace ipa::soft::algorithms { int Lut::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { - /* Gamma value is fixed */ - context.configuration.gamma = 1.0 / 2.2; updateGammaTable(context); return 0; @@ -37,6 +37,8 @@ int Lut::configure(IPAContext &context, void Lut::updateGammaTable(IPAContext &context) { const auto blackLevel = context.activeState.blc.level; + const auto gamma = + 1.0 / context.activeState.knobs.gamma.value_or(kDefaultGamma); const auto contrast = context.activeState.knobs.contrast.value_or(1.0); /* Convert 0..2 to 0..infinity; avoid actual inifinity at tan(pi/2) */ double contrastExp = tan(std::clamp(contrast * M_PI_4, 0.0, M_PI_2 - 0.00001)); @@ -52,8 +54,7 @@ void Lut::updateGammaTable(IPAContext &context) normalized = 0.5 * std::pow(normalized / 0.5, contrastExp); else normalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrastExp); - gammaTable[i] = UINT8_MAX * - std::pow(normalized, context.configuration.gamma); + gammaTable[i] = UINT8_MAX * std::pow(normalized, gamma); } /* * Due to CCM operations, the table lookup may reach indices below the black @@ -65,6 +66,7 @@ void Lut::updateGammaTable(IPAContext &context) gammaTable[blackIndex]); } + context.activeState.gamma.gamma = gamma; context.activeState.gamma.blackLevel = blackLevel; context.activeState.gamma.contrastExp = contrastExp; } @@ -131,7 +133,7 @@ void Lut::prepare(IPAContext &context, context.activeState.matrixChanged = false; } - params->gamma = context.configuration.gamma; + params->gamma = context.activeState.gamma.gamma; params->contrastExp = context.activeState.gamma.contrastExp; } diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index 58dcad290..680b72eb9 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -25,7 +25,6 @@ namespace libcamera { namespace ipa::soft { struct IPASessionConfiguration { - float gamma; struct { int32_t exposureMin, exposureMax; double againMin, againMax, again10, againMinStep; @@ -58,6 +57,7 @@ struct IPAActiveState { struct { std::array<double, kGammaLookupSize> gammaTable; uint8_t blackLevel; + float gamma; double contrast; double contrastExp; } gamma; @@ -67,6 +67,7 @@ struct IPAActiveState { bool matrixChanged = false; struct { + std::optional<float> gamma; /* 0..2 range, 1.0 = normal */ std::optional<double> contrast; std::optional<float> saturation; @@ -86,6 +87,7 @@ struct IPAFrameContext : public FrameContext { double blue; } gains; + std::optional<float> gamma; std::optional<double> contrast; std::optional<float> saturation; };
The gamma value is fixed in software ISP. Let's make it adjustable, similarly to contrast and saturation. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/algorithms/adjust.cpp | 8 ++++++++ src/ipa/simple/algorithms/adjust.h | 2 ++ src/ipa/simple/algorithms/lut.cpp | 12 +++++++----- src/ipa/simple/ipa_context.h | 4 +++- 4 files changed, 20 insertions(+), 6 deletions(-)