| Message ID | 20240627173305.1477718-2-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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. 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 ? > > Let's change the error to warning to not confuse users. > > Signed-off-by: Milan Zamazal <mzamazal@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 >
Hi Kieran, thank you for review. Kieran Bingham <kieran.bingham@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? >> Let's change the error to warning to not confuse users. >> >> Signed-off-by: Milan Zamazal <mzamazal@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 >>
Quoting Milan Zamazal (2024-07-01 13:55:07) > Hi Kieran, > > thank you for review. > > Kieran Bingham <kieran.bingham@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 -- Kieran > > >> Let's change the error to warning to not confuse users. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@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 > >> >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2024-07-01 13:55:07) >> Hi Kieran, >> > >> thank you for review. >> >> Kieran Bingham <kieran.bingham@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@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 >> >> >>
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 << "'";
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. Let's change the error to warning to not confuse users. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/ipa_proxy.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)