[v5,4/5] pipeline: simple: Do not apply controls twice

22868 diff mbox series

Apply controls directly only when there is no start frame emitter
device.

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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Quoting Stanislaw Gruszka (2025-02-25 16:41:15)
> Apply controls directly only when there is no start frame emitter
> device.
> 
> 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>

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

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 774c7824..b92b738b 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -911,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>  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 */

I wonder if in the future we would generate some software start event
here, which will be timed. But I think just setting is fine to do for
now.

> +       if (!frameStartEmitter_) {
> +               ControlList ctrls(sensorControls);
> +               sensor_->setControls(&ctrls);
> +       }
>  }
>  
>  /* Retrieve all source pads connected to a sink pad through active routes. */
> -- 
> 2.43.0
>

On Sat, Mar 01, 2025 at 11:40:35PM +0000, Kieran Bingham wrote:
> Quoting Stanislaw Gruszka (2025-02-25 16:41:15)
> > Apply controls directly only when there is no start frame emitter
> > device.
> > 
> > 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>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 774c7824..b92b738b 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -911,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> >  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 */
> 
> I wonder if in the future we would generate some software start event
> here, which will be timed. But I think just setting is fine to do for
> now.

We clearly need to do better than this when there is no frame start
source, as setting controls here completely bypass delayed controls, and
will result in regulation issues. Let's add a comment about it.

	/*
	 * Directly apply controls now if there is no frameStart signal.
	 *
	 * \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.
	 */

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +       if (!frameStartEmitter_) {
> > +               ControlList ctrls(sensorControls);
> > +               sensor_->setControls(&ctrls);
> > +       }
> >  }
> >  
> >  /* Retrieve all source pads connected to a sink pad through active routes. */