ipa: simple: agc: Make sure activeState.agc expo/again are always initialized
diff mbox series

Message ID 20251220172703.321681-1-johannes.goede@oss.qualcomm.com
State Accepted
Commit 03fc5f6c940fd59efbd445b462606d5a728930f5
Headers show
Series
  • ipa: simple: agc: Make sure activeState.agc expo/again are always initialized
Related show

Commit Message

Hans de Goede Dec. 20, 2025, 5:27 p.m. UTC 
If the first frame of a stream is bad, the IPA will not get called with
frame == 0, leaving activeState.agc expo/again uninitialized. This causes
the agc algorithm to set a very low gain and exposure on the next run
(where it will hit the if (!stats->valid) {} path) resulting in starting
with a black image.

Fix this by using a valid flag instead of checking for frame == 0.

The entire activeState gets cleared to 0 on configure() resetting the new
valid flag.

Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 src/ipa/simple/algorithms/agc.cpp | 5 +++--
 src/ipa/simple/ipa_context.h      | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Milan Zamazal Jan. 2, 2026, 10:30 a.m. UTC | #1 
Hi Hans,

thank you for the patch.

Hans de Goede <johannes.goede@oss.qualcomm.com> writes:

> If the first frame of a stream is bad, the IPA will not get called with
> frame == 0, leaving activeState.agc expo/again uninitialized. This causes
> the agc algorithm to set a very low gain and exposure on the next run
> (where it will hit the if (!stats->valid) {} path) resulting in starting
> with a black image.
>
> Fix this by using a valid flag instead of checking for frame == 0.
>
> The entire activeState gets cleared to 0 on configure() resetting the new
> valid flag.
>
> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> ---
>  src/ipa/simple/algorithms/agc.cpp | 5 +++--
>  src/ipa/simple/ipa_context.h      | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
> index 189de770..2f7e040c 100644
> --- a/src/ipa/simple/algorithms/agc.cpp
> +++ b/src/ipa/simple/algorithms/agc.cpp
> @@ -100,7 +100,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>  }
>  
>  void Agc::process(IPAContext &context,
> -		  const uint32_t frame,
> +		  [[maybe_unused]] const uint32_t frame,
>  		  IPAFrameContext &frameContext,
>  		  const SwIspStats *stats,
>  		  ControlList &metadata)
> @@ -110,13 +110,14 @@ void Agc::process(IPAContext &context,
>  	metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
>  	metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
>  
> -	if (frame == 0) {
> +	if (!context.activeState.agc.valid) {
>  		/*
>  		 * Init active-state from sensor values in case updateExposure()
>  		 * does not run for the first frame.
>  		 */
>  		context.activeState.agc.exposure = frameContext.sensor.exposure;
>  		context.activeState.agc.again = frameContext.sensor.gain;
> +		context.activeState.agc.valid = true;
>  	}
>  
>  	if (!stats->valid) {
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index c3081e30..26b60fb6 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -40,6 +40,7 @@ struct IPAActiveState {
>  	struct {
>  		int32_t exposure;
>  		double again;
> +		bool valid;
>  	} agc;
>  
>  	struct {
Milan Zamazal Jan. 2, 2026, 10:41 a.m. UTC | #2 
Milan Zamazal <mzamazal@redhat.com> writes:

> Hi Hans,
>
> thank you for the patch.
>
> Hans de Goede <johannes.goede@oss.qualcomm.com> writes:
>
>> If the first frame of a stream is bad, the IPA will not get called with
>> frame == 0, leaving activeState.agc expo/again uninitialized. This causes
>> the agc algorithm to set a very low gain and exposure on the next run
>> (where it will hit the if (!stats->valid) {} path) resulting in starting
>> with a black image.
>>
>> Fix this by using a valid flag instead of checking for frame == 0.
>>
>> The entire activeState gets cleared to 0 on configure() resetting the new
>> valid flag.
>>
>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

I think it should fix
https://gitlab.freedesktop.org/camera/libcamera/-/issues/298
?

> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>
>> ---
>>  src/ipa/simple/algorithms/agc.cpp | 5 +++--
>>  src/ipa/simple/ipa_context.h      | 1 +
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
>> index 189de770..2f7e040c 100644
>> --- a/src/ipa/simple/algorithms/agc.cpp
>> +++ b/src/ipa/simple/algorithms/agc.cpp
>> @@ -100,7 +100,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>>  }
>>  
>>  void Agc::process(IPAContext &context,
>> -		  const uint32_t frame,
>> +		  [[maybe_unused]] const uint32_t frame,
>>  		  IPAFrameContext &frameContext,
>>  		  const SwIspStats *stats,
>>  		  ControlList &metadata)
>> @@ -110,13 +110,14 @@ void Agc::process(IPAContext &context,
>>  	metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
>>  	metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
>>  
>> -	if (frame == 0) {
>> +	if (!context.activeState.agc.valid) {
>>  		/*
>>  		 * Init active-state from sensor values in case updateExposure()
>>  		 * does not run for the first frame.
>>  		 */
>>  		context.activeState.agc.exposure = frameContext.sensor.exposure;
>>  		context.activeState.agc.again = frameContext.sensor.gain;
>> +		context.activeState.agc.valid = true;
>>  	}
>>  
>>  	if (!stats->valid) {
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index c3081e30..26b60fb6 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -40,6 +40,7 @@ struct IPAActiveState {
>>  	struct {
>>  		int32_t exposure;
>>  		double again;
>> +		bool valid;
>>  	} agc;
>>  
>>  	struct {
Hans de Goede Jan. 3, 2026, 12:10 p.m. UTC | #3 
Hi,

On 2-Jan-26 11:41, Milan Zamazal wrote:
> Milan Zamazal <mzamazal@redhat.com> writes:
> 
>> Hi Hans,
>>
>> thank you for the patch.
>>
>> Hans de Goede <johannes.goede@oss.qualcomm.com> writes:
>>
>>> If the first frame of a stream is bad, the IPA will not get called with
>>> frame == 0, leaving activeState.agc expo/again uninitialized. This causes
>>> the agc algorithm to set a very low gain and exposure on the next run
>>> (where it will hit the if (!stats->valid) {} path) resulting in starting
>>> with a black image.
>>>
>>> Fix this by using a valid flag instead of checking for frame == 0.
>>>
>>> The entire activeState gets cleared to 0 on configure() resetting the new
>>> valid flag.
>>>
>>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> 
> I think it should fix
> https://gitlab.freedesktop.org/camera/libcamera/-/issues/298
> ?

Yes it fixes that. I've already added the fix to the Fedora libcamera
pkgs and Fedora users with the same laptop as from that bug have
confirmed it fixes the oscilation.

Regards,

Hans


> 
>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>>
>>> ---
>>>  src/ipa/simple/algorithms/agc.cpp | 5 +++--
>>>  src/ipa/simple/ipa_context.h      | 1 +
>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
>>> index 189de770..2f7e040c 100644
>>> --- a/src/ipa/simple/algorithms/agc.cpp
>>> +++ b/src/ipa/simple/algorithms/agc.cpp
>>> @@ -100,7 +100,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>>>  }
>>>  
>>>  void Agc::process(IPAContext &context,
>>> -		  const uint32_t frame,
>>> +		  [[maybe_unused]] const uint32_t frame,
>>>  		  IPAFrameContext &frameContext,
>>>  		  const SwIspStats *stats,
>>>  		  ControlList &metadata)
>>> @@ -110,13 +110,14 @@ void Agc::process(IPAContext &context,
>>>  	metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
>>>  	metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
>>>  
>>> -	if (frame == 0) {
>>> +	if (!context.activeState.agc.valid) {
>>>  		/*
>>>  		 * Init active-state from sensor values in case updateExposure()
>>>  		 * does not run for the first frame.
>>>  		 */
>>>  		context.activeState.agc.exposure = frameContext.sensor.exposure;
>>>  		context.activeState.agc.again = frameContext.sensor.gain;
>>> +		context.activeState.agc.valid = true;
>>>  	}
>>>  
>>>  	if (!stats->valid) {
>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>>> index c3081e30..26b60fb6 100644
>>> --- a/src/ipa/simple/ipa_context.h
>>> +++ b/src/ipa/simple/ipa_context.h
>>> @@ -40,6 +40,7 @@ struct IPAActiveState {
>>>  	struct {
>>>  		int32_t exposure;
>>>  		double again;
>>> +		bool valid;
>>>  	} agc;
>>>  
>>>  	struct {
>
Kieran Bingham Jan. 5, 2026, 7:31 a.m. UTC | #4 
Quoting Hans de Goede (2026-01-03 12:10:32)
> Hi,
> 
> On 2-Jan-26 11:41, Milan Zamazal wrote:
> > Milan Zamazal <mzamazal@redhat.com> writes:
> > 
> >> Hi Hans,
> >>
> >> thank you for the patch.
> >>
> >> Hans de Goede <johannes.goede@oss.qualcomm.com> writes:
> >>
> >>> If the first frame of a stream is bad, the IPA will not get called with
> >>> frame == 0, leaving activeState.agc expo/again uninitialized. This causes
> >>> the agc algorithm to set a very low gain and exposure on the next run
> >>> (where it will hit the if (!stats->valid) {} path) resulting in starting
> >>> with a black image.
> >>>
> >>> Fix this by using a valid flag instead of checking for frame == 0.
> >>>
> >>> The entire activeState gets cleared to 0 on configure() resetting the new
> >>> valid flag.
> >>>
> >>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> > 
> > I think it should fix
> > https://gitlab.freedesktop.org/camera/libcamera/-/issues/298
> > ?
> 
> Yes it fixes that. I've already added the fix to the Fedora libcamera
> pkgs and Fedora users with the same laptop as from that bug have
> confirmed it fixes the oscilation.

Lets add the tag then.

Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/298

I don't know if patchwork will pick that up as a tag automatically
though.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Regards,
> 
> Hans
> 
> 
> > 
> >> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> >>
> >>> ---
> >>>  src/ipa/simple/algorithms/agc.cpp | 5 +++--
> >>>  src/ipa/simple/ipa_context.h      | 1 +
> >>>  2 files changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
> >>> index 189de770..2f7e040c 100644
> >>> --- a/src/ipa/simple/algorithms/agc.cpp
> >>> +++ b/src/ipa/simple/algorithms/agc.cpp
> >>> @@ -100,7 +100,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
> >>>  }
> >>>  
> >>>  void Agc::process(IPAContext &context,
> >>> -             const uint32_t frame,
> >>> +             [[maybe_unused]] const uint32_t frame,
> >>>               IPAFrameContext &frameContext,
> >>>               const SwIspStats *stats,
> >>>               ControlList &metadata)
> >>> @@ -110,13 +110,14 @@ void Agc::process(IPAContext &context,
> >>>     metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> >>>     metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> >>>  
> >>> -   if (frame == 0) {
> >>> +   if (!context.activeState.agc.valid) {
> >>>             /*
> >>>              * Init active-state from sensor values in case updateExposure()
> >>>              * does not run for the first frame.
> >>>              */
> >>>             context.activeState.agc.exposure = frameContext.sensor.exposure;
> >>>             context.activeState.agc.again = frameContext.sensor.gain;
> >>> +           context.activeState.agc.valid = true;
> >>>     }
> >>>  
> >>>     if (!stats->valid) {
> >>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> >>> index c3081e30..26b60fb6 100644
> >>> --- a/src/ipa/simple/ipa_context.h
> >>> +++ b/src/ipa/simple/ipa_context.h
> >>> @@ -40,6 +40,7 @@ struct IPAActiveState {
> >>>     struct {
> >>>             int32_t exposure;
> >>>             double again;
> >>> +           bool valid;
> >>>     } agc;
> >>>  
> >>>     struct {
> > 
>

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
index 189de770..2f7e040c 100644
--- a/src/ipa/simple/algorithms/agc.cpp
+++ b/src/ipa/simple/algorithms/agc.cpp
@@ -100,7 +100,7 @@  void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
 }
 
 void Agc::process(IPAContext &context,
-		  const uint32_t frame,
+		  [[maybe_unused]] const uint32_t frame,
 		  IPAFrameContext &frameContext,
 		  const SwIspStats *stats,
 		  ControlList &metadata)
@@ -110,13 +110,14 @@  void Agc::process(IPAContext &context,
 	metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
 	metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
 
-	if (frame == 0) {
+	if (!context.activeState.agc.valid) {
 		/*
 		 * Init active-state from sensor values in case updateExposure()
 		 * does not run for the first frame.
 		 */
 		context.activeState.agc.exposure = frameContext.sensor.exposure;
 		context.activeState.agc.again = frameContext.sensor.gain;
+		context.activeState.agc.valid = true;
 	}
 
 	if (!stats->valid) {
diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
index c3081e30..26b60fb6 100644
--- a/src/ipa/simple/ipa_context.h
+++ b/src/ipa/simple/ipa_context.h
@@ -40,6 +40,7 @@  struct IPAActiveState {
 	struct {
 		int32_t exposure;
 		double again;
+		bool valid;
 	} agc;
 
 	struct {