[PATCH v4 1/2] utils: libtuning: modules: Add skeletal AGC module

Laurent Pinchart laurent.pinchart at ideasonboard.com
Sat Jun 15 19:45:50 CEST 2024


Hi Paul,

I know this has been merged, but I have a question.

On Fri, Jun 14, 2024 at 11:19:36PM +0900, Paul Elder wrote:
> On Fri, Jun 14, 2024 at 02:32:56PM +0100, Kieran Bingham wrote:
> > Quoting Paul Elder (2024-06-14 12:40:15)
> > > Add a skeletal AGC module just so that we can have some AGC tuning
> > > values that we can use to test during development of AGC in the IPAs. As
> > > rkisp1 is the main target, we only add support for rkisp1 for now.
> > > 
> > > The parameters are mostly copied from the hardcoded values in ctt,
> > > except for the metering modes.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > 
> > > ---
> > > Changes in v4:
> > > - remove support for both v10 and v12, and only support v10
> > > - remove piecewise linear function luminance targets
> > > 
> > > Changes in v3:
> > > - remove unused compatible string
> > > - make gain start from 2 in the exposure modes
> > > 
> > > Changes in v2:
> > > - remove unneccessary imports
> > > - add support for both v10 and v12
> > > ---
> > >  .../tuning/libtuning/modules/agc/__init__.py  |  6 ++
> > >  utils/tuning/libtuning/modules/agc/agc.py     | 21 +++++
> > >  utils/tuning/libtuning/modules/agc/rkisp1.py  | 79 +++++++++++++++++++
> > >  3 files changed, 106 insertions(+)
> > >  create mode 100644 utils/tuning/libtuning/modules/agc/__init__.py
> > >  create mode 100644 utils/tuning/libtuning/modules/agc/agc.py
> > >  create mode 100644 utils/tuning/libtuning/modules/agc/rkisp1.py
> > > 
> > > diff --git a/utils/tuning/libtuning/modules/agc/__init__.py b/utils/tuning/libtuning/modules/agc/__init__.py
> > > new file mode 100644
> > > index 000000000000..4db9ca371c68
> > > --- /dev/null
> > > +++ b/utils/tuning/libtuning/modules/agc/__init__.py
> > > @@ -0,0 +1,6 @@
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +#
> > > +# Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> > > +
> > > +from libtuning.modules.agc.agc import AGC
> > > +from libtuning.modules.agc.rkisp1 import AGCRkISP1
> > > diff --git a/utils/tuning/libtuning/modules/agc/agc.py b/utils/tuning/libtuning/modules/agc/agc.py
> > > new file mode 100644
> > > index 000000000000..9c8899badc79
> > > --- /dev/null
> > > +++ b/utils/tuning/libtuning/modules/agc/agc.py
> > > @@ -0,0 +1,21 @@
> > > +# SPDX-License-Identifier: BSD-2-Clause
> > > +#
> > > +# Copyright (C) 2019, Raspberry Pi Ltd
> > > +# Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> > > +
> > > +from ..module import Module
> > > +
> > > +import libtuning as lt
> > > +
> > > +
> > > +class AGC(Module):
> > > +    type = 'agc'
> > > +    hr_name = 'AGC (Base)'
> > 
> > What's an hr_name? (hr?)
> 
> human-readable (which ironically itself is not very human-readable but)
> 
> > > +    out_name = 'GenericAGC'
> > > +
> > > +    # \todo Add sector shapes and stuff just like lsc
> > > +    def __init__(self, *,
> > > +                 debug: list):
> > > +        super().__init__()
> > > +
> > > +        self.debug = debug
> > > diff --git a/utils/tuning/libtuning/modules/agc/rkisp1.py b/utils/tuning/libtuning/modules/agc/rkisp1.py
> > > new file mode 100644
> > > index 000000000000..babb441a8300
> > > --- /dev/null
> > > +++ b/utils/tuning/libtuning/modules/agc/rkisp1.py
> > > @@ -0,0 +1,79 @@
> > > +# SPDX-License-Identifier: BSD-2-Clause
> > > +#
> > > +# Copyright (C) 2019, Raspberry Pi Ltd
> > > +# Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> > > +#
> > > +# rkisp1.py - AGC module for tuning rkisp1
> > > +
> > > +from .agc import AGC
> > > +
> > > +import libtuning as lt
> > > +
> > > +
> > > +class AGCRkISP1(AGC):
> > > +    hr_name = 'AGC (RkISP1)'
> > > +    out_name = 'Agc'
> > > +
> > > +    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
> > 
> > Would you ever anticipate putting these hardcoded values into a separate
> > config file?
> 
> Oh maybe.
> 
> > 
> > Anyway ... I think this gets us moving forwards so:
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> 
> Thanks :)
> 
> 
> Paul
> 
> > > +
> > > +    def _generate_metering_modes(self) -> dict:
> > > +        centre_weighted = [
> > > +            0, 0,  0, 0, 0,
> > > +            0, 6,  8, 6, 0,
> > > +            0, 8, 16, 8, 0,
> > > +            0, 6,  8, 6, 0,
> > > +            0, 0,  0, 0, 0
> > > +        ]
> > > +
> > > +        spot = [
> > > +            0, 0,  0, 0, 0,
> > > +            0, 2,  4, 2, 0,
> > > +            0, 4, 16, 4, 0,
> > > +            0, 2,  4, 2, 0,
> > > +            0, 0,  0, 0, 0
> > > +        ]

