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

Message ID 20211015015804.11758-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 5ae92dae5882f5450c647b4af95ef37ba0fded72
Headers show
Series
  • [libcamera-devel,1/2] ipa: ipu3: awb: Don't pass member variable to member function
Related show

Commit Message

Laurent Pinchart Oct. 15, 2021, 1:58 a.m. UTC 
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@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/awb.cpp | 12 ++++++++----
 src/ipa/ipu3/algorithms/awb.h   |  2 +-
 2 files changed, 9 insertions(+), 5 deletions(-)


base-commit: ccec150589a177654b9039d21163c36ccff85cff

Comments

Jean-Michel Hautbois Oct. 15, 2021, 7:03 a.m. UTC | #1 
Hi Laurent,

Thanks for the patch !

On 15/10/2021 03:58, Laurent Pinchart wrote:
> 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@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();
> +
>  	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
> 

Should we always include this in our future patches ? I can see you make
it for your series systematically :-) ?

Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Kieran Bingham Oct. 15, 2021, 9:11 p.m. UTC | #2 
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@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 ;-)

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@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
>
Laurent Pinchart Oct. 15, 2021, 9:17 p.m. UTC | #3 
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@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@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
Laurent Pinchart Oct. 15, 2021, 9:25 p.m. UTC | #4 
Hi Jean-Michel,

On Fri, Oct 15, 2021 at 09:03:42AM +0200, Jean-Michel Hautbois wrote:
> On 15/10/2021 03:58, Laurent Pinchart wrote:
> > 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@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();
> > +
> >  	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
> 
> Should we always include this in our future patches ? I can see you make
> it for your series systematically :-) ?

I've enabled the base=auto option for git format-patch by default. It's
nice to have, but for libcamera, probably won't be that useful yet.

> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Paul Elder Oct. 18, 2021, 4:22 a.m. UTC | #5 
Hi Laurent,

On Fri, Oct 15, 2021 at 04:58:03AM +0300, Laurent Pinchart wrote:
> 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@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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();
> +
>  	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
>

Patch
diff mbox series 

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();
+
 	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();