| Message ID | 20210416103141.1483745-4-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for version 2 of this patch, and especially for re-basing on top of my still-pending patch! Weirdly this one, which I'd have expected to be the "3/3" has ended up as a reply to the cover note, so not sure what's going on there. But anyway... On Fri, 16 Apr 2021 at 11:31, 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. Is it worth a remark on how we don't rate-limit while dropping frames? I don't really mind, though. > > 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..e96b169ca612 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 Strictly speaking, I guess they're still Prepare() and Process()... (until such time as the lower-casing elves strike those files!) > + * 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); > > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > + frameTimestamp - lastRunTimestamp_ < 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. All other metadata values > + * must remain the same as the last frame. > + */ > + DeviceStatus currentDeviceStatus; > + > + rpiMetadata_.Get<DeviceStatus>("device.status", currentDeviceStatus); > + rpiMetadata_ = std::move(lastMetadata); > + rpiMetadata_.Set("device.status", currentDeviceStatus); > + processPending_ = false; > + LOG(IPARPI, Debug) << "Rate-limiting the controller! inter-frame duration: " > + << frameTimestamp - lastRunTimestamp_ > + << ", min duration " > + << controllerMinFrameDuration * 1e3; > + return; > + } > + > + lastRunTimestamp_ = frameTimestamp; > + processPending_ = true; > + > ControlList ctrls(ispCtrls_); Hmm, yes, I see there's an interesting manoeuvre going on with the metadata. I wonder if there's a way to rearrange this that involves a bit less shuffling, maybe: first delete these lines: + lastMetadata = std::move(rpiMetadata_); fillDeviceStatus(data.controls); next, in place of: + DeviceStatus currentDeviceStatus; + + rpiMetadata_.Get<DeviceStatus>("device.status", currentDeviceStatus); + rpiMetadata_ = std::move(lastMetadata); + rpiMetadata_.Set("device.status", currentDeviceStatus); do this: fillDeviceStatus(data.controls); and finally after the if-block for the "skip the algos" case put these back: rpiMetadata_.Clear(); fillDeviceStatus(data.controls); I don't know if this ends up tidier or not, perhaps it's best if you decide what you prefer. But I'm fine either way, so: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > > 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); > -- > 2.25.1 >
Hi David, Thank you for your feedback. On Fri, 16 Apr 2021 at 14:44, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for version 2 of this patch, and especially for re-basing on > top of my still-pending patch! > > Weirdly this one, which I'd have expected to be the "3/3" has ended up > as a reply to the cover note, so not sure what's going on there. But > anyway... > I think that is gmail causing issues by bunching 0/3 and 3/3 into a conversation for some reason, even though the subjects are (slightly) different. Happens on my view as well! > > On Fri, 16 Apr 2021 at 11:31, 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. > > Is it worth a remark on how we don't rate-limit while dropping frames? > I don't really mind, though. > > > > > 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..e96b169ca612 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 > > Strictly speaking, I guess they're still Prepare() and Process()... > (until such time as the lower-casing elves strike those files!) > > > + * 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); > > > > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > > + frameTimestamp - lastRunTimestamp_ < > 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. All other > metadata values > > + * must remain the same as the last frame. > > + */ > > + DeviceStatus currentDeviceStatus; > > + > > + rpiMetadata_.Get<DeviceStatus>("device.status", > currentDeviceStatus); > > + rpiMetadata_ = std::move(lastMetadata); > > + rpiMetadata_.Set("device.status", currentDeviceStatus); > > + processPending_ = false; > > + LOG(IPARPI, Debug) << "Rate-limiting the controller! > inter-frame duration: " > > + << frameTimestamp - lastRunTimestamp_ > > + << ", min duration " > > + << controllerMinFrameDuration * 1e3; > > + return; > > + } > > + > > + lastRunTimestamp_ = frameTimestamp; > > + processPending_ = true; > > + > > ControlList ctrls(ispCtrls_); > > Hmm, yes, I see there's an interesting manoeuvre going on with the > metadata. I wonder if there's a way to rearrange this that involves a > bit less shuffling, maybe: > > first delete these lines: > + lastMetadata = std::move(rpiMetadata_); > fillDeviceStatus(data.controls); > > next, in place of: > + DeviceStatus currentDeviceStatus; > + > + rpiMetadata_.Get<DeviceStatus>("device.status", > currentDeviceStatus); > + rpiMetadata_ = std::move(lastMetadata); > + rpiMetadata_.Set("device.status", currentDeviceStatus); > > do this: > fillDeviceStatus(data.controls); > > and finally after the if-block for the "skip the algos" case put these > back: > rpiMetadata_.Clear(); > fillDeviceStatus(data.controls); > I did try this type of logic initially, but I don't think it would work. We need to call rpiMetadata_.Clear() and fillDeviceStatus() before helper_->Prepare() is called at the beginning of the function. I cannot move the latter call further down as it needs to run before my skip algos check and the returnEmbeddedBuffer() call. So I think the existing manoeuvre is needed without more refactoring. Regards, Naush > > I don't know if this ends up tidier or not, perhaps it's best if you > decide what you prefer. But I'm fine either way, so: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks! > David > > > > > 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); > > -- > > 2.25.1 > > >
Hi Naush Thanks for explaining. On Fri, 16 Apr 2021 at 15:36, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi David, > > Thank you for your feedback. > > On Fri, 16 Apr 2021 at 14:44, David Plowman <david.plowman@raspberrypi.com> wrote: >> >> Hi Naush >> >> Thanks for version 2 of this patch, and especially for re-basing on >> top of my still-pending patch! >> >> Weirdly this one, which I'd have expected to be the "3/3" has ended up >> as a reply to the cover note, so not sure what's going on there. But >> anyway... > > > I think that is gmail causing issues by bunching 0/3 and 3/3 into a conversation > for some reason, even though the subjects are (slightly) different. Happens > on my view as well! > >> >> >> On Fri, 16 Apr 2021 at 11:31, 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. >> >> Is it worth a remark on how we don't rate-limit while dropping frames? >> I don't really mind, though. >> >> >> > >> > 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..e96b169ca612 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 >> >> Strictly speaking, I guess they're still Prepare() and Process()... >> (until such time as the lower-casing elves strike those files!) >> >> > + * 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); >> > >> > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && >> > + frameTimestamp - lastRunTimestamp_ < 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. All other metadata values >> > + * must remain the same as the last frame. >> > + */ >> > + DeviceStatus currentDeviceStatus; >> > + >> > + rpiMetadata_.Get<DeviceStatus>("device.status", currentDeviceStatus); >> > + rpiMetadata_ = std::move(lastMetadata); >> > + rpiMetadata_.Set("device.status", currentDeviceStatus); >> > + processPending_ = false; >> > + LOG(IPARPI, Debug) << "Rate-limiting the controller! inter-frame duration: " >> > + << frameTimestamp - lastRunTimestamp_ >> > + << ", min duration " >> > + << controllerMinFrameDuration * 1e3; >> > + return; >> > + } >> > + >> > + lastRunTimestamp_ = frameTimestamp; >> > + processPending_ = true; >> > + >> > ControlList ctrls(ispCtrls_); >> >> Hmm, yes, I see there's an interesting manoeuvre going on with the >> metadata. I wonder if there's a way to rearrange this that involves a >> bit less shuffling, maybe: >> >> first delete these lines: >> + lastMetadata = std::move(rpiMetadata_); >> fillDeviceStatus(data.controls); >> >> next, in place of: >> + DeviceStatus currentDeviceStatus; >> + >> + rpiMetadata_.Get<DeviceStatus>("device.status", >> currentDeviceStatus); >> + rpiMetadata_ = std::move(lastMetadata); >> + rpiMetadata_.Set("device.status", currentDeviceStatus); >> >> do this: >> fillDeviceStatus(data.controls); >> >> and finally after the if-block for the "skip the algos" case put these back: >> rpiMetadata_.Clear(); >> fillDeviceStatus(data.controls); > > > I did try this type of logic initially, but I don't think it would work. > > We need to call rpiMetadata_.Clear() and fillDeviceStatus() before > helper_->Prepare() is called at the beginning of the function. I cannot > move the latter call further down as it needs to run before my skip algos > check and the returnEmbeddedBuffer() call. So I think the existing manoeuvre > is needed without more refactoring. Ah yes, indeed, you're right - that helper_->Prepare() is troublesome. Back to Plan A then... Actually one more little annoyance comes to mind. What happens if helper_->Prepare() gives us more metadata than just the DeviceStatus. Would we expect that to get copied over the previous metadata too? In practice I would guess it won't matter if that metadata can't escape to the application, though I think we can already imagine future sensors where that might happen. What do you think? Would a "merge" method (which "moves" the data items) be the fix for this? Am I over-thinking? Best regards David > > Regards, > Naush > >> >> >> I don't know if this ends up tidier or not, perhaps it's best if you >> decide what you prefer. But I'm fine either way, so: >> >> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> >> >> Thanks! >> David >> >> > >> > 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); >> > -- >> > 2.25.1 >> >
Hi David, On Fri, 16 Apr 2021, 5:00 pm David Plowman, <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for explaining. > > On Fri, 16 Apr 2021 at 15:36, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > Hi David, > > > > Thank you for your feedback. > > > > On Fri, 16 Apr 2021 at 14:44, David Plowman < > david.plowman@raspberrypi.com> wrote: > >> > >> Hi Naush > >> > >> Thanks for version 2 of this patch, and especially for re-basing on > >> top of my still-pending patch! > >> > >> Weirdly this one, which I'd have expected to be the "3/3" has ended up > >> as a reply to the cover note, so not sure what's going on there. But > >> anyway... > > > > > > I think that is gmail causing issues by bunching 0/3 and 3/3 into a > conversation > > for some reason, even though the subjects are (slightly) different. > Happens > > on my view as well! > > > >> > >> > >> On Fri, 16 Apr 2021 at 11:31, 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. > >> > >> Is it worth a remark on how we don't rate-limit while dropping frames? > >> I don't really mind, though. > >> > >> > >> > > >> > 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..e96b169ca612 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 > >> > >> Strictly speaking, I guess they're still Prepare() and Process()... > >> (until such time as the lower-casing elves strike those files!) > >> > >> > + * 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); > >> > > >> > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > >> > + frameTimestamp - lastRunTimestamp_ < > 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. All other > metadata values > >> > + * must remain the same as the last frame. > >> > + */ > >> > + DeviceStatus currentDeviceStatus; > >> > + > >> > + rpiMetadata_.Get<DeviceStatus>("device.status", > currentDeviceStatus); > >> > + rpiMetadata_ = std::move(lastMetadata); > >> > + rpiMetadata_.Set("device.status", > currentDeviceStatus); > >> > + processPending_ = false; > >> > + LOG(IPARPI, Debug) << "Rate-limiting the controller! > inter-frame duration: " > >> > + << frameTimestamp - > lastRunTimestamp_ > >> > + << ", min duration " > >> > + << controllerMinFrameDuration * > 1e3; > >> > + return; > >> > + } > >> > + > >> > + lastRunTimestamp_ = frameTimestamp; > >> > + processPending_ = true; > >> > + > >> > ControlList ctrls(ispCtrls_); > >> > >> Hmm, yes, I see there's an interesting manoeuvre going on with the > >> metadata. I wonder if there's a way to rearrange this that involves a > >> bit less shuffling, maybe: > >> > >> first delete these lines: > >> + lastMetadata = std::move(rpiMetadata_); > >> fillDeviceStatus(data.controls); > >> > >> next, in place of: > >> + DeviceStatus currentDeviceStatus; > >> + > >> + rpiMetadata_.Get<DeviceStatus>("device.status", > >> currentDeviceStatus); > >> + rpiMetadata_ = std::move(lastMetadata); > >> + rpiMetadata_.Set("device.status", currentDeviceStatus); > >> > >> do this: > >> fillDeviceStatus(data.controls); > >> > >> and finally after the if-block for the "skip the algos" case put these > back: > >> rpiMetadata_.Clear(); > >> fillDeviceStatus(data.controls); > > > > > > I did try this type of logic initially, but I don't think it would work. > > > > We need to call rpiMetadata_.Clear() and fillDeviceStatus() before > > helper_->Prepare() is called at the beginning of the function. I cannot > > move the latter call further down as it needs to run before my skip algos > > check and the returnEmbeddedBuffer() call. So I think the existing > manoeuvre > > is needed without more refactoring. > > Ah yes, indeed, you're right - that helper_->Prepare() is troublesome. > Back to Plan A then... > > Actually one more little annoyance comes to mind. What happens if > helper_->Prepare() gives us more metadata than just the DeviceStatus. > Would we expect that to get copied over the previous metadata too? In > practice I would guess it won't matter if that metadata can't escape > to the application, though I think we can already imagine future > sensors where that might happen. What do you think? Would a "merge" > method (which "moves" the data items) be the fix for this? Am I > over-thinking? > I think this is one to worry about in the future. A merge method would be useful in these situations, but given that we only really care about exposure and gain right now, propose we kick the can down the road. Regards, Naush > Best regards > David > > > > > Regards, > > Naush > > > >> > >> > >> I don't know if this ends up tidier or not, perhaps it's best if you > >> decide what you prefer. But I'm fine either way, so: > >> > >> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > >> > >> Thanks! > >> David > >> > >> > > >> > 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); > >> > -- > >> > 2.25.1 > >> > >
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index f6d1ab16a290..e96b169ca612 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); + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && + frameTimestamp - lastRunTimestamp_ < 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. All other metadata values + * must remain the same as the last frame. + */ + DeviceStatus currentDeviceStatus; + + rpiMetadata_.Get<DeviceStatus>("device.status", currentDeviceStatus); + rpiMetadata_ = std::move(lastMetadata); + rpiMetadata_.Set("device.status", currentDeviceStatus); + processPending_ = false; + LOG(IPARPI, Debug) << "Rate-limiting the controller! inter-frame duration: " + << frameTimestamp - lastRunTimestamp_ + << ", min duration " + << controllerMinFrameDuration * 1e3; + 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. 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(-)