| Message ID | 20241030164853.87586-3-jacopo.mondi@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Wed, Oct 30, 2024 at 05:48:52PM +0100, Jacopo Mondi wrote: > The RkISP1 AGC algorithms assumes the metering mode to be > "MeteringMatrix" and pre-fill an array of weights associated > with it stored in meteringModes_[MeteringMatrix] when intializing > the algorithm in parseMeteringModes(). > > It laters fetches the weights when computing parameters using the > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > When a Request gets queued to the algorithm, the > FrameContext.agc.meteringMode index is populated from the algorithm's > active state, and optionally updated if the application has specified > a different metering mode. > > In case of Request underrun however, a non-initialized FrameContext > gets provided to the algorithm, causing an (unhandled) exception when > accessing the meteringModes_ array. > > To handle the situation when a Request underrun occours and let > algorithms initialize a FrameContext to safe defaults: > > - Add an 'underrun' flag to FrameContext > - Add an 'initFrameContext()' function to RkISP1 algorithms > - If a FrameContext is get() before being allocated, pass it through > the algorithms' initFrameContext() to safely initialize it > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/libipa/fc_queue.cpp | 6 ++++++ > src/ipa/libipa/fc_queue.h | 5 +++++ > src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++ > src/ipa/rkisp1/algorithms/agc.h | 4 ++++ > src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++ > src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++ > 6 files changed, 37 insertions(+) > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp > index 0365e9197748..44f9d866ad1b 100644 > --- a/src/ipa/libipa/fc_queue.cpp > +++ b/src/ipa/libipa/fc_queue.cpp > @@ -36,6 +36,12 @@ namespace ipa { > * > * \var FrameContext::frame > * \brief The frame number > + * > + * \var FrameContext::underrun > + * \brief Boolean flag that reports if the FrameContext has been accessed with > + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set > + * to True then the frame context needs to be initialized by algorithms to safe > + * defaults as it won't be initialized with any non-user provided control. > */ > > /** > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h > index a1d136521107..d70ca9601bd7 100644 > --- a/src/ipa/libipa/fc_queue.h > +++ b/src/ipa/libipa/fc_queue.h > @@ -22,6 +22,9 @@ template<typename FrameContext> > class FCQueue; > > struct FrameContext { > +public: > + bool underrun = false; > + > private: > template<typename T> friend class FCQueue; > uint32_t frame; > @@ -97,6 +100,7 @@ public: > * is called before alloc() by the IPA for frame#0. > */ > init(frameContext, frame); > + frameContext.underrun = true; I can't find the place where the flag is reset to false (In case there are temporary underruns). > > return frameContext; > } > @@ -117,6 +121,7 @@ public: > << "Obtained an uninitialised FrameContext for " << frame; > > init(frameContext, frame); > + frameContext.underrun = true; > > return frameContext; > } > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 301b7ec26508..4122f665b3ee 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > return 0; > } > > +void Agc::initFrameContext(IPAContext &context, > + IPAFrameContext &frameContext) > +{ > + auto &agc = context.activeState.agc; > + > + frameContext.agc.meteringMode = agc.meteringMode; > +} > + I'm unsure if we really need the separate function. From an algorithm point of view, I can't see the difference between calling initFrameContext(ipaContext, frameContext) and queueRequest(ipaContext, frameContext.frame, frameContext, {}) Couldn't we just rename queueRequest to initFrameContext, so that we don't have to implement two separate functions? > /** > * \copydoc libcamera::ipa::Algorithm::queueRequest > */ > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > index aa86f2c5bc21..c1adf91bbc4e 100644 > --- a/src/ipa/rkisp1/algorithms/agc.h > +++ b/src/ipa/rkisp1/algorithms/agc.h > @@ -30,6 +30,10 @@ public: > > int init(IPAContext &context, const YamlObject &tuningData) override; > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; > + > + void initFrameContext(IPAContext &context, > + IPAFrameContext &frameContext) override; > + > void queueRequest(IPAContext &context, > const uint32_t frame, > IPAFrameContext &frameContext, > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h > index 715cfcd8298b..82603b7b372d 100644 > --- a/src/ipa/rkisp1/algorithms/algorithm.h > +++ b/src/ipa/rkisp1/algorithms/algorithm.h > @@ -23,6 +23,11 @@ public: > { > } > > + virtual void initFrameContext([[maybe_unused]] IPAContext &context, > + [[maybe_unused]] IPAFrameContext &frameContext) > + { > + } > + > bool disabled_; > bool supportsRaw_; > }; > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 9e161cabdea4..b743de9ff6af 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > { > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > + if (frameContext.underrun) { > + for (auto const &a : algorithms()) { > + Algorithm *algo = static_cast<Algorithm *>(a.get()); > + if (algo->disabled_) > + continue; > + algo->initFrameContext(context_, frameContext); > + } Maybe that would be the place for frameContext.underrun = false? Best regards, Stefan > + } > + > /* > * In raw capture mode, the ISP is bypassed and no statistics buffer is > * provided. > -- > 2.47.0 >
Hi Stefan thanks for the review On Tue, Nov 12, 2024 at 05:35:52PM +0100, Stefan Klug wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Oct 30, 2024 at 05:48:52PM +0100, Jacopo Mondi wrote: > > The RkISP1 AGC algorithms assumes the metering mode to be > > "MeteringMatrix" and pre-fill an array of weights associated > > with it stored in meteringModes_[MeteringMatrix] when intializing > > the algorithm in parseMeteringModes(). > > > > It laters fetches the weights when computing parameters using the > > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > > > When a Request gets queued to the algorithm, the > > FrameContext.agc.meteringMode index is populated from the algorithm's > > active state, and optionally updated if the application has specified > > a different metering mode. > > > > In case of Request underrun however, a non-initialized FrameContext > > gets provided to the algorithm, causing an (unhandled) exception when > > accessing the meteringModes_ array. > > > > To handle the situation when a Request underrun occours and let > > algorithms initialize a FrameContext to safe defaults: > > > > - Add an 'underrun' flag to FrameContext > > - Add an 'initFrameContext()' function to RkISP1 algorithms > > - If a FrameContext is get() before being allocated, pass it through > > the algorithms' initFrameContext() to safely initialize it > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/ipa/libipa/fc_queue.cpp | 6 ++++++ > > src/ipa/libipa/fc_queue.h | 5 +++++ > > src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++ > > src/ipa/rkisp1/algorithms/agc.h | 4 ++++ > > src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++ > > src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++ > > 6 files changed, 37 insertions(+) > > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp > > index 0365e9197748..44f9d866ad1b 100644 > > --- a/src/ipa/libipa/fc_queue.cpp > > +++ b/src/ipa/libipa/fc_queue.cpp > > @@ -36,6 +36,12 @@ namespace ipa { > > * > > * \var FrameContext::frame > > * \brief The frame number > > + * > > + * \var FrameContext::underrun > > + * \brief Boolean flag that reports if the FrameContext has been accessed with > > + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set > > + * to True then the frame context needs to be initialized by algorithms to safe > > + * defaults as it won't be initialized with any non-user provided control. > > */ > > > > /** > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h > > index a1d136521107..d70ca9601bd7 100644 > > --- a/src/ipa/libipa/fc_queue.h > > +++ b/src/ipa/libipa/fc_queue.h > > @@ -22,6 +22,9 @@ template<typename FrameContext> > > class FCQueue; > > > > struct FrameContext { > > +public: > > + bool underrun = false; > > + > > private: > > template<typename T> friend class FCQueue; > > uint32_t frame; > > @@ -97,6 +100,7 @@ public: > > * is called before alloc() by the IPA for frame#0. > > */ > > init(frameContext, frame); > > + frameContext.underrun = true; > > I can't find the place where the flag is reset to false (In case there > are temporary underruns). It happens in init() void init(FrameContext &frameContext, const uint32_t frame) { frameContext = {}; and 'underrun' gets set to true only in the case get(x) is called before alloc(x) I should probably reset it in clear() too though > > > > > return frameContext; > > } > > @@ -117,6 +121,7 @@ public: > > << "Obtained an uninitialised FrameContext for " << frame; > > > > init(frameContext, frame); > > + frameContext.underrun = true; > > > > return frameContext; > > } > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > index 301b7ec26508..4122f665b3ee 100644 > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > return 0; > > } > > > > +void Agc::initFrameContext(IPAContext &context, > > + IPAFrameContext &frameContext) > > +{ > > + auto &agc = context.activeState.agc; > > + > > + frameContext.agc.meteringMode = agc.meteringMode; > > +} > > + > > I'm unsure if we really need the separate function. > > From an algorithm point of view, I can't see the difference between > calling > > initFrameContext(ipaContext, frameContext) > > and > > queueRequest(ipaContext, frameContext.frame, frameContext, {}) I like this way of thinking about it > > Couldn't we just rename queueRequest to initFrameContext, so that we > don't have to implement two separate functions? > I like this ideas as well Let's bikeshed the names a bit we have init() configure() queueRequest() prepare() process() with the last three operations being called repetidly in a streaming session. I was never in love with the "prepare()" and "process()" names, and I would rather use "prepare()" for queueRequest(), but that's what we have right now. The idea is that queueRequest actually initializes a frame context, optionally with user provided controls. So I think that using initFrameContext() is actually a good suggestion. <End of bikeshedding> > > /** > > * \copydoc libcamera::ipa::Algorithm::queueRequest > > */ > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > > index aa86f2c5bc21..c1adf91bbc4e 100644 > > --- a/src/ipa/rkisp1/algorithms/agc.h > > +++ b/src/ipa/rkisp1/algorithms/agc.h > > @@ -30,6 +30,10 @@ public: > > > > int init(IPAContext &context, const YamlObject &tuningData) override; > > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; > > + > > + void initFrameContext(IPAContext &context, > > + IPAFrameContext &frameContext) override; > > + > > void queueRequest(IPAContext &context, > > const uint32_t frame, > > IPAFrameContext &frameContext, > > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h > > index 715cfcd8298b..82603b7b372d 100644 > > --- a/src/ipa/rkisp1/algorithms/algorithm.h > > +++ b/src/ipa/rkisp1/algorithms/algorithm.h > > @@ -23,6 +23,11 @@ public: > > { > > } > > > > + virtual void initFrameContext([[maybe_unused]] IPAContext &context, > > + [[maybe_unused]] IPAFrameContext &frameContext) > > + { > > + } > > + > > bool disabled_; > > bool supportsRaw_; > > }; > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 9e161cabdea4..b743de9ff6af 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > > { > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > > + if (frameContext.underrun) { > > + for (auto const &a : algorithms()) { > > + Algorithm *algo = static_cast<Algorithm *>(a.get()); > > + if (algo->disabled_) > > + continue; > > + algo->initFrameContext(context_, frameContext); > > + } > > Maybe that would be the place for frameContext.underrun = false? I don't think the FrameContext status flags should be handled outside of the FCQ implementation, otherwise each algo implementation would have to do that correctly by its own. Thanks j > > Best regards, > Stefan > > > + } > > + > > /* > > * In raw capture mode, the ISP is bypassed and no statistics buffer is > > * provided. > > -- > > 2.47.0 > >
Hi Jacopo, On Wed, Nov 13, 2024 at 08:35:24AM +0100, Jacopo Mondi wrote: > Hi Stefan > thanks for the review > > On Tue, Nov 12, 2024 at 05:35:52PM +0100, Stefan Klug wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Wed, Oct 30, 2024 at 05:48:52PM +0100, Jacopo Mondi wrote: > > > The RkISP1 AGC algorithms assumes the metering mode to be > > > "MeteringMatrix" and pre-fill an array of weights associated > > > with it stored in meteringModes_[MeteringMatrix] when intializing > > > the algorithm in parseMeteringModes(). > > > > > > It laters fetches the weights when computing parameters using the > > > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > > > > > When a Request gets queued to the algorithm, the > > > FrameContext.agc.meteringMode index is populated from the algorithm's > > > active state, and optionally updated if the application has specified > > > a different metering mode. > > > > > > In case of Request underrun however, a non-initialized FrameContext > > > gets provided to the algorithm, causing an (unhandled) exception when > > > accessing the meteringModes_ array. > > > > > > To handle the situation when a Request underrun occours and let > > > algorithms initialize a FrameContext to safe defaults: > > > > > > - Add an 'underrun' flag to FrameContext > > > - Add an 'initFrameContext()' function to RkISP1 algorithms > > > - If a FrameContext is get() before being allocated, pass it through > > > the algorithms' initFrameContext() to safely initialize it > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > src/ipa/libipa/fc_queue.cpp | 6 ++++++ > > > src/ipa/libipa/fc_queue.h | 5 +++++ > > > src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++ > > > src/ipa/rkisp1/algorithms/agc.h | 4 ++++ > > > src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++ > > > src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++ > > > 6 files changed, 37 insertions(+) > > > > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp > > > index 0365e9197748..44f9d866ad1b 100644 > > > --- a/src/ipa/libipa/fc_queue.cpp > > > +++ b/src/ipa/libipa/fc_queue.cpp > > > @@ -36,6 +36,12 @@ namespace ipa { > > > * > > > * \var FrameContext::frame > > > * \brief The frame number > > > + * > > > + * \var FrameContext::underrun > > > + * \brief Boolean flag that reports if the FrameContext has been accessed with > > > + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set > > > + * to True then the frame context needs to be initialized by algorithms to safe > > > + * defaults as it won't be initialized with any non-user provided control. > > > */ > > > > > > /** > > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h > > > index a1d136521107..d70ca9601bd7 100644 > > > --- a/src/ipa/libipa/fc_queue.h > > > +++ b/src/ipa/libipa/fc_queue.h > > > @@ -22,6 +22,9 @@ template<typename FrameContext> > > > class FCQueue; > > > > > > struct FrameContext { > > > +public: > > > + bool underrun = false; > > > + > > > private: > > > template<typename T> friend class FCQueue; > > > uint32_t frame; > > > @@ -97,6 +100,7 @@ public: > > > * is called before alloc() by the IPA for frame#0. > > > */ > > > init(frameContext, frame); > > > + frameContext.underrun = true; > > > > I can't find the place where the flag is reset to false (In case there > > are temporary underruns). > > It happens in init() > > void init(FrameContext &frameContext, const uint32_t frame) > { > frameContext = {}; > > and 'underrun' gets set to true only in the case get(x) is called > before alloc(x) > > I should probably reset it in clear() too though > Oh right, sorry for the noise. > > > > > > > > return frameContext; > > > } > > > @@ -117,6 +121,7 @@ public: > > > << "Obtained an uninitialised FrameContext for " << frame; > > > > > > init(frameContext, frame); > > > + frameContext.underrun = true; > > > > > > return frameContext; > > > } > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > index 301b7ec26508..4122f665b3ee 100644 > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > return 0; > > > } > > > > > > +void Agc::initFrameContext(IPAContext &context, > > > + IPAFrameContext &frameContext) > > > +{ > > > + auto &agc = context.activeState.agc; > > > + > > > + frameContext.agc.meteringMode = agc.meteringMode; > > > +} > > > + > > > > I'm unsure if we really need the separate function. > > > > From an algorithm point of view, I can't see the difference between > > calling > > > > initFrameContext(ipaContext, frameContext) > > > > and > > > > queueRequest(ipaContext, frameContext.frame, frameContext, {}) > > I like this way of thinking about it > > > > > Couldn't we just rename queueRequest to initFrameContext, so that we > > don't have to implement two separate functions? > > > > I like this ideas as well > > Let's bikeshed the names a bit > > we have > > init() > configure() > queueRequest() > prepare() > process() > > with the last three operations being called repetidly in a streaming > session. > > I was never in love with the "prepare()" and "process()" names, and I > would rather use "prepare()" for queueRequest(), but that's what we > have right now. > > The idea is that queueRequest actually initializes a frame context, > optionally with user provided controls. So I think that using > initFrameContext() is actually a good suggestion. > > <End of bikeshedding> Glad you like it. The only thing we should then clarify is the definition of frame. In the docs it says "The frame number is the Request sequence number and identifies the desired corresponding frame to target for the controls to take effect." I guess in the new pfc scheme that will become the internal/sensor frame number, and translation to request sequence happens outside. From an algorithm point of view that seems sensible. Best regards, Stefan > > > > /** > > > * \copydoc libcamera::ipa::Algorithm::queueRequest > > > */ > > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > > > index aa86f2c5bc21..c1adf91bbc4e 100644 > > > --- a/src/ipa/rkisp1/algorithms/agc.h > > > +++ b/src/ipa/rkisp1/algorithms/agc.h > > > @@ -30,6 +30,10 @@ public: > > > > > > int init(IPAContext &context, const YamlObject &tuningData) override; > > > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; > > > + > > > + void initFrameContext(IPAContext &context, > > > + IPAFrameContext &frameContext) override; > > > + > > > void queueRequest(IPAContext &context, > > > const uint32_t frame, > > > IPAFrameContext &frameContext, > > > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h > > > index 715cfcd8298b..82603b7b372d 100644 > > > --- a/src/ipa/rkisp1/algorithms/algorithm.h > > > +++ b/src/ipa/rkisp1/algorithms/algorithm.h > > > @@ -23,6 +23,11 @@ public: > > > { > > > } > > > > > > + virtual void initFrameContext([[maybe_unused]] IPAContext &context, > > > + [[maybe_unused]] IPAFrameContext &frameContext) > > > + { > > > + } > > > + > > > bool disabled_; > > > bool supportsRaw_; > > > }; > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > index 9e161cabdea4..b743de9ff6af 100644 > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > > > { > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > > > > + if (frameContext.underrun) { > > > + for (auto const &a : algorithms()) { > > > + Algorithm *algo = static_cast<Algorithm *>(a.get()); > > > + if (algo->disabled_) > > > + continue; > > > + algo->initFrameContext(context_, frameContext); > > > + } > > > > Maybe that would be the place for frameContext.underrun = false? > > I don't think the FrameContext status flags should be handled outside > of the FCQ implementation, otherwise each algo implementation would > have to do that correctly by its own. > > Thanks > j > > > > > Best regards, > > Stefan > > > > > + } > > > + > > > /* > > > * In raw capture mode, the ISP is bypassed and no statistics buffer is > > > * provided. > > > -- > > > 2.47.0 > > >
Hi Stefan On Wed, Nov 13, 2024 at 09:43:55AM +0100, Stefan Klug wrote: > Hi Jacopo, > > On Wed, Nov 13, 2024 at 08:35:24AM +0100, Jacopo Mondi wrote: > > Hi Stefan > > thanks for the review > > > > On Tue, Nov 12, 2024 at 05:35:52PM +0100, Stefan Klug wrote: > > > Hi Jacopo, > > > > > > Thank you for the patch. > > > > > > On Wed, Oct 30, 2024 at 05:48:52PM +0100, Jacopo Mondi wrote: > > > > The RkISP1 AGC algorithms assumes the metering mode to be > > > > "MeteringMatrix" and pre-fill an array of weights associated > > > > with it stored in meteringModes_[MeteringMatrix] when intializing > > > > the algorithm in parseMeteringModes(). > > > > > > > > It laters fetches the weights when computing parameters using the > > > > FrameContext.agc.meteringMode as index of the meteringModes_ array. > > > > > > > > When a Request gets queued to the algorithm, the > > > > FrameContext.agc.meteringMode index is populated from the algorithm's > > > > active state, and optionally updated if the application has specified > > > > a different metering mode. > > > > > > > > In case of Request underrun however, a non-initialized FrameContext > > > > gets provided to the algorithm, causing an (unhandled) exception when > > > > accessing the meteringModes_ array. > > > > > > > > To handle the situation when a Request underrun occours and let > > > > algorithms initialize a FrameContext to safe defaults: > > > > > > > > - Add an 'underrun' flag to FrameContext > > > > - Add an 'initFrameContext()' function to RkISP1 algorithms > > > > - If a FrameContext is get() before being allocated, pass it through > > > > the algorithms' initFrameContext() to safely initialize it > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > src/ipa/libipa/fc_queue.cpp | 6 ++++++ > > > > src/ipa/libipa/fc_queue.h | 5 +++++ > > > > src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++ > > > > src/ipa/rkisp1/algorithms/agc.h | 4 ++++ > > > > src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++ > > > > src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++ > > > > 6 files changed, 37 insertions(+) > > > > > > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp > > > > index 0365e9197748..44f9d866ad1b 100644 > > > > --- a/src/ipa/libipa/fc_queue.cpp > > > > +++ b/src/ipa/libipa/fc_queue.cpp > > > > @@ -36,6 +36,12 @@ namespace ipa { > > > > * > > > > * \var FrameContext::frame > > > > * \brief The frame number > > > > + * > > > > + * \var FrameContext::underrun > > > > + * \brief Boolean flag that reports if the FrameContext has been accessed with > > > > + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set > > > > + * to True then the frame context needs to be initialized by algorithms to safe > > > > + * defaults as it won't be initialized with any non-user provided control. > > > > */ > > > > > > > > /** > > > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h > > > > index a1d136521107..d70ca9601bd7 100644 > > > > --- a/src/ipa/libipa/fc_queue.h > > > > +++ b/src/ipa/libipa/fc_queue.h > > > > @@ -22,6 +22,9 @@ template<typename FrameContext> > > > > class FCQueue; > > > > > > > > struct FrameContext { > > > > +public: > > > > + bool underrun = false; > > > > + > > > > private: > > > > template<typename T> friend class FCQueue; > > > > uint32_t frame; > > > > @@ -97,6 +100,7 @@ public: > > > > * is called before alloc() by the IPA for frame#0. > > > > */ > > > > init(frameContext, frame); > > > > + frameContext.underrun = true; > > > > > > I can't find the place where the flag is reset to false (In case there > > > are temporary underruns). > > > > It happens in init() > > > > void init(FrameContext &frameContext, const uint32_t frame) > > { > > frameContext = {}; > > > > and 'underrun' gets set to true only in the case get(x) is called > > before alloc(x) > > > > I should probably reset it in clear() too though > > > > Oh right, sorry for the noise. > > > > > > > > > > > > return frameContext; > > > > } > > > > @@ -117,6 +121,7 @@ public: > > > > << "Obtained an uninitialised FrameContext for " << frame; > > > > > > > > init(frameContext, frame); > > > > + frameContext.underrun = true; > > > > > > > > return frameContext; > > > > } > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > index 301b7ec26508..4122f665b3ee 100644 > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > > return 0; > > > > } > > > > > > > > +void Agc::initFrameContext(IPAContext &context, > > > > + IPAFrameContext &frameContext) > > > > +{ > > > > + auto &agc = context.activeState.agc; > > > > + > > > > + frameContext.agc.meteringMode = agc.meteringMode; > > > > +} > > > > + > > > > > > I'm unsure if we really need the separate function. > > > > > > From an algorithm point of view, I can't see the difference between > > > calling > > > > > > initFrameContext(ipaContext, frameContext) > > > > > > and > > > > > > queueRequest(ipaContext, frameContext.frame, frameContext, {}) > > > > I like this way of thinking about it > > > > > > > > Couldn't we just rename queueRequest to initFrameContext, so that we > > > don't have to implement two separate functions? > > > > > > > I like this ideas as well > > > > Let's bikeshed the names a bit > > > > we have > > > > init() > > configure() > > queueRequest() > > prepare() > > process() > > > > with the last three operations being called repetidly in a streaming > > session. > > > > I was never in love with the "prepare()" and "process()" names, and I > > would rather use "prepare()" for queueRequest(), but that's what we > > have right now. > > > > The idea is that queueRequest actually initializes a frame context, > > optionally with user provided controls. So I think that using > > initFrameContext() is actually a good suggestion. > > > > <End of bikeshedding> > > Glad you like it. The only thing we should then clarify is the > definition of frame. In the docs it says "The frame number is the > Request sequence number and identifies the desired corresponding frame > to target for the controls to take effect." I guess in the new pfc > scheme that will become the internal/sensor frame number, and > translation to request sequence happens outside. From an algorithm point > of view that seems sensible. We have a mix of Request::sequence and HW frame number in the doc and implementation. We should aim to clearly separate the two. We'll get there ;) > > Best regards, > Stefan > > > > > > > /** > > > > * \copydoc libcamera::ipa::Algorithm::queueRequest > > > > */ > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > > > > index aa86f2c5bc21..c1adf91bbc4e 100644 > > > > --- a/src/ipa/rkisp1/algorithms/agc.h > > > > +++ b/src/ipa/rkisp1/algorithms/agc.h > > > > @@ -30,6 +30,10 @@ public: > > > > > > > > int init(IPAContext &context, const YamlObject &tuningData) override; > > > > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; > > > > + > > > > + void initFrameContext(IPAContext &context, > > > > + IPAFrameContext &frameContext) override; > > > > + > > > > void queueRequest(IPAContext &context, > > > > const uint32_t frame, > > > > IPAFrameContext &frameContext, > > > > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h > > > > index 715cfcd8298b..82603b7b372d 100644 > > > > --- a/src/ipa/rkisp1/algorithms/algorithm.h > > > > +++ b/src/ipa/rkisp1/algorithms/algorithm.h > > > > @@ -23,6 +23,11 @@ public: > > > > { > > > > } > > > > > > > > + virtual void initFrameContext([[maybe_unused]] IPAContext &context, > > > > + [[maybe_unused]] IPAFrameContext &frameContext) > > > > + { > > > > + } > > > > + > > > > bool disabled_; > > > > bool supportsRaw_; > > > > }; > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > index 9e161cabdea4..b743de9ff6af 100644 > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > > > > { > > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > > > > > > + if (frameContext.underrun) { > > > > + for (auto const &a : algorithms()) { > > > > + Algorithm *algo = static_cast<Algorithm *>(a.get()); > > > > + if (algo->disabled_) > > > > + continue; > > > > + algo->initFrameContext(context_, frameContext); > > > > + } > > > > > > Maybe that would be the place for frameContext.underrun = false? > > > > I don't think the FrameContext status flags should be handled outside > > of the FCQ implementation, otherwise each algo implementation would > > have to do that correctly by its own. > > > > Thanks > > j > > > > > > > > Best regards, > > > Stefan > > > > > > > + } > > > > + > > > > /* > > > > * In raw capture mode, the ISP is bypassed and no statistics buffer is > > > > * provided. > > > > -- > > > > 2.47.0 > > > >
diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp index 0365e9197748..44f9d866ad1b 100644 --- a/src/ipa/libipa/fc_queue.cpp +++ b/src/ipa/libipa/fc_queue.cpp @@ -36,6 +36,12 @@ namespace ipa { * * \var FrameContext::frame * \brief The frame number + * + * \var FrameContext::underrun + * \brief Boolean flag that reports if the FrameContext has been accessed with + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set + * to True then the frame context needs to be initialized by algorithms to safe + * defaults as it won't be initialized with any non-user provided control. */ /** diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h index a1d136521107..d70ca9601bd7 100644 --- a/src/ipa/libipa/fc_queue.h +++ b/src/ipa/libipa/fc_queue.h @@ -22,6 +22,9 @@ template<typename FrameContext> class FCQueue; struct FrameContext { +public: + bool underrun = false; + private: template<typename T> friend class FCQueue; uint32_t frame; @@ -97,6 +100,7 @@ public: * is called before alloc() by the IPA for frame#0. */ init(frameContext, frame); + frameContext.underrun = true; return frameContext; } @@ -117,6 +121,7 @@ public: << "Obtained an uninitialised FrameContext for " << frame; init(frameContext, frame); + frameContext.underrun = true; return frameContext; } diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 301b7ec26508..4122f665b3ee 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) return 0; } +void Agc::initFrameContext(IPAContext &context, + IPAFrameContext &frameContext) +{ + auto &agc = context.activeState.agc; + + frameContext.agc.meteringMode = agc.meteringMode; +} + /** * \copydoc libcamera::ipa::Algorithm::queueRequest */ diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h index aa86f2c5bc21..c1adf91bbc4e 100644 --- a/src/ipa/rkisp1/algorithms/agc.h +++ b/src/ipa/rkisp1/algorithms/agc.h @@ -30,6 +30,10 @@ public: int init(IPAContext &context, const YamlObject &tuningData) override; int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; + + void initFrameContext(IPAContext &context, + IPAFrameContext &frameContext) override; + void queueRequest(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h index 715cfcd8298b..82603b7b372d 100644 --- a/src/ipa/rkisp1/algorithms/algorithm.h +++ b/src/ipa/rkisp1/algorithms/algorithm.h @@ -23,6 +23,11 @@ public: { } + virtual void initFrameContext([[maybe_unused]] IPAContext &context, + [[maybe_unused]] IPAFrameContext &frameContext) + { + } + bool disabled_; bool supportsRaw_; }; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 9e161cabdea4..b743de9ff6af 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId { IPAFrameContext &frameContext = context_.frameContexts.get(frame); + if (frameContext.underrun) { + for (auto const &a : algorithms()) { + Algorithm *algo = static_cast<Algorithm *>(a.get()); + if (algo->disabled_) + continue; + algo->initFrameContext(context_, frameContext); + } + } + /* * In raw capture mode, the ISP is bypassed and no statistics buffer is * provided.
The RkISP1 AGC algorithms assumes the metering mode to be "MeteringMatrix" and pre-fill an array of weights associated with it stored in meteringModes_[MeteringMatrix] when intializing the algorithm in parseMeteringModes(). It laters fetches the weights when computing parameters using the FrameContext.agc.meteringMode as index of the meteringModes_ array. When a Request gets queued to the algorithm, the FrameContext.agc.meteringMode index is populated from the algorithm's active state, and optionally updated if the application has specified a different metering mode. In case of Request underrun however, a non-initialized FrameContext gets provided to the algorithm, causing an (unhandled) exception when accessing the meteringModes_ array. To handle the situation when a Request underrun occours and let algorithms initialize a FrameContext to safe defaults: - Add an 'underrun' flag to FrameContext - Add an 'initFrameContext()' function to RkISP1 algorithms - If a FrameContext is get() before being allocated, pass it through the algorithms' initFrameContext() to safely initialize it Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/ipa/libipa/fc_queue.cpp | 6 ++++++ src/ipa/libipa/fc_queue.h | 5 +++++ src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++ src/ipa/rkisp1/algorithms/agc.h | 4 ++++ src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++ src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++ 6 files changed, 37 insertions(+)