[v4,15/49] ipa: libipa: agc_mean_luminance: constraintClampGain(): Mark `const`

27688 diff mbox series

There is no reason not to have this member function as `const`. Change the
map lookup to use `at()` just like it is done for `calculateNewEv()`, with
the same requirement that validation is the job of the caller.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/libipa/agc_mean_luminance.cpp | 4 ++--
 src/ipa/libipa/agc_mean_luminance.h   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Hi Barnabás,

Quoting Barnabás Pőcze (2026-08-10 12:38:11)
> There is no reason not to have this member function as `const`. Change the
> map lookup to use `at()` just like it is done for `calculateNewEv()`, with

These are conceptually unrelated, are they? With a quick glance I
couldn't spot the reference this mentions. Tbh I like the [] syntax
more. Is there a performance penalty involved with the [] in cases where
the entry exists?

> the same requirement that validation is the job of the caller.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

With an explanation why at() is preferred:

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

> ---
>  src/ipa/libipa/agc_mean_luminance.cpp | 4 ++--
>  src/ipa/libipa/agc_mean_luminance.h   | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index bf7d164346..3a14d778a3 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -538,7 +538,7 @@ double AgcMeanLuminance::estimateInitialGain(const Traits &traits) const
>   */
>  double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>                                              const Histogram &hist,
> -                                            double gain)
> +                                            double gain) const
>  {
>         auto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {
>                 double lux = lux_;
> @@ -568,7 +568,7 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>                 }
>         };
>  
> -       std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex];
> +       const std::vector<AgcConstraint> &constraints = constraintModes_.at(constraintModeIndex);
>         std::for_each(constraints.begin(), constraints.end(), applyConstraint);
>  
>         std::for_each(additionalConstraints_.begin(), additionalConstraints_.end(), applyConstraint);
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index eb9d6fec2b..9b82d97ab6 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -105,7 +105,7 @@ private:
>         double estimateInitialGain(const Traits &traits) const;
>         double constraintClampGain(uint32_t constraintModeIndex,
>                                    const Histogram &hist,
> -                                  double gain);
> +                                  double gain) const;
>         utils::Duration filterExposure(utils::Duration exposureValue);
>  
>         utils::Duration filteredExposure_;
> -- 
> 2.55.0
>

Hi

2026. 08. 11. 15:36 keltezéssel, Stefan Klug írta:
> Hi Barnabás,
> 
> Quoting Barnabás Pőcze (2026-08-10 12:38:11)
>> There is no reason not to have this member function as `const`. Change the
>> map lookup to use `at()` just like it is done for `calculateNewEv()`, with
> 
> These are conceptually unrelated, are they? With a quick glance I
> couldn't spot the reference this mentions. Tbh I like the [] syntax
> more. Is there a performance penalty involved with the [] in cases where
> the entry exists?

`operator[]` cannot be used on a const qualified map, hence the change. I meant
that `calculateNewEv()` uses `exposureModeHelpers_.at(...)`, not `exposureModeHelpers_[...]`.


> 
>> the same requirement that validation is the job of the caller.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> With an explanation why at() is preferred:
> 
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
>> ---
>>   src/ipa/libipa/agc_mean_luminance.cpp | 4 ++--
>>   src/ipa/libipa/agc_mean_luminance.h   | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
>> index bf7d164346..3a14d778a3 100644
>> --- a/src/ipa/libipa/agc_mean_luminance.cpp
>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
>> @@ -538,7 +538,7 @@ double AgcMeanLuminance::estimateInitialGain(const Traits &traits) const
>>    */
>>   double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>>                                               const Histogram &hist,
>> -                                            double gain)
>> +                                            double gain) const
>>   {
>>          auto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {
>>                  double lux = lux_;
>> @@ -568,7 +568,7 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>>                  }
>>          };
>>   
>> -       std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex];
>> +       const std::vector<AgcConstraint> &constraints = constraintModes_.at(constraintModeIndex);
>>          std::for_each(constraints.begin(), constraints.end(), applyConstraint);
>>   
>>          std::for_each(additionalConstraints_.begin(), additionalConstraints_.end(), applyConstraint);
>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
>> index eb9d6fec2b..9b82d97ab6 100644
>> --- a/src/ipa/libipa/agc_mean_luminance.h
>> +++ b/src/ipa/libipa/agc_mean_luminance.h
>> @@ -105,7 +105,7 @@ private:
>>          double estimateInitialGain(const Traits &traits) const;
>>          double constraintClampGain(uint32_t constraintModeIndex,
>>                                     const Histogram &hist,
>> -                                  double gain);
>> +                                  double gain) const;
>>          utils::Duration filterExposure(utils::Duration exposureValue);
>>   
>>          utils::Duration filteredExposure_;
>> -- 
>> 2.55.0
>>

Quoting Barnabás Pőcze (2026-08-11 15:43:30)
> Hi
> 
> 2026. 08. 11. 15:36 keltezéssel, Stefan Klug írta:
> > Hi Barnabás,
> > 
> > Quoting Barnabás Pőcze (2026-08-10 12:38:11)
> >> There is no reason not to have this member function as `const`. Change the
> >> map lookup to use `at()` just like it is done for `calculateNewEv()`, with
> > 
> > These are conceptually unrelated, are they? With a quick glance I
> > couldn't spot the reference this mentions. Tbh I like the [] syntax
> > more. Is there a performance penalty involved with the [] in cases where
> > the entry exists?
> 
> `operator[]` cannot be used on a const qualified map, hence the change. I meant
> that `calculateNewEv()` uses `exposureModeHelpers_.at(...)`, not `exposureModeHelpers_[...]`.

Yeah, sorry I missed that one. Makes sense.

Best regards,
Stefan

> 
> 
> > 
> >> the same requirement that validation is the job of the caller.
> >>
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > 
> > With an explanation why at() is preferred:
> > 
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> >> ---
> >>   src/ipa/libipa/agc_mean_luminance.cpp | 4 ++--
> >>   src/ipa/libipa/agc_mean_luminance.h   | 2 +-
> >>   2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> >> index bf7d164346..3a14d778a3 100644
> >> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> >> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> >> @@ -538,7 +538,7 @@ double AgcMeanLuminance::estimateInitialGain(const Traits &traits) const
> >>    */
> >>   double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> >>                                               const Histogram &hist,
> >> -                                            double gain)
> >> +                                            double gain) const
> >>   {
> >>          auto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {
> >>                  double lux = lux_;
> >> @@ -568,7 +568,7 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> >>                  }
> >>          };
> >>   
> >> -       std::vector<AgcConstraint> &constraints = constraintModes_[constraintModeIndex];
> >> +       const std::vector<AgcConstraint> &constraints = constraintModes_.at(constraintModeIndex);
> >>          std::for_each(constraints.begin(), constraints.end(), applyConstraint);
> >>   
> >>          std::for_each(additionalConstraints_.begin(), additionalConstraints_.end(), applyConstraint);
> >> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> >> index eb9d6fec2b..9b82d97ab6 100644
> >> --- a/src/ipa/libipa/agc_mean_luminance.h
> >> +++ b/src/ipa/libipa/agc_mean_luminance.h
> >> @@ -105,7 +105,7 @@ private:
> >>          double estimateInitialGain(const Traits &traits) const;
> >>          double constraintClampGain(uint32_t constraintModeIndex,
> >>                                     const Histogram &hist,
> >> -                                  double gain);
> >> +                                  double gain) const;
> >>          utils::Duration filterExposure(utils::Duration exposureValue);
> >>   
> >>          utils::Duration filteredExposure_;
> >> -- 
> >> 2.55.0
> >>
>