[libcamera-devel,v2,1/2] utils: raspberrypi: ctt: Add alsc_only method

16582 diff mbox series

From: William Vinnicombe <william.vinnicombe@raspberrypi.com>

The ctt would not work if only passed alsc images.

Add alsc_only.py to run alsc calibration only, and modify check_imgs
to allow for no macbeth chart images.

Example usage would be ./alsc_only.py -i tuning-images/ -o sensor.json
with the same optional arguments as the original ctt.

Signed-off-by: William Vinnicombe <william.vinnicombe@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 utils/raspberrypi/ctt/alsc_only.py | 34 ++++++++++++++++++++++++++++++
 utils/raspberrypi/ctt/ctt.py       | 17 ++++++++++-----
 2 files changed, 46 insertions(+), 5 deletions(-)
 create mode 100755 utils/raspberrypi/ctt/alsc_only.py

Hi William,

Thank you for the patch.

On Mon, Jul 11, 2022 at 11:26:27AM +0100, william.vinnicombe--- via libcamera-devel wrote:
> From: William Vinnicombe <william.vinnicombe@raspberrypi.com>
> 
> The ctt would not work if only passed alsc images.
> 
> Add alsc_only.py to run alsc calibration only, and modify check_imgs
> to allow for no macbeth chart images.
> 
> Example usage would be ./alsc_only.py -i tuning-images/ -o sensor.json
> with the same optional arguments as the original ctt.
> 
> Signed-off-by: William Vinnicombe <william.vinnicombe@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  utils/raspberrypi/ctt/alsc_only.py | 34 ++++++++++++++++++++++++++++++
>  utils/raspberrypi/ctt/ctt.py       | 17 ++++++++++-----
>  2 files changed, 46 insertions(+), 5 deletions(-)
>  create mode 100755 utils/raspberrypi/ctt/alsc_only.py
> 
> diff --git a/utils/raspberrypi/ctt/alsc_only.py b/utils/raspberrypi/ctt/alsc_only.py
> new file mode 100755
> index 00000000..7cd0ac01
> --- /dev/null
> +++ b/utils/raspberrypi/ctt/alsc_only.py
> @@ -0,0 +1,34 @@
> +#!/usr/bin/env python3
> +#
> +# SPDX-License-Identifier: BSD-2-Clause
> +#
> +# Copyright (C) 2022, Raspberry Pi (Trading) Limited
> +#
> +# alsc_only.py - alsc tuning tool
> +
> +from ctt import *
> +
> +
> +if __name__ == '__main__':
> +    """
> +    initialise calibration
> +    """
> +    if len(sys.argv) == 1:
> +        print("""
> +    Pisp Camera Tuning Tool version 1.0
> +
> +    Required Arguments:
> +    '-i' : Calibration image directory.
> +    '-o' : Name of output json file.
> +
> +    Optional Arguments:
> +    '-c' : Config file for the CTT. If not passed, default parameters used.
> +    '-l' : Name of output log file. If not passed, 'ctt_log.txt' used.
> +              """)
> +        quit(0)
> +    else:
> +        """
> +        parse input arguments
> +        """
> +        json_output, directory, config, log_output = parse_input()
> +        run_ctt(json_output, directory, config, log_output, alsc_only=True)

That's much better than v1, thanks.

Maybe it's a stupid question, but given that the only difference between
this and the main function of ctt.py is the alsc_only parameter passed
to run_ctt(), could we go one step further and add an optional command
line argument to select the ALSC-only mode, and avoid adding a separate
tool ?

The rest looks fine.

