| Message ID | 20260703153819.1088752-2-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2026-07-03 16:38:03) > Check existence before the values are retrieved. > > Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 > Fixes: fb8ad13dc3e3 ("libcamera: software_isp: Move exposure+gain to an algorithm module") > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/simple/soft_simple.cpp | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index 629e1a32de..2a41b5bac8 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -299,6 +299,13 @@ void IPASoftSimple::processStats(const uint32_t frame, > [[maybe_unused]] const uint32_t bufferId, > const ControlList &sensorControls) > { > + /* Sanity check */ > + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || > + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { > + LOG(IPASoft, Error) << "Control(s) missing"; > + return; > + } > + > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > frameContext.sensor.exposure = As the access to obtain the exposure and gain are right here, indeed removing from below looks like it makes sense, but if we can't process - then in fact I think this check/enforcement should move to the init or configure functions and not run on every frame. And looking at IPASoftSimple::init if (sensorControls.find(V4L2_CID_EXPOSURE) == sensorControls.end()) { LOG(IPASoft, Error) << "Don't have exposure control"; return -EINVAL; } if (sensorControls.find(V4L2_CID_ANALOGUE_GAIN) == sensorControls.end()) { LOG(IPASoft, Error) << "Don't have gain control"; return -EINVAL; } I think we can just remove the sanity check rather than move it. -- Kieran > @@ -311,13 +318,6 @@ void IPASoftSimple::processStats(const uint32_t frame, > algo->process(context_, frame, frameContext, stats_, metadata); > metadataReady.emit(frame, metadata); > > - /* Sanity check */ > - if (!sensorControls.contains(V4L2_CID_EXPOSURE) || > - !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { > - LOG(IPASoft, Error) << "Control(s) missing"; > - return; > - } > - > ControlList ctrls(sensorInfoMap_); > > auto &againNew = frameContext.sensor.gain; > -- > 2.54.0 >
2026. 07. 04. 6:45 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2026-07-03 16:38:03) >> Check existence before the values are retrieved. >> >> Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 >> Fixes: fb8ad13dc3e3 ("libcamera: software_isp: Move exposure+gain to an algorithm module") >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/ipa/simple/soft_simple.cpp | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index 629e1a32de..2a41b5bac8 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -299,6 +299,13 @@ void IPASoftSimple::processStats(const uint32_t frame, >> [[maybe_unused]] const uint32_t bufferId, >> const ControlList &sensorControls) >> { >> + /* Sanity check */ >> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >> + LOG(IPASoft, Error) << "Control(s) missing"; >> + return; >> + } >> + >> IPAFrameContext &frameContext = context_.frameContexts.get(frame); >> >> frameContext.sensor.exposure = > > As the access to obtain the exposure and gain are right here, indeed > removing from below looks like it makes sense, but if we can't process - > then in fact I think this check/enforcement should move to the init or > configure functions and not run on every frame. > > And looking at IPASoftSimple::init > > > if (sensorControls.find(V4L2_CID_EXPOSURE) == sensorControls.end()) { > LOG(IPASoft, Error) << "Don't have exposure control"; > return -EINVAL; > } > > if (sensorControls.find(V4L2_CID_ANALOGUE_GAIN) == sensorControls.end()) { > LOG(IPASoft, Error) << "Don't have gain control"; > return -EINVAL; > } > > I think we can just remove the sanity check rather than move it. I suppose that's true, and ideally it should be so, but the presence of https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 suggests that this part can still run with an "incorrect" control list. So what should be done? > > -- > Kieran > > > > >> @@ -311,13 +318,6 @@ void IPASoftSimple::processStats(const uint32_t frame, >> algo->process(context_, frame, frameContext, stats_, metadata); >> metadataReady.emit(frame, metadata); >> >> - /* Sanity check */ >> - if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >> - !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >> - LOG(IPASoft, Error) << "Control(s) missing"; >> - return; >> - } >> - >> ControlList ctrls(sensorInfoMap_); >> >> auto &againNew = frameContext.sensor.gain; >> -- >> 2.54.0 >>
Hi Barnabás On Wed, Jul 15, 2026 at 10:55:21AM +0200, Barnabás Pőcze wrote: > 2026. 07. 04. 6:45 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2026-07-03 16:38:03) > > > Check existence before the values are retrieved. > > > > > > Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 > > > Fixes: fb8ad13dc3e3 ("libcamera: software_isp: Move exposure+gain to an algorithm module") > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > src/ipa/simple/soft_simple.cpp | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > > > index 629e1a32de..2a41b5bac8 100644 > > > --- a/src/ipa/simple/soft_simple.cpp > > > +++ b/src/ipa/simple/soft_simple.cpp > > > @@ -299,6 +299,13 @@ void IPASoftSimple::processStats(const uint32_t frame, > > > [[maybe_unused]] const uint32_t bufferId, > > > const ControlList &sensorControls) > > > { > > > + /* Sanity check */ > > > + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || > > > + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { > > > + LOG(IPASoft, Error) << "Control(s) missing"; > > > + return; > > > + } > > > + > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > frameContext.sensor.exposure = > > > > As the access to obtain the exposure and gain are right here, indeed > > removing from below looks like it makes sense, but if we can't process - > > then in fact I think this check/enforcement should move to the init or > > configure functions and not run on every frame. > > > > And looking at IPASoftSimple::init > > > > > > if (sensorControls.find(V4L2_CID_EXPOSURE) == sensorControls.end()) { > > LOG(IPASoft, Error) << "Don't have exposure control"; > > return -EINVAL; > > } > > > > if (sensorControls.find(V4L2_CID_ANALOGUE_GAIN) == sensorControls.end()) { > > LOG(IPASoft, Error) << "Don't have gain control"; > > return -EINVAL; > > } > > > > I think we can just remove the sanity check rather than move it. > > I suppose that's true, and ideally it should be so, but the presence of > https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 suggests > that this part can still run with an "incorrect" control list. So what > should be done? If I read #241 right: This can happen when some frames are dropped due to CSI-2 bus errors at the beginning of streaming. The index evaluated from frame number can exceed number of entries queued already to values_ ControlRingBuffer. At the beginning of streaming the ControlRingBuffer is not yet filled. Isn't the problem due to a missing FrameContext rather than the controls ? > > > > > > -- > > Kieran > > > > > > > > > > > @@ -311,13 +318,6 @@ void IPASoftSimple::processStats(const uint32_t frame, > > > algo->process(context_, frame, frameContext, stats_, metadata); > > > metadataReady.emit(frame, metadata); > > > - /* Sanity check */ > > > - if (!sensorControls.contains(V4L2_CID_EXPOSURE) || > > > - !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { > > > - LOG(IPASoft, Error) << "Control(s) missing"; > > > - return; > > > - } > > > - > > > ControlList ctrls(sensorInfoMap_); > > > auto &againNew = frameContext.sensor.gain; > > > -- > > > 2.54.0 > > > >
2026. 07. 15. 17:14 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Wed, Jul 15, 2026 at 10:55:21AM +0200, Barnabás Pőcze wrote: >> 2026. 07. 04. 6:45 keltezéssel, Kieran Bingham írta: >>> Quoting Barnabás Pőcze (2026-07-03 16:38:03) >>>> Check existence before the values are retrieved. >>>> >>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 >>>> Fixes: fb8ad13dc3e3 ("libcamera: software_isp: Move exposure+gain to an algorithm module") >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> src/ipa/simple/soft_simple.cpp | 14 +++++++------- >>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >>>> index 629e1a32de..2a41b5bac8 100644 >>>> --- a/src/ipa/simple/soft_simple.cpp >>>> +++ b/src/ipa/simple/soft_simple.cpp >>>> @@ -299,6 +299,13 @@ void IPASoftSimple::processStats(const uint32_t frame, >>>> [[maybe_unused]] const uint32_t bufferId, >>>> const ControlList &sensorControls) >>>> { >>>> + /* Sanity check */ >>>> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >>>> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >>>> + LOG(IPASoft, Error) << "Control(s) missing"; >>>> + return; >>>> + } >>>> + >>>> IPAFrameContext &frameContext = context_.frameContexts.get(frame); >>>> frameContext.sensor.exposure = >>> >>> As the access to obtain the exposure and gain are right here, indeed >>> removing from below looks like it makes sense, but if we can't process - >>> then in fact I think this check/enforcement should move to the init or >>> configure functions and not run on every frame. >>> >>> And looking at IPASoftSimple::init >>> >>> >>> if (sensorControls.find(V4L2_CID_EXPOSURE) == sensorControls.end()) { >>> LOG(IPASoft, Error) << "Don't have exposure control"; >>> return -EINVAL; >>> } >>> >>> if (sensorControls.find(V4L2_CID_ANALOGUE_GAIN) == sensorControls.end()) { >>> LOG(IPASoft, Error) << "Don't have gain control"; >>> return -EINVAL; >>> } >>> >>> I think we can just remove the sanity check rather than move it. >> >> I suppose that's true, and ideally it should be so, but the presence of >> https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 suggests >> that this part can still run with an "incorrect" control list. So what >> should be done? > > If I read #241 right: > > This can happen when some frames are dropped due to CSI-2 bus errors > at the beginning of streaming. The index evaluated from frame number > can exceed number of entries queued already to values_ > ControlRingBuffer. At the beginning of streaming the ControlRingBuffer > is not yet filled. > > Isn't the problem due to a missing FrameContext rather than the > controls ? I think so. The question is: do we want to have the workaround or not? Currently the workaround is there, but non-functional. This patch makes it functional. But it could very well be removed. > >> >> >>> >>> -- >>> Kieran >>> >>> >>> >>> >>>> @@ -311,13 +318,6 @@ void IPASoftSimple::processStats(const uint32_t frame, >>>> algo->process(context_, frame, frameContext, stats_, metadata); >>>> metadataReady.emit(frame, metadata); >>>> - /* Sanity check */ >>>> - if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >>>> - !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >>>> - LOG(IPASoft, Error) << "Control(s) missing"; >>>> - return; >>>> - } >>>> - >>>> ControlList ctrls(sensorInfoMap_); >>>> auto &againNew = frameContext.sensor.gain; >>>> -- >>>> 2.54.0 >>>> >>
Hi, On 15-Jul-26 17:16, Barnabás Pőcze wrote: > 2026. 07. 15. 17:14 keltezéssel, Jacopo Mondi írta: >> Hi Barnabás >> >> On Wed, Jul 15, 2026 at 10:55:21AM +0200, Barnabás Pőcze wrote: >>> 2026. 07. 04. 6:45 keltezéssel, Kieran Bingham írta: >>>> Quoting Barnabás Pőcze (2026-07-03 16:38:03) >>>>> Check existence before the values are retrieved. >>>>> >>>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 >>>>> Fixes: fb8ad13dc3e3 ("libcamera: software_isp: Move exposure+gain to an algorithm module") >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>> --- >>>>> src/ipa/simple/soft_simple.cpp | 14 +++++++------- >>>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >>>>> index 629e1a32de..2a41b5bac8 100644 >>>>> --- a/src/ipa/simple/soft_simple.cpp >>>>> +++ b/src/ipa/simple/soft_simple.cpp >>>>> @@ -299,6 +299,13 @@ void IPASoftSimple::processStats(const uint32_t frame, >>>>> [[maybe_unused]] const uint32_t bufferId, >>>>> const ControlList &sensorControls) >>>>> { >>>>> + /* Sanity check */ >>>>> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >>>>> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >>>>> + LOG(IPASoft, Error) << "Control(s) missing"; >>>>> + return; >>>>> + } >>>>> + >>>>> IPAFrameContext &frameContext = context_.frameContexts.get(frame); >>>>> frameContext.sensor.exposure = >>>> >>>> As the access to obtain the exposure and gain are right here, indeed >>>> removing from below looks like it makes sense, but if we can't process - >>>> then in fact I think this check/enforcement should move to the init or >>>> configure functions and not run on every frame. >>>> >>>> And looking at IPASoftSimple::init >>>> >>>> >>>> if (sensorControls.find(V4L2_CID_EXPOSURE) == sensorControls.end()) { >>>> LOG(IPASoft, Error) << "Don't have exposure control"; >>>> return -EINVAL; >>>> } >>>> >>>> if (sensorControls.find(V4L2_CID_ANALOGUE_GAIN) == sensorControls.end()) { >>>> LOG(IPASoft, Error) << "Don't have gain control"; >>>> return -EINVAL; >>>> } >>>> >>>> I think we can just remove the sanity check rather than move it. >>> >>> I suppose that's true, and ideally it should be so, but the presence of >>> https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 suggests >>> that this part can still run with an "incorrect" control list. So what >>> should be done? >> >> If I read #241 right: >> >> This can happen when some frames are dropped due to CSI-2 bus errors >> at the beginning of streaming. The index evaluated from frame number >> can exceed number of entries queued already to values_ >> ControlRingBuffer. At the beginning of streaming the ControlRingBuffer >> is not yet filled. >> >> Isn't the problem due to a missing FrameContext rather than the >> controls ? > > I think so. The question is: do we want to have the workaround or not? > Currently the workaround is there, but non-functional. This patch makes > it functional. But it could very well be removed. So I've been hitting the #241 issue with camss + softISP on the Arduino Uno Q, but not due to CSI errors but due the CPU not keeping up and sometimes frames already getting dropped before the control ringbuffer being fully filled. I also tried moving the check up as this patch does, but it does not help for #241. I did some debugging and IIRC the problem is the ringbuffer contains empty controls, so we don't get a nullptr ControlValue for e.g. gain, but rather an empty ControlValue and then we hit the first assert in include/libcamera/controls.h: T get() const { assert(type_ == details::control_type<std::remove_cv_t<T>>::value); assert(!isArray_); return *reinterpret_cast<const T *>(data().data()); } so since it seems we do actually get a gain control pointer, the check does not help. The problem is we get an empty ControlValue leading to hitting this assert. I think that the fix is to change the delayed_controls code to instead of only init the first entry in the ringbuffer, to just init all entries with the value currently used to only init the first entry. And then the check discussed here can indeed be completely dropped. Or alternatively the check should be changed to check for the ControlValue not being empty. Regards, Hans > > >> >>> >>> >>>> >>>> -- >>>> Kieran >>>> >>>> >>>> >>>> >>>>> @@ -311,13 +318,6 @@ void IPASoftSimple::processStats(const uint32_t frame, >>>>> algo->process(context_, frame, frameContext, stats_, metadata); >>>>> metadataReady.emit(frame, metadata); >>>>> - /* Sanity check */ >>>>> - if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >>>>> - !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >>>>> - LOG(IPASoft, Error) << "Control(s) missing"; >>>>> - return; >>>>> - } >>>>> - >>>>> ControlList ctrls(sensorInfoMap_); >>>>> auto &againNew = frameContext.sensor.gain; >>>>> -- >>>>> 2.54.0 >>>>> >>> >
2026. 07. 16. 16:23 keltezéssel, johannes.goede@oss.qualcomm.com írta: > Hi, > > On 15-Jul-26 17:16, Barnabás Pőcze wrote: >> 2026. 07. 15. 17:14 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Wed, Jul 15, 2026 at 10:55:21AM +0200, Barnabás Pőcze wrote: >>>> 2026. 07. 04. 6:45 keltezéssel, Kieran Bingham írta: >>>>> Quoting Barnabás Pőcze (2026-07-03 16:38:03) >>>>>> Check existence before the values are retrieved. >>>>>> >>>>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 >>>>>> Fixes: fb8ad13dc3e3 ("libcamera: software_isp: Move exposure+gain to an algorithm module") >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>> --- >>>>>> src/ipa/simple/soft_simple.cpp | 14 +++++++------- >>>>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >>>>>> index 629e1a32de..2a41b5bac8 100644 >>>>>> --- a/src/ipa/simple/soft_simple.cpp >>>>>> +++ b/src/ipa/simple/soft_simple.cpp >>>>>> @@ -299,6 +299,13 @@ void IPASoftSimple::processStats(const uint32_t frame, >>>>>> [[maybe_unused]] const uint32_t bufferId, >>>>>> const ControlList &sensorControls) >>>>>> { >>>>>> + /* Sanity check */ >>>>>> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >>>>>> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >>>>>> + LOG(IPASoft, Error) << "Control(s) missing"; >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> IPAFrameContext &frameContext = context_.frameContexts.get(frame); >>>>>> frameContext.sensor.exposure = >>>>> >>>>> As the access to obtain the exposure and gain are right here, indeed >>>>> removing from below looks like it makes sense, but if we can't process - >>>>> then in fact I think this check/enforcement should move to the init or >>>>> configure functions and not run on every frame. >>>>> >>>>> And looking at IPASoftSimple::init >>>>> >>>>> >>>>> if (sensorControls.find(V4L2_CID_EXPOSURE) == sensorControls.end()) { >>>>> LOG(IPASoft, Error) << "Don't have exposure control"; >>>>> return -EINVAL; >>>>> } >>>>> >>>>> if (sensorControls.find(V4L2_CID_ANALOGUE_GAIN) == sensorControls.end()) { >>>>> LOG(IPASoft, Error) << "Don't have gain control"; >>>>> return -EINVAL; >>>>> } >>>>> >>>>> I think we can just remove the sanity check rather than move it. >>>> >>>> I suppose that's true, and ideally it should be so, but the presence of >>>> https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 suggests >>>> that this part can still run with an "incorrect" control list. So what >>>> should be done? >>> >>> If I read #241 right: >>> >>> This can happen when some frames are dropped due to CSI-2 bus errors >>> at the beginning of streaming. The index evaluated from frame number >>> can exceed number of entries queued already to values_ >>> ControlRingBuffer. At the beginning of streaming the ControlRingBuffer >>> is not yet filled. >>> >>> Isn't the problem due to a missing FrameContext rather than the >>> controls ? >> >> I think so. The question is: do we want to have the workaround or not? >> Currently the workaround is there, but non-functional. This patch makes >> it functional. But it could very well be removed. > > So I've been hitting the #241 issue with camss + softISP on the Arduino > Uno Q, but not due to CSI errors but due the CPU not keeping up and > sometimes frames already getting dropped before the control ringbuffer > being fully filled. > > I also tried moving the check up as this patch does, but it does not > help for #241. I did some debugging and IIRC the problem is the ringbuffer > contains empty controls, so we don't get a nullptr ControlValue for e.g. > gain, but rather an empty ControlValue and then we hit the first assert in > include/libcamera/controls.h: > > T get() const > { > assert(type_ == details::control_type<std::remove_cv_t<T>>::value); > assert(!isArray_); > > return *reinterpret_cast<const T *>(data().data()); > } > > so since it seems we do actually get a gain control pointer, the check > does not help. The problem is we get an empty ControlValue leading to > hitting this assert. Ahh, of course, you're right. I should have taken better look at the gdb output in the above issue description. Arguably `DelayedControls::get()` could skip adding "none" control values to the result. But indeed this check regardless of where it is, so it should be removed. > > I think that the fix is to change the delayed_controls code to instead > of only init the first entry in the ringbuffer, to just init all entries > with the value currently used to only init the first entry. > > And then the check discussed here can indeed be completely dropped. > > Or alternatively the check should be changed to check for the ControlValue > not being empty. > > Regards, > > Hans > > > > >> >> >>> >>>> >>>> >>>>> >>>>> -- >>>>> Kieran >>>>> >>>>> >>>>> >>>>> >>>>>> @@ -311,13 +318,6 @@ void IPASoftSimple::processStats(const uint32_t frame, >>>>>> algo->process(context_, frame, frameContext, stats_, metadata); >>>>>> metadataReady.emit(frame, metadata); >>>>>> - /* Sanity check */ >>>>>> - if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >>>>>> - !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >>>>>> - LOG(IPASoft, Error) << "Control(s) missing"; >>>>>> - return; >>>>>> - } >>>>>> - >>>>>> ControlList ctrls(sensorInfoMap_); >>>>>> auto &againNew = frameContext.sensor.gain; >>>>>> -- >>>>>> 2.54.0 >>>>>> >>>> >> >
Hi, On 16-Jul-26 16:32, Barnabás Pőcze wrote: > 2026. 07. 16. 16:23 keltezéssel, johannes.goede@oss.qualcomm.com írta: >> Hi, >> >> On 15-Jul-26 17:16, Barnabás Pőcze wrote: >>> 2026. 07. 15. 17:14 keltezéssel, Jacopo Mondi írta: >>>> Hi Barnabás >>>> >>>> On Wed, Jul 15, 2026 at 10:55:21AM +0200, Barnabás Pőcze wrote: >>>>> 2026. 07. 04. 6:45 keltezéssel, Kieran Bingham írta: >>>>>> Quoting Barnabás Pőcze (2026-07-03 16:38:03) >>>>>>> Check existence before the values are retrieved. >>>>>>> >>>>>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 >>>>>>> Fixes: fb8ad13dc3e3 ("libcamera: software_isp: Move exposure+gain to an algorithm module") >>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>>> --- >>>>>>> src/ipa/simple/soft_simple.cpp | 14 +++++++------- >>>>>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >>>>>>> index 629e1a32de..2a41b5bac8 100644 >>>>>>> --- a/src/ipa/simple/soft_simple.cpp >>>>>>> +++ b/src/ipa/simple/soft_simple.cpp >>>>>>> @@ -299,6 +299,13 @@ void IPASoftSimple::processStats(const uint32_t frame, >>>>>>> [[maybe_unused]] const uint32_t bufferId, >>>>>>> const ControlList &sensorControls) >>>>>>> { >>>>>>> + /* Sanity check */ >>>>>>> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >>>>>>> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >>>>>>> + LOG(IPASoft, Error) << "Control(s) missing"; >>>>>>> + return; >>>>>>> + } >>>>>>> + >>>>>>> IPAFrameContext &frameContext = context_.frameContexts.get(frame); >>>>>>> frameContext.sensor.exposure = >>>>>> >>>>>> As the access to obtain the exposure and gain are right here, indeed >>>>>> removing from below looks like it makes sense, but if we can't process - >>>>>> then in fact I think this check/enforcement should move to the init or >>>>>> configure functions and not run on every frame. >>>>>> >>>>>> And looking at IPASoftSimple::init >>>>>> >>>>>> >>>>>> if (sensorControls.find(V4L2_CID_EXPOSURE) == sensorControls.end()) { >>>>>> LOG(IPASoft, Error) << "Don't have exposure control"; >>>>>> return -EINVAL; >>>>>> } >>>>>> >>>>>> if (sensorControls.find(V4L2_CID_ANALOGUE_GAIN) == sensorControls.end()) { >>>>>> LOG(IPASoft, Error) << "Don't have gain control"; >>>>>> return -EINVAL; >>>>>> } >>>>>> >>>>>> I think we can just remove the sanity check rather than move it. >>>>> >>>>> I suppose that's true, and ideally it should be so, but the presence of >>>>> https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 suggests >>>>> that this part can still run with an "incorrect" control list. So what >>>>> should be done? >>>> >>>> If I read #241 right: >>>> >>>> This can happen when some frames are dropped due to CSI-2 bus errors >>>> at the beginning of streaming. The index evaluated from frame number >>>> can exceed number of entries queued already to values_ >>>> ControlRingBuffer. At the beginning of streaming the ControlRingBuffer >>>> is not yet filled. >>>> >>>> Isn't the problem due to a missing FrameContext rather than the >>>> controls ? >>> >>> I think so. The question is: do we want to have the workaround or not? >>> Currently the workaround is there, but non-functional. This patch makes >>> it functional. But it could very well be removed. >> >> So I've been hitting the #241 issue with camss + softISP on the Arduino >> Uno Q, but not due to CSI errors but due the CPU not keeping up and >> sometimes frames already getting dropped before the control ringbuffer >> being fully filled. >> >> I also tried moving the check up as this patch does, but it does not >> help for #241. I did some debugging and IIRC the problem is the ringbuffer >> contains empty controls, so we don't get a nullptr ControlValue for e.g. >> gain, but rather an empty ControlValue and then we hit the first assert in >> include/libcamera/controls.h: >> >> T get() const >> { >> assert(type_ == details::control_type<std::remove_cv_t<T>>::value); >> assert(!isArray_); >> >> return *reinterpret_cast<const T *>(data().data()); >> } >> >> so since it seems we do actually get a gain control pointer, the check >> does not help. The problem is we get an empty ControlValue leading to >> hitting this assert. > > Ahh, of course, you're right. I should have taken better look at the gdb output > in the above issue description. > > Arguably `DelayedControls::get()` could skip adding "none" control values > to the result. But indeed this check regardless of where it is, so it should be removed. Right, we still need to fix #241 though, since that is a real problem which people are actually hitting. So instead of dropping the check maybe make it check that the type is not "none" (and move it up) ? That should fix #241 + print a message hinting at things not keeping up. Alternatively we could make sure the entire ring-buffer is pre-filled with valid values. Note generally speaking this problem may lead to too old gain / expo values getting used, so I wonder if need to store the actual frame-id together with the control values and return an empty set rather then old values? Regards, Hans > > >> >> I think that the fix is to change the delayed_controls code to instead >> of only init the first entry in the ringbuffer, to just init all entries >> with the value currently used to only init the first entry. >> >> And then the check discussed here can indeed be completely dropped. >> >> Or alternatively the check should be changed to check for the ControlValue >> not being empty. >> >> Regards, >> >> Hans >> >> >> >> >>> >>> >>>> >>>>> >>>>> >>>>>> >>>>>> -- >>>>>> Kieran >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> @@ -311,13 +318,6 @@ void IPASoftSimple::processStats(const uint32_t frame, >>>>>>> algo->process(context_, frame, frameContext, stats_, metadata); >>>>>>> metadataReady.emit(frame, metadata); >>>>>>> - /* Sanity check */ >>>>>>> - if (!sensorControls.contains(V4L2_CID_EXPOSURE) || >>>>>>> - !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { >>>>>>> - LOG(IPASoft, Error) << "Control(s) missing"; >>>>>>> - return; >>>>>>> - } >>>>>>> - >>>>>>> ControlList ctrls(sensorInfoMap_); >>>>>>> auto &againNew = frameContext.sensor.gain; >>>>>>> -- >>>>>>> 2.54.0 >>>>>>> >>>>> >>> >> >
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index 629e1a32de..2a41b5bac8 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -299,6 +299,13 @@ void IPASoftSimple::processStats(const uint32_t frame, [[maybe_unused]] const uint32_t bufferId, const ControlList &sensorControls) { + /* Sanity check */ + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { + LOG(IPASoft, Error) << "Control(s) missing"; + return; + } + IPAFrameContext &frameContext = context_.frameContexts.get(frame); frameContext.sensor.exposure = @@ -311,13 +318,6 @@ void IPASoftSimple::processStats(const uint32_t frame, algo->process(context_, frame, frameContext, stats_, metadata); metadataReady.emit(frame, metadata); - /* Sanity check */ - if (!sensorControls.contains(V4L2_CID_EXPOSURE) || - !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { - LOG(IPASoft, Error) << "Control(s) missing"; - return; - } - ControlList ctrls(sensorInfoMap_); auto &againNew = frameContext.sensor.gain;
Check existence before the values are retrieved. Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/241 Fixes: fb8ad13dc3e3 ("libcamera: software_isp: Move exposure+gain to an algorithm module") Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/simple/soft_simple.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)