| Message ID | 20210419133451.263733-6-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Mon, Apr 19, 2021 at 02:34:51PM +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. Out of curiosity, is the inter-frame time something you would envision being made dynamic in the future, maybe coming from the IPA configuration file ? > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 50 +++++++++++++++++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++ > 2 files changed, 51 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index f6d1ab16a290..eac1cf8cfe44 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) { This doesn't apply cleanly. I'm afraid I've lost track, is this series based on another pending series, or have other patches been merged in the meantime that would require a rebase ? > @@ -920,6 +936,32 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) > if (data.embeddedBufferPresent) > returnEmbeddedBuffer(data.embeddedBufferId); > > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > + frameTimestamp - lastRunTimestamp_ < controllerMinFrameDuration * 1e3) { The intent, I assume, is to run the algorithms at max 60fps. Shouldn't we allow for an error margin in the comparison ? Otherwise, with the jitter, we would skip IPA runs for some frames when running at exactly 60fps. > + /* > + * 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 merging the current frame's metadata into the > + * previous frame's metadata object, and then moving the latter > + * into the former. > + */ > + lastMetadata.Merge(rpiMetadata_); > + rpiMetadata_ = std::move(lastMetadata); > + processPending_ = false; > + LOG(IPARPI, Debug) << "Rate-limiting the controller! inter-frame duration: " > + << frameTimestamp - lastRunTimestamp_ > + << ", min duration: " > + << controllerMinFrameDuration * 1e3; That would result in loooots of debug messages with a > 60fps frame rate. Is it intended ? > + 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 Thanks for the review. Answering at least one of your questions for Naush (as it relates to one of my patches)... On Tue, 27 Apr 2021 at 08:34, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Mon, Apr 19, 2021 at 02:34:51PM +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. > > Out of curiosity, is the inter-frame time something you would envision > being made dynamic in the future, maybe coming from the IPA > configuration file ? > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 50 +++++++++++++++++-- > > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++ > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index f6d1ab16a290..eac1cf8cfe44 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) { > > This doesn't apply cleanly. I'm afraid I've lost track, is this series > based on another pending series, or have other patches been merged in > the meantime that would require a rebase ? I believe Naush helpfully rebased this patch on top of my " ipa: raspberrypi: Use CamHelpers to generalise sensor metadata parsing" patch. I think that one is still pending...? Thanks David > > > @@ -920,6 +936,32 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data) > > if (data.embeddedBufferPresent) > > returnEmbeddedBuffer(data.embeddedBufferId); > > > > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > > + frameTimestamp - lastRunTimestamp_ < controllerMinFrameDuration * 1e3) { > > The intent, I assume, is to run the algorithms at max 60fps. Shouldn't > we allow for an error margin in the comparison ? Otherwise, with the > jitter, we would skip IPA runs for some frames when running at exactly > 60fps. > > > + /* > > + * 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 merging the current frame's metadata into the > > + * previous frame's metadata object, and then moving the latter > > + * into the former. > > + */ > > + lastMetadata.Merge(rpiMetadata_); > > + rpiMetadata_ = std::move(lastMetadata); > > + processPending_ = false; > > + LOG(IPARPI, Debug) << "Rate-limiting the controller! inter-frame duration: " > > + << frameTimestamp - lastRunTimestamp_ > > + << ", min duration: " > > + << controllerMinFrameDuration * 1e3; > > That would result in loooots of debug messages with a > 60fps frame > rate. Is it intended ? > > > + 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 > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thank you for your review feedback. On Tue, 27 Apr 2021 at 08:34, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Mon, Apr 19, 2021 at 02:34:51PM +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. > > Out of curiosity, is the inter-frame time something you would envision > being made dynamic in the future, maybe coming from the IPA > configuration file ? > I did question this myself, and to be honest I don't really know. Initially I decided to make it a constant, but did wonder if we should make it a config parameter for devices that are somewhat underpowered (e.g. Pi Zero). In the end, I went with a constant value to avoid users having another config parameter to think about. If we do decide the other way, it is easy enough to switch. > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 50 +++++++++++++++++-- > > .../pipeline/raspberrypi/raspberrypi.cpp | 5 ++ > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index f6d1ab16a290..eac1cf8cfe44 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) { > > This doesn't apply cleanly. I'm afraid I've lost track, is this series > based on another pending series, or have other patches been merged in > the meantime that would require a rebase ? > This commit will need to be applied over https://patchwork.libcamera.org/patch/11730/ > > > @@ -920,6 +936,32 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig > &data) > > if (data.embeddedBufferPresent) > > returnEmbeddedBuffer(data.embeddedBufferId); > > > > + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && > > + frameTimestamp - lastRunTimestamp_ < > controllerMinFrameDuration * 1e3) { > > The intent, I assume, is to run the algorithms at max 60fps. Shouldn't > we allow for an error margin in the comparison ? Otherwise, with the > jitter, we would skip IPA runs for some frames when running at exactly > 60fps. > Good point, I will add a 10% margin on the comparison above. > > > + /* > > + * 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 merging the current frame's metadata into > the > > + * previous frame's metadata object, and then moving the > latter > > + * into the former. > > + */ > > + lastMetadata.Merge(rpiMetadata_); > > + rpiMetadata_ = std::move(lastMetadata); > > + processPending_ = false; > > + LOG(IPARPI, Debug) << "Rate-limiting the controller! > inter-frame duration: " > > + << frameTimestamp - lastRunTimestamp_ > > + << ", min duration: " > > + << controllerMinFrameDuration * 1e3; > > That would result in loooots of debug messages with a > 60fps frame > rate. Is it intended ? > No, I certainly don't want to be too noisy. What is the overhead of adding a log point like this, but the logging level is set to not display the message? I would like to keep some indication of us rate-limiting the IPA run if possible, but not at the cost of excess overhead. Regards, Naush > > > + 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..eac1cf8cfe44 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,32 @@ 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. 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 merging the current frame's metadata into the + * previous frame's metadata object, and then moving the latter + * into the former. + */ + lastMetadata.Merge(rpiMetadata_); + rpiMetadata_ = std::move(lastMetadata); + 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);