> diff --git a/utils/raspberrypi/ctt/ctt.py b/utils/raspberrypi/ctt/ctt.py
> index 15064634..2bd97514 100755
> --- a/utils/raspberrypi/ctt/ctt.py
> +++ b/utils/raspberrypi/ctt/ctt.py
> @@ -664,7 +664,7 @@ class Camera:
>          - incorrect filename/extension
>          - images from different cameras
>      """
> -    def check_imgs(self):
> +    def check_imgs(self, macbeth=True):
>          self.log += '\n\nImages found:'
>          self.log += '\nMacbeth : {}'.format(len(self.imgs))
>          self.log += '\nALSC : {} '.format(len(self.imgs_alsc))
> @@ -672,10 +672,14 @@ class Camera:
>          """
>          check usable images found
>          """
> -        if len(self.imgs) == 0:
> +        if len(self.imgs) == 0 and macbeth:
>              print('\nERROR: No usable macbeth chart images found')
>              self.log += '\nERROR: No usable macbeth chart images found'
>              return 0
> +        elif len(self.imgs) == 0 and len(self.imgs_alsc) == 0:
> +            print('\nERROR: No usable images found')
> +            self.log += '\nERROR: No usable images found'
> +            return 0
>          """
>          Double check that every image has come from the same camera...
>          """
> @@ -704,7 +708,7 @@ class Camera:
>              return 0
>  
>  
> -def run_ctt(json_output, directory, config, log_output):
> +def run_ctt(json_output, directory, config, log_output, alsc_only=False):
>      """
>      check input files are jsons
>      """
> @@ -766,6 +770,8 @@ def run_ctt(json_output, directory, config, log_output):
>      try:
>          Cam = Camera(json_output)
>          Cam.log_user_input(json_output, directory, config, log_output)
> +        if alsc_only:
> +            disable = set(Cam.json.keys()).symmetric_difference({"rpi.alsc"})
>          Cam.disable = disable
>          Cam.plot = plot
>          Cam.add_imgs(directory, mac_config, blacklevel)
> @@ -779,8 +785,9 @@ def run_ctt(json_output, directory, config, log_output):
>      ccm also technically does an awb but it measures this from the macbeth
>      chart in the image rather than using calibration data
>      """
> -    if Cam.check_imgs():
> -        Cam.json['rpi.black_level']['black_level'] = Cam.blacklevel_16
> +    if Cam.check_imgs(macbeth=not alsc_only):
> +        if not alsc_only:
> +            Cam.json['rpi.black_level']['black_level'] = Cam.blacklevel_16
>          Cam.json_remove(disable)
>          print('\nSTARTING CALIBRATIONS')
>          Cam.alsc_cal(luminance_strength, do_alsc_colour)

Hi Laurent,

Thanks for your comments.

I think we were hoping to make it a separate file, so it's easier for users
to run
only the lens shading, as that's the aspect of the tool we expect to be
used more
given users will want to use different lenses.

Best,

William Vinnicombe

