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

[Stefan Klug]

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 at redhat.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at 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
>