[v7,3/5] pipeline: simple: Enable frame start events

23112 diff mbox series

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.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> # v6
Co-developed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 49 +++++++++++++++++++++---
 1 file changed, 43 insertions(+), 6 deletions(-)

Quoting Stanislaw Gruszka (2025-04-03 08:45:49)
> 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.
> 
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> # v6
> Co-developed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 49 +++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 06e805d89caa..c97904076b63 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -327,6 +327,7 @@ public:
>         std::list<Entity> entities_;
>         std::unique_ptr<CameraSensor> sensor_;
>         V4L2VideoDevice *video_;
> +       V4L2Subdevice *frameStartEmitter_;
>  
>         std::vector<Configuration> configs_;
>         std::map<PixelFormat, std::vector<const Configuration *>> formats_;
> @@ -633,6 +634,20 @@ int SimpleCameraData::init()
>  
>         properties_ = sensor_->properties();
>  
> +       /* Find the first subdev that can generate a frame start signal, if any. */
> +       frameStartEmitter_ = nullptr;
> +       for (const Entity &entity : entities_) {
> +               V4L2Subdevice *sd = pipe->subdev(entity.entity);
> +               if (!sd || !sd->supportsFrameStartEvent())
> +                       continue;
> +
> +               LOG(SimplePipeline, Debug)
> +                       << "Using frameStart signal from '"
> +                       << entity.entity->name() << "'";
> +               frameStartEmitter_ = sd;
> +               break;
> +       }
> +
>         return 0;
>  }
>  
> @@ -983,8 +998,18 @@ void SimpleCameraData::metadataReady(uint32_t frame, const ControlList &metadata
>  void SimpleCameraData::setSensorControls(const ControlList &sensorControls)
>  {
>         delayedCtrls_->push(sensorControls);
> -       ControlList ctrls(sensorControls);
> -       sensor_->setControls(&ctrls);
> +       /*
> +         * Directly apply controls now if there is no frameStart signal.

Smallest nit on whitespace here that I'll fix up when applying.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +        *
> +        * \todo Applying controls directly not only increases the risk of
> +        * applying them to the wrong frame (or across a frame boundary),
> +        * but it also bypasses delayedCtrls_, creating AGC regulation issues.
> +        * Both problems should be fixed.
> +        */
> +       if (!frameStartEmitter_) {
> +               ControlList ctrls(sensorControls);
> +               sensor_->setControls(&ctrls);
> +       }
>  }
>  
>  /* Retrieve all source pads connected to a sink pad through active routes. */
> @@ -1409,6 +1434,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);
> @@ -1438,8 +1464,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  
>         video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);
>  
> -       data->video_->frameStart.connect(data->delayedCtrls_.get(),
> -                                        &DelayedControls::applyControls);
> +       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) {
> @@ -1472,9 +1505,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  {
>         SimpleCameraData *data = cameraData(camera);
>         V4L2VideoDevice *video = data->video_;
> +       V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;
>  
> -       data->video_->frameStart.disconnect(data->delayedCtrls_.get(),
> -                                           &DelayedControls::applyControls);
> +       if (frameStartEmitter) {
> +               frameStartEmitter->setFrameStartEnabled(false);
> +               frameStartEmitter->frameStart.disconnect(data->delayedCtrls_.get(),
> +                                                        &DelayedControls::applyControls);
> +       }
>  
>         if (data->useConversion_) {
>                 if (data->converter_)
> -- 
> 2.43.0
>