On Thu, 14 Jul 2022 at 14:54, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi William,
>
> Thank you for the patch.
>
> On Mon, Jul 11, 2022 at 11:26:27AM +0100, william.vinnicombe--- via
> libcamera-devel wrote:
> > From: William Vinnicombe <william.vinnicombe@raspberrypi.com>
> >
> > The ctt would not work if only passed alsc images.
> >
> > Add alsc_only.py to run alsc calibration only, and modify check_imgs
> > to allow for no macbeth chart images.
> >
> > Example usage would be ./alsc_only.py -i tuning-images/ -o sensor.json
> > with the same optional arguments as the original ctt.
> >
> > Signed-off-by: William Vinnicombe <william.vinnicombe@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  utils/raspberrypi/ctt/alsc_only.py | 34 ++++++++++++++++++++++++++++++
> >  utils/raspberrypi/ctt/ctt.py       | 17 ++++++++++-----
> >  2 files changed, 46 insertions(+), 5 deletions(-)
> >  create mode 100755 utils/raspberrypi/ctt/alsc_only.py
> >
> > diff --git a/utils/raspberrypi/ctt/alsc_only.py
> b/utils/raspberrypi/ctt/alsc_only.py
> > new file mode 100755
> > index 00000000..7cd0ac01
> > --- /dev/null
> > +++ b/utils/raspberrypi/ctt/alsc_only.py
> > @@ -0,0 +1,34 @@
> > +#!/usr/bin/env python3
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause
> > +#
> > +# Copyright (C) 2022, Raspberry Pi (Trading) Limited
> > +#
> > +# alsc_only.py - alsc tuning tool
> > +
> > +from ctt import *
> > +
> > +
> > +if __name__ == '__main__':
> > +    """
> > +    initialise calibration
> > +    """
> > +    if len(sys.argv) == 1:
> > +        print("""
> > +    Pisp Camera Tuning Tool version 1.0
> > +
> > +    Required Arguments:
> > +    '-i' : Calibration image directory.
> > +    '-o' : Name of output json file.
> > +
> > +    Optional Arguments:
> > +    '-c' : Config file for the CTT. If not passed, default parameters
> used.
> > +    '-l' : Name of output log file. If not passed, 'ctt_log.txt' used.
> > +              """)
> > +        quit(0)
> > +    else:
> > +        """
> > +        parse input arguments
> > +        """
> > +        json_output, directory, config, log_output = parse_input()
> > +        run_ctt(json_output, directory, config, log_output,
> alsc_only=True)
>
> That's much better than v1, thanks.
>
> Maybe it's a stupid question, but given that the only difference between
> this and the main function of ctt.py is the alsc_only parameter passed
> to run_ctt(), could we go one step further and add an optional command
> line argument to select the ALSC-only mode, and avoid adding a separate
> tool ?
>
> The rest looks fine.
>
> > diff --git a/utils/raspberrypi/ctt/ctt.py b/utils/raspberrypi/ctt/ctt.py
> > index 15064634..2bd97514 100755
> > --- a/utils/raspberrypi/ctt/ctt.py
> > +++ b/utils/raspberrypi/ctt/ctt.py
> > @@ -664,7 +664,7 @@ class Camera:
> >          - incorrect filename/extension
> >          - images from different cameras
> >      """
> > -    def check_imgs(self):
> > +    def check_imgs(self, macbeth=True):
> >          self.log += '\n\nImages found:'
> >          self.log += '\nMacbeth : {}'.format(len(self.imgs))
> >          self.log += '\nALSC : {} '.format(len(self.imgs_alsc))
> > @@ -672,10 +672,14 @@ class Camera:
> >          """
> >          check usable images found
> >          """
> > -        if len(self.imgs) == 0:
> > +        if len(self.imgs) == 0 and macbeth:
> >              print('\nERROR: No usable macbeth chart images found')
> >              self.log += '\nERROR: No usable macbeth chart images found'
> >              return 0
> > +        elif len(self.imgs) == 0 and len(self.imgs_alsc) == 0:
> > +            print('\nERROR: No usable images found')
> > +            self.log += '\nERROR: No usable images found'
> > +            return 0
> >          """
> >          Double check that every image has come from the same camera...
> >          """
> > @@ -704,7 +708,7 @@ class Camera:
> >              return 0
> >
> >
> > -def run_ctt(json_output, directory, config, log_output):
> > +def run_ctt(json_output, directory, config, log_output,
> alsc_only=False):
> >      """
> >      check input files are jsons
> >      """
> > @@ -766,6 +770,8 @@ def run_ctt(json_output, directory, config,
> log_output):
> >      try:
> >          Cam = Camera(json_output)
> >          Cam.log_user_input(json_output, directory, config, log_output)
> > +        if alsc_only:
> > +            disable =
> set(Cam.json.keys()).symmetric_difference({"rpi.alsc"})
> >          Cam.disable = disable
> >          Cam.plot = plot
> >          Cam.add_imgs(directory, mac_config, blacklevel)
> > @@ -779,8 +785,9 @@ def run_ctt(json_output, directory, config,
> log_output):
> >      ccm also technically does an awb but it measures this from the
> macbeth
> >      chart in the image rather than using calibration data
> >      """
> > -    if Cam.check_imgs():
> > -        Cam.json['rpi.black_level']['black_level'] = Cam.blacklevel_16
> > +    if Cam.check_imgs(macbeth=not alsc_only):
> > +        if not alsc_only:
> > +            Cam.json['rpi.black_level']['black_level'] =
> Cam.blacklevel_16
> >          Cam.json_remove(disable)
> >          print('\nSTARTING CALIBRATIONS')
> >          Cam.alsc_cal(luminance_strength, do_alsc_colour)
>
> --
> Regards,
>
> Laurent Pinchart
>

Hi William,

On Fri, Jul 15, 2022 at 03:22:44PM +0100, William Vinnicombe wrote:
> Hi Laurent,
> 
> Thanks for your comments.
> 
> I think we were hoping to make it a separate file, so it's easier for users to run
> only the lens shading, as that's the aspect of the tool we expect to be used more
> given users will want to use different lenses.

Fair enough.

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

