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

[Stanislaw Gruszka]

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 ? 

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

Regards
Stanislaw