[v3,4/6] ipa: rpi: Use centralised libipa helpers
diff mbox series

Message ID 20241115074628.417215-5-dan.scally@ideasonboard.com
State Accepted
Headers show
Series
  • Centralise common functions in IPA modules
Related show

Commit Message

Dan Scally Nov. 15, 2024, 7:46 a.m. UTC 
Use the centralised libipa helpers rather than open coding common
functions.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v3:

	- None

Changes in v2:

	- None. Jacopo; this isn't in the libcamera::ipa namespace. As an
	  alternative to the prefix I could "using namespace libcamera::ipa" if
	  that's better?

 src/ipa/rpi/controller/rpi/agc_channel.cpp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Kieran Bingham Nov. 15, 2024, 12:01 p.m. UTC | #1 
Quoting Daniel Scally (2024-11-15 07:46:26)
> Use the centralised libipa helpers rather than open coding common
> functions.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v3:
> 
>         - None
> 
> Changes in v2:
> 
>         - None. Jacopo; this isn't in the libcamera::ipa namespace. As an
>           alternative to the prefix I could "using namespace libcamera::ipa" if
>           that's better?
> 
>  src/ipa/rpi/controller/rpi/agc_channel.cpp | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index c9df9b5b..8583f4f3 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -12,6 +12,8 @@
>  
>  #include <libcamera/base/log.h>
>  
> +#include "libipa/colours.h"
> +
>  #include "../awb_status.h"
>  #include "../device_status.h"
>  #include "../histogram.h"
> @@ -694,11 +696,11 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,
>         double ySum;
>         /* Factor in the AWB correction if needed. */
>         if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {
> -               ySum = rSum * awb.gainR * .299 +
> -                      gSum * awb.gainG * .587 +
> -                      bSum * awb.gainB * .114;
> +               ySum = ipa::rec601LuminanceFromRGB(rSum * awb.gainR,
> +                                                  gSum * awb.gainG,
> +                                                  bSum * awb.gainB);
>         } else
> -               ySum = rSum * .299 + gSum * .587 + bSum * .114;
> +               ySum = ipa::rec601LuminanceFromRGB(rSum, gSum, bSum);

This looks fine to me - but it needs an Ack from RPi.

I think it's up to RPi whether to specify using namespace libcamera::ipa
or not too.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
>         return ySum / pixelSum / (1 << 16);
>  }
> -- 
> 2.30.2
>
Laurent Pinchart Nov. 18, 2024, 12:54 a.m. UTC | #2 
Hi Dabn

Thank you for the patch.

On Fri, Nov 15, 2024 at 07:46:26AM +0000, Daniel Scally wrote:
> Use the centralised libipa helpers rather than open coding common
> functions.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes in v3:
> 
> 	- None
> 
> Changes in v2:
> 
> 	- None. Jacopo; this isn't in the libcamera::ipa namespace. As an
> 	  alternative to the prefix I could "using namespace libcamera::ipa" if
> 	  that's better?
> 
>  src/ipa/rpi/controller/rpi/agc_channel.cpp | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index c9df9b5b..8583f4f3 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -12,6 +12,8 @@
>  
>  #include <libcamera/base/log.h>
>  
> +#include "libipa/colours.h"
> +
>  #include "../awb_status.h"
>  #include "../device_status.h"
>  #include "../histogram.h"
> @@ -694,11 +696,11 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,
>  	double ySum;
>  	/* Factor in the AWB correction if needed. */
>  	if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {
> -		ySum = rSum * awb.gainR * .299 +
> -		       gSum * awb.gainG * .587 +
> -		       bSum * awb.gainB * .114;
> +		ySum = ipa::rec601LuminanceFromRGB(rSum * awb.gainR,
> +						   gSum * awb.gainG,
> +						   bSum * awb.gainB);
>  	} else
> -		ySum = rSum * .299 + gSum * .587 + bSum * .114;
> +		ySum = ipa::rec601LuminanceFromRGB(rSum, gSum, bSum);
>  
>  	return ySum / pixelSum / (1 << 16);
>  }
Dan Scally Nov. 18, 2024, 3:27 p.m. UTC | #3 
+CC to David & Naush

