| Message ID | 20210510095814.3732400-7-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 99feb66df02988831c9e8535649b99fad9a78069 |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch. For reasons unknown, gmail seems to have tacked it on as a reply to the cover note... On Mon, 10 May 2021 at 10:59, Naushir Patuck <naush@raspberrypi.com> 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 | 42 +++++++++++++++++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++ > 2 files changed, 43 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 07409621a03a..52d91db282ea 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_) I guess the only thing that caused my brain to pause for a moment was the need to convince myself that processPending_ can't be uninitialised. But prepareISP always sets it and must run before this, so we're all good... > 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::SensorTimestamp); Just wondering if there's any possibility that the first time stamp could be zero, thereby matching the initial value of lastRunTimestamp. The result would be that we start running the control algos a frame or so late. If we're happy that this doesn't happen, then I'm OK with it too. > + RPiController::Metadata lastMetadata; > Span<uint8_t> embeddedBuffer; > > - rpiMetadata_.Clear(); > - > + lastMetadata = std::move(rpiMetadata_); > fillDeviceStatus(data.controls); > > if (data.embeddedBufferPresent) { > @@ -920,6 +936,24 @@ 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) { Might this be easier to read if we defined all these things in nanoseconds right from the start? But I really can't get too worked up over it! Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Best regards David > + /* > + * Ensure we merge the previous frame's metadata with the current > + * frame. This will not overwrite exposure/gain values for the > + * current frame, or any other bits of metadata that were added > + * in helper_->Prepare(). > + */ > + rpiMetadata_.Merge(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 b22564938cdc..d26ae2a9772a 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1426,6 +1426,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::SensorTimestamp, buffer->metadata().timestamp); > bayerQueue_.push({ buffer, std::move(ctrl) }); > } else { > embeddedQueue_.push(buffer); > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi David, Thank you for your feedback. On Mon, 10 May 2021 at 14:35, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for this patch. For reasons unknown, gmail seems to have tacked > it on as a reply to the cover note... > > On Mon, 10 May 2021 at 10:59, Naushir Patuck <naush@raspberrypi.com> > 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 | 42 +++++++++++++++++-- > > .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++ > > 2 files changed, 43 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index 07409621a03a..52d91db282ea 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_) > > I guess the only thing that caused my brain to pause for a moment was > the need to convince myself that processPending_ can't be > uninitialised. But prepareISP always sets it and must run before this, > so we're all good... > Yes, this is correct. > > > 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::SensorTimestamp); > > Just wondering if there's any possibility that the first time stamp > could be zero, thereby matching the initial value of lastRunTimestamp. > The result would be that we start running the control algos a frame or > so late. If we're happy that this doesn't happen, then I'm OK with it > too. > I don't think the first timestamp from the kernel can ever be zero. But I do also test that lastRunTimestamp_ > 0 in the calculation below. > > > + RPiController::Metadata lastMetadata; > > Span<uint8_t> embeddedBuffer; > > > > - rpiMetadata_.Clear(); > > - > > + lastMetadata = std::move(rpiMetadata_); > > fillDeviceStatus(data.controls); > > > > if (data.embeddedBufferPresent) { > > @@ -920,6 +936,24 @@ 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) { > > Might this be easier to read if we defined all these things in > nanoseconds right from the start? But I really can't get too worked up > over it! > Yes I agree. However, I would prefer to leave this as-is for now, as it matches the other time based constants. One of my next tasks is to switch to using std::chrono::duration for our time base variables, which remove these const factors. Regards, Naush > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Best regards > David > > > + /* > > + * Ensure we merge the previous frame's metadata with > the current > > + * frame. This will not overwrite exposure/gain values > for the > > + * current frame, or any other bits of metadata that > were added > > + * in helper_->Prepare(). > > + */ > > + rpiMetadata_.Merge(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 b22564938cdc..d26ae2a9772a 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1426,6 +1426,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::SensorTimestamp, > buffer->metadata().timestamp); > > bayerQueue_.push({ buffer, std::move(ctrl) }); > > } else { > > embeddedQueue_.push(buffer); > > -- > > 2.25.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Naush, Thank you for the patch. On Mon, May 10, 2021 at 02:43:42PM +0100, Naushir Patuck wrote: > On Mon, 10 May 2021 at 14:35, David Plowman wrote: > > Hi Naush > > > > Thanks for this patch. For reasons unknown, gmail seems to have tacked > > it on as a reply to the cover note... > > > > On Mon, 10 May 2021 at 10:59, 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 | 42 +++++++++++++++++-- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++ > > > 2 files changed, 43 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index 07409621a03a..52d91db282ea 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_) > > > > I guess the only thing that caused my brain to pause for a moment was > > the need to convince myself that processPending_ can't be > > uninitialised. But prepareISP always sets it and must run before this, > > so we're all good... > > Yes, this is correct. > > > > 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::SensorTimestamp); > > > > Just wondering if there's any possibility that the first time stamp > > could be zero, thereby matching the initial value of lastRunTimestamp. > > The result would be that we start running the control algos a frame or > > so late. If we're happy that this doesn't happen, then I'm OK with it > > too. > > I don't think the first timestamp from the kernel can ever be zero. But I > do also test > that lastRunTimestamp_ > 0 in the calculation below. > > > > + RPiController::Metadata lastMetadata; > > > Span<uint8_t> embeddedBuffer; > > > > > > - rpiMetadata_.Clear(); > > > - > > > + lastMetadata = std::move(rpiMetadata_); > > > fillDeviceStatus(data.controls); > > > > > > if (data.embeddedBufferPresent) { > > > @@ -920,6 +936,24 @@ 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) { > > > > Might this be easier to read if we defined all these things in > > nanoseconds right from the start? But I really can't get too worked up > > over it! > > Yes I agree. However, I would prefer to leave this as-is for now, as it > matches the other time > based constants. One of my next tasks is to switch to using > std::chrono::duration for our time base > variables, which remove these const factors. I'm thinking about doing the same in the libcamera core. And as part of that work, switching everything to nanoseconds may be a code idea, to be done with it once and for all. > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + /* > > > + * Ensure we merge the previous frame's metadata with the current > > > + * frame. This will not overwrite exposure/gain values for the > > > + * current frame, or any other bits of metadata that were added > > > + * in helper_->Prepare(). > > > + */ > > > + rpiMetadata_.Merge(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 b22564938cdc..d26ae2a9772a 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -1426,6 +1426,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::SensorTimestamp, buffer->metadata().timestamp); > > > bayerQueue_.push({ buffer, std::move(ctrl) }); > > > } else { > > > embeddedQueue_.push(buffer);
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 07409621a03a..52d91db282ea 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::SensorTimestamp); + RPiController::Metadata lastMetadata; Span<uint8_t> embeddedBuffer; - rpiMetadata_.Clear(); - + lastMetadata = std::move(rpiMetadata_); fillDeviceStatus(data.controls); if (data.embeddedBufferPresent) { @@ -920,6 +936,24 @@ 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 merge the previous frame's metadata with the current + * frame. This will not overwrite exposure/gain values for the + * current frame, or any other bits of metadata that were added + * in helper_->Prepare(). + */ + rpiMetadata_.Merge(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 b22564938cdc..d26ae2a9772a 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1426,6 +1426,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::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 | 42 +++++++++++++++++-- .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++ 2 files changed, 43 insertions(+), 4 deletions(-)