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

[Laurent Pinchart]

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 at 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 at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush at 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