[PATCH v2 09/25] libtuning: Improve filename parsing

[Laurent Pinchart]

On Mon, Jul 01, 2024 at 05:31:33PM +0200, Stefan Klug wrote:
> Hi Laurent,
> 
> On Sat, Jun 29, 2024 at 02:05:54AM +0300, Laurent Pinchart wrote:
> > Hi Stefan,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Jun 28, 2024 at 12:47:02PM +0200, Stefan Klug wrote:
> > > In the tuning datasets, the files had names like
> > > 'imx335_1600l_3000k_1.dng'. That failed on the old filename parsing
> > > function. As there is no need to dictate the order of the tags, split
> > > the big regex into chunks and parse them one by one. This also makes
> > > the code easier to digest.
> > 
> > Is there a risk we'll later add tags we can't differentiate without
> > considering their position ? I suppose we can decide how to format them,
> > so it should be fine ?
> 
> Yes, I guess the risk is limited. The only thing I see right now is the
> addition of a blc tag. But that works nicely with the scheme.
> 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > >  utils/tuning/libtuning/utils.py | 30 ++++++++++++++++++++++--------
> > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/utils/tuning/libtuning/utils.py b/utils/tuning/libtuning/utils.py
> > > index 178c6957c581..00cf5a57512f 100644
> > > --- a/utils/tuning/libtuning/utils.py
> > > +++ b/utils/tuning/libtuning/utils.py
> > > @@ -43,14 +43,28 @@ def _list_image_files(directory):
> > >  
> > >  
> > >  def _parse_image_filename(fn: Path):
> > > -    result = re.search(r'^(alsc_)?(\d+)[kK]_(\d+)?[lLuU]?.\w{3,4}$', fn.name)
> > > -    if result is None:
> > > -        logger.error(f'The file name of {fn.name} is incorrectly formatted')
> > > -        return None, None, None
> > > -
> > > -    color = int(result.group(2))
> > > -    lsc_only = result.group(1) is not None
> > > -    lux = None if lsc_only else int(result.group(3))
> > > +    lsc_only = False
> > > +    color = None
> > 
> > color_temperature ?
> 
> Yes, I tried to modify as little as possible. Maybe too little :-)
> 
> > 
> > > +    lux = None
> > > +
> > > +    parts = fn.stem.split('_')
> > > +    for part in parts:
> > > +        if part == 'alsc':
> > 
> > Should we rename this to 'lsc' at some point ? The *adaptive* LSC is
> > specific to Raspberry Pi.
> > 
> > > +            lsc_only = True
> > > +            continue
> > 
> > Maybe a blank line here and after the next continue to let the code
> > breathe a bit more.
> > 
> > > +        r = re.match(r'(\d+)[kK]', part)
> > 
> > Would
> > 
> >         r = re.match(r'([0-9]+)[kK]', part)
> > 
> > be more readable ?
> 
> I guess that is a personal preference. I prefer \d because it's less
> characters.
> 
> > Do we need to be case-insensitive here, or could we standardize on one
> > option and stick to it ?
> 
> At the point of writing that (and I would say still now) I didn't have a
> full overview on the tuning datasets that are floating out there and
> that are expected to work. So I didn't question that and just kept it
> compatible. I don't have a problem with case-insensitive here (maybe we
> should just do a lower() on the whole string before parsing). If we want
> to decide for a casing I (as a programmer) would prefer the lower k, but
> the official SI unit is K...grrr
> 
> > > +        if r:
> > > +            color = int(r.group(1))
> > > +            continue
> > > +        r = re.match(r'(\d+)[lLuU]', part)
> > 
> > Same here, and additionally, do we need to support both L and U ? Is
> > there a difference ?
> 
> Good question. As above I wanted to stay compatible, because there was
> no other defined standard (Or I wasn't aware of it). If we go for SI
> units it would be lx...
> 
> Writing all that I think I'd lean for case-insensistive and SI-units (So
> lx instead of l). The u still tbd.

SI unit sound good.

The reason I like case-sensitive is mostly consistency, as it avoids
proliferation of files with different cases, which can mess up sorting.
It's a minor issue in the end, so up to you.

> > > +        if r:
> > > +            lux = int(r.group(1))
> > > +
> > > +    if color is None:
> > > +        logger.error(f'The file name of {fn.name} does not contain a color temperature')
> > 
> >         logger.error(f'The file name "{fn.name}" does not contain a color temperature')
> > 
> > Same below.
> 
> This and the color_temperature above are fixed locally.
> 
> > > +
> > > +    if lux is None and lsc_only is False:
> > > +        logger.error(f'The file name of {fn.name} must either contain alsc or a lux level')
> > >  
> > >      return color, lux, lsc_only
> > >  

-- 
Regards,

Laurent Pinchart