[PATCH v6 3/5] pipeline: simple: Enable frame start events

[Stefan Klug]

Hi Stanislaw,

On Wed, Apr 02, 2025 at 11:08:24AM +0200, Stanislaw Gruszka wrote:
> Hi Stefan,
> 
> thanks for the review.
> 
> On Mon, Mar 31, 2025 at 05:52:16PM +0200, Stefan Klug wrote:
> > > @@ -897,8 +912,18 @@ 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.
> > > +	 *
> > > +	 * \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);
> > > +	}
> > 
> > If I get it right, we don't use delayedControls::applyControls() here
> > because we don't have the sequence number available. Wouldn't it be
> > better to reduce this to delayedControls_->push()
> > 
> > and to add a
> > 
> > if(!frameStartEmitter_) {
> > 	delayedControls_::applyControls(buffer->metadata().sequence);
> > }
> > 
> > to SimpleCameraData::imageBufferReady().
> > 
> > then we get the "per control delays" from delayed controls. Timing might
> > still be off, but it would be the best we can do for now.
> 
> I'm testing below change and sometimes get strange low frequency flickering.
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 659b1123a717..c71bacc90bfb 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -597,7 +597,7 @@ int SimpleCameraData::init()
>  		LOG(SimplePipeline, Debug)
>  			<< "Using frameStart signal from '"
>  			<< entity.entity->name() << "'";
> -		frameStartEmitter_ = sd;
> +		// frameStartEmitter_ = sd;
>  		break;
>  	}
>  
> @@ -851,6 +851,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>  		}
>  	}
>  
> +	if (!frameStartEmitter_) {
> +		delayedCtrls_->applyControls(buffer->metadata().sequence);
> +	}
> +
>  	if (request)
>  		request->metadata().set(controls::SensorTimestamp,
>  					buffer->metadata().timestamp);
> @@ -927,10 +931,10 @@ void SimpleCameraData::setSensorControls(const ControlList &sensorControls)
>  	 * but it also bypasses delayedCtrls_, creating AGC regulation issues.
>  	 * Both problems should be fixed.
>  	 */
> -	if (!frameStartEmitter_) {
> -		ControlList ctrls(sensorControls);
> -		sensor_->setControls(&ctrls);
> -	}
> +	// if (!frameStartEmitter_) {
> +	// 	ControlList ctrls(sensorControls);
> +	// 	sensor_->setControls(&ctrls);
> +	// }
>  }
>  
>  /* Retrieve all source pads connected to a sink pad through active routes. */
> 
> Could we apply this one for now with \todo and do
> applyControls(buffer->metadata().sequence) change later on top,
> once I figure out why I'm getting the flickering ? 

Thanks for testing that out. I'm fine with merging this and doing the
rest later (There was a bit of hope it could "just work"). There are
also some changes and fixes to delayed controls in my pipeline that I
hope to post as an RFC in the not too far future.

So

Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com> 

Best regards,
Stefan

> 
> I think that should be fine as this patch does not change existing
> behaviour for !frameStartEmitter case.
> 
> Regards
> Stanislaw
>