[PATCH v5 3/5] pipeline: simple: Connect and disconnect start frame events
Stanislaw Gruszka
stanislaw.gruszka at linux.intel.com
Tue Mar 4 15:36:35 CET 2025
Hi Kieran and Laurent,
On Sun, Mar 02, 2025 at 03:37:26AM +0200, Laurent Pinchart wrote:
> On Sat, Mar 01, 2025 at 11:39:13PM +0000, Kieran Bingham wrote:
> > Quoting Stanislaw Gruszka (2025-02-25 16:41:14)
> > > Enable frame events on the emitter device during
> > > SimplePipelineHandler::start(), move signal connection there
> > > from SimplePipelineHandler::configure().
> > >
> > > Also reset delayed controls on start() to prevent using
> > > stale values.
> >
> > Is that the main cause of https://bugs.libcamera.org/show_bug.cgi?id=241
> > ?
Is not. The issue is somewhat complex and require few different conditions
to happen:
- beginning of processing, i.e. withing less then first 16 frames,
(DelayedControl::ControlRingBuffer length) to have empty values
in ControlRingBuffer
- having frames come with not consecutive numbers (happen when there are CSI
bus errors)
- not handling or enabling start frame event
Fixing the last point will prevent the problem as we will fill proper frame_nr
entry in ControlRingBuffer and later DelayedControls::get() will get the right
value instead of None - what triggers the 241 assertion.
And yes, the issue will can still happen if we do not handle the event.
Actually I think having delayedCtrls_->reset() will make it more reproducible.
Though alternative is using old/stale ControlValues after every camera restart.
> > > Accordingly disable/disconnect on SimplePipelineHandler::stopDevice().
> > >
> > > This fixes the assertion like below:
> > >
> > > ../src/libcamera/delayed_controls.cpp:227:
> > > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):
> > > Assertion `info.type() != ControlTypeNone' failed
> > >
> > > which can happen rarely at the beginning of streaming when
> > > ControlRingBuffer is not yet filled and there are errors on CSI-2.
> > > In such rare scenario we can have call to DelayedControls::get()
> > > with frame number that exceed number of saved entries. Handing
> > > frame start signal assure we do DelayedConntrols::applyControls()
> > > with (possibly out of sequence) frame number and later call
> > > to DelayedControls::get() will get proper not-empty control entry.
>
> This doesn't explain why connecting the signal is moved to start() time.
> The commit message needs improvements.
>
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241
> > > Co-developed-by: Hans de Goede <hdegoede at redhat.com>
> > > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
> > > ---
> > > src/libcamera/pipeline/simple/simple.cpp | 23 +++++++++++++++++++----
> > > 1 file changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 063a098f..774c7824 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -1229,7 +1229,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > > static_cast<SimpleCameraConfiguration *>(c);
> > > SimpleCameraData *data = cameraData(camera);
> > > V4L2VideoDevice *video = data->video_;
> > > - V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;
> > > int ret;
> > >
> > > /*
> > > @@ -1300,9 +1299,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > > data->delayedCtrls_ =
> > > std::make_unique<DelayedControls>(data->sensor_->device(),
> > > params);
> > > - if (frameStartEmitter)
> > > - frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(),
> > > - &DelayedControls::applyControls);
> >
> > Ayee, that was definitely wrong before - reconnecting on every
> > reconfigure.
> >
> > I am really surprised we don't have a detection in Signal that reports
> > if we connect a same object/slot combination multiple times...
>
> We have such a check, and it doesn't trigger before the DelayedControls
> instance is also recreated at every configure().
>
> > >
> > > StreamConfiguration inputCfg;
> > > inputCfg.pixelFormat = pipeConfig->captureFormat;
> > > @@ -1341,6 +1337,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> > > {
> > > SimpleCameraData *data = cameraData(camera);
> > > V4L2VideoDevice *video = data->video_;
> > > + V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;
> > > int ret;
> > >
> > > const MediaPad *pad = acquirePipeline(data);
> > > @@ -1370,6 +1367,17 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> > >
> > > video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);
> > >
> > > + data->delayedCtrls_->reset();
> >
> > It's only one line, but it feels like this one line deserves it's own
> > patch at the moment.
>
> At the very least the reason should be explained in the commit message.
> Splitting this to a separate patch would ensure that.
I plan to post it as separate patch:
commit efa05db4b055051fdefcf81b0b3888594c31d9ee
Author: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
Date: Mon Mar 3 14:00:50 2025 +0100
pipeline: simple: Reset delayedCtrls at start.
Similar like in other pipelines (IPU3, rpi) avoid using stale
values of DelayedControls class when the same camera is started
second time.
Co-developed-by: Hans de Goede <hdegoede at redhat.com>
Signed-off-by: Hans de Goede <hdegoede at redhat.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 58aa3dba..659b1123 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -1376,6 +1376,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);
+ data->delayedCtrls_->reset();
if (frameStartEmitter) {
ret = frameStartEmitter->setFrameStartEnabled(true);
if (ret) {
> I'm also concerned about what happens when there is no frame start
> source. There should be no assertion failure in that case. If the
> reset() call fixes it, then that's great. Otherwise, if the issue gets
> fixed by actually using the frame start event, then we still have a
> problem.
Yes, the problem still could happen if there is no startFrameEmitter.
Do we expect pipelines with no startFrame event devices.
> > But maybe it's fine. I suspect that's the real
> > cause of some of the crashes? in particular #241 - and while handling
> > the frameStart correctly is good - I don't see that as the root cause of
> > the bug?
I tried to address this assertion directly
https://patchwork.libcamera.org/patch/22210/
And conclusion FWICT was that assertion did good job as it detected
real problem - not handling start frame event in the pipeline.
> > I could look the other way though as it's all about making sure the
> > controls are updated at the right timing.
> >
> > > + if (frameStartEmitter) {
> > > + ret = frameStartEmitter->setFrameStartEnabled(true);
>
> I'm surprised that we currently don't call setFrameStartEnabled(), I had
> missed that completely. This means this series won't be bisectable :-/
> On way to fix that would be to split patches differently. You could
> start with one patch that moves connection/disconnection of the signal
> (without calling setFrameStartEnable()). Here's a candidate for the
> commit message:
>
> --------
> pipeline: simple: Connect/disconnect frameStart signal at start/stop time
>
> The frameStart signal from the frame start emitter is connected in the
> configure() function, and is never disconnected. This means that each
> time the camera is configured a new connection is made, causing the
> DelayedControls::applyControls() to be called multiple times. Fix it by
> connecting and disconnecting the signal when starting and stopping the
> camera.
> --------
>
> You can then squash the addition of the setFrameStartEnabled() calls
> with patch 4/5.
>
> --------
> pipeline: simple: Enable frame start events
>
> The simple pipeline handler uses frame start events to apply sensor
> controls through the DelayedControls class. The setSensorControls()
> function applies the controls directly, which would result in controls
> being applied twice, if it wasn't for the fact that the pipeline handler
> forgot to enable the frame start events in the first place. Those two
> issues cancel each other, but cause controls to always be applied
> directly.
>
> Fix the issue by only applying controls directly in setSensorControls()
> if no frame start event emitter is available, and by enabling the frame
> start events in startDevice() otherwise. Disable them in stopDevice()
> for symmetry.
> --------
Thanks for those!
> If the reset() call is what fixes the assertion failure, it should go to
> a separate patch. Otherwise it can be combined with one of the other
> patches, with an appropriate explanation in the commit message.
>
> > > + if (ret) {
> > > + stop(camera);
>
> I'd be surprised if this worked fine. Calling stop() from start() on
> failure isn't right. At the very least I'd expect a call to stopDevice()
> instead, but I'm not even sure that would work fine. This isn't a new
> issue though, the stop() function is already called in start(), so I can
> ignore this for now. A fix would of course be nice, it can be done on
> top of this series.
I've checked that error path and did not notice problem.
Got "Failed to start capture" message and empty qcam window,
no crash or assertion.
Regards
Stanislaw
More information about the libcamera-devel
mailing list