[v6,3/5] pipeline: simple: Enable frame start events
diff mbox series

Message ID 20250305135256.801351-4-stanislaw.gruszka@linux.intel.com
State Accepted
Headers show
Series
  • libcamera: start frame events changes
Related show

Commit Message

Stanislaw Gruszka March 5, 2025, 1:52 p.m. UTC 
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.

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

Comments

Stefan Klug March 31, 2025, 3:52 p.m. UTC | #1 
Hi Stanislaw,

Thank you for the patch.

On Wed, Mar 05, 2025 at 02:52:54PM +0100, Stanislaw Gruszka wrote:
> 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.
> 
> 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 8345a771..7be49017 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -277,6 +277,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_;
> @@ -579,6 +580,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;
>  }
>  
> @@ -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.

Best regards,
Stefan


>  }
>  
>  /* Retrieve all source pads connected to a sink pad through active routes. */
> @@ -1323,6 +1348,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);
> @@ -1352,8 +1378,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) {
> @@ -1386,9 +1419,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
>
Stanislaw Gruszka April 2, 2025, 9:08 a.m. UTC | #2 
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
Stefan Klug April 2, 2025, 2 p.m. UTC | #3 
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@ideasonboard.com> 

Best regards,
Stefan

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

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 8345a771..7be49017 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -277,6 +277,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_;
@@ -579,6 +580,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;
 }
 
@@ -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);
+	}
 }
 
 /* Retrieve all source pads connected to a sink pad through active routes. */
@@ -1323,6 +1348,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);
@@ -1352,8 +1378,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) {
@@ -1386,9 +1419,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_)