[PATCH 1/2] libcamera: ipa_proxy: Report a missing configuration as a warning

[Milan Zamazal]

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-07-01 13:55:07)
>> Hi Kieran,
>> 
>
>> thank you for review.
>> 
>> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
>> 
>> > Quoting Milan Zamazal (2024-06-27 18:33:04)
>> >> When the configuration file for an IPA module is missing, it is reported
>> >> as an error in the log, for example:
>> >
>> >> 
>> >>   ERROR IPAProxy ipa_proxy.cpp:149 Configuration file 'imx219.yaml' not found for IPA module
>> >> 'simple'
>> >> 
>> >> This is misleading because several pipelines use uncalibrated.yaml in
>> >> such a case and can continue working.  And in case of software ISP,
>> >> there is currently no other configuration file so the error is always
>> >> reported.
>> >
>> > I'm in two minds for this. At the moment, the SoftISP doesn't use a
>> > tuning file because it's not implemented.
>> >
>> > But I /would/ expect there to be one in the future.
>> 
>> Yes.
>> 
>> > I believe we report it as an error as without a tuning file, we expect
>> > there will always be a degraded image quality.
>> >
>> > But ... indeed - it still continues. So what's the difference between an
>> > error and a warning ?
>> 
>> I cannot know better than you what is a contingent libcamera policy, but
>> generally, I'd say error is something that prevents some important part
>> from working at all while warning is something that the user should pay
>> attention to but the action can still be performed, although perhaps in
>> a degraded or unintended way.
>> 
>> In this specific case I have experienced the following interaction with
>> a user:
>> 
>> - "Hmm, it doesn't work."
>> - "Sure, it reports an error!"
>> - "But this one is harmless."
>> - "Are you sure?  It clearly reports that it is an error."
>> - "Yes, it always prints this error, it's not the problem."
>> - "Really?  Hmm, OK."
>> 
>> I think that at least with software ISP, it shouldn't be reported as an
>> error.  But I don't insist on this fix, another obvious fix might be
>> implementing tuning files in software ISP, or perhaps some workaround.
>> What would you suggest?
>
> I too have had to tell people "Oh ... yeah you can ignore that error" (I
> can't remember if it was this or another case" ... and I think if
> something can be ignored ... is it really an error?
>
> The tricky part here is ... on RPi - it really is considered an error
> and the camera will not be constructed without a tuning file I believe,
> while on other platforms (at least for now) it is only a warning...
>
> I still don't know the right answer yet I'm afraid :S

OK, let's instruct IPAProxy::configurationFile what to do in such a
case.  See v2 where I add a fallback file argument, which looks a bit
better to me than just adding a boolean flag, although it may not be
actually necessarily better.  Let's try and discuss.

> --
> Kieran
>
>
>> 
>> >> Let's change the error to warning to not confuse users.
>> >> 
>> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> >> ---
>> >>  src/libcamera/ipa_proxy.cpp | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> 
>> >> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
>> >> index 6c17c456..494ed736 100644
>> >> --- a/src/libcamera/ipa_proxy.cpp
>> >> +++ b/src/libcamera/ipa_proxy.cpp
>> >> @@ -146,7 +146,7 @@ std::string IPAProxy::configurationFile(const std::string &name) const
>> >>                 }
>> >>         }
>> >>  
>> >> -       LOG(IPAProxy, Error)
>> >> +       LOG(IPAProxy, Warning)
>> >>                 << "Configuration file '" << name
>> >>                 << "' not found for IPA module '" << ipaName << "'";
>> >>  
>> >> -- 
>> >> 2.44.1
>> >>
>>