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

22397 diff mbox series

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. Additionally remove direct call to sensor_->setControls() if
the emitter device was found.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=241
Co-developed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
v1 -> v2:
 - make eventEmitter_ subdevice part of SimpleCameraData
 - add debug log when found event emitter device
 - nullify eventEmitter_ on stop
 - remove direct sensor_->setControls()
 - add delayedCtrls_->reset() on start

 src/libcamera/pipeline/simple/simple.cpp | 39 +++++++++++++++++++++---
 1 file changed, 35 insertions(+), 4 deletions(-)

--
2.43.0

Hi,

On 18-Dec-24 3:22 PM, 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. Additionally remove direct call to sensor_->setControls() if
> the emitter device was found.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=241
> Co-developed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

which is a bit weird since Stanislaw gave me Co-developed-by credits,
but still...

Also:

Tested-by: Hans de Goede <hdegoede@redhat.com> # Lenovo X1 Yoga IPU6 + ov2740

Regards,

Hans




> ---
> v1 -> v2:
>  - make eventEmitter_ subdevice part of SimpleCameraData
>  - add debug log when found event emitter device
>  - nullify eventEmitter_ on stop
>  - remove direct sensor_->setControls()
>  - add delayedCtrls_->reset() on start
> 
>  src/libcamera/pipeline/simple/simple.cpp | 39 +++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 8ac24e6e..a7594c2c 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 *eventEmitter_;
> 
>  	std::vector<Configuration> configs_;
>  	std::map<PixelFormat, std::vector<const Configuration *>> formats_;
> @@ -911,8 +912,11 @@ 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 */
> +	if (!eventEmitter_) {
> +		ControlList ctrls(sensorControls);
> +		sensor_->setControls(&ctrls);
> +	}
>  }
> 
>  /* Retrieve all source pads connected to a sink pad through active routes. */
> @@ -1299,8 +1303,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 +1370,28 @@ 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;
> +
> +		LOG(SimplePipeline, Debug)
> +			<< "Using " << it->entity->name() << " frameStart signal";
> +
> +		sd->frameStart.connect(data->delayedCtrls_.get(),
> +				       &DelayedControls::applyControls);
> +		data->eventEmitter_ = sd;
> +		break;
> +	}
> +
> +	data->delayedCtrls_->reset();
> +
>  	ret = video->streamOn();
>  	if (ret < 0) {
>  		stop(camera);
> @@ -1400,6 +1424,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  	SimpleCameraData *data = cameraData(camera);
>  	V4L2VideoDevice *video = data->video_;
> 
> +	if (data->eventEmitter_) {
> +		data->eventEmitter_->setFrameStartEnabled(false);
> +		data->eventEmitter_->frameStart.disconnect(data->delayedCtrls_.get(),
> +							   &DelayedControls::applyControls);
> +		data->eventEmitter_ = NULL;
> +	}
> +
>  	if (data->useConversion_) {
>  		if (data->converter_)
>  			data->converter_->stop();
> --
> 2.43.0
>