[v2,15/25] libtuning: modules: Add initial CCM module
diff mbox series

Message ID 20240628104828.2928109-16-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • Add ccm calibration to libtuning
Related show

Commit Message

Stefan Klug June 28, 2024, 10:47 a.m. UTC 
From: Paul Elder <paul.elder@ideasonboard.com>

Implement a minimal ccm calibration module. For now it doesn't take the
results from lsc into account and supports rkisp1 only.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 .../tuning/libtuning/modules/ccm/__init__.py  |  6 +++
 utils/tuning/libtuning/modules/ccm/ccm.py     | 44 +++++++++++++++++++
 utils/tuning/libtuning/modules/ccm/rkisp1.py  | 34 ++++++++++++++
 utils/tuning/rkisp1.py                        |  4 +-
 4 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100644 utils/tuning/libtuning/modules/ccm/__init__.py
 create mode 100644 utils/tuning/libtuning/modules/ccm/ccm.py
 create mode 100644 utils/tuning/libtuning/modules/ccm/rkisp1.py

Comments

Laurent Pinchart June 28, 2024, 11:47 p.m. UTC | #1 
Hi Paul and Stefan,

Thank you for the patch.

On Fri, Jun 28, 2024 at 12:47:08PM +0200, Stefan Klug wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
> 
> Implement a minimal ccm calibration module. For now it doesn't take the
> results from lsc into account and supports rkisp1 only.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  .../tuning/libtuning/modules/ccm/__init__.py  |  6 +++
>  utils/tuning/libtuning/modules/ccm/ccm.py     | 44 +++++++++++++++++++
>  utils/tuning/libtuning/modules/ccm/rkisp1.py  | 34 ++++++++++++++
>  utils/tuning/rkisp1.py                        |  4 +-
>  4 files changed, 87 insertions(+), 1 deletion(-)
>  create mode 100644 utils/tuning/libtuning/modules/ccm/__init__.py
>  create mode 100644 utils/tuning/libtuning/modules/ccm/ccm.py
>  create mode 100644 utils/tuning/libtuning/modules/ccm/rkisp1.py
> 
> diff --git a/utils/tuning/libtuning/modules/ccm/__init__.py b/utils/tuning/libtuning/modules/ccm/__init__.py
> new file mode 100644
> index 000000000000..322602afe63b
> --- /dev/null
> +++ b/utils/tuning/libtuning/modules/ccm/__init__.py
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> +
> +from libtuning.modules.ccm.ccm import CCM
> +from libtuning.modules.ccm.rkisp1 import CCMRkISP1
> diff --git a/utils/tuning/libtuning/modules/ccm/ccm.py b/utils/tuning/libtuning/modules/ccm/ccm.py
> new file mode 100644
> index 000000000000..50d435ad84a3
> --- /dev/null
> +++ b/utils/tuning/libtuning/modules/ccm/ccm.py
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> +# Copyright (C) 2024, Ideas on Board
> +#
> +# Base Ccm tuning module
> +
> +from ..module import Module
> +
> +from libtuning.ctt_ccm import ccm
> +import logging
> +import time
> +
> +import numpy as np

time and numpy don't seem needed.

> +
> +logger = logging.getLogger(__name__)
> +
> +
> +class CCM(Module):
> +    type = 'ccm'
> +    hr_name = 'CCM (Base)'

Unrelated to this patch, renaming this to display_name may make the code
more readable.

> +    out_name = 'GenericCCM'

If the base algorithms are not meant to be used directly, should we drop
the out_name ?

> +
> +    def __init__(self, debug: list):
> +        super().__init__()
> +
> +        self.debug = debug
> +
> +    def do_calibration(self, images):
> +        logger.info('Starting CCM calibration')
> +
> +        imgs = [img for img in images if img.macbeth is not None]
> +
> +        # todo: take alsc calibration results into account

s/alsc/LSC/

> +        cal_cr_list = None
> +        cal_cb_list = None
> +
> +        try:
> +            ccms = ccm(imgs, cal_cr_list, cal_cb_list)
> +        except ArithmeticError:
> +            logger.error('CCM calibration failed')
> +            return 1
> +
> +        return ccms
> diff --git a/utils/tuning/libtuning/modules/ccm/rkisp1.py b/utils/tuning/libtuning/modules/ccm/rkisp1.py
> new file mode 100644
> index 000000000000..a74d0d93c764
> --- /dev/null
> +++ b/utils/tuning/libtuning/modules/ccm/rkisp1.py
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> +# Copyright (C) 2024, Ideas on Board
> +#
> +# Ccm module for tuning rkisp1
> +
> +from .ccm import CCM
> +
> +import libtuning as lt
> +import libtuning.utils as utils
> +
> +from numbers import Number
> +import numpy as np

