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

[William Vinnicombe]

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 at 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 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220715/9d2c6e27/attachment.htm>