[libcamera-devel] utils: raspberrypi: ctt: Fix alsc green averaging

17638 diff mbox series

The alsc component of ctt meant to average the two green channels into
one, but used incorrect indexing resulting in only the first green
channel being used. Fix this.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 utils/raspberrypi/ctt/ctt_alsc.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Paul,

(CC'ing Naush and David)

Thank you for the patch.

On Wed, Oct 19, 2022 at 08:41:43PM +0900, Paul Elder via libcamera-devel wrote:
> The alsc component of ctt meant to average the two green channels into
> one, but used incorrect indexing resulting in only the first green
> channel being used. Fix this.

Well spotted.

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

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

I'd like to have an ack from David or Naush too.

This makes me wonder how different the two green channels can be if this
has never been noticed before.

> ---
>  utils/raspberrypi/ctt/ctt_alsc.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utils/raspberrypi/ctt/ctt_alsc.py b/utils/raspberrypi/ctt/ctt_alsc.py
> index c0ba7b63..e51d6931 100644
> --- a/utils/raspberrypi/ctt/ctt_alsc.py
> +++ b/utils/raspberrypi/ctt/ctt_alsc.py
> @@ -132,7 +132,7 @@ def alsc(Cam, Img, do_alsc_colour, plot=False):
>      """
>      average the green channels into one
>      """
> -    av_ch_g = np.mean((channels[1:2]), axis=0)
> +    av_ch_g = np.mean((channels[1:3]), axis=0)
>      if do_alsc_colour:
>          """
>          obtain 16x12 grid of intensities for each channel and subtract black level

Hi Paul

Thanks for spotting this, indeed I assume it was probably the
intention to do it this way.

On Wed, 19 Oct 2022 at 17:09, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Paul,
>
> (CC'ing Naush and David)
>
> Thank you for the patch.
>
> On Wed, Oct 19, 2022 at 08:41:43PM +0900, Paul Elder via libcamera-devel wrote:
> > The alsc component of ctt meant to average the two green channels into
> > one, but used incorrect indexing resulting in only the first green
> > channel being used. Fix this.
>
> Well spotted.
>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

>
> I'd like to have an ack from David or Naush too.
>
> This makes me wonder how different the two green channels can be if this
> has never been noticed before.

Hmm, I'm not sure that you could ever notice. If the red or blue
channels are made flat compared to the Gr channel (for example),
they're probably also flat compared to the Gb. So you might find that
the red values are (again, as an example) 50% of the Gr values
everywhere but maybe 53% of the Gb. But that should be irrelevant
because we always scale the lens shading gains so that the smallest
one is exactly 1 before applying them. (The AWB then worries about the
relative magnitudes of the reds and greens.)

So I'm somewhat suspecting it makes no real difference, though the
change is probably worthwhile just to stop discussions like this!!

Thanks again
David

>
> > ---
> >  utils/raspberrypi/ctt/ctt_alsc.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/utils/raspberrypi/ctt/ctt_alsc.py b/utils/raspberrypi/ctt/ctt_alsc.py
> > index c0ba7b63..e51d6931 100644
> > --- a/utils/raspberrypi/ctt/ctt_alsc.py
> > +++ b/utils/raspberrypi/ctt/ctt_alsc.py
> > @@ -132,7 +132,7 @@ def alsc(Cam, Img, do_alsc_colour, plot=False):
> >      """
> >      average the green channels into one
> >      """
> > -    av_ch_g = np.mean((channels[1:2]), axis=0)
> > +    av_ch_g = np.mean((channels[1:3]), axis=0)
> >      if do_alsc_colour:
> >          """
> >          obtain 16x12 grid of intensities for each channel and subtract black level
>
> --
> Regards,
>
> Laurent Pinchart