Apart from CCM, none of thoese look needed either.

> +
> +
> +class CCMRkISP1(CCM):
> +    hr_name = 'Crosstalk Correction (RkISP1)'
> +    out_name = 'Ccm'

The ISP documentation mentions "color correction" twice and uses "cross
talk" everywhere else. In the IPA module, and in libtuning, we mostly
use "ccm". I think that's fine, and I think using the name from the ISP
documentation in hr_name is good.

> +
> +    def __init__(self, **kwargs):
> +        super().__init__(**kwargs)
> +
> +    # We don't actually need anything from the config file

s/actually //
s/$/./

> +    def validate_config(self, config: dict) -> bool:
> +        return True
> +
> +    def process(self, config: dict, images: list, outputs: dict) -> dict:
> +        output = {}
> +
> +        ctms = self.do_calibration(images)

Here, however, I would name the variable ccms.

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

> +        output['ccms'] = ctms
> +
> +        return output
> diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py
> index 2606e07a93f7..fae35783c656 100755
> --- a/utils/tuning/rkisp1.py
> +++ b/utils/tuning/rkisp1.py
> @@ -14,6 +14,7 @@ from libtuning.parsers import YamlParser
>  from libtuning.generators import YamlOutput
>  from libtuning.modules.lsc import LSCRkISP1
>  from libtuning.modules.agc import AGCRkISP1
> +from libtuning.modules.ccm import CCMRkISP1
>  
>  
>  coloredlogs.install(level=logging.INFO, fmt='%(name)s %(levelname)s %(message)s')
> @@ -39,9 +40,10 @@ tuner.add(LSCRkISP1(
>            smoothing_function=lt.smoothing.MedianBlur(3),
>            ))
>  tuner.add(AGCRkISP1(debug=[lt.Debug.Plot]))
> +tuner.add(CCMRkISP1(debug=[lt.Debug.Plot]))
>  tuner.set_input_parser(YamlParser())
>  tuner.set_output_formatter(YamlOutput())
> -tuner.set_output_order([AGCRkISP1, LSCRkISP1])
> +tuner.set_output_order([AGCRkISP1, CCMRkISP1, LSCRkISP1])
>  
>  if __name__ == '__main__':
>      sys.exit(tuner.run(sys.argv))
Stefan Klug July 2, 2024, 9:15 a.m. UTC | #2 
Hi Laurent,

On Sat, Jun 29, 2024 at 02:47:53AM +0300, Laurent Pinchart wrote:
> Hi Paul and Stefan,
> 
> Thank you for the patch.
> 
> On Fri, Jun 28, 2024 at 12:47:08PM +0200, Stefan Klug wrote:
> > From: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > Implement a minimal ccm calibration module. For now it doesn't take the
> > results from lsc into account and supports rkisp1 only.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  .../tuning/libtuning/modules/ccm/__init__.py  |  6 +++
> >  utils/tuning/libtuning/modules/ccm/ccm.py     | 44 +++++++++++++++++++
> >  utils/tuning/libtuning/modules/ccm/rkisp1.py  | 34 ++++++++++++++
> >  utils/tuning/rkisp1.py                        |  4 +-
> >  4 files changed, 87 insertions(+), 1 deletion(-)
> >  create mode 100644 utils/tuning/libtuning/modules/ccm/__init__.py
> >  create mode 100644 utils/tuning/libtuning/modules/ccm/ccm.py
> >  create mode 100644 utils/tuning/libtuning/modules/ccm/rkisp1.py
> > 
> > diff --git a/utils/tuning/libtuning/modules/ccm/__init__.py b/utils/tuning/libtuning/modules/ccm/__init__.py
> > new file mode 100644
> > index 000000000000..322602afe63b
> > --- /dev/null
> > +++ b/utils/tuning/libtuning/modules/ccm/__init__.py
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +#
> > +# Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > +
> > +from libtuning.modules.ccm.ccm import CCM
> > +from libtuning.modules.ccm.rkisp1 import CCMRkISP1
> > diff --git a/utils/tuning/libtuning/modules/ccm/ccm.py b/utils/tuning/libtuning/modules/ccm/ccm.py
> > new file mode 100644
> > index 000000000000..50d435ad84a3
> > --- /dev/null
> > +++ b/utils/tuning/libtuning/modules/ccm/ccm.py
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +#
> > +# Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > +# Copyright (C) 2024, Ideas on Board
> > +#
> > +# Base Ccm tuning module
> > +
> > +from ..module import Module
> > +
> > +from libtuning.ctt_ccm import ccm
> > +import logging
> > +import time
> > +
> > +import numpy as np
> 
> time and numpy don't seem needed.
> 
> > +
> > +logger = logging.getLogger(__name__)
> > +
> > +
> > +class CCM(Module):
> > +    type = 'ccm'
> > +    hr_name = 'CCM (Base)'
> 
> Unrelated to this patch, renaming this to display_name may make the code
> more readable.

