[libcamera-devel] [PATCH v2 2/6] ipu3: Add a class AiqResultsRingBuffer to reserve AiqResults history
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Nov 10 15:06:51 CET 2021
Quoting Hanlin Chen (2021-11-10 13:02:17)
> Hi Kieran,
> I'm sorry for the late reply.
Don't worry - it's all asynchronous ;-)
>
> On Tue, Nov 2, 2021 at 9:37 PM Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
> >
> > Quoting Han-Lin Chen (2021-10-29 12:59:57)
> > > The AIQ algorithm expects the statstistics comes with the effective AiqResults
> > > applied on the sensor, which may not always be the latest AiqResults,
> > > since pipeline handler may delay setting the controls based on SOF.
> > > Add a class to reserve the history of the AiqResults generated for previous
> > > frames, so IPA can have a chance to look for the suitable one backwards.
> >
> > Excellent, I'm pleased to see this development.
> >
> > This patch does a few things which should be explictily mentioned (or
> > broken in to independent patches)
> >
> > - Make the parameters to setXXX() functions const.
> > - Implement copy constructors
> > - Implement a RingBuffer to maintain AiqResults History
> Sounds great to me.
> >
> > > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > ---
> > > aiq/aiq_results.cpp | 85 ++++++++++++++++++++++++++++++++++++++++-----
> > > aiq/aiq_results.h | 38 +++++++++++++++-----
> > > 2 files changed, 107 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/aiq/aiq_results.cpp b/aiq/aiq_results.cpp
> > > index f727f36..deda4be 100644
> > > --- a/aiq/aiq_results.cpp
> > > +++ b/aiq/aiq_results.cpp
> > > @@ -63,7 +63,34 @@ AiqResults::AiqResults()
> > > sa_.channel_b = channelB_.data();
> > > }
> > >
> > > -void AiqResults::setAe(ia_aiq_ae_results *ae)
> > > +AiqResults::AiqResults(const AiqResults &other)
> > > + :AiqResults()
> > > +{
> > > + setAe(&other.ae_);
> > > + setAf(&other.af_);
> > > + setAfBracket(&other.afBracket_);
> > > + setAwb(&other.awb_);
> > > + setGbce(&other.gbce_);
> > > + setDetectedSceneMode(other.detectedSceneMode_);
> > > + setPa(&other.pa_);
> > > + setSa(&other.sa_);
> > > +}
> > > +
> > > +AiqResults& AiqResults::operator=(const AiqResults &other)
> > > +{
> > > + setAe(&other.ae_);
> > > + setAf(&other.af_);
> > > + setAfBracket(&other.afBracket_);
> > > + setAwb(&other.awb_);
> > > + setGbce(&other.gbce_);
> > > + setDetectedSceneMode(other.detectedSceneMode_);
> > > + setPa(&other.pa_);
> > > + setSa(&other.sa_);
> > > +
> > > + return *this;
> > > +}
> > > +
> > > +void AiqResults::setAe(const ia_aiq_ae_results *ae)
> > > {
> > > /* Todo: Potentially Requires copying
> > > * ia_aiq_aperture_control *aperture_control;
> > > @@ -121,7 +148,7 @@ void AiqResults::setAe(ia_aiq_ae_results *ae)
> > > }
> > > }
> > >
> > > -void AiqResults::setAf(ia_aiq_af_results *af)
> > > +void AiqResults::setAf(const ia_aiq_af_results *af)
> > > {
> > > ASSERT(af);
> > >
> > > @@ -133,7 +160,7 @@ void AiqResults::setAf(ia_aiq_af_results *af)
> > > af_.final_lens_position_reached = af->final_lens_position_reached;
> > > }
> > >
> > > -void AiqResults::setAfBracket(ia_aiq_af_bracket_results *afBracket)
> > > +void AiqResults::setAfBracket(const ia_aiq_af_bracket_results *afBracket)
> > > {
> > > ASSERT(afBracket);
> > >
> > > @@ -145,7 +172,7 @@ void AiqResults::setAfBracket(ia_aiq_af_bracket_results *afBracket)
> > > afBracket_.lens_positions_bracketing = afBracket->lens_positions_bracketing;
> > > }
> > >
> > > -void AiqResults::setAwb(ia_aiq_awb_results *awb)
> > > +void AiqResults::setAwb(const ia_aiq_awb_results *awb)
> > > {
> > > ASSERT(awb);
> > >
> > > @@ -157,7 +184,7 @@ void AiqResults::setAwb(ia_aiq_awb_results *awb)
> > > awb_.distance_from_convergence = awb->distance_from_convergence;
> > > }
> > >
> > > -void AiqResults::setGbce(ia_aiq_gbce_results *gbce)
> > > +void AiqResults::setGbce(const ia_aiq_gbce_results *gbce)
> > > {
> > > ASSERT(gbce);
> > >
> > > @@ -181,12 +208,12 @@ void AiqResults::setGbce(ia_aiq_gbce_results *gbce)
> > > }
> > > }
> > >
> > > -void AiqResults::setDetectedSceneMode(ia_aiq_scene_mode dsm)
> > > +void AiqResults::setDetectedSceneMode(const ia_aiq_scene_mode dsm)
> > > {
> > > detectedSceneMode_ = dsm;
> > > }
> > >
> > > -void AiqResults::setPa(ia_aiq_pa_results *pa)
> > > +void AiqResults::setPa(const ia_aiq_pa_results *pa)
> > > {
> > > ASSERT(pa);
> > >
> > > @@ -234,7 +261,7 @@ void AiqResults::setPa(ia_aiq_pa_results *pa)
> > > pa_.brightness_level = pa->brightness_level;
> > > }
> > >
> > > -void AiqResults::setSa(ia_aiq_sa_results *sa)
> > > +void AiqResults::setSa(const ia_aiq_sa_results *sa)
> > > {
> > > ASSERT(sa && sa->channel_r && sa->channel_gr &&
> > > sa->channel_gb && sa->channel_b);
> > > @@ -275,6 +302,48 @@ void AiqResults::setSa(ia_aiq_sa_results *sa)
> > > sa_.frame_params = sa->frame_params;
> > > }
> > >
> > > +AiqResults& AiqResultsRingBuffer::operator[](unsigned int index)
> > > +{
> > > + return std::array<AiqResults, bufferSize>::operator[](index % bufferSize);
> > > +}
> > > +
> > > +const AiqResults& AiqResultsRingBuffer::operator[](unsigned int index) const
> > > +{
> > > + return std::array<AiqResults, bufferSize>::operator[](index % bufferSize);
> > > +}
> > > +
> > > +void AiqResultsRingBuffer::Reset()
> > > +{
> > > + start_ = end_ = size_ = 0;
> > > +}
> > > +
> > > +void AiqResultsRingBuffer::Push(const AiqResults& result)
> > > +{
> > > + operator[](end_) = result;
> > > + end_ = (end_ + 1) % bufferSize;
> > > + size_ = std::min(size_ + 1, bufferSize);
> > > + if (size_ == bufferSize) {
> > > + start_ = end_;
> > > + }
> > > +}
> > > +
> > > +AiqResults& AiqResultsRingBuffer::searchBackward(
> > > + const std::function<bool(AiqResults&)> pred, AiqResults& def)
> > > +{
> > > + if (size_ == 0)
> > > + return def;
> > > +
> > > + unsigned int i = (end_ - 1) % bufferSize;
> > > + while (i != start_) {
> > > + if (pred(operator[](i))) {
> > > + return operator[](i);
> > > + }
> > > + i = --i % bufferSize;
> > > + }
> > > +
> > > + return def;
> > > +}
> > > +
> > > } /* namespace ipa::ipu3::aiq */
> > >
> > > } /* namespace libcamera */
> > > diff --git a/aiq/aiq_results.h b/aiq/aiq_results.h
> > > index ae19a6c..7005a47 100644
> > > --- a/aiq/aiq_results.h
> > > +++ b/aiq/aiq_results.h
> > > @@ -8,6 +8,8 @@
> > > * of the aiq result structures.
> > > */
> > >
> > > +#include <array>
> > > +#include <functional>
> > > #include <vector>
> > >
> > > #include <ia_imaging/ia_aiq.h>
> > > @@ -37,6 +39,8 @@ class AiqResults
> > > {
> > > public:
> > > AiqResults();
> > > + AiqResults(const AiqResults &other);
> > > + AiqResults &operator=(const AiqResults &other);
> >
> > Should the move constructor/assingment operators be deleted or
> > implemented?
> I think AiqResult doesn't have a "move" semantics ;-).
> The move constructor/assignment would be suppressed automatically when
> the copy constructor is defined, but it's good to make it more
> explicit :/
Ah yes I recall there are implicit rules that delete things. I can never
remember them ;-) All I have in my head is the rule of 3/5/x where if
you define one you should define or delete all of them ...
> >
> >
> > > const ia_aiq_ae_results *ae() { return &ae_; }
> > > ia_aiq_af_results *af() { return &af_; }
> > > @@ -46,14 +50,14 @@ public:
> > > const ia_aiq_pa_results *pa() { return &pa_; }
> > > const ia_aiq_sa_results *sa() { return &sa_; }
> > >
> > > - void setAe(ia_aiq_ae_results *ae);
> > > - void setAf(ia_aiq_af_results *af);
> > > - void setAfBracket(ia_aiq_af_bracket_results *afBracket);
> > > - void setAwb(ia_aiq_awb_results *awb);
> > > - void setGbce(ia_aiq_gbce_results *gbce);
> > > - void setDetectedSceneMode(ia_aiq_scene_mode dsm);
> > > - void setPa(ia_aiq_pa_results *pa);
> > > - void setSa(ia_aiq_sa_results *sa);
> > > + void setAe(const ia_aiq_ae_results *ae);
> > > + void setAf(const ia_aiq_af_results *af);
> > > + void setAfBracket(const ia_aiq_af_bracket_results *afBracket);
> > > + void setAwb(const ia_aiq_awb_results *awb);
> > > + void setGbce(const ia_aiq_gbce_results *gbce);
> > > + void setDetectedSceneMode(const ia_aiq_scene_mode dsm);
> > > + void setPa(const ia_aiq_pa_results *pa);
> > > + void setSa(const ia_aiq_sa_results *sa);
> > >
> > > private:
> > > ia_aiq_ae_results ae_;
> > > @@ -109,6 +113,24 @@ private:
> > > std::vector<float> channelGb_;
> > > };
> > >
> > > +static constexpr unsigned int bufferSize = 16;
> > > +class AiqResultsRingBuffer: public std::array<AiqResults, bufferSize>
> > > +{
> > > +public:
> > > + AiqResults &operator[](unsigned int index);
> > > + const AiqResults &operator[](unsigned int index) const;
> > > + void Reset();
> > > + void Push(const AiqResults& result);
> > > + unsigned int size() { return size_; }
> > > + AiqResults& searchBackward(const std::function<bool(AiqResults&)> pred,
> > > + AiqResults& def);
> > > +
> > > +private:
> > > + unsigned int start_ = 0;
> > > + unsigned int end_ = 0;
> > > + unsigned int size_ = 0;
> > > +};
> > > +
> > > } /* namespace libcamera::ipa::ipu3::aiq */
> > >
> > > #endif /* IPA_IPU3_AIQ_RESULTS_H */
> > > --
> > > 2.33.1.1089.g2158813163f-goog
> > >
More information about the libcamera-devel
mailing list