On 15/11/2024 12:01, Kieran Bingham wrote:
> Quoting Daniel Scally (2024-11-15 07:46:26)
>> Use the centralised libipa helpers rather than open coding common
>> functions.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> Changes in v3:
>>
>>          - None
>>
>> Changes in v2:
>>
>>          - None. Jacopo; this isn't in the libcamera::ipa namespace. As an
>>            alternative to the prefix I could "using namespace libcamera::ipa" if
>>            that's better?
>>
>>   src/ipa/rpi/controller/rpi/agc_channel.cpp | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
>> index c9df9b5b..8583f4f3 100644
>> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
>> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
>> @@ -12,6 +12,8 @@
>>   
>>   #include <libcamera/base/log.h>
>>   
>> +#include "libipa/colours.h"
>> +
>>   #include "../awb_status.h"
>>   #include "../device_status.h"
>>   #include "../histogram.h"
>> @@ -694,11 +696,11 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,
>>          double ySum;
>>          /* Factor in the AWB correction if needed. */
>>          if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {
>> -               ySum = rSum * awb.gainR * .299 +
>> -                      gSum * awb.gainG * .587 +
>> -                      bSum * awb.gainB * .114;
>> +               ySum = ipa::rec601LuminanceFromRGB(rSum * awb.gainR,
>> +                                                  gSum * awb.gainG,
>> +                                                  bSum * awb.gainB);
>>          } else
>> -               ySum = rSum * .299 + gSum * .587 + bSum * .114;
>> +               ySum = ipa::rec601LuminanceFromRGB(rSum, gSum, bSum);
> This looks fine to me - but it needs an Ack from RPi.
>
> I think it's up to RPi whether to specify using namespace libcamera::ipa
> or not too.


Sorry to chase but we wanted to try to merge this set to cut back the number of conflicting series 
in flight - is this change alright by you?


Thanks

Dan

>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>>   
>>          return ySum / pixelSum / (1 << 16);
>>   }
>> -- 
>> 2.30.2
>>
Naushir Patuck Nov. 18, 2024, 3:39 p.m. UTC | #4 
Hi Dan,


On Fri, 15 Nov 2024 at 07:46, Daniel Scally <dan.scally@ideasonboard.com> wrote:
>
> Use the centralised libipa helpers rather than open coding common
> functions.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

This looks fine.

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
> Changes in v3:
>
>         - None
>
> Changes in v2:
>
>         - None. Jacopo; this isn't in the libcamera::ipa namespace. As an
>           alternative to the prefix I could "using namespace libcamera::ipa" if
>           that's better?
>
>  src/ipa/rpi/controller/rpi/agc_channel.cpp | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index c9df9b5b..8583f4f3 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -12,6 +12,8 @@
>
>  #include <libcamera/base/log.h>
>
> +#include "libipa/colours.h"
> +
>  #include "../awb_status.h"
>  #include "../device_status.h"
>  #include "../histogram.h"
> @@ -694,11 +696,11 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,
>         double ySum;
>         /* Factor in the AWB correction if needed. */
>         if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {
> -               ySum = rSum * awb.gainR * .299 +
> -                      gSum * awb.gainG * .587 +
> -                      bSum * awb.gainB * .114;
> +               ySum = ipa::rec601LuminanceFromRGB(rSum * awb.gainR,
> +                                                  gSum * awb.gainG,
> +                                                  bSum * awb.gainB);
>         } else
> -               ySum = rSum * .299 + gSum * .587 + bSum * .114;
> +               ySum = ipa::rec601LuminanceFromRGB(rSum, gSum, bSum);
>
>         return ySum / pixelSum / (1 << 16);
>  }
> --
> 2.30.2
>

Patch
diff mbox series 

diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
index c9df9b5b..8583f4f3 100644
--- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
+++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
@@ -12,6 +12,8 @@ 
 
 #include <libcamera/base/log.h>
 
+#include "libipa/colours.h"
+
 #include "../awb_status.h"
 #include "../device_status.h"
 #include "../histogram.h"
@@ -694,11 +696,11 @@  static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,
 	double ySum;
 	/* Factor in the AWB correction if needed. */
 	if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {
-		ySum = rSum * awb.gainR * .299 +
-		       gSum * awb.gainG * .587 +
-		       bSum * awb.gainB * .114;
+		ySum = ipa::rec601LuminanceFromRGB(rSum * awb.gainR,
+						   gSum * awb.gainG,
+						   bSum * awb.gainB);
 	} else
-		ySum = rSum * .299 + gSum * .587 + bSum * .114;
+		ySum = ipa::rec601LuminanceFromRGB(rSum, gSum, bSum);
 
 	return ySum / pixelSum / (1 << 16);
 }