[PATCH] pipeline: simple: Use proper device for frame start events

[Hans de Goede]

Hi Stanislaw,

Thank you for your patch.

On 17-Dec-24 11:52 AM, Stanislaw Gruszka wrote:
> Currently we use frame start event from video capture device to
> apply controls. But the capture device might not generate the events.
> Usually CSI-2 receiver is proper device to subscribe for start
> frame events.
> 
> Without DelayedConntrols:applyControls() is possible that we can get
> call to DelayedControls::get() with frame number that exceed number
> of saved entries and get below assertion failure:
> 
> ../src/libcamera/delayed_controls.cpp:227:
> libcamera::ControlList libcamera::DelayedControls::get(uint32_t):
> Assertion `info.type() != ControlTypeNone' failed
> 
> Assertion failure can happen at the beginning of streaming when
> ControlRingBuffer is not yet filled and there are errors on CSI-2.
> 
> To fix, loop over devices in the pipeline (starting from the last one),
> find one that emits start frame events and connect applyControls()
> to it.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=241
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>

I noticed the issue with connecting to the framestart signal of
the /dev/video# capture node which cannot work myself too and I was
about to post this patch for this:

https://github.com/jwrdegoede/libcamera/commit/5abbf43118e80bb4be6b893a6e5e28c65b59744a

But your solution is obviously better.

Note that the simple-pipelinehandler already contains a workaroud for this
in the form of setControls() directly applying the controls rather then
only pushing them into delayedCtrls_.

That workaround needs to be disabled in your patch when the frameStart signal
is used successfully.

See my attached patch with suggested changes to your patch.

I have successfully tested this with the suggested changes:

Tested-by: Hans de Goede <hdegoede at redhat.com>

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 26 ++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 8ac24e6e..52f3d520 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -382,6 +382,7 @@ private:
>  	std::map<const MediaEntity *, EntityData> entities_;
>  
>  	MediaDevice *converter_;
> +	V4L2Subdevice *eventEmitter_;
>  	bool swIspEnabled_;
>  };
>  

The simplepipeline handler can register and potentially operate multiple
cameras at the same time, so this needs to be part of SimpleCameraData.


> @@ -1299,8 +1300,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	data->delayedCtrls_ =
>  		std::make_unique<DelayedControls>(data->sensor_->device(),
>  						  params);
> -	data->video_->frameStart.connect(data->delayedCtrls_.get(),
> -					 &DelayedControls::applyControls);
>  
>  	StreamConfiguration inputCfg;
>  	inputCfg.pixelFormat = pipeConfig->captureFormat;
> @@ -1368,6 +1367,23 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  
>  	video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);
>  
> +	/*
> +	 * Enable frame start event on last device in the pipeline
> +	 * that provides the events.
> +	 */
> +	for (auto it = data->entities_.rbegin(); it != data->entities_.rend(); ++it) {
> +		V4L2Subdevice *sd = subdev(it->entity);
> +		if (!sd)
> +			continue;
> +		if (sd->setFrameStartEnabled(true) < 0)
> +			continue;
> +

IMHO it would be good to have a debug log here which entity is used for the frameStart
signal (see attached suggested changes).

> +		sd->frameStart.connect(data->delayedCtrls_.get(),
> +				       &DelayedControls::applyControls);
> +		eventEmitter_ = sd;
> +		break;
> +	}
> +
>  	ret = video->streamOn();
>  	if (ret < 0) {
>  		stop(camera);
> @@ -1400,6 +1416,12 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  	SimpleCameraData *data = cameraData(camera);
>  	V4L2VideoDevice *video = data->video_;
>  
> +	if (eventEmitter_) {
> +		eventEmitter_->setFrameStartEnabled(false);
> +		eventEmitter_->frameStart.disconnect(data->delayedCtrls_.get(),
> +						     &DelayedControls::applyControls);

This should also et eventEmitter_ to NULL, in case we fail to get it again
when re-starting the stream.


> +	}
> +
>  	if (data->useConversion_) {
>  		if (data->converter_)
>  			data->converter_->stop();


A semi related issue is that atm data->delayedCtrls_->reset(); is not called
on pipeline start() so on a second start of the same camera there will be
stale values in the delayedCtrls class and/or things will outright not
work because of the writeQueue internal counter being out of sync.

Regards,

Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-pipeline-simple-Use-proper-device-for-frame-start-ev.patch
Type: text/x-patch
Size: 2764 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20241217/15480bab/attachment.bin>