[PATCH v5 3/5] pipeline: simple: Connect and disconnect start frame events
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Mar 2 02:37:26 CET 2025
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 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'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_)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list