> On Thu, 14 Jul 2022 at 14:54, Laurent Pinchart wrote:
> > On Mon, Jul 11, 2022 at 11:26:27AM +0100, william.vinnicombe--- via libcamera-devel wrote:
> > > From: William Vinnicombe <william.vinnicombe@raspberrypi.com>
> > >
> > > The ctt would not work if only passed alsc images.
> > >
> > > Add alsc_only.py to run alsc calibration only, and modify check_imgs
> > > to allow for no macbeth chart images.
> > >
> > > Example usage would be ./alsc_only.py -i tuning-images/ -o sensor.json
> > > with the same optional arguments as the original ctt.
> > >
> > > Signed-off-by: William Vinnicombe <william.vinnicombe@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  utils/raspberrypi/ctt/alsc_only.py | 34 ++++++++++++++++++++++++++++++
> > >  utils/raspberrypi/ctt/ctt.py       | 17 ++++++++++-----
> > >  2 files changed, 46 insertions(+), 5 deletions(-)
> > >  create mode 100755 utils/raspberrypi/ctt/alsc_only.py
> > >
> > > diff --git a/utils/raspberrypi/ctt/alsc_only.py b/utils/raspberrypi/ctt/alsc_only.py
> > > new file mode 100755
> > > index 00000000..7cd0ac01
> > > --- /dev/null
> > > +++ b/utils/raspberrypi/ctt/alsc_only.py
> > > @@ -0,0 +1,34 @@
> > > +#!/usr/bin/env python3
> > > +#
> > > +# SPDX-License-Identifier: BSD-2-Clause
> > > +#
> > > +# Copyright (C) 2022, Raspberry Pi (Trading) Limited
> > > +#
> > > +# alsc_only.py - alsc tuning tool
> > > +
> > > +from ctt import *
> > > +
> > > +
> > > +if __name__ == '__main__':
> > > +    """
> > > +    initialise calibration
> > > +    """
> > > +    if len(sys.argv) == 1:
> > > +        print("""
> > > +    Pisp Camera Tuning Tool version 1.0
> > > +
> > > +    Required Arguments:
> > > +    '-i' : Calibration image directory.
> > > +    '-o' : Name of output json file.
> > > +
> > > +    Optional Arguments:
> > > +    '-c' : Config file for the CTT. If not passed, default parameters used.
> > > +    '-l' : Name of output log file. If not passed, 'ctt_log.txt' used.
> > > +              """)
> > > +        quit(0)
> > > +    else:
> > > +        """
> > > +        parse input arguments
> > > +        """
> > > +        json_output, directory, config, log_output = parse_input()
> > > +        run_ctt(json_output, directory, config, log_output, alsc_only=True)
> >
> > That's much better than v1, thanks.
> >
> > Maybe it's a stupid question, but given that the only difference between
> > this and the main function of ctt.py is the alsc_only parameter passed
> > to run_ctt(), could we go one step further and add an optional command
> > line argument to select the ALSC-only mode, and avoid adding a separate
> > tool ?
> >
> > The rest looks fine.
> >
> > > diff --git a/utils/raspberrypi/ctt/ctt.py b/utils/raspberrypi/ctt/ctt.py
> > > index 15064634..2bd97514 100755
> > > --- a/utils/raspberrypi/ctt/ctt.py
> > > +++ b/utils/raspberrypi/ctt/ctt.py
> > > @@ -664,7 +664,7 @@ class Camera:
> > >          - incorrect filename/extension
> > >          - images from different cameras
> > >      """
> > > -    def check_imgs(self):
> > > +    def check_imgs(self, macbeth=True):
> > >          self.log += '\n\nImages found:'
> > >          self.log += '\nMacbeth : {}'.format(len(self.imgs))
> > >          self.log += '\nALSC : {} '.format(len(self.imgs_alsc))
> > > @@ -672,10 +672,14 @@ class Camera:
> > >          """
> > >          check usable images found
> > >          """
> > > -        if len(self.imgs) == 0:
> > > +        if len(self.imgs) == 0 and macbeth:
> > >              print('\nERROR: No usable macbeth chart images found')
> > >              self.log += '\nERROR: No usable macbeth chart images found'
> > >              return 0
> > > +        elif len(self.imgs) == 0 and len(self.imgs_alsc) == 0:
> > > +            print('\nERROR: No usable images found')
> > > +            self.log += '\nERROR: No usable images found'
> > > +            return 0
> > >          """
> > >          Double check that every image has come from the same camera...
> > >          """
> > > @@ -704,7 +708,7 @@ class Camera:
> > >              return 0
> > >
> > >
> > > -def run_ctt(json_output, directory, config, log_output):
> > > +def run_ctt(json_output, directory, config, log_output, alsc_only=False):
> > >      """
> > >      check input files are jsons
> > >      """
> > > @@ -766,6 +770,8 @@ def run_ctt(json_output, directory, config, log_output):
> > >      try:
> > >          Cam = Camera(json_output)
> > >          Cam.log_user_input(json_output, directory, config, log_output)
> > > +        if alsc_only:
> > > +            disable = set(Cam.json.keys()).symmetric_difference({"rpi.alsc"})
> > >          Cam.disable = disable
> > >          Cam.plot = plot
> > >          Cam.add_imgs(directory, mac_config, blacklevel)
> > > @@ -779,8 +785,9 @@ def run_ctt(json_output, directory, config, log_output):
> > >      ccm also technically does an awb but it measures this from the macbeth
> > >      chart in the image rather than using calibration data
> > >      """
> > > -    if Cam.check_imgs():
> > > -        Cam.json['rpi.black_level']['black_level'] = Cam.blacklevel_16
> > > +    if Cam.check_imgs(macbeth=not alsc_only):
> > > +        if not alsc_only:
> > > +            Cam.json['rpi.black_level']['black_level'] = Cam.blacklevel_16
> > >          Cam.json_remove(disable)
> > >          print('\nSTARTING CALIBRATIONS')
> > >          Cam.alsc_cal(luminance_strength, do_alsc_colour)