| Message ID | 20221019090107.19975-7-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:06) > Pass an IPA cookie from the pipeline handler to the IPA and eventually back to > the pipeline handler through the setDelayedControls signal. This cookie is used > to index the RPiController::Metadata object to be used for the frame. > > The IPA cookie is then returned from DelayedControls when the frame with the > applied controls has been returned from the sensor, and eventually passed back > to the IPA from the signalIspPrepare signal. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > Tested-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/ipa/raspberrypi.mojom | 4 +++- > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++----- > .../pipeline/raspberrypi/raspberrypi.cpp | 19 +++++++++++++------ > 3 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > index 40f78d9e3b3f..bb5abd895262 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -37,6 +37,8 @@ struct ISPConfig { > uint32 bayerBufferId; > bool embeddedBufferPresent; > libcamera.ControlList controls; > + uint32 ipaCookie; > + uint32 delayCookie; > }; > > struct IPAConfig { > @@ -137,5 +139,5 @@ interface IPARPiEventInterface { > runIsp(uint32 bufferId); > embeddedComplete(uint32 bufferId); > setIspControls(libcamera.ControlList controls); > - setDelayedControls(libcamera.ControlList controls); > + setDelayedControls(libcamera.ControlList controls, uint32 delayCookie); > }; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 9e7792f5dfbe..aed8f68aded9 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -168,6 +168,7 @@ private: > RPiController::Controller controller_; > std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > unsigned int metadataIdx_; > + unsigned int lastMetadataIdx_; > > /* > * We count frames to decide if the frame must be hidden (e.g. from > @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig) > > firstStart_ = false; > lastRunTimestamp_ = 0; > - metadataIdx_ = 0; > } > > void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) > @@ -535,6 +535,8 @@ void IPARPi::signalIspPrepare(const ISPConfig &data) > * avoid running the control algos for a few frames in case > * they are "unreliable". > */ > + lastMetadataIdx_ = metadataIdx_; > + metadataIdx_ = data.ipaCookie % rpiMetadata_.size(); I really fear this metadataIdx_ is a code smell ... but I am tempted to put a peg on my nose ... > prepareISP(data); > frameCount_++; > > @@ -1011,11 +1013,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId) > void IPARPi::prepareISP(const ISPConfig &data) > { > int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0); > - RPiController::Metadata lastMetadata; > RPiController::Metadata &rpiMetadata = rpiMetadata_[metadataIdx_]; > Span<uint8_t> embeddedBuffer; > > - lastMetadata = std::move(rpiMetadata); > + rpiMetadata.clear(); > fillDeviceStatus(data.controls); > > if (data.embeddedBufferPresent) { > @@ -1048,7 +1049,8 @@ void IPARPi::prepareISP(const ISPConfig &data) > * current frame, or any other bits of metadata that were added > * in helper_->Prepare(). > */ > - rpiMetadata.merge(lastMetadata); > + RPiController::Metadata &lastMetadata = rpiMetadata_[lastMetadataIdx_]; > + rpiMetadata.mergeCopy(lastMetadata); > processPending_ = false; > return; > } > @@ -1147,7 +1149,7 @@ void IPARPi::processStats(unsigned int bufferId) > ControlList ctrls(sensorCtrls_); > applyAGC(&agcStatus, ctrls); > > - setDelayedControls.emit(ctrls); > + setDelayedControls.emit(ctrls, metadataIdx_); > } > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 23f2460190f4..8f6c6c0ce89f 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -206,7 +206,7 @@ public: > void runIsp(uint32_t bufferId); > void embeddedComplete(uint32_t bufferId); > void setIspControls(const ControlList &controls); > - void setDelayedControls(const ControlList &controls); > + void setDelayedControls(const ControlList &controls, uint32_t delayCookie); > void setSensorControls(ControlList &controls); > void unicamTimeout(); > > @@ -262,6 +262,7 @@ public: > struct BayerFrame { > FrameBuffer *buffer; > ControlList controls; > + unsigned int delayCookie; > }; > > std::queue<BayerFrame> bayerQueue_; > @@ -294,6 +295,9 @@ public: > /* Have internal buffers been allocated? */ > bool buffersAllocated_; > > + /* Frame cookie to pass to the IPA */ > + unsigned int ipaCookie_; > + > private: > void checkRequestCompleted(); > void fillRequestMetadata(const ControlList &bufferControls, > @@ -1064,7 +1068,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > * Reset the delayed controls with the gain and exposure values set by > * the IPA. > */ > - data->delayedCtrls_->reset(); > + data->ipaCookie_ = 0; > + data->delayedCtrls_->reset(data->ipaCookie_); I don't think I've understood why resetting delayed controls desires a cookie to be given. I guess it's so that the 'starting' condition is known? And perhaps it shouldn't always be zero? > data->state_ = RPiCameraData::State::Idle; > > @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const ControlList &controls) > handleState(); > } > > -void RPiCameraData::setDelayedControls(const ControlList &controls) > +void RPiCameraData::setDelayedControls(const ControlList &controls, uint32_t delayCookie) > { > - if (!delayedCtrls_->push(controls)) > + if (!delayedCtrls_->push(controls, delayCookie)) > LOG(RPI, Error) << "V4L2 DelayedControl set failed"; > handleState(); > } > @@ -1867,13 +1872,13 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > * Lookup the sensor controls used for this frame sequence from > * DelayedControl and queue them along with the frame buffer. > */ > - auto [ctrl, cookie] = delayedCtrls_->get(buffer->metadata().sequence); > + auto [ctrl, delayCookie] = 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) }); > + bayerQueue_.push({ buffer, std::move(ctrl), delayCookie }); > } else { > embeddedQueue_.push(buffer); > } > @@ -2168,6 +2173,8 @@ void RPiCameraData::tryRunPipeline() > ipa::RPi::ISPConfig ispPrepare; > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId; > ispPrepare.controls = std::move(bayerFrame.controls); > + ispPrepare.ipaCookie = ipaCookie_++; I'd be intrigued to know if this should be the request sequence number, as we would use in the RKISP/IPU3, rather than a new sequence number. But I suspect it doesn't matter. It depends how you want to reference the completed metadata against a queued request, which I think you are not too worried about explicitly like that. Small worries aside, I expect this is working... so it's your discretion if you think the cookies/sequences should be handled differently. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + ispPrepare.delayCookie = bayerFrame.delayCookie; > > if (embeddedBuffer) { > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > -- > 2.25.1 >
Hi Kieran, Thanks for all the reviews! On Fri, 28 Oct 2022 at 13:34, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:06) > > Pass an IPA cookie from the pipeline handler to the IPA and eventually > back to > > the pipeline handler through the setDelayedControls signal. This cookie > is used > > to index the RPiController::Metadata object to be used for the frame. > > > > The IPA cookie is then returned from DelayedControls when the frame with > the > > applied controls has been returned from the sensor, and eventually > passed back > > to the IPA from the signalIspPrepare signal. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/ipa/raspberrypi.mojom | 4 +++- > > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++----- > > .../pipeline/raspberrypi/raspberrypi.cpp | 19 +++++++++++++------ > > 3 files changed, 23 insertions(+), 12 deletions(-) > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > > index 40f78d9e3b3f..bb5abd895262 100644 > > --- a/include/libcamera/ipa/raspberrypi.mojom > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > @@ -37,6 +37,8 @@ struct ISPConfig { > > uint32 bayerBufferId; > > bool embeddedBufferPresent; > > libcamera.ControlList controls; > > + uint32 ipaCookie; > > + uint32 delayCookie; > > }; > > > > struct IPAConfig { > > @@ -137,5 +139,5 @@ interface IPARPiEventInterface { > > runIsp(uint32 bufferId); > > embeddedComplete(uint32 bufferId); > > setIspControls(libcamera.ControlList controls); > > - setDelayedControls(libcamera.ControlList controls); > > + setDelayedControls(libcamera.ControlList controls, uint32 > delayCookie); > > }; > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index 9e7792f5dfbe..aed8f68aded9 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -168,6 +168,7 @@ private: > > RPiController::Controller controller_; > > std::array<RPiController::Metadata, numMetadataContexts> > rpiMetadata_; > > unsigned int metadataIdx_; > > + unsigned int lastMetadataIdx_; > > > > /* > > * We count frames to decide if the frame must be hidden (e.g. > from > > @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList &controls, > StartConfig *startConfig) > > > > firstStart_ = false; > > lastRunTimestamp_ = 0; > > - metadataIdx_ = 0; > > } > > > > void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) > > @@ -535,6 +535,8 @@ void IPARPi::signalIspPrepare(const ISPConfig &data) > > * avoid running the control algos for a few frames in case > > * they are "unreliable". > > */ > > + lastMetadataIdx_ = metadataIdx_; > > + metadataIdx_ = data.ipaCookie % rpiMetadata_.size(); > > I really fear this metadataIdx_ is a code smell ... but I am tempted to > put a peg on my nose ... > Perhaps this is better named context_? and ipaCookie could also be renamed ipaContext? > > > prepareISP(data); > > frameCount_++; > > > > @@ -1011,11 +1013,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned int > bufferId) > > void IPARPi::prepareISP(const ISPConfig &data) > > { > > int64_t frameTimestamp = > data.controls.get(controls::SensorTimestamp).value_or(0); > > - RPiController::Metadata lastMetadata; > > RPiController::Metadata &rpiMetadata = > rpiMetadata_[metadataIdx_]; > > Span<uint8_t> embeddedBuffer; > > > > - lastMetadata = std::move(rpiMetadata); > > + rpiMetadata.clear(); > > fillDeviceStatus(data.controls); > > > > if (data.embeddedBufferPresent) { > > @@ -1048,7 +1049,8 @@ void IPARPi::prepareISP(const ISPConfig &data) > > * current frame, or any other bits of metadata that > were added > > * in helper_->Prepare(). > > */ > > - rpiMetadata.merge(lastMetadata); > > + RPiController::Metadata &lastMetadata = > rpiMetadata_[lastMetadataIdx_]; > > + rpiMetadata.mergeCopy(lastMetadata); > > processPending_ = false; > > return; > > } > > @@ -1147,7 +1149,7 @@ void IPARPi::processStats(unsigned int bufferId) > > ControlList ctrls(sensorCtrls_); > > applyAGC(&agcStatus, ctrls); > > > > - setDelayedControls.emit(ctrls); > > + setDelayedControls.emit(ctrls, metadataIdx_); > > } > > } > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 23f2460190f4..8f6c6c0ce89f 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -206,7 +206,7 @@ public: > > void runIsp(uint32_t bufferId); > > void embeddedComplete(uint32_t bufferId); > > void setIspControls(const ControlList &controls); > > - void setDelayedControls(const ControlList &controls); > > + void setDelayedControls(const ControlList &controls, uint32_t > delayCookie); > > void setSensorControls(ControlList &controls); > > void unicamTimeout(); > > > > @@ -262,6 +262,7 @@ public: > > struct BayerFrame { > > FrameBuffer *buffer; > > ControlList controls; > > + unsigned int delayCookie; > > }; > > > > std::queue<BayerFrame> bayerQueue_; > > @@ -294,6 +295,9 @@ public: > > /* Have internal buffers been allocated? */ > > bool buffersAllocated_; > > > > + /* Frame cookie to pass to the IPA */ > > + unsigned int ipaCookie_; > > + > > private: > > void checkRequestCompleted(); > > void fillRequestMetadata(const ControlList &bufferControls, > > @@ -1064,7 +1068,8 @@ int PipelineHandlerRPi::start(Camera *camera, > const ControlList *controls) > > * Reset the delayed controls with the gain and exposure values > set by > > * the IPA. > > */ > > - data->delayedCtrls_->reset(); > > + data->ipaCookie_ = 0; > > + data->delayedCtrls_->reset(data->ipaCookie_); > > I don't think I've understood why resetting delayed controls desires a > cookie to be given. I guess it's so that the 'starting' condition is > known? > Correct, it just assigns a cookie value to the starting conditions. > > And perhaps it shouldn't always be zero? > It does not strictly have to be 0, but if you consider it analogous to the frame sequence number, it makes sense for it to. > > > > data->state_ = RPiCameraData::State::Idle; > > > > @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const > ControlList &controls) > > handleState(); > > } > > > > -void RPiCameraData::setDelayedControls(const ControlList &controls) > > +void RPiCameraData::setDelayedControls(const ControlList &controls, > uint32_t delayCookie) > > { > > - if (!delayedCtrls_->push(controls)) > > + if (!delayedCtrls_->push(controls, delayCookie)) > > LOG(RPI, Error) << "V4L2 DelayedControl set failed"; > > handleState(); > > } > > @@ -1867,13 +1872,13 @@ void > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > * Lookup the sensor controls used for this frame > sequence from > > * DelayedControl and queue them along with the frame > buffer. > > */ > > - auto [ctrl, cookie] = > delayedCtrls_->get(buffer->metadata().sequence); > > + auto [ctrl, delayCookie] = > 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) }); > > + bayerQueue_.push({ buffer, std::move(ctrl), delayCookie > }); > > } else { > > embeddedQueue_.push(buffer); > > } > > @@ -2168,6 +2173,8 @@ void RPiCameraData::tryRunPipeline() > > ipa::RPi::ISPConfig ispPrepare; > > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId; > > ispPrepare.controls = std::move(bayerFrame.controls); > > + ispPrepare.ipaCookie = ipaCookie_++; > > I'd be intrigued to know if this should be the request sequence number, > as we would use in the RKISP/IPU3, rather than a new sequence number. > Yes, this would work, assuming the request sequence number is strictly monotonically increasing? Happy to make these changes if it matches closer to the ipu3/rkisp implementation. Regards, Naush > But I suspect it doesn't matter. It depends how you want to reference > the completed metadata against a queued request, which I think you are > not too worried about explicitly like that. > > Small worries aside, I expect this is working... so it's your discretion > if you think the cookies/sequences should be handled differently. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > + ispPrepare.delayCookie = bayerFrame.delayCookie; > > > > if (embeddedBuffer) { > > unsigned int embeddedId = > unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > -- > > 2.25.1 > > >
Quoting Naushir Patuck (2022-10-28 13:58:00) > Hi Kieran, > > Thanks for all the reviews! > > > On Fri, 28 Oct 2022 at 13:34, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:06) > > > Pass an IPA cookie from the pipeline handler to the IPA and eventually > > back to > > > the pipeline handler through the setDelayedControls signal. This cookie > > is used > > > to index the RPiController::Metadata object to be used for the frame. > > > > > > The IPA cookie is then returned from DelayedControls when the frame with > > the > > > applied controls has been returned from the sensor, and eventually > > passed back > > > to the IPA from the signalIspPrepare signal. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/ipa/raspberrypi.mojom | 4 +++- > > > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++----- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 19 +++++++++++++------ > > > 3 files changed, 23 insertions(+), 12 deletions(-) > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > > b/include/libcamera/ipa/raspberrypi.mojom > > > index 40f78d9e3b3f..bb5abd895262 100644 > > > --- a/include/libcamera/ipa/raspberrypi.mojom > > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > > @@ -37,6 +37,8 @@ struct ISPConfig { > > > uint32 bayerBufferId; > > > bool embeddedBufferPresent; > > > libcamera.ControlList controls; > > > + uint32 ipaCookie; > > > + uint32 delayCookie; > > > }; > > > > > > struct IPAConfig { > > > @@ -137,5 +139,5 @@ interface IPARPiEventInterface { > > > runIsp(uint32 bufferId); > > > embeddedComplete(uint32 bufferId); > > > setIspControls(libcamera.ControlList controls); > > > - setDelayedControls(libcamera.ControlList controls); > > > + setDelayedControls(libcamera.ControlList controls, uint32 > > delayCookie); > > > }; > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > > b/src/ipa/raspberrypi/raspberrypi.cpp > > > index 9e7792f5dfbe..aed8f68aded9 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -168,6 +168,7 @@ private: > > > RPiController::Controller controller_; > > > std::array<RPiController::Metadata, numMetadataContexts> > > rpiMetadata_; > > > unsigned int metadataIdx_; > > > + unsigned int lastMetadataIdx_; > > > > > > /* > > > * We count frames to decide if the frame must be hidden (e.g. > > from > > > @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList &controls, > > StartConfig *startConfig) > > > > > > firstStart_ = false; > > > lastRunTimestamp_ = 0; > > > - metadataIdx_ = 0; > > > } > > > > > > void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) > > > @@ -535,6 +535,8 @@ void IPARPi::signalIspPrepare(const ISPConfig &data) > > > * avoid running the control algos for a few frames in case > > > * they are "unreliable". > > > */ > > > + lastMetadataIdx_ = metadataIdx_; > > > + metadataIdx_ = data.ipaCookie % rpiMetadata_.size(); > > > > I really fear this metadataIdx_ is a code smell ... but I am tempted to > > put a peg on my nose ... > > > > Perhaps this is better named context_? and ipaCookie could also be renamed > ipaContext? I think it's more the nature of using this global variable to set which metadata to read, for events that in my head are 'asynchronous' (even though they can't be called in parallel). > > > > > > > prepareISP(data); > > > frameCount_++; > > > > > > @@ -1011,11 +1013,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned int > > bufferId) > > > void IPARPi::prepareISP(const ISPConfig &data) > > > { > > > int64_t frameTimestamp = > > data.controls.get(controls::SensorTimestamp).value_or(0); > > > - RPiController::Metadata lastMetadata; > > > RPiController::Metadata &rpiMetadata = > > rpiMetadata_[metadataIdx_]; > > > Span<uint8_t> embeddedBuffer; > > > > > > - lastMetadata = std::move(rpiMetadata); > > > + rpiMetadata.clear(); > > > fillDeviceStatus(data.controls); > > > > > > if (data.embeddedBufferPresent) { > > > @@ -1048,7 +1049,8 @@ void IPARPi::prepareISP(const ISPConfig &data) > > > * current frame, or any other bits of metadata that > > were added > > > * in helper_->Prepare(). > > > */ > > > - rpiMetadata.merge(lastMetadata); > > > + RPiController::Metadata &lastMetadata = > > rpiMetadata_[lastMetadataIdx_]; > > > + rpiMetadata.mergeCopy(lastMetadata); > > > processPending_ = false; > > > return; > > > } > > > @@ -1147,7 +1149,7 @@ void IPARPi::processStats(unsigned int bufferId) > > > ControlList ctrls(sensorCtrls_); > > > applyAGC(&agcStatus, ctrls); > > > > > > - setDelayedControls.emit(ctrls); > > > + setDelayedControls.emit(ctrls, metadataIdx_); > > > } > > > } > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 23f2460190f4..8f6c6c0ce89f 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -206,7 +206,7 @@ public: > > > void runIsp(uint32_t bufferId); > > > void embeddedComplete(uint32_t bufferId); > > > void setIspControls(const ControlList &controls); > > > - void setDelayedControls(const ControlList &controls); > > > + void setDelayedControls(const ControlList &controls, uint32_t > > delayCookie); > > > void setSensorControls(ControlList &controls); > > > void unicamTimeout(); > > > > > > @@ -262,6 +262,7 @@ public: > > > struct BayerFrame { > > > FrameBuffer *buffer; > > > ControlList controls; > > > + unsigned int delayCookie; > > > }; > > > > > > std::queue<BayerFrame> bayerQueue_; > > > @@ -294,6 +295,9 @@ public: > > > /* Have internal buffers been allocated? */ > > > bool buffersAllocated_; > > > > > > + /* Frame cookie to pass to the IPA */ > > > + unsigned int ipaCookie_; > > > + > > > private: > > > void checkRequestCompleted(); > > > void fillRequestMetadata(const ControlList &bufferControls, > > > @@ -1064,7 +1068,8 @@ int PipelineHandlerRPi::start(Camera *camera, > > const ControlList *controls) > > > * Reset the delayed controls with the gain and exposure values > > set by > > > * the IPA. > > > */ > > > - data->delayedCtrls_->reset(); > > > + data->ipaCookie_ = 0; > > > + data->delayedCtrls_->reset(data->ipaCookie_); > > > > I don't think I've understood why resetting delayed controls desires a > > cookie to be given. I guess it's so that the 'starting' condition is > > known? > > > > Correct, it just assigns a cookie value to the starting conditions. > > > > > > And perhaps it shouldn't always be zero? > > > > It does not strictly have to be 0, but if you consider it analogous to the > frame sequence number, it makes sense for it to. I suspect it's analogous to the controls that were set at the start() .. but I'm not sure that can be modelled easily, so keeping it as 0 seems reasonable. I'm just not sure it would ever make sense to be ... 1052 (some other number..) Anyway, I don't think it's crucial here. > > > data->state_ = RPiCameraData::State::Idle; > > > > > > @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const > > ControlList &controls) > > > handleState(); > > > } > > > > > > -void RPiCameraData::setDelayedControls(const ControlList &controls) > > > +void RPiCameraData::setDelayedControls(const ControlList &controls, > > uint32_t delayCookie) > > > { > > > - if (!delayedCtrls_->push(controls)) > > > + if (!delayedCtrls_->push(controls, delayCookie)) > > > LOG(RPI, Error) << "V4L2 DelayedControl set failed"; > > > handleState(); > > > } > > > @@ -1867,13 +1872,13 @@ void > > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > * Lookup the sensor controls used for this frame > > sequence from > > > * DelayedControl and queue them along with the frame > > buffer. > > > */ > > > - auto [ctrl, cookie] = > > delayedCtrls_->get(buffer->metadata().sequence); > > > + auto [ctrl, delayCookie] = > > 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) }); > > > + bayerQueue_.push({ buffer, std::move(ctrl), delayCookie > > }); > > > } else { > > > embeddedQueue_.push(buffer); > > > } > > > @@ -2168,6 +2173,8 @@ void RPiCameraData::tryRunPipeline() > > > ipa::RPi::ISPConfig ispPrepare; > > > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId; > > > ispPrepare.controls = std::move(bayerFrame.controls); > > > + ispPrepare.ipaCookie = ipaCookie_++; > > > > I'd be intrigued to know if this should be the request sequence number, > > as we would use in the RKISP/IPU3, rather than a new sequence number. > > > > Yes, this would work, assuming the request sequence number is strictly > monotonically increasing? Request sequences are 1 for 1 for every request that is queued. It's handled by - https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n410 It always starts from zero, and should always increase by one for every request queued to a Camera object. (It's stored in the CameraData). It gets reset at Camera::stop() time. Requests after that are sequenced from zero again. Note that any requests rejected and not queued will not increment the sequence number, as they won't make it into the pipeline handler: - https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1113 So actually, that could impact you depending on how you handle IPA contining through a start stop cycle. Do you expect to reference metadata from a previous streaming session? If it's an issue - keep it with your own counter. -- Kieran > > Happy to make these changes if it matches closer to the > ipu3/rkisp implementation. > > Regards, > Naush > > > > But I suspect it doesn't matter. It depends how you want to reference > > the completed metadata against a queued request, which I think you are > > not too worried about explicitly like that. > > > > Small worries aside, I expect this is working... so it's your discretion > > if you think the cookies/sequences should be handled differently. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > + ispPrepare.delayCookie = bayerFrame.delayCookie; > > > > > > if (embeddedBuffer) { > > > unsigned int embeddedId = > > unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > > -- > > > 2.25.1 > > > > >
On Fri, 28 Oct 2022 at 14:15, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2022-10-28 13:58:00) > > Hi Kieran, > > > > Thanks for all the reviews! > > > > > > On Fri, 28 Oct 2022 at 13:34, Kieran Bingham < > > kieran.bingham@ideasonboard.com> wrote: > > > > > Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:06) > > > > Pass an IPA cookie from the pipeline handler to the IPA and > eventually > > > back to > > > > the pipeline handler through the setDelayedControls signal. This > cookie > > > is used > > > > to index the RPiController::Metadata object to be used for the frame. > > > > > > > > The IPA cookie is then returned from DelayedControls when the frame > with > > > the > > > > applied controls has been returned from the sensor, and eventually > > > passed back > > > > to the IPA from the signalIspPrepare signal. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > include/libcamera/ipa/raspberrypi.mojom | 4 +++- > > > > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++----- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 19 > +++++++++++++------ > > > > 3 files changed, 23 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > > > b/include/libcamera/ipa/raspberrypi.mojom > > > > index 40f78d9e3b3f..bb5abd895262 100644 > > > > --- a/include/libcamera/ipa/raspberrypi.mojom > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > > > @@ -37,6 +37,8 @@ struct ISPConfig { > > > > uint32 bayerBufferId; > > > > bool embeddedBufferPresent; > > > > libcamera.ControlList controls; > > > > + uint32 ipaCookie; > > > > + uint32 delayCookie; > > > > }; > > > > > > > > struct IPAConfig { > > > > @@ -137,5 +139,5 @@ interface IPARPiEventInterface { > > > > runIsp(uint32 bufferId); > > > > embeddedComplete(uint32 bufferId); > > > > setIspControls(libcamera.ControlList controls); > > > > - setDelayedControls(libcamera.ControlList controls); > > > > + setDelayedControls(libcamera.ControlList controls, uint32 > > > delayCookie); > > > > }; > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > > > b/src/ipa/raspberrypi/raspberrypi.cpp > > > > index 9e7792f5dfbe..aed8f68aded9 100644 > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > @@ -168,6 +168,7 @@ private: > > > > RPiController::Controller controller_; > > > > std::array<RPiController::Metadata, numMetadataContexts> > > > rpiMetadata_; > > > > unsigned int metadataIdx_; > > > > + unsigned int lastMetadataIdx_; > > > > > > > > /* > > > > * We count frames to decide if the frame must be hidden > (e.g. > > > from > > > > @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList &controls, > > > StartConfig *startConfig) > > > > > > > > firstStart_ = false; > > > > lastRunTimestamp_ = 0; > > > > - metadataIdx_ = 0; > > > > } > > > > > > > > void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) > > > > @@ -535,6 +535,8 @@ void IPARPi::signalIspPrepare(const ISPConfig > &data) > > > > * avoid running the control algos for a few frames in case > > > > * they are "unreliable". > > > > */ > > > > + lastMetadataIdx_ = metadataIdx_; > > > > + metadataIdx_ = data.ipaCookie % rpiMetadata_.size(); > > > > > > I really fear this metadataIdx_ is a code smell ... but I am tempted to > > > put a peg on my nose ... > > > > > > > Perhaps this is better named context_? and ipaCookie could also be > renamed > > ipaContext? > > I think it's more the nature of using this global variable to set which > metadata to read, for events that in my head are 'asynchronous' (even > though they can't be called in parallel). > Storing the metadataIdx_ was a bit of a shortcut really. I could remove this variable and pass the same "cookie" into the process phase as well. Would that be cleaner? However, I do need to store lastMetadataIdx_ if I use the request sequence number as the cookie, and they do not increment by 1 on each call... see below. > > > > > > > > > > > > prepareISP(data); > > > > frameCount_++; > > > > > > > > @@ -1011,11 +1013,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned > int > > > bufferId) > > > > void IPARPi::prepareISP(const ISPConfig &data) > > > > { > > > > int64_t frameTimestamp = > > > data.controls.get(controls::SensorTimestamp).value_or(0); > > > > - RPiController::Metadata lastMetadata; > > > > RPiController::Metadata &rpiMetadata = > > > rpiMetadata_[metadataIdx_]; > > > > Span<uint8_t> embeddedBuffer; > > > > > > > > - lastMetadata = std::move(rpiMetadata); > > > > + rpiMetadata.clear(); > > > > fillDeviceStatus(data.controls); > > > > > > > > if (data.embeddedBufferPresent) { > > > > @@ -1048,7 +1049,8 @@ void IPARPi::prepareISP(const ISPConfig &data) > > > > * current frame, or any other bits of metadata that > > > were added > > > > * in helper_->Prepare(). > > > > */ > > > > - rpiMetadata.merge(lastMetadata); > > > > + RPiController::Metadata &lastMetadata = > > > rpiMetadata_[lastMetadataIdx_]; > > > > + rpiMetadata.mergeCopy(lastMetadata); > > > > processPending_ = false; > > > > return; > > > > } > > > > @@ -1147,7 +1149,7 @@ void IPARPi::processStats(unsigned int > bufferId) > > > > ControlList ctrls(sensorCtrls_); > > > > applyAGC(&agcStatus, ctrls); > > > > > > > > - setDelayedControls.emit(ctrls); > > > > + setDelayedControls.emit(ctrls, metadataIdx_); > > > > } > > > > } > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 23f2460190f4..8f6c6c0ce89f 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -206,7 +206,7 @@ public: > > > > void runIsp(uint32_t bufferId); > > > > void embeddedComplete(uint32_t bufferId); > > > > void setIspControls(const ControlList &controls); > > > > - void setDelayedControls(const ControlList &controls); > > > > + void setDelayedControls(const ControlList &controls, uint32_t > > > delayCookie); > > > > void setSensorControls(ControlList &controls); > > > > void unicamTimeout(); > > > > > > > > @@ -262,6 +262,7 @@ public: > > > > struct BayerFrame { > > > > FrameBuffer *buffer; > > > > ControlList controls; > > > > + unsigned int delayCookie; > > > > }; > > > > > > > > std::queue<BayerFrame> bayerQueue_; > > > > @@ -294,6 +295,9 @@ public: > > > > /* Have internal buffers been allocated? */ > > > > bool buffersAllocated_; > > > > > > > > + /* Frame cookie to pass to the IPA */ > > > > + unsigned int ipaCookie_; > > > > + > > > > private: > > > > void checkRequestCompleted(); > > > > void fillRequestMetadata(const ControlList &bufferControls, > > > > @@ -1064,7 +1068,8 @@ int PipelineHandlerRPi::start(Camera *camera, > > > const ControlList *controls) > > > > * Reset the delayed controls with the gain and exposure > values > > > set by > > > > * the IPA. > > > > */ > > > > - data->delayedCtrls_->reset(); > > > > + data->ipaCookie_ = 0; > > > > + data->delayedCtrls_->reset(data->ipaCookie_); > > > > > > I don't think I've understood why resetting delayed controls desires a > > > cookie to be given. I guess it's so that the 'starting' condition is > > > known? > > > > > > > Correct, it just assigns a cookie value to the starting conditions. > > > > > > > > > > And perhaps it shouldn't always be zero? > > > > > > > It does not strictly have to be 0, but if you consider it analogous to > the > > frame sequence number, it makes sense for it to. > > I suspect it's analogous to the controls that were set at the start() .. > but I'm not sure that can be modelled easily, so keeping it as 0 seems > reasonable. > > I'm just not sure it would ever make sense to be ... 1052 (some other > number..) > > Anyway, I don't think it's crucial here. > > > > > > data->state_ = RPiCameraData::State::Idle; > > > > > > > > @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const > > > ControlList &controls) > > > > handleState(); > > > > } > > > > > > > > -void RPiCameraData::setDelayedControls(const ControlList &controls) > > > > +void RPiCameraData::setDelayedControls(const ControlList &controls, > > > uint32_t delayCookie) > > > > { > > > > - if (!delayedCtrls_->push(controls)) > > > > + if (!delayedCtrls_->push(controls, delayCookie)) > > > > LOG(RPI, Error) << "V4L2 DelayedControl set failed"; > > > > handleState(); > > > > } > > > > @@ -1867,13 +1872,13 @@ void > > > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > * Lookup the sensor controls used for this frame > > > sequence from > > > > * DelayedControl and queue them along with the frame > > > buffer. > > > > */ > > > > - auto [ctrl, cookie] = > > > delayedCtrls_->get(buffer->metadata().sequence); > > > > + auto [ctrl, delayCookie] = > > > 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) }); > > > > + bayerQueue_.push({ buffer, std::move(ctrl), > delayCookie > > > }); > > > > } else { > > > > embeddedQueue_.push(buffer); > > > > } > > > > @@ -2168,6 +2173,8 @@ void RPiCameraData::tryRunPipeline() > > > > ipa::RPi::ISPConfig ispPrepare; > > > > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId; > > > > ispPrepare.controls = std::move(bayerFrame.controls); > > > > + ispPrepare.ipaCookie = ipaCookie_++; > > > > > > I'd be intrigued to know if this should be the request sequence number, > > > as we would use in the RKISP/IPU3, rather than a new sequence number. > > > > > > > Yes, this would work, assuming the request sequence number is strictly > > monotonically increasing? > > Request sequences are 1 for 1 for every request that is queued. It's > handled by > > - > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n410 > > It always starts from zero, and should always increase by one for every > request queued to a Camera object. (It's stored in the CameraData). > > It gets reset at Camera::stop() time. Requests after that are > sequenced from zero again. > > Note that any requests rejected and not queued will not increment the > sequence number, as they won't make it into the pipeline handler: > > - > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1113 > > > So actually, that could impact you depending on how you handle IPA > contining through a start stop cycle. Do you expect to reference > metadata from a previous streaming session? > > If it's an issue - keep it with your own counter. > We don't need to reference metadata from previous streaming sessions, so assuming the request counter that the pipeline handler sees are monotonically increasing by 1, I can switch to that. So perhaps it does make sense to switch to request sequence numbers...? Naush > > -- > Kieran > > > > > > Happy to make these changes if it matches closer to the > > ipu3/rkisp implementation. > > > > Regards, > > Naush > > > > > > > But I suspect it doesn't matter. It depends how you want to reference > > > the completed metadata against a queued request, which I think you are > > > not too worried about explicitly like that. > > > > > > Small worries aside, I expect this is working... so it's your > discretion > > > if you think the cookies/sequences should be handled differently. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > > > > > > + ispPrepare.delayCookie = bayerFrame.delayCookie; > > > > > > > > if (embeddedBuffer) { > > > > unsigned int embeddedId = > > > unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > > > -- > > > > 2.25.1 > > > > > > > >
Quoting Naushir Patuck (2022-10-28 14:26:11) > On Fri, 28 Oct 2022 at 14:15, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Quoting Naushir Patuck (2022-10-28 13:58:00) > > > Hi Kieran, > > > > > > Thanks for all the reviews! > > > > > > > > > On Fri, 28 Oct 2022 at 13:34, Kieran Bingham < > > > kieran.bingham@ideasonboard.com> wrote: > > > > > > > Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:06) > > > > > Pass an IPA cookie from the pipeline handler to the IPA and > > eventually > > > > back to > > > > > the pipeline handler through the setDelayedControls signal. This > > cookie > > > > is used > > > > > to index the RPiController::Metadata object to be used for the frame. > > > > > > > > > > The IPA cookie is then returned from DelayedControls when the frame > > with > > > > the > > > > > applied controls has been returned from the sensor, and eventually > > > > passed back > > > > > to the IPA from the signalIspPrepare signal. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > include/libcamera/ipa/raspberrypi.mojom | 4 +++- > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++----- > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 19 > > +++++++++++++------ > > > > > 3 files changed, 23 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > > > > b/include/libcamera/ipa/raspberrypi.mojom > > > > > index 40f78d9e3b3f..bb5abd895262 100644 > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > > > > @@ -37,6 +37,8 @@ struct ISPConfig { > > > > > uint32 bayerBufferId; > > > > > bool embeddedBufferPresent; > > > > > libcamera.ControlList controls; > > > > > + uint32 ipaCookie; > > > > > + uint32 delayCookie; > > > > > }; > > > > > > > > > > struct IPAConfig { > > > > > @@ -137,5 +139,5 @@ interface IPARPiEventInterface { > > > > > runIsp(uint32 bufferId); > > > > > embeddedComplete(uint32 bufferId); > > > > > setIspControls(libcamera.ControlList controls); > > > > > - setDelayedControls(libcamera.ControlList controls); > > > > > + setDelayedControls(libcamera.ControlList controls, uint32 > > > > delayCookie); > > > > > }; > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > > > > b/src/ipa/raspberrypi/raspberrypi.cpp > > > > > index 9e7792f5dfbe..aed8f68aded9 100644 > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > > @@ -168,6 +168,7 @@ private: > > > > > RPiController::Controller controller_; > > > > > std::array<RPiController::Metadata, numMetadataContexts> > > > > rpiMetadata_; > > > > > unsigned int metadataIdx_; > > > > > + unsigned int lastMetadataIdx_; > > > > > > > > > > /* > > > > > * We count frames to decide if the frame must be hidden > > (e.g. > > > > from > > > > > @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList &controls, > > > > StartConfig *startConfig) > > > > > > > > > > firstStart_ = false; > > > > > lastRunTimestamp_ = 0; > > > > > - metadataIdx_ = 0; > > > > > } > > > > > > > > > > void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) > > > > > @@ -535,6 +535,8 @@ void IPARPi::signalIspPrepare(const ISPConfig > > &data) > > > > > * avoid running the control algos for a few frames in case > > > > > * they are "unreliable". > > > > > */ > > > > > + lastMetadataIdx_ = metadataIdx_; > > > > > + metadataIdx_ = data.ipaCookie % rpiMetadata_.size(); > > > > > > > > I really fear this metadataIdx_ is a code smell ... but I am tempted to > > > > put a peg on my nose ... > > > > > > > > > > Perhaps this is better named context_? and ipaCookie could also be > > renamed > > > ipaContext? > > > > I think it's more the nature of using this global variable to set which > > metadata to read, for events that in my head are 'asynchronous' (even > > though they can't be called in parallel). > > > > Storing the metadataIdx_ was a bit of a shortcut really. I could remove > this variable and pass the same "cookie" into the process phase as well. > Would that be cleaner? It's the 'shortcut' that worried me yes, but I can look away if you really want it. > However, I do need to store lastMetadataIdx_ if I use the request sequence > number as the cookie, and they do not increment by 1 on each call... see > below. I think you're inferring that this is ok because they do ? Of course be wary of request sequence 0, with 'lastMetadataIdx_ = -1' in that case. What do you obtain from the 'previous' metadata? I think we were going to store the 'previous' - but we don't - we keep an 'ActiveState' in our contexts, which keep the most up to date information for the algorithm. I wonder if it's equivalent - but that doesn't mean you need to do it the same way. > > > > > prepareISP(data); > > > > > frameCount_++; > > > > > > > > > > @@ -1011,11 +1013,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned > > int > > > > bufferId) > > > > > void IPARPi::prepareISP(const ISPConfig &data) > > > > > { > > > > > int64_t frameTimestamp = > > > > data.controls.get(controls::SensorTimestamp).value_or(0); > > > > > - RPiController::Metadata lastMetadata; > > > > > RPiController::Metadata &rpiMetadata = > > > > rpiMetadata_[metadataIdx_]; > > > > > Span<uint8_t> embeddedBuffer; > > > > > > > > > > - lastMetadata = std::move(rpiMetadata); > > > > > + rpiMetadata.clear(); > > > > > fillDeviceStatus(data.controls); > > > > > > > > > > if (data.embeddedBufferPresent) { > > > > > @@ -1048,7 +1049,8 @@ void IPARPi::prepareISP(const ISPConfig &data) > > > > > * current frame, or any other bits of metadata that > > > > were added > > > > > * in helper_->Prepare(). > > > > > */ > > > > > - rpiMetadata.merge(lastMetadata); > > > > > + RPiController::Metadata &lastMetadata = > > > > rpiMetadata_[lastMetadataIdx_]; > > > > > + rpiMetadata.mergeCopy(lastMetadata); > > > > > processPending_ = false; > > > > > return; > > > > > } > > > > > @@ -1147,7 +1149,7 @@ void IPARPi::processStats(unsigned int > > bufferId) > > > > > ControlList ctrls(sensorCtrls_); > > > > > applyAGC(&agcStatus, ctrls); > > > > > > > > > > - setDelayedControls.emit(ctrls); > > > > > + setDelayedControls.emit(ctrls, metadataIdx_); > > > > > } > > > > > } > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > index 23f2460190f4..8f6c6c0ce89f 100644 > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > @@ -206,7 +206,7 @@ public: > > > > > void runIsp(uint32_t bufferId); > > > > > void embeddedComplete(uint32_t bufferId); > > > > > void setIspControls(const ControlList &controls); > > > > > - void setDelayedControls(const ControlList &controls); > > > > > + void setDelayedControls(const ControlList &controls, uint32_t > > > > delayCookie); > > > > > void setSensorControls(ControlList &controls); > > > > > void unicamTimeout(); > > > > > > > > > > @@ -262,6 +262,7 @@ public: > > > > > struct BayerFrame { > > > > > FrameBuffer *buffer; > > > > > ControlList controls; > > > > > + unsigned int delayCookie; > > > > > }; > > > > > > > > > > std::queue<BayerFrame> bayerQueue_; > > > > > @@ -294,6 +295,9 @@ public: > > > > > /* Have internal buffers been allocated? */ > > > > > bool buffersAllocated_; > > > > > > > > > > + /* Frame cookie to pass to the IPA */ > > > > > + unsigned int ipaCookie_; > > > > > + > > > > > private: > > > > > void checkRequestCompleted(); > > > > > void fillRequestMetadata(const ControlList &bufferControls, > > > > > @@ -1064,7 +1068,8 @@ int PipelineHandlerRPi::start(Camera *camera, > > > > const ControlList *controls) > > > > > * Reset the delayed controls with the gain and exposure > > values > > > > set by > > > > > * the IPA. > > > > > */ > > > > > - data->delayedCtrls_->reset(); > > > > > + data->ipaCookie_ = 0; > > > > > + data->delayedCtrls_->reset(data->ipaCookie_); > > > > > > > > I don't think I've understood why resetting delayed controls desires a > > > > cookie to be given. I guess it's so that the 'starting' condition is > > > > known? > > > > > > > > > > Correct, it just assigns a cookie value to the starting conditions. > > > > > > > > > > > > > > And perhaps it shouldn't always be zero? > > > > > > > > > > It does not strictly have to be 0, but if you consider it analogous to > > the > > > frame sequence number, it makes sense for it to. > > > > I suspect it's analogous to the controls that were set at the start() .. > > but I'm not sure that can be modelled easily, so keeping it as 0 seems > > reasonable. > > > > I'm just not sure it would ever make sense to be ... 1052 (some other > > number..) > > > > Anyway, I don't think it's crucial here. > > > > > > > > > data->state_ = RPiCameraData::State::Idle; > > > > > > > > > > @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const > > > > ControlList &controls) > > > > > handleState(); > > > > > } > > > > > > > > > > -void RPiCameraData::setDelayedControls(const ControlList &controls) > > > > > +void RPiCameraData::setDelayedControls(const ControlList &controls, > > > > uint32_t delayCookie) > > > > > { > > > > > - if (!delayedCtrls_->push(controls)) > > > > > + if (!delayedCtrls_->push(controls, delayCookie)) > > > > > LOG(RPI, Error) << "V4L2 DelayedControl set failed"; > > > > > handleState(); > > > > > } > > > > > @@ -1867,13 +1872,13 @@ void > > > > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > > * Lookup the sensor controls used for this frame > > > > sequence from > > > > > * DelayedControl and queue them along with the frame > > > > buffer. > > > > > */ > > > > > - auto [ctrl, cookie] = > > > > delayedCtrls_->get(buffer->metadata().sequence); > > > > > + auto [ctrl, delayCookie] = > > > > 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) }); > > > > > + bayerQueue_.push({ buffer, std::move(ctrl), > > delayCookie > > > > }); > > > > > } else { > > > > > embeddedQueue_.push(buffer); > > > > > } > > > > > @@ -2168,6 +2173,8 @@ void RPiCameraData::tryRunPipeline() > > > > > ipa::RPi::ISPConfig ispPrepare; > > > > > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId; > > > > > ispPrepare.controls = std::move(bayerFrame.controls); > > > > > + ispPrepare.ipaCookie = ipaCookie_++; > > > > > > > > I'd be intrigued to know if this should be the request sequence number, > > > > as we would use in the RKISP/IPU3, rather than a new sequence number. > > > > > > > > > > Yes, this would work, assuming the request sequence number is strictly > > > monotonically increasing? > > > > Request sequences are 1 for 1 for every request that is queued. It's > > handled by > > > > - > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n410 > > > > It always starts from zero, and should always increase by one for every > > request queued to a Camera object. (It's stored in the CameraData). > > > > It gets reset at Camera::stop() time. Requests after that are > > sequenced from zero again. > > > > Note that any requests rejected and not queued will not increment the > > sequence number, as they won't make it into the pipeline handler: > > > > - > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1113 > > > > > > So actually, that could impact you depending on how you handle IPA > > contining through a start stop cycle. Do you expect to reference > > metadata from a previous streaming session? > > > > If it's an issue - keep it with your own counter. > > > > We don't need to reference metadata from previous streaming sessions, so > assuming the request counter that the pipeline handler sees are > monotonically increasing by 1, I can switch to that. > So perhaps it does make sense to switch to request sequence numbers...? In my understanding, the request sequence number represents the sequence number of any given controls, which therefore is the same as the metadata index. So it makes sense to me - but it's only worth switching if it makes sense to you ;-) > Naush > > > > > > > -- > > Kieran > > > > > > > > > > Happy to make these changes if it matches closer to the > > > ipu3/rkisp implementation. > > > > > > Regards, > > > Naush > > > > > > > > > > But I suspect it doesn't matter. It depends how you want to reference > > > > the completed metadata against a queued request, which I think you are > > > > not too worried about explicitly like that. > > > > > > > > Small worries aside, I expect this is working... so it's your > > discretion > > > > if you think the cookies/sequences should be handled differently. > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > > > > > > > > > > > + ispPrepare.delayCookie = bayerFrame.delayCookie; > > > > > > > > > > if (embeddedBuffer) { > > > > > unsigned int embeddedId = > > > > unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > > > > -- > > > > > 2.25.1 > > > > > > > > > > >
On Fri, 28 Oct 2022 at 14:36, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2022-10-28 14:26:11) > > On Fri, 28 Oct 2022 at 14:15, Kieran Bingham < > > kieran.bingham@ideasonboard.com> wrote: > > > > > Quoting Naushir Patuck (2022-10-28 13:58:00) > > > > Hi Kieran, > > > > > > > > Thanks for all the reviews! > > > > > > > > > > > > On Fri, 28 Oct 2022 at 13:34, Kieran Bingham < > > > > kieran.bingham@ideasonboard.com> wrote: > > > > > > > > > Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:06) > > > > > > Pass an IPA cookie from the pipeline handler to the IPA and > > > eventually > > > > > back to > > > > > > the pipeline handler through the setDelayedControls signal. This > > > cookie > > > > > is used > > > > > > to index the RPiController::Metadata object to be used for the > frame. > > > > > > > > > > > > The IPA cookie is then returned from DelayedControls when the > frame > > > with > > > > > the > > > > > > applied controls has been returned from the sensor, and > eventually > > > > > passed back > > > > > > to the IPA from the signalIspPrepare signal. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > --- > > > > > > include/libcamera/ipa/raspberrypi.mojom | 4 +++- > > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++----- > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 19 > > > +++++++++++++------ > > > > > > 3 files changed, 23 insertions(+), 12 deletions(-) > > > > > > > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > > > > > b/include/libcamera/ipa/raspberrypi.mojom > > > > > > index 40f78d9e3b3f..bb5abd895262 100644 > > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom > > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > > > > > @@ -37,6 +37,8 @@ struct ISPConfig { > > > > > > uint32 bayerBufferId; > > > > > > bool embeddedBufferPresent; > > > > > > libcamera.ControlList controls; > > > > > > + uint32 ipaCookie; > > > > > > + uint32 delayCookie; > > > > > > }; > > > > > > > > > > > > struct IPAConfig { > > > > > > @@ -137,5 +139,5 @@ interface IPARPiEventInterface { > > > > > > runIsp(uint32 bufferId); > > > > > > embeddedComplete(uint32 bufferId); > > > > > > setIspControls(libcamera.ControlList controls); > > > > > > - setDelayedControls(libcamera.ControlList controls); > > > > > > + setDelayedControls(libcamera.ControlList controls, uint32 > > > > > delayCookie); > > > > > > }; > > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > > > > > b/src/ipa/raspberrypi/raspberrypi.cpp > > > > > > index 9e7792f5dfbe..aed8f68aded9 100644 > > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > > > @@ -168,6 +168,7 @@ private: > > > > > > RPiController::Controller controller_; > > > > > > std::array<RPiController::Metadata, numMetadataContexts> > > > > > rpiMetadata_; > > > > > > unsigned int metadataIdx_; > > > > > > + unsigned int lastMetadataIdx_; > > > > > > > > > > > > /* > > > > > > * We count frames to decide if the frame must be hidden > > > (e.g. > > > > > from > > > > > > @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList > &controls, > > > > > StartConfig *startConfig) > > > > > > > > > > > > firstStart_ = false; > > > > > > lastRunTimestamp_ = 0; > > > > > > - metadataIdx_ = 0; > > > > > > } > > > > > > > > > > > > void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) > > > > > > @@ -535,6 +535,8 @@ void IPARPi::signalIspPrepare(const ISPConfig > > > &data) > > > > > > * avoid running the control algos for a few frames in > case > > > > > > * they are "unreliable". > > > > > > */ > > > > > > + lastMetadataIdx_ = metadataIdx_; > > > > > > + metadataIdx_ = data.ipaCookie % rpiMetadata_.size(); > > > > > > > > > > I really fear this metadataIdx_ is a code smell ... but I am > tempted to > > > > > put a peg on my nose ... > > > > > > > > > > > > > Perhaps this is better named context_? and ipaCookie could also be > > > renamed > > > > ipaContext? > > > > > > I think it's more the nature of using this global variable to set which > > > metadata to read, for events that in my head are 'asynchronous' (even > > > though they can't be called in parallel). > > > > > > > Storing the metadataIdx_ was a bit of a shortcut really. I could remove > > this variable and pass the same "cookie" into the process phase as well. > > Would that be cleaner? > > It's the 'shortcut' that worried me yes, but I can look away if you > really want it. > > > However, I do need to store lastMetadataIdx_ if I use the request > sequence > > number as the cookie, and they do not increment by 1 on each call... see > > below. > > I think you're inferring that this is ok because they do ? Of course be > wary of request sequence 0, with 'lastMetadataIdx_ = -1' in that case. > > What do you obtain from the 'previous' metadata? I think we were going > to store the 'previous' - but we don't - we keep an 'ActiveState' in our > contexts, which keep the most up to date information for the algorithm. > I wonder if it's equivalent - but that doesn't mean you need to do it > the same way. > We rate limit out IPA to run no higher than 30Hz. If the sensor is running faster the IPA still gets called, but all our control algorithm results get copied from the last metadata context. Hence we need to keep this around. It's not a problem if we are assured that the context index is incrementing by 1 on each invocation, which seems to be the case. We do handle the case where 'lastMetadataIdx_ = -1' as the metadata object will be empty so nothing to do! Naush > > > > > > > prepareISP(data); > > > > > > frameCount_++; > > > > > > > > > > > > @@ -1011,11 +1013,10 @@ void > IPARPi::returnEmbeddedBuffer(unsigned > > > int > > > > > bufferId) > > > > > > void IPARPi::prepareISP(const ISPConfig &data) > > > > > > { > > > > > > int64_t frameTimestamp = > > > > > data.controls.get(controls::SensorTimestamp).value_or(0); > > > > > > - RPiController::Metadata lastMetadata; > > > > > > RPiController::Metadata &rpiMetadata = > > > > > rpiMetadata_[metadataIdx_]; > > > > > > Span<uint8_t> embeddedBuffer; > > > > > > > > > > > > - lastMetadata = std::move(rpiMetadata); > > > > > > + rpiMetadata.clear(); > > > > > > fillDeviceStatus(data.controls); > > > > > > > > > > > > if (data.embeddedBufferPresent) { > > > > > > @@ -1048,7 +1049,8 @@ void IPARPi::prepareISP(const ISPConfig > &data) > > > > > > * current frame, or any other bits of metadata > that > > > > > were added > > > > > > * in helper_->Prepare(). > > > > > > */ > > > > > > - rpiMetadata.merge(lastMetadata); > > > > > > + RPiController::Metadata &lastMetadata = > > > > > rpiMetadata_[lastMetadataIdx_]; > > > > > > + rpiMetadata.mergeCopy(lastMetadata); > > > > > > processPending_ = false; > > > > > > return; > > > > > > } > > > > > > @@ -1147,7 +1149,7 @@ void IPARPi::processStats(unsigned int > > > bufferId) > > > > > > ControlList ctrls(sensorCtrls_); > > > > > > applyAGC(&agcStatus, ctrls); > > > > > > > > > > > > - setDelayedControls.emit(ctrls); > > > > > > + setDelayedControls.emit(ctrls, metadataIdx_); > > > > > > } > > > > > > } > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > index 23f2460190f4..8f6c6c0ce89f 100644 > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > @@ -206,7 +206,7 @@ public: > > > > > > void runIsp(uint32_t bufferId); > > > > > > void embeddedComplete(uint32_t bufferId); > > > > > > void setIspControls(const ControlList &controls); > > > > > > - void setDelayedControls(const ControlList &controls); > > > > > > + void setDelayedControls(const ControlList &controls, > uint32_t > > > > > delayCookie); > > > > > > void setSensorControls(ControlList &controls); > > > > > > void unicamTimeout(); > > > > > > > > > > > > @@ -262,6 +262,7 @@ public: > > > > > > struct BayerFrame { > > > > > > FrameBuffer *buffer; > > > > > > ControlList controls; > > > > > > + unsigned int delayCookie; > > > > > > }; > > > > > > > > > > > > std::queue<BayerFrame> bayerQueue_; > > > > > > @@ -294,6 +295,9 @@ public: > > > > > > /* Have internal buffers been allocated? */ > > > > > > bool buffersAllocated_; > > > > > > > > > > > > + /* Frame cookie to pass to the IPA */ > > > > > > + unsigned int ipaCookie_; > > > > > > + > > > > > > private: > > > > > > void checkRequestCompleted(); > > > > > > void fillRequestMetadata(const ControlList > &bufferControls, > > > > > > @@ -1064,7 +1068,8 @@ int PipelineHandlerRPi::start(Camera > *camera, > > > > > const ControlList *controls) > > > > > > * Reset the delayed controls with the gain and exposure > > > values > > > > > set by > > > > > > * the IPA. > > > > > > */ > > > > > > - data->delayedCtrls_->reset(); > > > > > > + data->ipaCookie_ = 0; > > > > > > + data->delayedCtrls_->reset(data->ipaCookie_); > > > > > > > > > > I don't think I've understood why resetting delayed controls > desires a > > > > > cookie to be given. I guess it's so that the 'starting' condition > is > > > > > known? > > > > > > > > > > > > > Correct, it just assigns a cookie value to the starting conditions. > > > > > > > > > > > > > > > > > > And perhaps it shouldn't always be zero? > > > > > > > > > > > > > It does not strictly have to be 0, but if you consider it analogous > to > > > the > > > > frame sequence number, it makes sense for it to. > > > > > > I suspect it's analogous to the controls that were set at the start() > .. > > > but I'm not sure that can be modelled easily, so keeping it as 0 seems > > > reasonable. > > > > > > I'm just not sure it would ever make sense to be ... 1052 (some other > > > number..) > > > > > > Anyway, I don't think it's crucial here. > > > > > > > > > > > > data->state_ = RPiCameraData::State::Idle; > > > > > > > > > > > > @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const > > > > > ControlList &controls) > > > > > > handleState(); > > > > > > } > > > > > > > > > > > > -void RPiCameraData::setDelayedControls(const ControlList > &controls) > > > > > > +void RPiCameraData::setDelayedControls(const ControlList > &controls, > > > > > uint32_t delayCookie) > > > > > > { > > > > > > - if (!delayedCtrls_->push(controls)) > > > > > > + if (!delayedCtrls_->push(controls, delayCookie)) > > > > > > LOG(RPI, Error) << "V4L2 DelayedControl set > failed"; > > > > > > handleState(); > > > > > > } > > > > > > @@ -1867,13 +1872,13 @@ void > > > > > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > > > * Lookup the sensor controls used for this frame > > > > > sequence from > > > > > > * DelayedControl and queue them along with the > frame > > > > > buffer. > > > > > > */ > > > > > > - auto [ctrl, cookie] = > > > > > delayedCtrls_->get(buffer->metadata().sequence); > > > > > > + auto [ctrl, delayCookie] = > > > > > 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) }); > > > > > > + bayerQueue_.push({ buffer, std::move(ctrl), > > > delayCookie > > > > > }); > > > > > > } else { > > > > > > embeddedQueue_.push(buffer); > > > > > > } > > > > > > @@ -2168,6 +2173,8 @@ void RPiCameraData::tryRunPipeline() > > > > > > ipa::RPi::ISPConfig ispPrepare; > > > > > > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | > bayerId; > > > > > > ispPrepare.controls = std::move(bayerFrame.controls); > > > > > > + ispPrepare.ipaCookie = ipaCookie_++; > > > > > > > > > > I'd be intrigued to know if this should be the request sequence > number, > > > > > as we would use in the RKISP/IPU3, rather than a new sequence > number. > > > > > > > > > > > > > Yes, this would work, assuming the request sequence number is > strictly > > > > monotonically increasing? > > > > > > Request sequences are 1 for 1 for every request that is queued. It's > > > handled by > > > > > > - > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n410 > > > > > > It always starts from zero, and should always increase by one for every > > > request queued to a Camera object. (It's stored in the CameraData). > > > > > > It gets reset at Camera::stop() time. Requests after that are > > > sequenced from zero again. > > > > > > Note that any requests rejected and not queued will not increment the > > > sequence number, as they won't make it into the pipeline handler: > > > > > > - > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1113 > > > > > > > > > So actually, that could impact you depending on how you handle IPA > > > contining through a start stop cycle. Do you expect to reference > > > metadata from a previous streaming session? > > > > > > If it's an issue - keep it with your own counter. > > > > > > > We don't need to reference metadata from previous streaming sessions, so > > assuming the request counter that the pipeline handler sees are > > monotonically increasing by 1, I can switch to that. > > So perhaps it does make sense to switch to request sequence numbers...? > > In my understanding, the request sequence number represents the sequence > number of any given controls, which therefore is the same as the > metadata index. So it makes sense to me - but it's only worth switching > if it makes sense to you ;-) > > > > > Naush > > > > > > > > > > > > -- > > > Kieran > > > > > > > > > > > > > > Happy to make these changes if it matches closer to the > > > > ipu3/rkisp implementation. > > > > > > > > Regards, > > > > Naush > > > > > > > > > > > > > But I suspect it doesn't matter. It depends how you want to > reference > > > > > the completed metadata against a queued request, which I think you > are > > > > > not too worried about explicitly like that. > > > > > > > > > > Small worries aside, I expect this is working... so it's your > > > discretion > > > > > if you think the cookies/sequences should be handled differently. > > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > > > > > > > > > > > > > > > > + ispPrepare.delayCookie = bayerFrame.delayCookie; > > > > > > > > > > > > if (embeddedBuffer) { > > > > > > unsigned int embeddedId = > > > > > unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > > > > > > >
diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom index 40f78d9e3b3f..bb5abd895262 100644 --- a/include/libcamera/ipa/raspberrypi.mojom +++ b/include/libcamera/ipa/raspberrypi.mojom @@ -37,6 +37,8 @@ struct ISPConfig { uint32 bayerBufferId; bool embeddedBufferPresent; libcamera.ControlList controls; + uint32 ipaCookie; + uint32 delayCookie; }; struct IPAConfig { @@ -137,5 +139,5 @@ interface IPARPiEventInterface { runIsp(uint32 bufferId); embeddedComplete(uint32 bufferId); setIspControls(libcamera.ControlList controls); - setDelayedControls(libcamera.ControlList controls); + setDelayedControls(libcamera.ControlList controls, uint32 delayCookie); }; diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 9e7792f5dfbe..aed8f68aded9 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -168,6 +168,7 @@ private: RPiController::Controller controller_; std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; unsigned int metadataIdx_; + unsigned int lastMetadataIdx_; /* * We count frames to decide if the frame must be hidden (e.g. from @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig) firstStart_ = false; lastRunTimestamp_ = 0; - metadataIdx_ = 0; } void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) @@ -535,6 +535,8 @@ void IPARPi::signalIspPrepare(const ISPConfig &data) * avoid running the control algos for a few frames in case * they are "unreliable". */ + lastMetadataIdx_ = metadataIdx_; + metadataIdx_ = data.ipaCookie % rpiMetadata_.size(); prepareISP(data); frameCount_++; @@ -1011,11 +1013,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId) void IPARPi::prepareISP(const ISPConfig &data) { int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0); - RPiController::Metadata lastMetadata; RPiController::Metadata &rpiMetadata = rpiMetadata_[metadataIdx_]; Span<uint8_t> embeddedBuffer; - lastMetadata = std::move(rpiMetadata); + rpiMetadata.clear(); fillDeviceStatus(data.controls); if (data.embeddedBufferPresent) { @@ -1048,7 +1049,8 @@ void IPARPi::prepareISP(const ISPConfig &data) * current frame, or any other bits of metadata that were added * in helper_->Prepare(). */ - rpiMetadata.merge(lastMetadata); + RPiController::Metadata &lastMetadata = rpiMetadata_[lastMetadataIdx_]; + rpiMetadata.mergeCopy(lastMetadata); processPending_ = false; return; } @@ -1147,7 +1149,7 @@ void IPARPi::processStats(unsigned int bufferId) ControlList ctrls(sensorCtrls_); applyAGC(&agcStatus, ctrls); - setDelayedControls.emit(ctrls); + setDelayedControls.emit(ctrls, metadataIdx_); } } diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 23f2460190f4..8f6c6c0ce89f 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -206,7 +206,7 @@ public: void runIsp(uint32_t bufferId); void embeddedComplete(uint32_t bufferId); void setIspControls(const ControlList &controls); - void setDelayedControls(const ControlList &controls); + void setDelayedControls(const ControlList &controls, uint32_t delayCookie); void setSensorControls(ControlList &controls); void unicamTimeout(); @@ -262,6 +262,7 @@ public: struct BayerFrame { FrameBuffer *buffer; ControlList controls; + unsigned int delayCookie; }; std::queue<BayerFrame> bayerQueue_; @@ -294,6 +295,9 @@ public: /* Have internal buffers been allocated? */ bool buffersAllocated_; + /* Frame cookie to pass to the IPA */ + unsigned int ipaCookie_; + private: void checkRequestCompleted(); void fillRequestMetadata(const ControlList &bufferControls, @@ -1064,7 +1068,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) * Reset the delayed controls with the gain and exposure values set by * the IPA. */ - data->delayedCtrls_->reset(); + data->ipaCookie_ = 0; + data->delayedCtrls_->reset(data->ipaCookie_); data->state_ = RPiCameraData::State::Idle; @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const ControlList &controls) handleState(); } -void RPiCameraData::setDelayedControls(const ControlList &controls) +void RPiCameraData::setDelayedControls(const ControlList &controls, uint32_t delayCookie) { - if (!delayedCtrls_->push(controls)) + if (!delayedCtrls_->push(controls, delayCookie)) LOG(RPI, Error) << "V4L2 DelayedControl set failed"; handleState(); } @@ -1867,13 +1872,13 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) * Lookup the sensor controls used for this frame sequence from * DelayedControl and queue them along with the frame buffer. */ - auto [ctrl, cookie] = delayedCtrls_->get(buffer->metadata().sequence); + auto [ctrl, delayCookie] = 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) }); + bayerQueue_.push({ buffer, std::move(ctrl), delayCookie }); } else { embeddedQueue_.push(buffer); } @@ -2168,6 +2173,8 @@ void RPiCameraData::tryRunPipeline() ipa::RPi::ISPConfig ispPrepare; ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId; ispPrepare.controls = std::move(bayerFrame.controls); + ispPrepare.ipaCookie = ipaCookie_++; + ispPrepare.delayCookie = bayerFrame.delayCookie; if (embeddedBuffer) { unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);