Yes, that sound good.

> 
> > +    out_name = 'GenericCCM'
> 
> If the base algorithms are not meant to be used directly, should we drop
> the out_name ?

I think we should question that again. Why not a using the base algo
directly? If the content is really generic (and ccm is a likely
candidate) I don't see much benefit in the derived class. I made a
mental note for later :-)

The rest is fixed locally.

Regards,
Stefan

> 
> > +
> > +    def __init__(self, debug: list):
> > +        super().__init__()
> > +
> > +        self.debug = debug
> > +
> > +    def do_calibration(self, images):
> > +        logger.info('Starting CCM calibration')
> > +
> > +        imgs = [img for img in images if img.macbeth is not None]
> > +
> > +        # todo: take alsc calibration results into account
> 
> s/alsc/LSC/
> 
> > +        cal_cr_list = None
> > +        cal_cb_list = None
> > +
> > +        try:
> > +            ccms = ccm(imgs, cal_cr_list, cal_cb_list)
> > +        except ArithmeticError:
> > +            logger.error('CCM calibration failed')
> > +            return 1
> > +
> > +        return ccms
> > diff --git a/utils/tuning/libtuning/modules/ccm/rkisp1.py b/utils/tuning/libtuning/modules/ccm/rkisp1.py
> > new file mode 100644
> > index 000000000000..a74d0d93c764
> > --- /dev/null
> > +++ b/utils/tuning/libtuning/modules/ccm/rkisp1.py
> > @@ -0,0 +1,34 @@
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +#
> > +# Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> > +# Copyright (C) 2024, Ideas on Board
> > +#
> > +# Ccm module for tuning rkisp1
> > +
> > +from .ccm import CCM
> > +
> > +import libtuning as lt
> > +import libtuning.utils as utils
> > +
> > +from numbers import Number
> > +import numpy as np
> 
> Apart from CCM, none of thoese look needed either.
> 
> > +
> > +
> > +class CCMRkISP1(CCM):
> > +    hr_name = 'Crosstalk Correction (RkISP1)'
> > +    out_name = 'Ccm'
> 
> The ISP documentation mentions "color correction" twice and uses "cross
> talk" everywhere else. In the IPA module, and in libtuning, we mostly
> use "ccm". I think that's fine, and I think using the name from the ISP
> documentation in hr_name is good.
> 
> > +
> > +    def __init__(self, **kwargs):
> > +        super().__init__(**kwargs)
> > +
> > +    # We don't actually need anything from the config file
> 
> s/actually //
> s/$/./
> 
> > +    def validate_config(self, config: dict) -> bool:
> > +        return True
> > +
> > +    def process(self, config: dict, images: list, outputs: dict) -> dict:
> > +        output = {}
> > +
> > +        ctms = self.do_calibration(images)
> 
> Here, however, I would name the variable ccms.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +        output['ccms'] = ctms
> > +
> > +        return output
> > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py
> > index 2606e07a93f7..fae35783c656 100755
> > --- a/utils/tuning/rkisp1.py
> > +++ b/utils/tuning/rkisp1.py
> > @@ -14,6 +14,7 @@ from libtuning.parsers import YamlParser
> >  from libtuning.generators import YamlOutput
> >  from libtuning.modules.lsc import LSCRkISP1
> >  from libtuning.modules.agc import AGCRkISP1
> > +from libtuning.modules.ccm import CCMRkISP1
> >  
> >  
> >  coloredlogs.install(level=logging.INFO, fmt='%(name)s %(levelname)s %(message)s')
> > @@ -39,9 +40,10 @@ tuner.add(LSCRkISP1(
> >            smoothing_function=lt.smoothing.MedianBlur(3),
> >            ))
> >  tuner.add(AGCRkISP1(debug=[lt.Debug.Plot]))
> > +tuner.add(CCMRkISP1(debug=[lt.Debug.Plot]))
> >  tuner.set_input_parser(YamlParser())
> >  tuner.set_output_formatter(YamlOutput())
> > -tuner.set_output_order([AGCRkISP1, LSCRkISP1])
> > +tuner.set_output_order([AGCRkISP1, CCMRkISP1, LSCRkISP1])
> >  
> >  if __name__ == '__main__':
> >      sys.exit(tuner.run(sys.argv))
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/utils/tuning/libtuning/modules/ccm/__init__.py b/utils/tuning/libtuning/modules/ccm/__init__.py
new file mode 100644
index 000000000000..322602afe63b
--- /dev/null
+++ b/utils/tuning/libtuning/modules/ccm/__init__.py
@@ -0,0 +1,6 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
+
+from libtuning.modules.ccm.ccm import CCM
+from libtuning.modules.ccm.rkisp1 import CCMRkISP1
diff --git a/utils/tuning/libtuning/modules/ccm/ccm.py b/utils/tuning/libtuning/modules/ccm/ccm.py
new file mode 100644
index 000000000000..50d435ad84a3
--- /dev/null
+++ b/utils/tuning/libtuning/modules/ccm/ccm.py
@@ -0,0 +1,44 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
+# Copyright (C) 2024, Ideas on Board
+#
+# Base Ccm tuning module
+
+from ..module import Module
+
+from libtuning.ctt_ccm import ccm
+import logging
+import time
+
+import numpy as np
+
+logger = logging.getLogger(__name__)
+
+
+class CCM(Module):
+    type = 'ccm'
+    hr_name = 'CCM (Base)'
+    out_name = 'GenericCCM'
+
+    def __init__(self, debug: list):
+        super().__init__()
+
+        self.debug = debug
+
+    def do_calibration(self, images):
+        logger.info('Starting CCM calibration')
+
+        imgs = [img for img in images if img.macbeth is not None]
+
+        # todo: take alsc calibration results into account
+        cal_cr_list = None
+        cal_cb_list = None
+
+        try:
+            ccms = ccm(imgs, cal_cr_list, cal_cb_list)
+        except ArithmeticError:
+            logger.error('CCM calibration failed')
+            return 1
+
+        return ccms
diff --git a/utils/tuning/libtuning/modules/ccm/rkisp1.py b/utils/tuning/libtuning/modules/ccm/rkisp1.py
new file mode 100644
index 000000000000..a74d0d93c764
--- /dev/null
+++ b/utils/tuning/libtuning/modules/ccm/rkisp1.py
@@ -0,0 +1,34 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
+# Copyright (C) 2024, Ideas on Board
+#
+# Ccm module for tuning rkisp1
+
+from .ccm import CCM
+
+import libtuning as lt
+import libtuning.utils as utils
+
+from numbers import Number
+import numpy as np
+
+
+class CCMRkISP1(CCM):
+    hr_name = 'Crosstalk Correction (RkISP1)'
+    out_name = 'Ccm'
+
+    def __init__(self, **kwargs):
+        super().__init__(**kwargs)
+
+    # We don't actually need anything from the config file
+    def validate_config(self, config: dict) -> bool:
+        return True
+
+    def process(self, config: dict, images: list, outputs: dict) -> dict:
+        output = {}
+
+        ctms = self.do_calibration(images)
+        output['ccms'] = ctms
+
+        return output
diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py
index 2606e07a93f7..fae35783c656 100755
--- a/utils/tuning/rkisp1.py
+++ b/utils/tuning/rkisp1.py
@@ -14,6 +14,7 @@  from libtuning.parsers import YamlParser
 from libtuning.generators import YamlOutput
 from libtuning.modules.lsc import LSCRkISP1
 from libtuning.modules.agc import AGCRkISP1
+from libtuning.modules.ccm import CCMRkISP1
 
 
 coloredlogs.install(level=logging.INFO, fmt='%(name)s %(levelname)s %(message)s')
@@ -39,9 +40,10 @@  tuner.add(LSCRkISP1(
           smoothing_function=lt.smoothing.MedianBlur(3),
           ))
 tuner.add(AGCRkISP1(debug=[lt.Debug.Plot]))
+tuner.add(CCMRkISP1(debug=[lt.Debug.Plot]))
 tuner.set_input_parser(YamlParser())
 tuner.set_output_formatter(YamlOutput())
-tuner.set_output_order([AGCRkISP1, LSCRkISP1])
+tuner.set_output_order([AGCRkISP1, CCMRkISP1, LSCRkISP1])
 
 if __name__ == '__main__':
     sys.exit(tuner.run(sys.argv))