| Message ID | 20241019184340.111785-1-robert.mader@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Robert, thank you for the patch. Robert Mader <robert.mader@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. > 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@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>();
Milan Zamazal <mzamazal@redhat.com> writes: > Hi Robert, > > thank you for the patch. > > Robert Mader <robert.mader@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@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>();
On 06.11.24 11:34, Milan Zamazal wrote: > Milan Zamazal <mzamazal@redhat.com> writes: > >> Hi Robert, >> >> thank you for the patch. >> >> Robert Mader <robert.mader@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@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>();
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>();
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. Clearing the activeState resulted in additional, arguably undesired churn, very noticable when switching back and forth between cameras. 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@collabora.com> --- src/ipa/simple/soft_simple.cpp | 2 -- 1 file changed, 2 deletions(-)