[{"id":30133,"web_url":"https://patchwork.libcamera.org/comment/30133/","msgid":"<20240628230554.GG30900@pendragon.ideasonboard.com>","date":"2024-06-28T23:05:54","subject":"Re: [PATCH v2 09/25] libtuning: Improve filename parsing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Fri, Jun 28, 2024 at 12:47:02PM +0200, Stefan Klug wrote:\n> In the tuning datasets, the files had names like\n> 'imx335_1600l_3000k_1.dng'. That failed on the old filename parsing\n> function. As there is no need to dictate the order of the tags, split\n> the big regex into chunks and parse them one by one. This also makes\n> the code easier to digest.\n\nIs there a risk we'll later add tags we can't differentiate without\nconsidering their position ? I suppose we can decide how to format them,\nso it should be fine ?\n\n> Signed-off-by: Stefan Klug \n> Reviewed-by: Paul Elder \n> ---\n> utils/tuning/libtuning/utils.py | 30 ++++++++++++++++++++++--------\n> 1 file changed, 22 insertions(+), 8 deletions(-)\n> \n> diff --git a/utils/tuning/libtuning/utils.py b/utils/tuning/libtuning/utils.py\n> index 178c6957c581..00cf5a57512f 100644\n> --- a/utils/tuning/libtuning/utils.py\n> +++ b/utils/tuning/libtuning/utils.py\n> @@ -43,14 +43,28 @@ def _list_image_files(directory):\n> \n> \n> def _parse_image_filename(fn: Path):\n> - result = re.search(r'^(alsc_)?(\\d+)[kK]_(\\d+)?[lLuU]?.\\w{3,4}$', fn.name)\n> - if result is None:\n> - logger.error(f'The file name of {fn.name} is incorrectly formatted')\n> - return None, None, None\n> -\n> - color = int(result.group(2))\n> - lsc_only = result.group(1) is not None\n> - lux = None if lsc_only else int(result.group(3))\n> + lsc_only = False\n> + color = None\n\ncolor_temperature ?\n\n> + lux = None\n> +\n> + parts = fn.stem.split('_')\n> + for part in parts:\n> + if part == 'alsc':\n\nShould we rename this to 'lsc' at some point ? The *adaptive* LSC is\nspecific to Raspberry Pi.\n\n> + lsc_only = True\n> + continue\n\nMaybe a blank line here and after the next continue to let the code\nbreathe a bit more.\n\n> + r = re.match(r'(\\d+)[kK]', part)\n\nWould\n\n r = re.match(r'([0-9]+)[kK]', part)\n\nbe more readable ?\n\nDo we need to be case-insensitive here, or could we standardize on one\noption and stick to it ?\n\n> + if r:\n> + color = int(r.group(1))\n> + continue\n> + r = re.match(r'(\\d+)[lLuU]', part)\n\nSame here, and additionally, do we need to support both L and U ? Is\nthere a difference ?\n\n> + if r:\n> + lux = int(r.group(1))\n> +\n> + if color is None:\n> + logger.error(f'The file name of {fn.name} does not contain a color temperature')\n\n logger.error(f'The file name \"{fn.name}\" does not contain a color temperature')\n\nSame below.\n\n> +\n> + if lux is None and lsc_only is False:\n> + logger.error(f'The file name of {fn.name} must either contain alsc or a lux level')\n> \n> return color, lux, lsc_only\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9F14FBD87C\n\tfor ;\n\tFri, 28 Jun 2024 23:06:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0B3962C99;\n\tSat, 29 Jun 2024 01:06:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D63A619E8\n\tfor ;\n\tSat, 29 Jun 2024 01:06:16 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0C0E4471;\n\tSat, 29 Jun 2024 01:05:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SflSWk0w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719615951;\n\tbh=lc7iCdx+Y8RLQooG6dlzZS11sV1WjtUCJcKNpwiXA0M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SflSWk0wZ4y4YJG4725ul4Up7qYfgToXUlQpEQtn0UgmxTwKyZIINYfpDxikHrwVU\n\tgElgbatR9bglaKJPOEy3SueXOazIFCQdf3lZBlcdHnCoJVpa48wdyhwE28PaVJcFmf\n\t764KZI5qleEMqnGQt7EKurZMvVbHa9luOhtanryA=","Date":"Sat, 29 Jun 2024 02:05:54 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v2 09/25] libtuning: Improve filename parsing","Message-ID":"<20240628230554.GG30900@pendragon.ideasonboard.com>","References":"<20240628104828.2928109-1-stefan.klug@ideasonboard.com>\n\t<20240628104828.2928109-10-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240628104828.2928109-10-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30190,"web_url":"https://patchwork.libcamera.org/comment/30190/","msgid":"","date":"2024-07-01T15:31:33","subject":"Re: [PATCH v2 09/25] libtuning: Improve filename parsing","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Sat, Jun 29, 2024 at 02:05:54AM +0300, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Fri, Jun 28, 2024 at 12:47:02PM +0200, Stefan Klug wrote:\n> > In the tuning datasets, the files had names like\n> > 'imx335_1600l_3000k_1.dng'. That failed on the old filename parsing\n> > function. As there is no need to dictate the order of the tags, split\n> > the big regex into chunks and parse them one by one. This also makes\n> > the code easier to digest.\n> \n> Is there a risk we'll later add tags we can't differentiate without\n> considering their position ? I suppose we can decide how to format them,\n> so it should be fine ?\n\nYes, I guess the risk is limited. The only thing I see right now is the\naddition of a blc tag. But that works nicely with the scheme.\n\n> \n> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Paul Elder \n> > ---\n> > utils/tuning/libtuning/utils.py | 30 ++++++++++++++++++++++--------\n> > 1 file changed, 22 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/utils/tuning/libtuning/utils.py b/utils/tuning/libtuning/utils.py\n> > index 178c6957c581..00cf5a57512f 100644\n> > --- a/utils/tuning/libtuning/utils.py\n> > +++ b/utils/tuning/libtuning/utils.py\n> > @@ -43,14 +43,28 @@ def _list_image_files(directory):\n> > \n> > \n> > def _parse_image_filename(fn: Path):\n> > - result = re.search(r'^(alsc_)?(\\d+)[kK]_(\\d+)?[lLuU]?.\\w{3,4}$', fn.name)\n> > - if result is None:\n> > - logger.error(f'The file name of {fn.name} is incorrectly formatted')\n> > - return None, None, None\n> > -\n> > - color = int(result.group(2))\n> > - lsc_only = result.group(1) is not None\n> > - lux = None if lsc_only else int(result.group(3))\n> > + lsc_only = False\n> > + color = None\n> \n> color_temperature ?\n\nYes, I tried to modify as little as possible. Maybe too little :-)\n\n> \n> > + lux = None\n> > +\n> > + parts = fn.stem.split('_')\n> > + for part in parts:\n> > + if part == 'alsc':\n> \n> Should we rename this to 'lsc' at some point ? The *adaptive* LSC is\n> specific to Raspberry Pi.\n> \n> > + lsc_only = True\n> > + continue\n> \n> Maybe a blank line here and after the next continue to let the code\n> breathe a bit more.\n> \n> > + r = re.match(r'(\\d+)[kK]', part)\n> \n> Would\n> \n> r = re.match(r'([0-9]+)[kK]', part)\n> \n> be more readable ?\n\nI guess that is a personal preference. I prefer \\d because it's less\ncharacters.\n\n> \n> Do we need to be case-insensitive here, or could we standardize on one\n> option and stick to it ?\n\nAt the point of writing that (and I would say still now) I didn't have a\nfull overview on the tuning datasets that are floating out there and\nthat are expected to work. So I didn't question that and just kept it\ncompatible. I don't have a problem with case-insensitive here (maybe we\nshould just do a lower() on the whole string before parsing). If we want\nto decide for a casing I (as a programmer) would prefer the lower k, but\nthe official SI unit is K...grrr\n\n> \n> > + if r:\n> > + color = int(r.group(1))\n> > + continue\n> > + r = re.match(r'(\\d+)[lLuU]', part)\n> \n> Same here, and additionally, do we need to support both L and U ? Is\n> there a difference ?\n\nGood question. As above I wanted to stay compatible, because there was\nno other defined standard (Or I wasn't aware of it). If we go for SI\nunits it would be lx...\n\nWriting all that I think I'd lean for case-insensistive and SI-units (So\nlx instead of l). The u still tbd.\n\n> \n> > + if r:\n> > + lux = int(r.group(1))\n> > +\n> > + if color is None:\n> > + logger.error(f'The file name of {fn.name} does not contain a color temperature')\n> \n> logger.error(f'The file name \"{fn.name}\" does not contain a color temperature')\n> \n> Same below.\n\nThis and the color_temperature above are fixed locally.\n\nRegards,\nStefan\n\n> \n> > +\n> > + if lux is None and lsc_only is False:\n> > + logger.error(f'The file name of {fn.name} must either contain alsc or a lux level')\n> > \n> > return color, lux, lsc_only\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A1A13BEFBE\n\tfor ;\n\tMon, 1 Jul 2024 15:31:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A5E862E22;\n\tMon, 1 Jul 2024 17:31:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C3BE604C1\n\tfor ;\n\tMon, 1 Jul 2024 17:31:37 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:89b2:f6c7:b29b:4e5c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5C7AD63D;\n\tMon, 1 Jul 2024 17:31:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dmIpZIvX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719847870;\n\tbh=t5OT14Bvh9thEtGcp3tF70DymNjvqVrrjNjqt7MsU4o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dmIpZIvXZplyeY12kd0x3Igc0WFv1VMWvWKLqwaPy/nHy423DyWRker7vlKYirOwM\n\tXfqcEuQFLctUnodBWN+uz8e0VjfKsbFG8HMO6s90jllKRb425sfP6+JmvNH9tCRroI\n\tqJf92GO50Bu+cf2q+o0M6truEAnzxT9j8J+QkAto=","Date":"Mon, 1 Jul 2024 17:31:33 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH v2 09/25] libtuning: Improve filename parsing","Message-ID":"","References":"<20240628104828.2928109-1-stefan.klug@ideasonboard.com>\n\t<20240628104828.2928109-10-stefan.klug@ideasonboard.com>\n\t<20240628230554.GG30900@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240628230554.GG30900@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30203,"web_url":"https://patchwork.libcamera.org/comment/30203/","msgid":"<20240702100924.GB26760@pendragon.ideasonboard.com>","date":"2024-07-02T10:09:24","subject":"Re: [PATCH v2 09/25] libtuning: Improve filename parsing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jul 01, 2024 at 05:31:33PM +0200, Stefan Klug wrote:\n> Hi Laurent,\n> \n> On Sat, Jun 29, 2024 at 02:05:54AM +0300, Laurent Pinchart wrote:\n> > Hi Stefan,\n> > \n> > Thank you for the patch.\n> > \n> > On Fri, Jun 28, 2024 at 12:47:02PM +0200, Stefan Klug wrote:\n> > > In the tuning datasets, the files had names like\n> > > 'imx335_1600l_3000k_1.dng'. That failed on the old filename parsing\n> > > function. As there is no need to dictate the order of the tags, split\n> > > the big regex into chunks and parse them one by one. This also makes\n> > > the code easier to digest.\n> > \n> > Is there a risk we'll later add tags we can't differentiate without\n> > considering their position ? I suppose we can decide how to format them,\n> > so it should be fine ?\n> \n> Yes, I guess the risk is limited. The only thing I see right now is the\n> addition of a blc tag. But that works nicely with the scheme.\n> \n> > > Signed-off-by: Stefan Klug \n> > > Reviewed-by: Paul Elder \n> > > ---\n> > > utils/tuning/libtuning/utils.py | 30 ++++++++++++++++++++++--------\n> > > 1 file changed, 22 insertions(+), 8 deletions(-)\n> > > \n> > > diff --git a/utils/tuning/libtuning/utils.py b/utils/tuning/libtuning/utils.py\n> > > index 178c6957c581..00cf5a57512f 100644\n> > > --- a/utils/tuning/libtuning/utils.py\n> > > +++ b/utils/tuning/libtuning/utils.py\n> > > @@ -43,14 +43,28 @@ def _list_image_files(directory):\n> > > \n> > > \n> > > def _parse_image_filename(fn: Path):\n> > > - result = re.search(r'^(alsc_)?(\\d+)[kK]_(\\d+)?[lLuU]?.\\w{3,4}$', fn.name)\n> > > - if result is None:\n> > > - logger.error(f'The file name of {fn.name} is incorrectly formatted')\n> > > - return None, None, None\n> > > -\n> > > - color = int(result.group(2))\n> > > - lsc_only = result.group(1) is not None\n> > > - lux = None if lsc_only else int(result.group(3))\n> > > + lsc_only = False\n> > > + color = None\n> > \n> > color_temperature ?\n> \n> Yes, I tried to modify as little as possible. Maybe too little :-)\n> \n> > \n> > > + lux = None\n> > > +\n> > > + parts = fn.stem.split('_')\n> > > + for part in parts:\n> > > + if part == 'alsc':\n> > \n> > Should we rename this to 'lsc' at some point ? The *adaptive* LSC is\n> > specific to Raspberry Pi.\n> > \n> > > + lsc_only = True\n> > > + continue\n> > \n> > Maybe a blank line here and after the next continue to let the code\n> > breathe a bit more.\n> > \n> > > + r = re.match(r'(\\d+)[kK]', part)\n> > \n> > Would\n> > \n> > r = re.match(r'([0-9]+)[kK]', part)\n> > \n> > be more readable ?\n> \n> I guess that is a personal preference. I prefer \\d because it's less\n> characters.\n> \n> > Do we need to be case-insensitive here, or could we standardize on one\n> > option and stick to it ?\n> \n> At the point of writing that (and I would say still now) I didn't have a\n> full overview on the tuning datasets that are floating out there and\n> that are expected to work. So I didn't question that and just kept it\n> compatible. I don't have a problem with case-insensitive here (maybe we\n> should just do a lower() on the whole string before parsing). If we want\n> to decide for a casing I (as a programmer) would prefer the lower k, but\n> the official SI unit is K...grrr\n> \n> > > + if r:\n> > > + color = int(r.group(1))\n> > > + continue\n> > > + r = re.match(r'(\\d+)[lLuU]', part)\n> > \n> > Same here, and additionally, do we need to support both L and U ? Is\n> > there a difference ?\n> \n> Good question. As above I wanted to stay compatible, because there was\n> no other defined standard (Or I wasn't aware of it). If we go for SI\n> units it would be lx...\n> \n> Writing all that I think I'd lean for case-insensistive and SI-units (So\n> lx instead of l). The u still tbd.\n\nSI unit sound good.\n\nThe reason I like case-sensitive is mostly consistency, as it avoids\nproliferation of files with different cases, which can mess up sorting.\nIt's a minor issue in the end, so up to you.\n\n> > > + if r:\n> > > + lux = int(r.group(1))\n> > > +\n> > > + if color is None:\n> > > + logger.error(f'The file name of {fn.name} does not contain a color temperature')\n> > \n> > logger.error(f'The file name \"{fn.name}\" does not contain a color temperature')\n> > \n> > Same below.\n> \n> This and the color_temperature above are fixed locally.\n> \n> > > +\n> > > + if lux is None and lsc_only is False:\n> > > + logger.error(f'The file name of {fn.name} must either contain alsc or a lux level')\n> > > \n> > > return color, lux, lsc_only\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 37F96BEFBE\n\tfor ;\n\tTue, 2 Jul 2024 10:09:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D06E062E22;\n\tTue, 2 Jul 2024 12:09:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 12629619CC\n\tfor ;\n\tTue, 2 Jul 2024 12:09:46 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B78BB5A4;\n\tTue, 2 Jul 2024 12:09:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lEQHnKyk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719914958;\n\tbh=ufFKEuVcApRBIvXEckDm13xr7WhwBcx6+YqJi+4mYVw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lEQHnKyk41bBsz3gaiUksv4Cyzb+riDIbyAJd0CbfnmcbhfHjz6odqmPbpfRjuHRE\n\toiFGQwDykwQ9goAh143FLFacymwFNfjp0lmomZfnO0blfK+Y/kCghWslVYXrgqGMNp\n\tMMCbUi+iUWgvYd0AKq2WMCY0MduXnTRubrEQ91CM=","Date":"Tue, 2 Jul 2024 13:09:24 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v2 09/25] libtuning: Improve filename parsing","Message-ID":"<20240702100924.GB26760@pendragon.ideasonboard.com>","References":"<20240628104828.2928109-1-stefan.klug@ideasonboard.com>\n\t<20240628104828.2928109-10-stefan.klug@ideasonboard.com>\n\t<20240628230554.GG30900@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]