[v3,21/23] libtuning: lsc: rkisp1: Do not calculate ratios to green
diff mbox series

Message ID 20240703141726.252368-22-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • Add ccm calibration to libtuning
Related show

Commit Message

Stefan Klug July 3, 2024, 2:17 p.m. UTC
The current LSC algorithm for the rkisp1 just forwards the LSC tables to
the hardware, so absolute factors are needed and not ratios compared to
green. Therefore every channel needs to be calculated independently.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 utils/tuning/libtuning/modules/lsc/rkisp1.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart July 4, 2024, 10:23 a.m. UTC | #1
Hi Stefan,

Thank you for the patch.

On Wed, Jul 03, 2024 at 04:17:10PM +0200, Stefan Klug wrote:
> The current LSC algorithm for the rkisp1 just forwards the LSC tables to
> the hardware, so absolute factors are needed and not ratios compared to
> green. Therefore every channel needs to be calculated independently.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  utils/tuning/libtuning/modules/lsc/rkisp1.py | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/tuning/libtuning/modules/lsc/rkisp1.py b/utils/tuning/libtuning/modules/lsc/rkisp1.py
> index 512233aeae9d..56df23ec7b42 100644
> --- a/utils/tuning/libtuning/modules/lsc/rkisp1.py
> +++ b/utils/tuning/libtuning/modules/lsc/rkisp1.py
> @@ -38,8 +38,12 @@ class LSCRkISP1(LSC):
>  
>          # \todo Should these ratio against the average of both greens or just
>          # each green like we've done here?

I think this comment can be dropped.

> -        cr, _ = self._lsc_single_channel(image.channels[lt.Color.R], image, gr)
> -        cb, _ = self._lsc_single_channel(image.channels[lt.Color.B], image, gb)

gr and gb are not used anymore, so you can drop them from the lines
above this hunk.

> +
> +        # The LSC tables in the our rkisp1 algorithm represent gains on the
> +        # corresponding channel, not ratios with respect to green, so calculate
> +        # them independently
> +        cr, _ = self._lsc_single_channel(image.channels[lt.Color.R], image, None)
> +        cb, _ = self._lsc_single_channel(image.channels[lt.Color.B], image, None)

None is the default value for the last parameter so you can drop it.

I think all of this should become

        # Perform LSC on each colour channel independently. A future enhancement
	# worth investigating would be splitting the luminance and chrominance
	# LSC as done by Raspberry Pi.
        cgr, _ = self._lsc_single_channel(image.channels[lt.Color.GR], image)
        cgb, _ = self._lsc_single_channel(image.channels[lt.Color.GB], image)
        cr, _ = self._lsc_single_channel(image.channels[lt.Color.R], image)
        cb, _ = self._lsc_single_channel(image.channels[lt.Color.B], image)

You can drop the future enhancement sentence if desired.

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

>  
>          return image.color, cr.flatten(), cb.flatten(), cgr.flatten(), cgb.flatten()
>
Paul Elder July 5, 2024, 6:28 a.m. UTC | #2
On Wed, Jul 03, 2024 at 04:17:10PM +0200, Stefan Klug wrote:
> The current LSC algorithm for the rkisp1 just forwards the LSC tables to
> the hardware, so absolute factors are needed and not ratios compared to
> green. Therefore every channel needs to be calculated independently.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

With Laurent's comments addressed,

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  utils/tuning/libtuning/modules/lsc/rkisp1.py | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/tuning/libtuning/modules/lsc/rkisp1.py b/utils/tuning/libtuning/modules/lsc/rkisp1.py
> index 512233aeae9d..56df23ec7b42 100644
> --- a/utils/tuning/libtuning/modules/lsc/rkisp1.py
> +++ b/utils/tuning/libtuning/modules/lsc/rkisp1.py
> @@ -38,8 +38,12 @@ class LSCRkISP1(LSC):
>  
>          # \todo Should these ratio against the average of both greens or just
>          # each green like we've done here?
> -        cr, _ = self._lsc_single_channel(image.channels[lt.Color.R], image, gr)
> -        cb, _ = self._lsc_single_channel(image.channels[lt.Color.B], image, gb)
> +
> +        # The LSC tables in the our rkisp1 algorithm represent gains on the
> +        # corresponding channel, not ratios with respect to green, so calculate
> +        # them independently
> +        cr, _ = self._lsc_single_channel(image.channels[lt.Color.R], image, None)
> +        cb, _ = self._lsc_single_channel(image.channels[lt.Color.B], image, None)
>  
>          return image.color, cr.flatten(), cb.flatten(), cgr.flatten(), cgb.flatten()
>  
> -- 
> 2.43.0
>

Patch
diff mbox series

diff --git a/utils/tuning/libtuning/modules/lsc/rkisp1.py b/utils/tuning/libtuning/modules/lsc/rkisp1.py
index 512233aeae9d..56df23ec7b42 100644
--- a/utils/tuning/libtuning/modules/lsc/rkisp1.py
+++ b/utils/tuning/libtuning/modules/lsc/rkisp1.py
@@ -38,8 +38,12 @@  class LSCRkISP1(LSC):
 
         # \todo Should these ratio against the average of both greens or just
         # each green like we've done here?
-        cr, _ = self._lsc_single_channel(image.channels[lt.Color.R], image, gr)
-        cb, _ = self._lsc_single_channel(image.channels[lt.Color.B], image, gb)
+
+        # The LSC tables in the our rkisp1 algorithm represent gains on the
+        # corresponding channel, not ratios with respect to green, so calculate
+        # them independently
+        cr, _ = self._lsc_single_channel(image.channels[lt.Color.R], image, None)
+        cb, _ = self._lsc_single_channel(image.channels[lt.Color.B], image, None)
 
         return image.color, cr.flatten(), cb.flatten(), cgr.flatten(), cgb.flatten()