[PATCH v5 3/5] pipeline: simple: Connect and disconnect start frame events

[Kieran Bingham]

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 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...

> 
>         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
>