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