[v5,26/47] ipa: libipa: agc: Adjust debug messages slightly
diff mbox series

Message ID 20260817114349.994123-27-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: libipa: agc rework
Related show

Commit Message

Barnabás Pőcze Aug. 17, 2026, 11:43 a.m. UTC 
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(-)

Comments

Jacopo Mondi Aug. 17, 2026, 3:12 p.m. UTC | #1 
HI Barnabás

On Mon, Aug 17, 2026 at 01:43:27PM +0200, Barnabás Pőcze wrote:
> 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 5d85cc69e2..ea7e095b0d 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -305,8 +305,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 << ']'
> +		<< " line-duration:" << session.lineDuration
> +		<< " sensor-output:" << session.sensor.outputSize;

I don't know if this is intentional, but you seem to have lost all
spaces between ':' and the values.

>
>  	/*
>  	 * Compute the frame duration limits.
> @@ -634,10 +636,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>  	state.automatic.yTarget = newEv.yTarget;
>
>  	LOG(Agc, Debug)
> -		<< "Divided up exposure time, analogue gain, quantization gain"
> -		<< " and digital gain are " << newEv.exposureTime
> -		<< ", " << state.automatic.gain << ", " << state.automatic.quantizationGain
> -		<< " and " << newEv.digitalGain;
> +		<< "exposure-time:" << newEv.exposureTime
> +		<< " analogue-gain:" << state.automatic.gain
> +		<< " quantization-gain:" << state.automatic.quantizationGain
> +		<< " digital-gain:" << newEv.digitalGain;
>
>  	/*
>  	 * Expand the target frame duration so that we do not run faster than
> --
> 2.55.0
>
Barnabás Pőcze Aug. 18, 2026, 12:43 p.m. UTC | #2 
2026. 08. 17. 17:12 keltezéssel, Jacopo Mondi írta:
> HI Barnabás
> 
> On Mon, Aug 17, 2026 at 01:43:27PM +0200, Barnabás Pőcze wrote:
>> 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 5d85cc69e2..ea7e095b0d 100644
>> --- a/src/ipa/libipa/agc.cpp
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -305,8 +305,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 << ']'
>> +		<< " line-duration:" << session.lineDuration
>> +		<< " sensor-output:" << session.sensor.outputSize;
> 
> I don't know if this is intentional, but you seem to have lost all
> spaces between ':' and the values.

It was intentional, I believe it was like that from the very first version.


> 
>>
>>   	/*
>>   	 * Compute the frame duration limits.
>> @@ -634,10 +636,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>   	state.automatic.yTarget = newEv.yTarget;
>>
>>   	LOG(Agc, Debug)
>> -		<< "Divided up exposure time, analogue gain, quantization gain"
>> -		<< " and digital gain are " << newEv.exposureTime
>> -		<< ", " << state.automatic.gain << ", " << state.automatic.quantizationGain
>> -		<< " and " << newEv.digitalGain;
>> +		<< "exposure-time:" << newEv.exposureTime
>> +		<< " analogue-gain:" << state.automatic.gain
>> +		<< " quantization-gain:" << state.automatic.quantizationGain
>> +		<< " digital-gain:" << newEv.digitalGain;
>>
>>   	/*
>>   	 * Expand the target frame duration so that we do not run faster than
>> --
>> 2.55.0
>>
Jacopo Mondi Aug. 19, 2026, 6:41 a.m. UTC | #3 
Hi Barnabás

On Tue, Aug 18, 2026 at 02:43:33PM +0200, Barnabás Pőcze wrote:
> 2026. 08. 17. 17:12 keltezéssel, Jacopo Mondi írta:
> > HI Barnabás
> >
> > On Mon, Aug 17, 2026 at 01:43:27PM +0200, Barnabás Pőcze wrote:
> > > 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 5d85cc69e2..ea7e095b0d 100644
> > > --- a/src/ipa/libipa/agc.cpp
> > > +++ b/src/ipa/libipa/agc.cpp
> > > @@ -305,8 +305,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 << ']'
> > > +		<< " line-duration:" << session.lineDuration
> > > +		<< " sensor-output:" << session.sensor.outputSize;
> >
> > I don't know if this is intentional, but you seem to have lost all
> > spaces between ':' and the values.
>
> It was intentional, I believe it was like that from the very first version.

Still, do you think

        exposure:[1234, 5678], gain:[1, 16]

reads better than
        exposure: [1234, 5678], gain: [1, 16]

Either drop the ':'

        exposure[1234, 5678], gain[1, 16]

Or insert a space after it ?

>
>
> >
> > >
> > >   	/*
> > >   	 * Compute the frame duration limits.
> > > @@ -634,10 +636,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
> > >   	state.automatic.yTarget = newEv.yTarget;
> > >
> > >   	LOG(Agc, Debug)
> > > -		<< "Divided up exposure time, analogue gain, quantization gain"
> > > -		<< " and digital gain are " << newEv.exposureTime
> > > -		<< ", " << state.automatic.gain << ", " << state.automatic.quantizationGain
> > > -		<< " and " << newEv.digitalGain;
> > > +		<< "exposure-time:" << newEv.exposureTime
> > > +		<< " analogue-gain:" << state.automatic.gain
> > > +		<< " quantization-gain:" << state.automatic.quantizationGain
> > > +		<< " digital-gain:" << newEv.digitalGain;

Even more so here, where you don't print out limits but the actual
values

> > >
> > >   	/*
> > >   	 * Expand the target frame duration so that we do not run faster than
> > > --
> > > 2.55.0
> > >
>
Barnabás Pőcze Aug. 19, 2026, 7:59 a.m. UTC | #4 
2026. 08. 19. 8:41 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Tue, Aug 18, 2026 at 02:43:33PM +0200, Barnabás Pőcze wrote:
>> 2026. 08. 17. 17:12 keltezéssel, Jacopo Mondi írta:
>>> HI Barnabás
>>>
>>> On Mon, Aug 17, 2026 at 01:43:27PM +0200, Barnabás Pőcze wrote:
>>>> 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 5d85cc69e2..ea7e095b0d 100644
>>>> --- a/src/ipa/libipa/agc.cpp
>>>> +++ b/src/ipa/libipa/agc.cpp
>>>> @@ -305,8 +305,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 << ']'
>>>> +		<< " line-duration:" << session.lineDuration
>>>> +		<< " sensor-output:" << session.sensor.outputSize;
>>>
>>> I don't know if this is intentional, but you seem to have lost all
>>> spaces between ':' and the values.
>>
>> It was intentional, I believe it was like that from the very first version.
> 
> Still, do you think
> 
>          exposure:[1234, 5678], gain:[1, 16]
> 
> reads better than
>          exposure: [1234, 5678], gain: [1, 16]
> 
> Either drop the ':'
> 
>          exposure[1234, 5678], gain[1, 16]
> 
> Or insert a space after it ?
> 
>>
>>
>>>
>>>>
>>>>    	/*
>>>>    	 * Compute the frame duration limits.
>>>> @@ -634,10 +636,10 @@ void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
>>>>    	state.automatic.yTarget = newEv.yTarget;
>>>>
>>>>    	LOG(Agc, Debug)
>>>> -		<< "Divided up exposure time, analogue gain, quantization gain"
>>>> -		<< " and digital gain are " << newEv.exposureTime
>>>> -		<< ", " << state.automatic.gain << ", " << state.automatic.quantizationGain
>>>> -		<< " and " << newEv.digitalGain;
>>>> +		<< "exposure-time:" << newEv.exposureTime
>>>> +		<< " analogue-gain:" << state.automatic.gain
>>>> +		<< " quantization-gain:" << state.automatic.quantizationGain
>>>> +		<< " digital-gain:" << newEv.digitalGain;
> 
> Even more so here, where you don't print out limits but the actual
> values

Well, to me `<label>:<value> <label>:<value> ...` looks best, but I now see this is far from
a universal opinion, I'll replace these with `<label>: <value>, <label>: <value>, ...`.



> 
>>>>
>>>>    	/*
>>>>    	 * Expand the target frame duration so that we do not run faster than
>>>> --
>>>> 2.55.0
>>>>
>>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
index 5d85cc69e2..ea7e095b0d 100644
--- a/src/ipa/libipa/agc.cpp
+++ b/src/ipa/libipa/agc.cpp
@@ -305,8 +305,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 << ']'
+		<< " line-duration:" << session.lineDuration
+		<< " sensor-output:" << session.sensor.outputSize;
 
 	/*
 	 * Compute the frame duration limits.
@@ -634,10 +636,10 @@  void AgcAlgorithm::process(const agc::Session &session, agc::ActiveState &state,
 	state.automatic.yTarget = newEv.yTarget;
 
 	LOG(Agc, Debug)
-		<< "Divided up exposure time, analogue gain, quantization gain"
-		<< " and digital gain are " << newEv.exposureTime
-		<< ", " << state.automatic.gain << ", " << state.automatic.quantizationGain
-		<< " and " << newEv.digitalGain;
+		<< "exposure-time:" << newEv.exposureTime
+		<< " analogue-gain:" << state.automatic.gain
+		<< " quantization-gain:" << state.automatic.quantizationGain
+		<< " digital-gain:" << newEv.digitalGain;
 
 	/*
 	 * Expand the target frame duration so that we do not run faster than