[{"id":30147,"web_url":"https://patchwork.libcamera.org/comment/30147/","msgid":"<20240629010635.GU30900@pendragon.ideasonboard.com>","date":"2024-06-29T01:06:35","subject":"Re: [PATCH v2 23/25] libtuning: Make blacklevel optional","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:16PM +0200, Stefan Klug wrote:\n> Not every sensor provides blacklevel information through the kernel\n> interface.\n\nActually, unless I'm mistaken, none do.\n\nRaspberry Pi is the only platform to report the black levels through the\nlibcamera API, and they come from the tuning file. There's a bit of a\ncircular dependency below :-)\n\nThe question is where black levels should come from. They are typically\nthe result of two phenomena:\n\n- Dark currents and other physical effects that add charge to pixels in\n the absence of light. Those can depend on the integration time and the\n sensor die temperature, and their contribution to pixel values depend\n on the sensor gains.\n\n- Pedestal added by the sensor to pixel values. They are typically\n fixed, sometimes programmable (in which case we could control them),\n and should be reported in datasheets (but documentation is not always\n available).\n\nOffsetting the dark currents is the most difficult part. This can be\ndone in multiple ways, including\n\n- Automatic BLC in the sensor based on dark lines. That's the easiest\n option, we only need to enable it and there's nothing else to do.\n\n- Calculating black levels in the sensor based on dark lines, reporting\n them (through embedded data or I2C polling) to the host, and\n subtracting them in the ISP. This is more work, but does not require\n any tuning, so it's still not too bad.\n\n- Reading the dark lines from the sensor, calculating the black levels\n in the ISP (or worst case in software) and subtracting them in the\n ISP. A bit more complicated than the previous option, and requires the\n ability to separate dark lines from visible image on the host side,\n which is not necessarily common.\n\n- Calculating black levels dynamically from tuning data, based on the\n integration time, and temperature and the gains. This requires\n measurements and is the only option involving tuning.\n\nThe data pedestal needs to be subtracted regardless of how we handle the\ndark currents, and is likely the biggest contribution to the overall\nblack level. If the value is fixed, it could be added to the sensor\ndatabase in libcamera. If it's programmable, then the driver should\nreport it (which would leave the question of what to set it to, but I\nthink that can be handled separately).\n\nI think we can handle the dark currents separately, and later. For the\npedestal, let's add it to the sensor database to start with.\n\n> We should therefore not require that information to be in the\n> raw images for the tuning process. It can be provided via the tuning config.\n\nI think that's not what it's for. The reason we need it for tuning is\nthat it should be subtracted from the raw image before we tune LSC\n(assuming the ISP subtract the black level before it performs LSC).\nProviding the value in the config file doesn't seem right.\n\n> Signed-off-by: Stefan Klug \n> ---\n> utils/tuning/libtuning/image.py | 10 ++++++++--\n> 1 file changed, 8 insertions(+), 2 deletions(-)\n> \n> diff --git a/utils/tuning/libtuning/image.py b/utils/tuning/libtuning/image.py\n> index c8911a0ff125..0b9c78fcd9c2 100644\n> --- a/utils/tuning/libtuning/image.py\n> +++ b/utils/tuning/libtuning/image.py\n> @@ -25,6 +25,8 @@ class Image:\n> self.color = -1\n> self.lux = -1\n> self.macbeth = None\n> + self.blacklevel = 0\n> + self.blacklevel_16 = 0\n> \n> try:\n> self._load_metadata_exif()\n> @@ -74,8 +76,12 @@ class Image:\n> self.againQ8 = metadata[f'Exif.{photo}.ISOSpeedRatings'].value * 256 / 100\n> self.againQ8_norm = self.againQ8 / 256\n> self.camName = metadata['Exif.Image.Model'].value\n> - self.blacklevel = int(metadata[f'Exif.{subimage}.BlackLevel'].value[0])\n> - self.blacklevel_16 = self.blacklevel << (16 - self.sigbits)\n> + if f'Exif.{subimage}.BlackLevel' in metadata:\n> + self.blacklevel = int(metadata[f'Exif.{subimage}.BlackLevel'].value[0])\n> + self.blacklevel_16 = self.blacklevel << (16 - self.sigbits)\n> + else:\n> + logger.warning(\"Image doesn't contain blacklevel info\")\n> +\n> \n> # Channel order depending on bayer pattern\n> # The key is the order given by exif, where 0 is R, 1 is G, and 2 is B","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 5A68FBDB1D\n\tfor ;\n\tSat, 29 Jun 2024 01:07:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30E8262C99;\n\tSat, 29 Jun 2024 03:07:02 +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 578DF619C7\n\tfor ;\n\tSat, 29 Jun 2024 03:06:57 +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 26617720;\n\tSat, 29 Jun 2024 03:06:32 +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=\"Bc8GGZ9F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719623192;\n\tbh=aL859ufV/Wwkoo4k3w7eJrAUexHaeP9I3BcRRyBNLnI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Bc8GGZ9FX63wzzy5Xdf9scmfzFrtaxC3k5lESxuoCyG6d+iDs/oE1XAc41zXVeOzw\n\t91p8gK2n/Yetrfr5CiWEQc3IRYEWp6jB9+Ts1oBNtyg2bU0L6K2rGSjcdCKCxbHvOx\n\tSI9jOP9INNuDCV27ZNKaLzIfqfQOjhYtRScD5u4o=","Date":"Sat, 29 Jun 2024 04:06:35 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 23/25] libtuning: Make blacklevel optional","Message-ID":"<20240629010635.GU30900@pendragon.ideasonboard.com>","References":"<20240628104828.2928109-1-stefan.klug@ideasonboard.com>\n\t<20240628104828.2928109-24-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240628104828.2928109-24-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\" "}}]