| Message ID | 20241217105206.357495-1-stanislaw.gruszka@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@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@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
Hi, 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@linux.intel.com> p.s. With this patch in place we can also introduce a custom frameStart handler, rather then directly using delayedCtrls_->apply() as handler and also get a timestamp from the framestart handler fixing the todo comment around line 832 of simple.cpp. This could/should be done in a follow-up patch to this one. Regards, Hans > --- > 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_; > }; > > @@ -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; > + > + 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); > + } > + > if (data->useConversion_) { > if (data->converter_) > data->converter_->stop();
Hi Hans, On Tue, Dec 17, 2024 at 02:41:43PM +0100, Hans de Goede wrote: > 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@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. All good points, I'll integrate your changes into v2 and repost. Thanks Stanislaw
Hi On Tue, Dec 17, 2024 at 02:43:49PM +0100, Hans de Goede wrote: > 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@linux.intel.com> > > p.s. > > With this patch in place we can also introduce a custom frameStart handler, > rather then directly using delayedCtrls_->apply() as handler and also > get a timestamp from the framestart handler fixing the todo comment > around line 832 of simple.cpp. > > This could/should be done in a follow-up patch to this one. I'm going to do this. Will post the change after this one get applied. Regards Stanislaw
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_; }; @@ -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; + + 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); + } + if (data->useConversion_) { if (data->converter_) data->converter_->stop();
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@linux.intel.com> --- src/libcamera/pipeline/simple/simple.cpp | 26 ++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)