[v4,25/49] ipa: libipa: agc: Adjust debug messages slightly

27698 diff mbox series

The log message showing the result of AGC seems too verbose to me,
and the labels and values are far from each other. And also log the
line duration when showing exposure and gain limits.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/libipa/agc.cpp | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Hi Barnabás,

Quoting Barnabás Pőcze (2026-08-10 12:38:21)
> The log message showing the result of AGC seems too verbose to me,
> and the labels and values are far from each other. And also log the
> line duration when showing exposure and gain limits.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/ipa/libipa/agc.cpp | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 6f10f8982f..3994d1c5e4 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -272,8 +272,10 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>         float defGain = config.sensor->gain(v4l2Gain.def().get<int32_t>());
>  
>         LOG(Agc, Debug)
> -               << "Exposure: [" << minExposure << ", " << maxExposure
> -               << "], gain: [" << minGain << ", " << maxGain << "]";
> +               << "exposure:[" << minExposure << ',' << maxExposure << ']'
> +               << " gain:[" << minGain << ',' << maxGain << ']'

I'd keep thespaces for readability. I don't know if there is an official
rule, but I think libcamera log messages usually start with uppercase
like a regular sentence.

> +               << " line-duration:" << session.lineDuration
> +               << " sensor-output:" << session.sensor.outputSize;
>  
>         /*
>          * Compute the frame duration limits.
> @@ -603,10 +605,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>                 newExposureTime = newEv.exposureTime;
>  
>                 LOG(Agc, Debug)
> -                       << "Divided up exposure time, analogue gain, quantization gain"
> -                       << " and digital gain are " << utils::Duration(state.automatic.exposure * lineDuration)
> -                       << ", " << state.automatic.gain << ", " << state.automatic.quantizationGain
> -                       << " and " << newEv.digitalGain;
> +                       << "exposure-time:" << utils::Duration(state.automatic.exposure * lineDuration)
> +                       << " analogue-gain:" << state.automatic.gain
> +                       << " quantization-gain:" << state.automatic.quantizationGain
> +                       << " digital-gain:" << newEv.digitalGain;

Agreed that the old one was quite verbose, but it now lost the
information that this is the result of the split operation. I think a
tiny bit of context is nice in debug logs to be able to read them
without knowing the code context. I think a space after a colon is usual
style in the rest of the codebase.

But I also can't find a nice word.

Best regards,
Stefan



>         }
>  
>         /*
> -- 
> 2.55.0
>