How did you come up with those values ? The centre weighted matrix, in
particular, seems quite, well, centered :-) Downsampling the rpi4 matrix
from ctt I would have expected

[[ 0  0  5  0  0]
 [ 0 10 10 10  0]
 [ 5 10 16 10  5]
 [ 0 10 10 10  0]
 [ 0  0  5  0  0]]

Similarly, for spot metering, the downsampled rpi4 matrix is closer to

[[ 0  0  0  0  0]
 [ 0  0  1  0  0]
 [ 0  1 16  1  0]
 [ 0  0  1  0  0]
 [ 0  0  0  0  0]]

We can use other values, but I wonder where they come from.

> > > +
> > > +        matrix = [1 for i in range(0, 25)]

Should this be 16, not 1, to fill up the histogram bins ? The IPA module
assumes weights of 16 when calculating the predivider.

OK, that was two questions :-)

> > > +
> > > +        return {
> > > +            'MeteringCentreWeighted': centre_weighted,
> > > +            'MeteringSpot': spot,
> > > +            'MeteringMatrix': matrix
> > > +        }
> > > +
> > > +    def _generate_exposure_modes(self) -> dict:
> > > +        normal = { 'shutter': [100, 10000, 30000, 60000, 120000],
> > > +                   'gain': [2.0, 4.0, 6.0, 6.0, 6.0]}
> > > +        short = { 'shutter': [100, 5000, 10000, 20000, 120000],
> > > +                  'gain': [2.0, 4.0, 6.0, 6.0, 6.0]}
> > > +
> > > +        return { 'ExposureNormal': normal, 'ExposureShort': short }
> > > +
> > > +    def _generate_constraint_modes(self) -> dict:
> > > +        normal = { 'lower': { 'qLo': 0.98, 'qHi': 1.0, 'yTarget': 0.5 } }
> > > +        highlight = {
> > > +            'lower': { 'qLo': 0.98, 'qHi': 1.0, 'yTarget': 0.5 },
> > > +            'upper': { 'qLo': 0.98, 'qHi': 1.0, 'yTarget': 0.8 }
> > > +        }
> > > +
> > > +        return { 'ConstraintNormal': normal, 'ConstraintHighlight': highlight }
> > > +
> > > +    def _generate_y_target(self) -> list:
> > > +        return 0.16
> > > +
> > > +    def process(self, config: dict, images: list, outputs: dict) -> dict:
> > > +        output = {}
> > > +
> > > +        output['AeMeteringMode'] = self._generate_metering_modes()
> > > +        output['AeExposureMode'] = self._generate_exposure_modes()
> > > +        output['AeConstraintMode'] = self._generate_constraint_modes()
> > > +        output['relativeLuminanceTarget'] = self._generate_y_target()
> > > +
> > > +        # \todo Debug functionality
> > > +
> > > +        return output

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list