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

[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 at 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 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