| Message ID | 20210507084042.31879-6-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Fri, May 07, 2021 at 09:40:42AM +0100, Naushir Patuck wrote: > The controller algorithms currently run on every frame provided to the > IPA by the pipeline handler. This may be undesirable for very fast fps > operating modes where it could significantly increase the computation > cycles (per unit time) without providing any significant changes to the > IQ parameters. The added latencies could also cause dropped frames. > > Pass the FrameBuffer timestamp to the IPA through the controls. This > timestamp will be used to rate-limit the controller algorithms to run > with a minimum inter-frame time given by a compile time constant, > currently set to 16.66ms. On startup, we don't rate-limit the algorithms > until after the number of frames required for convergence. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 48 +++++++++++++++++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++ > 2 files changed, 49 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index f6d1ab16a290..57f5497a6e6b 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -61,6 +61,14 @@ constexpr unsigned int DefaultExposureTime = 20000; > constexpr double defaultMinFrameDuration = 1e6 / 30.0; > constexpr double defaultMaxFrameDuration = 1e6 / 0.01; > > +/* > + * Determine the minimum allowable inter-frame duration (in us) to run the > + * controller algorithms. If the pipeline handler provider frames at a rate > + * higher than this, we rate-limit the controller Prepare() and Process() calls > + * to lower than or equal to this rate. > + */ > +constexpr double controllerMinFrameDuration = 1e6 / 60.0; > + > LOG_DEFINE_CATEGORY(IPARPI) > > class IPARPi : public ipa::RPi::IPARPiInterface > @@ -68,7 +76,7 @@ class IPARPi : public ipa::RPi::IPARPiInterface > public: > IPARPi() > : controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0), > - lsTable_(nullptr), firstStart_(true) > + lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true) > { > } > > @@ -146,6 +154,12 @@ private: > /* Number of frames that need to be dropped on startup. */ > unsigned int dropFrameCount_; > > + /* Frame timestamp for the last run of the controller. */ > + uint64_t lastRunTimestamp_; > + > + /* Do we run a Controller::process() for this frame? */ > + bool processPending_; > + > /* LS table allocation passed in from the pipeline handler. */ > FileDescriptor lsTableHandle_; > void *lsTable_; > @@ -262,6 +276,7 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf > startConfig->dropFrameCount = dropFrameCount_; > > firstStart_ = false; > + lastRunTimestamp_ = 0; > } > > void IPARPi::setMode(const CameraSensorInfo &sensorInfo) > @@ -406,7 +421,7 @@ void IPARPi::signalStatReady(uint32_t bufferId) > { > if (++checkCount_ != frameCount_) /* assert here? */ > LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!"; > - if (frameCount_ > mistrustCount_) > + if (processPending_ && frameCount_ > mistrustCount_) > processStats(bufferId); > > reportMetadata(); > @@ -894,10 +909,11 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId) > > void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) > { > + int64_t frameTimestamp = data.controls.get(controls::draft::SensorTimestamp); > + RPiController::Metadata lastMetadata; > Span<uint8_t> embeddedBuffer; > > - rpiMetadata_.Clear(); > - > + lastMetadata = std::move(rpiMetadata_); > fillDeviceStatus(data.controls); > > if (data.embeddedBufferPresent) { > @@ -920,6 +936,30 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) > if (data.embeddedBufferPresent) > returnEmbeddedBuffer(data.embeddedBufferId); > > + /* Allow a 10% margin on the comparison below. */ > + constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1; > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > + frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) { I would likely have written this if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3 * 0.9) { but it doesn't matter much. > + /* > + * Ensure we update the controller metadata with the new frame's > + * exposure/gain values so that the correct values are returned > + * out in libcamera metadata later on. Any other bits of metadata > + * that were added in helper_->Prepare() will also be moved across. > + * All other metadata values must remain the same as the last frame. > + * > + * We do this by overwriting the current frame's metadata into the > + * previous frame's metadata object, and then swapping the latter "overwriting ... into" sounds weird. If you can provide an updated text, I can fix when applying. > + * with the former. > + */ > + lastMetadata.Overwrite(rpiMetadata_); > + std::swap(rpiMetadata_, lastMetadata); This could have been written rpiMetadata_.Merge(lastMetadata); without a swap, with std::map::merge semantics for Metadata::Merge() :-) The double swap seems a bit cumbersome. If you're happy with the current implementation though, that's fine with me. In either case, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> If you think Merge() would be cleaner, I'll merge v7. > + processPending_ = false; > + return; > + } > + > + lastRunTimestamp_ = frameTimestamp; > + processPending_ = true; > + > ControlList ctrls(ispCtrls_); > > controller_.Prepare(&rpiMetadata_); > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 2a917455500f..9cf9c8c6cebd 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1414,6 +1414,11 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > * DelayedControl and queue them along with the frame buffer. > */ > ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence); > + /* > + * Add the frame timestamp to the ControlList for the IPA to use > + * as it does not receive the FrameBuffer object. > + */ > + ctrl.set(controls::draft::SensorTimestamp, buffer->metadata().timestamp); > bayerQueue_.push({ buffer, std::move(ctrl) }); > } else { > embeddedQueue_.push(buffer);
Hi Naush, On Sat, May 08, 2021 at 03:30:31AM +0300, Laurent Pinchart wrote: > On Fri, May 07, 2021 at 09:40:42AM +0100, Naushir Patuck wrote: > > The controller algorithms currently run on every frame provided to the > > IPA by the pipeline handler. This may be undesirable for very fast fps > > operating modes where it could significantly increase the computation > > cycles (per unit time) without providing any significant changes to the > > IQ parameters. The added latencies could also cause dropped frames. > > > > Pass the FrameBuffer timestamp to the IPA through the controls. This > > timestamp will be used to rate-limit the controller algorithms to run > > with a minimum inter-frame time given by a compile time constant, > > currently set to 16.66ms. On startup, we don't rate-limit the algorithms > > until after the number of frames required for convergence. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 48 +++++++++++++++++-- > > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++ > > 2 files changed, 49 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index f6d1ab16a290..57f5497a6e6b 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -61,6 +61,14 @@ constexpr unsigned int DefaultExposureTime = 20000; > > constexpr double defaultMinFrameDuration = 1e6 / 30.0; > > constexpr double defaultMaxFrameDuration = 1e6 / 0.01; > > > > +/* > > + * Determine the minimum allowable inter-frame duration (in us) to run the > > + * controller algorithms. If the pipeline handler provider frames at a rate > > + * higher than this, we rate-limit the controller Prepare() and Process() calls > > + * to lower than or equal to this rate. > > + */ > > +constexpr double controllerMinFrameDuration = 1e6 / 60.0; > > + > > LOG_DEFINE_CATEGORY(IPARPI) > > > > class IPARPi : public ipa::RPi::IPARPiInterface > > @@ -68,7 +76,7 @@ class IPARPi : public ipa::RPi::IPARPiInterface > > public: > > IPARPi() > > : controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0), > > - lsTable_(nullptr), firstStart_(true) > > + lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true) > > { > > } > > > > @@ -146,6 +154,12 @@ private: > > /* Number of frames that need to be dropped on startup. */ > > unsigned int dropFrameCount_; > > > > + /* Frame timestamp for the last run of the controller. */ > > + uint64_t lastRunTimestamp_; > > + > > + /* Do we run a Controller::process() for this frame? */ > > + bool processPending_; > > + > > /* LS table allocation passed in from the pipeline handler. */ > > FileDescriptor lsTableHandle_; > > void *lsTable_; > > @@ -262,6 +276,7 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf > > startConfig->dropFrameCount = dropFrameCount_; > > > > firstStart_ = false; > > + lastRunTimestamp_ = 0; > > } > > > > void IPARPi::setMode(const CameraSensorInfo &sensorInfo) > > @@ -406,7 +421,7 @@ void IPARPi::signalStatReady(uint32_t bufferId) > > { > > if (++checkCount_ != frameCount_) /* assert here? */ > > LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!"; > > - if (frameCount_ > mistrustCount_) > > + if (processPending_ && frameCount_ > mistrustCount_) > > processStats(bufferId); > > > > reportMetadata(); > > @@ -894,10 +909,11 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId) > > > > void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) > > { > > + int64_t frameTimestamp = data.controls.get(controls::draft::SensorTimestamp); Actually the series doesn't compile, as controls::draft::SensorTimestamp has become controls::SensorTimestamp. This should be easy to fix, but would you mind sending a v7 ? I'd be more comfortable merging a properly tested series than updating this when applying. > > + RPiController::Metadata lastMetadata; > > Span<uint8_t> embeddedBuffer; > > > > - rpiMetadata_.Clear(); > > - > > + lastMetadata = std::move(rpiMetadata_); > > fillDeviceStatus(data.controls); > > > > if (data.embeddedBufferPresent) { > > @@ -920,6 +936,30 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) > > if (data.embeddedBufferPresent) > > returnEmbeddedBuffer(data.embeddedBufferId); > > > > + /* Allow a 10% margin on the comparison below. */ > > + constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1; > > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > > + frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) { > > I would likely have written this > > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3 * 0.9) { > > but it doesn't matter much. > > > + /* > > + * Ensure we update the controller metadata with the new frame's > > + * exposure/gain values so that the correct values are returned > > + * out in libcamera metadata later on. Any other bits of metadata > > + * that were added in helper_->Prepare() will also be moved across. > > + * All other metadata values must remain the same as the last frame. > > + * > > + * We do this by overwriting the current frame's metadata into the > > + * previous frame's metadata object, and then swapping the latter > > "overwriting ... into" sounds weird. If you can provide an updated text, > I can fix when applying. > > > + * with the former. > > + */ > > + lastMetadata.Overwrite(rpiMetadata_); > > + std::swap(rpiMetadata_, lastMetadata); > > This could have been written > > rpiMetadata_.Merge(lastMetadata); > > without a swap, with std::map::merge semantics for Metadata::Merge() :-) > The double swap seems a bit cumbersome. > > If you're happy with the current implementation though, that's fine with > me. In either case, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > If you think Merge() would be cleaner, I'll merge v7. > > > + processPending_ = false; > > + return; > > + } > > + > > + lastRunTimestamp_ = frameTimestamp; > > + processPending_ = true; > > + > > ControlList ctrls(ispCtrls_); > > > > controller_.Prepare(&rpiMetadata_); > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 2a917455500f..9cf9c8c6cebd 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1414,6 +1414,11 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > * DelayedControl and queue them along with the frame buffer. > > */ > > ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence); > > + /* > > + * Add the frame timestamp to the ControlList for the IPA to use > > + * as it does not receive the FrameBuffer object. > > + */ > > + ctrl.set(controls::draft::SensorTimestamp, buffer->metadata().timestamp); > > bayerQueue_.push({ buffer, std::move(ctrl) }); > > } else { > > embeddedQueue_.push(buffer);
Hi Laurent, Thank you for your feedback. On Sat, 8 May 2021 at 01:30, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Fri, May 07, 2021 at 09:40:42AM +0100, Naushir Patuck wrote: > > The controller algorithms currently run on every frame provided to the > > IPA by the pipeline handler. This may be undesirable for very fast fps > > operating modes where it could significantly increase the computation > > cycles (per unit time) without providing any significant changes to the > > IQ parameters. The added latencies could also cause dropped frames. > > > > Pass the FrameBuffer timestamp to the IPA through the controls. This > > timestamp will be used to rate-limit the controller algorithms to run > > with a minimum inter-frame time given by a compile time constant, > > currently set to 16.66ms. On startup, we don't rate-limit the algorithms > > until after the number of frames required for convergence. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 48 +++++++++++++++++-- > > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++ > > 2 files changed, 49 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index f6d1ab16a290..57f5497a6e6b 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -61,6 +61,14 @@ constexpr unsigned int DefaultExposureTime = 20000; > > constexpr double defaultMinFrameDuration = 1e6 / 30.0; > > constexpr double defaultMaxFrameDuration = 1e6 / 0.01; > > > > +/* > > + * Determine the minimum allowable inter-frame duration (in us) to run > the > > + * controller algorithms. If the pipeline handler provider frames at a > rate > > + * higher than this, we rate-limit the controller Prepare() and > Process() calls > > + * to lower than or equal to this rate. > > + */ > > +constexpr double controllerMinFrameDuration = 1e6 / 60.0; > > + > > LOG_DEFINE_CATEGORY(IPARPI) > > > > class IPARPi : public ipa::RPi::IPARPiInterface > > @@ -68,7 +76,7 @@ class IPARPi : public ipa::RPi::IPARPiInterface > > public: > > IPARPi() > > : controller_(), frameCount_(0), checkCount_(0), > mistrustCount_(0), > > - lsTable_(nullptr), firstStart_(true) > > + lastRunTimestamp_(0), lsTable_(nullptr), > firstStart_(true) > > { > > } > > > > @@ -146,6 +154,12 @@ private: > > /* Number of frames that need to be dropped on startup. */ > > unsigned int dropFrameCount_; > > > > + /* Frame timestamp for the last run of the controller. */ > > + uint64_t lastRunTimestamp_; > > + > > + /* Do we run a Controller::process() for this frame? */ > > + bool processPending_; > > + > > /* LS table allocation passed in from the pipeline handler. */ > > FileDescriptor lsTableHandle_; > > void *lsTable_; > > @@ -262,6 +276,7 @@ void IPARPi::start(const ControlList &controls, > ipa::RPi::StartConfig *startConf > > startConfig->dropFrameCount = dropFrameCount_; > > > > firstStart_ = false; > > + lastRunTimestamp_ = 0; > > } > > > > void IPARPi::setMode(const CameraSensorInfo &sensorInfo) > > @@ -406,7 +421,7 @@ void IPARPi::signalStatReady(uint32_t bufferId) > > { > > if (++checkCount_ != frameCount_) /* assert here? */ > > LOG(IPARPI, Error) << "WARNING: Prepare/Process > mismatch!!!"; > > - if (frameCount_ > mistrustCount_) > > + if (processPending_ && frameCount_ > mistrustCount_) > > processStats(bufferId); > > > > reportMetadata(); > > @@ -894,10 +909,11 @@ void IPARPi::returnEmbeddedBuffer(unsigned int > bufferId) > > > > void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) > > { > > + int64_t frameTimestamp = > data.controls.get(controls::draft::SensorTimestamp); > > + RPiController::Metadata lastMetadata; > > Span<uint8_t> embeddedBuffer; > > > > - rpiMetadata_.Clear(); > > - > > + lastMetadata = std::move(rpiMetadata_); > > fillDeviceStatus(data.controls); > > > > if (data.embeddedBufferPresent) { > > @@ -920,6 +936,30 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig > &data) > > if (data.embeddedBufferPresent) > > returnEmbeddedBuffer(data.embeddedBufferId); > > > > + /* Allow a 10% margin on the comparison below. */ > > + constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1; > > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > > + frameTimestamp - lastRunTimestamp_ + eps < > controllerMinFrameDuration * 1e3) { > > I would likely have written this > > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > frameTimestamp - lastRunTimestamp_ + eps < > controllerMinFrameDuration * 1e3 * 0.9) { > > but it doesn't matter much. > > > + /* > > + * Ensure we update the controller metadata with the new > frame's > > + * exposure/gain values so that the correct values are > returned > > + * out in libcamera metadata later on. Any other bits of > metadata > > + * that were added in helper_->Prepare() will also be > moved across. > > + * All other metadata values must remain the same as the > last frame. > > + * > > + * We do this by overwriting the current frame's metadata > into the > > + * previous frame's metadata object, and then swapping the > latter > > "overwriting ... into" sounds weird. If you can provide an updated text, > I can fix when applying. > > > + * with the former. > > + */ > > + lastMetadata.Overwrite(rpiMetadata_); > > + std::swap(rpiMetadata_, lastMetadata); > > This could have been written > > rpiMetadata_.Merge(lastMetadata); > > without a swap, with std::map::merge semantics for Metadata::Merge() :-) > The double swap seems a bit cumbersome. > > If you're happy with the current implementation though, that's fine with > me. In either case, > You are right, a Metadata::Merge() would be a less cumbersome way to do this. I will rework this (and the previous) patch to add that and resubmit a v7. Thanks, Naush > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > If you think Merge() would be cleaner, I'll merge v7. > > > + processPending_ = false; > > + return; > > + } > > + > > + lastRunTimestamp_ = frameTimestamp; > > + processPending_ = true; > > + > > ControlList ctrls(ispCtrls_); > > > > controller_.Prepare(&rpiMetadata_); > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 2a917455500f..9cf9c8c6cebd 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1414,6 +1414,11 @@ void > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > * DelayedControl and queue them along with the frame > buffer. > > */ > > ControlList ctrl = > delayedCtrls_->get(buffer->metadata().sequence); > > + /* > > + * Add the frame timestamp to the ControlList for the IPA > to use > > + * as it does not receive the FrameBuffer object. > > + */ > > + ctrl.set(controls::draft::SensorTimestamp, > buffer->metadata().timestamp); > > bayerQueue_.push({ buffer, std::move(ctrl) }); > > } else { > > embeddedQueue_.push(buffer); > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index f6d1ab16a290..57f5497a6e6b 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -61,6 +61,14 @@ constexpr unsigned int DefaultExposureTime = 20000; constexpr double defaultMinFrameDuration = 1e6 / 30.0; constexpr double defaultMaxFrameDuration = 1e6 / 0.01; +/* + * Determine the minimum allowable inter-frame duration (in us) to run the + * controller algorithms. If the pipeline handler provider frames at a rate + * higher than this, we rate-limit the controller Prepare() and Process() calls + * to lower than or equal to this rate. + */ +constexpr double controllerMinFrameDuration = 1e6 / 60.0; + LOG_DEFINE_CATEGORY(IPARPI) class IPARPi : public ipa::RPi::IPARPiInterface @@ -68,7 +76,7 @@ class IPARPi : public ipa::RPi::IPARPiInterface public: IPARPi() : controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0), - lsTable_(nullptr), firstStart_(true) + lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true) { } @@ -146,6 +154,12 @@ private: /* Number of frames that need to be dropped on startup. */ unsigned int dropFrameCount_; + /* Frame timestamp for the last run of the controller. */ + uint64_t lastRunTimestamp_; + + /* Do we run a Controller::process() for this frame? */ + bool processPending_; + /* LS table allocation passed in from the pipeline handler. */ FileDescriptor lsTableHandle_; void *lsTable_; @@ -262,6 +276,7 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf startConfig->dropFrameCount = dropFrameCount_; firstStart_ = false; + lastRunTimestamp_ = 0; } void IPARPi::setMode(const CameraSensorInfo &sensorInfo) @@ -406,7 +421,7 @@ void IPARPi::signalStatReady(uint32_t bufferId) { if (++checkCount_ != frameCount_) /* assert here? */ LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!"; - if (frameCount_ > mistrustCount_) + if (processPending_ && frameCount_ > mistrustCount_) processStats(bufferId); reportMetadata(); @@ -894,10 +909,11 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId) void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) { + int64_t frameTimestamp = data.controls.get(controls::draft::SensorTimestamp); + RPiController::Metadata lastMetadata; Span<uint8_t> embeddedBuffer; - rpiMetadata_.Clear(); - + lastMetadata = std::move(rpiMetadata_); fillDeviceStatus(data.controls); if (data.embeddedBufferPresent) { @@ -920,6 +936,30 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) if (data.embeddedBufferPresent) returnEmbeddedBuffer(data.embeddedBufferId); + /* Allow a 10% margin on the comparison below. */ + constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1; + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && + frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) { + /* + * Ensure we update the controller metadata with the new frame's + * exposure/gain values so that the correct values are returned + * out in libcamera metadata later on. Any other bits of metadata + * that were added in helper_->Prepare() will also be moved across. + * All other metadata values must remain the same as the last frame. + * + * We do this by overwriting the current frame's metadata into the + * previous frame's metadata object, and then swapping the latter + * with the former. + */ + lastMetadata.Overwrite(rpiMetadata_); + std::swap(rpiMetadata_, lastMetadata); + processPending_ = false; + return; + } + + lastRunTimestamp_ = frameTimestamp; + processPending_ = true; + ControlList ctrls(ispCtrls_); controller_.Prepare(&rpiMetadata_); diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 2a917455500f..9cf9c8c6cebd 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1414,6 +1414,11 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) * DelayedControl and queue them along with the frame buffer. */ ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence); + /* + * Add the frame timestamp to the ControlList for the IPA to use + * as it does not receive the FrameBuffer object. + */ + ctrl.set(controls::draft::SensorTimestamp, buffer->metadata().timestamp); bayerQueue_.push({ buffer, std::move(ctrl) }); } else { embeddedQueue_.push(buffer);
The controller algorithms currently run on every frame provided to the IPA by the pipeline handler. This may be undesirable for very fast fps operating modes where it could significantly increase the computation cycles (per unit time) without providing any significant changes to the IQ parameters. The added latencies could also cause dropped frames. Pass the FrameBuffer timestamp to the IPA through the controls. This timestamp will be used to rate-limit the controller algorithms to run with a minimum inter-frame time given by a compile time constant, currently set to 16.66ms. On startup, we don't rate-limit the algorithms until after the number of frames required for convergence. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/raspberrypi.cpp | 48 +++++++++++++++++-- .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++ 2 files changed, 49 insertions(+), 4 deletions(-)