[libcamera-devel] [PATCH v2 2/6] ipu3: Add a class AiqResultsRingBuffer to reserve AiqResults history
Hanlin Chen
hanlinchen at chromium.org
Wed Nov 10 14:02:17 CET 2021
Hi Kieran,
I'm sorry for the late reply.
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 :/
>
>
> > 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