[PATCH 1/2] libcamera: software_isp: Stop clearing context config and state again

[Robert Mader]

On 06.11.24 11:34, Milan Zamazal wrote:
> Milan Zamazal <mzamazal at redhat.com> writes:
>
>> Hi Robert,
>>
>> thank you for the patch.
>>
>> Robert Mader <robert.mader at collabora.com> writes:
>>
>>> This partly reverts commit 41e3d61c, removing parts that had unintended
>>> side effects as the intention for the commit was purely to fix crashes.
>>>
>>> Clearing the configuration turned out to be problematic as some values such
>>> as configuration.black.level only get on initialization and thus were never
>>> used.
>> Oh, indeed, I must have done some mistake when testing this. :-(
>>
>> But rather than not clearing the configuration, a better approach should
>> be to store the black level from a tuning file to a class variable
>> rather than to the context.
> Hi Robert,
>
> do you plan to proceed with the patch or should I do something about it?

Hey Milan, I'd try to get back to it (and other pending patches) in the 
coming weeks -  specifically I wanted to check how this one relates to 
"libcamera: software_isp: Initialize exposure+gain before agc 
calculations" etc. But please feel free to pick it up!

>
>>> Clearing the activeState resulted in additional, arguably undesired churn,
>>> very noticable when switching back and forth between cameras.
>> Do you know what causes this?  Is it related to black level reset or
>> something else?
>>
>>> Whether this is desirable is AFAIK a matter of taste/policy and
>>> shouldn't have been done as part of a crash fix.
>>>
>>> Fixes: 41e3d61c ("libcamera: software_isp: Clear IPA context on configure and stop")
>>> Signed-off-by: Robert Mader <robert.mader at collabora.com>
>>> ---
>>>   src/ipa/simple/soft_simple.cpp | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>>> index c8ad55a2..065673dc 100644
>>> --- a/src/ipa/simple/soft_simple.cpp
>>> +++ b/src/ipa/simple/soft_simple.cpp
>>> @@ -185,8 +185,6 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>>   	const ControlInfo &gainInfo = sensorInfoMap_.find(V4L2_CID_ANALOGUE_GAIN)->second;
>>>   
>>>   	/* Clear the IPA context before the streaming session. */
>>> -	context_.configuration = {};
>>> -	context_.activeState = {};
>>>   	context_.frameContexts.clear();
>>>   
>>>   	context_.configuration.agc.exposureMin = exposureInfo.min().get<int32_t>();