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

[Milan Zamazal]

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?

>> 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>();