| Message ID | 20250225164116.414301-4-stanislaw.gruszka@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 ? > > 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. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241 > Co-developed-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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... > > 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. 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 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); > + if (ret) { > + stop(camera); > + return ret; > + } > + frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(), > + &DelayedControls::applyControls); > + } > + > ret = video->streamOn(); > if (ret < 0) { > stop(camera); > @@ -1401,6 +1409,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera) > { > SimpleCameraData *data = cameraData(camera); > V4L2VideoDevice *video = data->video_; > + V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_; > + > + if (frameStartEmitter) { > + frameStartEmitter->setFrameStartEnabled(false); > + frameStartEmitter->frameStart.disconnect(data->delayedCtrls_.get(), > + &DelayedControls::applyControls); > + } > > if (data->useConversion_) { > if (data->converter_) > -- > 2.43.0 >
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 > ? > > > 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@redhat.com> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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'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. > 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 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. -------- 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. > > + return ret; > > + } > > + frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(), > > + &DelayedControls::applyControls); > > + } > > + > > ret = video->streamOn(); > > if (ret < 0) { > > stop(camera); > > @@ -1401,6 +1409,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera) > > { > > SimpleCameraData *data = cameraData(camera); > > V4L2VideoDevice *video = data->video_; > > + V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_; > > + > > + if (frameStartEmitter) { > > + frameStartEmitter->setFrameStartEnabled(false); > > + frameStartEmitter->frameStart.disconnect(data->delayedCtrls_.get(), > > + &DelayedControls::applyControls); > > + } > > > > if (data->useConversion_) { > > if (data->converter_)
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@redhat.com> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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@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@redhat.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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
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); 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(); + if (frameStartEmitter) { + ret = frameStartEmitter->setFrameStartEnabled(true); + if (ret) { + stop(camera); + return ret; + } + frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(), + &DelayedControls::applyControls); + } + ret = video->streamOn(); if (ret < 0) { stop(camera); @@ -1401,6 +1409,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera) { SimpleCameraData *data = cameraData(camera); V4L2VideoDevice *video = data->video_; + V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_; + + if (frameStartEmitter) { + frameStartEmitter->setFrameStartEnabled(false); + frameStartEmitter->frameStart.disconnect(data->delayedCtrls_.get(), + &DelayedControls::applyControls); + } if (data->useConversion_) { if (data->converter_)