[libcamera-devel] [PATCH 1/2] ipa: ipu3: awb: Don't pass member variable to member function

[Laurent Pinchart]

Hi Kieran,

On Fri, Oct 15, 2021 at 10:11:17PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-10-15 02:58:03)
> > The Awb::generateZones() member function fills the zones vector passed
> > as an argument, which is actually a member variable. Use it directly in
> > the function.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/awb.cpp | 12 ++++++++----
> >  src/ipa/ipu3/algorithms/awb.h   |  2 +-
> >  2 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> > index e2b18336d582..809de66aa7fa 100644
> > --- a/src/ipa/ipu3/algorithms/awb.cpp
> > +++ b/src/ipa/ipu3/algorithms/awb.cpp
> > @@ -196,8 +196,10 @@ uint32_t Awb::estimateCCT(double red, double green, double blue)
> >  }
> >  
> >  /* Generate an RGB vector with the average values for each zone */
> > -void Awb::generateZones(std::vector<RGB> &zones)
> > +void Awb::generateZones()
> >  {
> > +       zones_.clear();
> > +
> 
> https://www.cplusplus.com/reference/vector/vector/clear/ states a
> reallocation is not guaranteed to happen and the vector capacity is not
> guaranteed to change due to calling this function.
> 
> That's a shame, it would be nice to be able to guarantee it won't
> reallocate to keep the memory reserved for the next usage ;-)

According to https://en.cppreference.com/w/cpp/container/vector/clear,

"Leaves the capacity() of the vector unchanged (note: the standard's
restriction on the changes to capacity is in the specification of
vector::reserve, see [1])"

[1] http://stackoverflow.com/a/18467916

> Anyway, that pre-dates this patch and is likely as correct as it can be
> anyway.
> 
> Seems a reasonable clean up.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >         for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
> >                 RGB zone;
> >                 double counted = awbStats_[i].counted;
> > @@ -206,7 +208,7 @@ void Awb::generateZones(std::vector<RGB> &zones)
> >                         if (zone.G >= kMinGreenLevelInZone) {
> >                                 zone.R = awbStats_[i].sum.red / counted;
> >                                 zone.B = awbStats_[i].sum.blue / counted;
> > -                               zones.push_back(zone);
> > +                               zones_.push_back(zone);
> >                         }
> >                 }
> >         }
> > @@ -298,11 +300,13 @@ void Awb::awbGreyWorld()
> >  void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> >  {
> >         ASSERT(stats->stats_3a_status.awb_en);
> > -       zones_.clear();
> > +
> >         clearAwbStats();
> >         generateAwbStats(stats);
> > -       generateZones(zones_);
> > +       generateZones();
> > +
> >         LOG(IPU3Awb, Debug) << "Valid zones: " << zones_.size();
> > +
> >         if (zones_.size() > 10) {
> >                 awbGreyWorld();
> >                 LOG(IPU3Awb, Debug) << "Gain found for red: " << asyncResults_.redGain
> > diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> > index 677384eda54a..3b81f600aa02 100644
> > --- a/src/ipa/ipu3/algorithms/awb.h
> > +++ b/src/ipa/ipu3/algorithms/awb.h
> > @@ -65,7 +65,7 @@ public:
> >  
> >  private:
> >         void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> > -       void generateZones(std::vector<RGB> &zones);
> > +       void generateZones();
> >         void generateAwbStats(const ipu3_uapi_stats_3a *stats);
> >         void clearAwbStats();
> >         void awbGreyWorld();
> > 
> > base-commit: ccec150589a177654b9039d21163c36ccff85cff

-- 
Regards,

Laurent Pinchart