[{"id":28933,"web_url":"https://patchwork.libcamera.org/comment/28933/","msgid":"<874jdb4l3l.fsf@redhat.com>","date":"2024-03-12T13:48:30","subject":"Re: [PATCH v5 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hans de Goede writes:\n\n> From: Milan Zamazal \n>\n> Black may not be represented as 0 pixel value for given hardware, it may be\n> higher. If this is not compensated then various problems may occur such as low\n> contrast or suboptimal exposure.\n>\n> The black pixel value can be either retrieved from a tuning file for the given\n> hardware, or automatically on fly. The former is the right and correct method,\n> while the latter can be used when a tuning file is not available for the given\n> hardware. Since there is currently no support for tuning files in software ISP,\n> the automatic, hardware independent way, is always used. Support for tuning\n> files should be added in future but it will require more work than this patch.\n>\n> The patch looks at the image histogram and assumes that black starts when pixel\n> values start occurring on the left. A certain amount of the darkest pixels is\n> ignored; it doesn't matter whether they represent various kinds of noise or are\n> real, they are better to omit in any case to make the image looking better. It\n> also doesn't matter whether the darkest pixels occur around the supposed black\n> level or are spread between 0 and the black level, the difference is not\n> important.\n>\n> An arbitrary threshold of 2% darkest pixels is applied; there is no magic about\n> that value.\n>\n> The patch assumes that the black values for different colors are the same and\n> doesn't attempt any other non-primitive enhancements. It cannot completely\n> replace tuning files and simplicity, while providing visible benefit, is its\n> goal. Anything more sophisticated is left for future patches.\n>\n> A possible cheap enhancement, if needed, could be setting exposure + gain to\n> minimum values temporarily, before setting the black level. In theory, the\n> black level should be fixed but it may not be reached in all images. For this\n> reason, the patch updates black level only if the observed value is lower than\n> the current one; it should be never increased.\n>\n> The purpose of the patch is to compensate for hardware properties. General\n> image contrast enhancements are out of scope of this patch.\n>\n> Stats are still gathered as an uncorrected histogram, to avoid any confusion and\n> to represent the raw image data. Exposure must be determined after the black\n> level correction -- it has no influence on the sub-black area and must be\n> correct after applying the black level correction. The granularity of the\n> histogram is increased from 16 to 64 to provide a better precision (there is no\n> theory behind either of those numbers).\n>\n> Reviewed-by: Hans de Goede \n> Signed-off-by: Milan Zamazal \n> Signed-off-by: Hans de Goede \n> ---\n\n[...]\n\n> +void BlackLevel::update(SwIspStats::histogram &yHistogram)\n> +{\n> +\t// The constant is selected to be \"good enough\", not overly conservative or\n> +\t// aggressive. There is no magic about the given value.\n> +\tconstexpr float ignoredPercentage_ = 0.02;\n> +\tconst unsigned int total =\n> +\t\tstd::accumulate(begin(yHistogram), end(yHistogram), 0);\n> +\tconst unsigned int pixelThreshold = ignoredPercentage_ * total;\n> +\tconst unsigned int currentBlackIdx =\n> +\t\tblackLevel_ / (256 / SwIspStats::kYHistogramSize);\n\nShould the newly introduced kRGBLookupSize constant be used instead of 256 here\nand in other places in this patch?\n\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 CBF0BBD160\n\tfor ;\n\tTue, 12 Mar 2024 13:48:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01EED62C80;\n\tTue, 12 Mar 2024 14:48:37 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6DE116286C\n\tfor ;\n\tTue, 12 Mar 2024 14:48:35 +0100 (CET)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-594-5CUF4WpwMTiwmn6JFeSJGg-1; Tue, 12 Mar 2024 09:48:32 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-412e992444eso23500405e9.3\n\tfor ;\n\tTue, 12 Mar 2024 06:48:32 -0700 (PDT)","from nuthatch (nat-pool-brq-t.redhat.com. [213.175.37.10])\n\tby smtp.gmail.com with ESMTPSA id\n\to17-20020a05600c4fd100b00413320f795fsm2804600wmq.35.2024.03.12.06.48.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 12 Mar 2024 06:48:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"HYf9XOvy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1710251314;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=nEh07K9fc6yS4UNkHKz77jvIGcAVh/hs15qgFC6VsuA=;\n\tb=HYf9XOvyxj9Ao8aOBMaWz6VFOImCsd0LaBUi1IRDWKp+l5xZsesb/+4ofPGjs3I78zcqMd\n\tf5EhUq41C+H4RHLkLEhNJSGIUZqQ3wBrr/91XU6WYjwl5xKdxSnMhAt7Ocmgi8rJzvlrZL\n\tCClAiOUuLyg1NmRIWQ/ee9uHBEYu/74=","X-MC-Unique":"5CUF4WpwMTiwmn6JFeSJGg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1710251311; x=1710856111;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=nEh07K9fc6yS4UNkHKz77jvIGcAVh/hs15qgFC6VsuA=;\n\tb=bdVDzp1oTdXdyV/H82e+CyMsoEZWAHooUfI9oSxgtGarPcE5DsbLf8eSNyqw52KZJY\n\tojO3t1aQ92je8f5Yc6CB/LO0FQrdUo74riHT6PweWQjDZFbym0HXMtWY92A7nW75oLvj\n\tGbxh9k3fEFQtAF9/JE7QTds/a4BJFplrOkuVzHH0muFi9GioXJjc4cKDq8mZyInohVkn\n\tiWt4r2AeC/NawtlRrf/nEafyPq2pRAHRiODAX9jWuy+27GQyymHOyO9q3APjOhrmtoXg\n\tUytaUn00zJOXeKMtmHQWckMc0+yEXzKaqQprzeBO+2LeSP0EaPh3n2Ydh8VzEl2xtXKb\n\t3pXg==","X-Gm-Message-State":"AOJu0YwtU69LLYfqPc/aeHDb1uYyUGuEOCV6sssdQ65tCkCDVMX+bp7Q\n\tQKVViGSMP3mN6+Jti46jfQJSwwPV17Ty9w48CCjqP9eFR8uZ6QcU+BY8tDezXvPW80U3ZS/Uolr\n\ttpuUTLRqQxpPljN30XkP9u/vmu8tbKtlRVhX4cLye/YT8IdSlBdvyZXwhU0HyG7uJEZ/XnRs=","X-Received":["by 2002:a05:600c:3492:b0:413:388f:a19d with SMTP id\n\ta18-20020a05600c349200b00413388fa19dmr1470027wmq.34.1710251311481; \n\tTue, 12 Mar 2024 06:48:31 -0700 (PDT)","by 2002:a05:600c:3492:b0:413:388f:a19d with SMTP id\n\ta18-20020a05600c349200b00413388fa19dmr1470012wmq.34.1710251311120; \n\tTue, 12 Mar 2024 06:48:31 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGfhnvgIqIz7SCF04x6uC0G9KU1kGcA+RW4xBTN45uSQYVa4DOMIM7tW1ShmDwkaPoUM/FXlA==","From":"Milan Zamazal ","To":"Hans de Goede ","Subject":"Re: [PATCH v5 17/18] libcamera: software_isp: Apply black level\n\tcompensation","In-Reply-To":"<20240311141524.27192-18-hdegoede@redhat.com> (Hans de Goede's\n\tmessage of \"Mon, 11 Mar 2024 15:15:21 +0100\")","References":"<20240311141524.27192-1-hdegoede@redhat.com>\n\t<20240311141524.27192-18-hdegoede@redhat.com>","Date":"Tue, 12 Mar 2024 14:48:30 +0100","Message-ID":"<874jdb4l3l.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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","Cc":"Bryan O'Donoghue ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tPavel Machek ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28937,"web_url":"https://patchwork.libcamera.org/comment/28937/","msgid":"<171032752593.3708697.16837380313470428748@ping.linuxembedded.co.uk>","date":"2024-03-13T10:58:45","subject":"Re: [PATCH v5 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-03-12 13:48:30)\n> Hans de Goede writes:\n> \n> > From: Milan Zamazal \n> >\n> > Black may not be represented as 0 pixel value for given hardware, it may be\n> > higher. If this is not compensated then various problems may occur such as low\n> > contrast or suboptimal exposure.\n> >\n> > The black pixel value can be either retrieved from a tuning file for the given\n> > hardware, or automatically on fly. The former is the right and correct method,\n> > while the latter can be used when a tuning file is not available for the given\n> > hardware. Since there is currently no support for tuning files in software ISP,\n> > the automatic, hardware independent way, is always used. Support for tuning\n> > files should be added in future but it will require more work than this patch.\n> >\n> > The patch looks at the image histogram and assumes that black starts when pixel\n> > values start occurring on the left. A certain amount of the darkest pixels is\n> > ignored; it doesn't matter whether they represent various kinds of noise or are\n> > real, they are better to omit in any case to make the image looking better. It\n> > also doesn't matter whether the darkest pixels occur around the supposed black\n> > level or are spread between 0 and the black level, the difference is not\n> > important.\n> >\n> > An arbitrary threshold of 2% darkest pixels is applied; there is no magic about\n> > that value.\n> >\n> > The patch assumes that the black values for different colors are the same and\n> > doesn't attempt any other non-primitive enhancements. It cannot completely\n> > replace tuning files and simplicity, while providing visible benefit, is its\n> > goal. Anything more sophisticated is left for future patches.\n> >\n> > A possible cheap enhancement, if needed, could be setting exposure + gain to\n> > minimum values temporarily, before setting the black level. In theory, the\n> > black level should be fixed but it may not be reached in all images. For this\n> > reason, the patch updates black level only if the observed value is lower than\n> > the current one; it should be never increased.\n> >\n> > The purpose of the patch is to compensate for hardware properties. General\n> > image contrast enhancements are out of scope of this patch.\n> >\n> > Stats are still gathered as an uncorrected histogram, to avoid any confusion and\n> > to represent the raw image data. Exposure must be determined after the black\n> > level correction -- it has no influence on the sub-black area and must be\n> > correct after applying the black level correction. The granularity of the\n> > histogram is increased from 16 to 64 to provide a better precision (there is no\n> > theory behind either of those numbers).\n> >\n> > Reviewed-by: Hans de Goede \n> > Signed-off-by: Milan Zamazal \n> > Signed-off-by: Hans de Goede \n> > ---\n> \n> [...]\n> \n> > +void BlackLevel::update(SwIspStats::histogram &yHistogram)\n> > +{\n> > + // The constant is selected to be \"good enough\", not overly conservative or\n> > + // aggressive. There is no magic about the given value.\n\nNit here too... for block comments we use kernel styles not C++ style.\n\n /*\n * The constant is selected to be \"good enough\", not overly\n * conservative or aggressive. There is no magic about the given value.\n */\n\n> > + constexpr float ignoredPercentage_ = 0.02;\n> > + const unsigned int total =\n> > + std::accumulate(begin(yHistogram), end(yHistogram), 0);\n> > + const unsigned int pixelThreshold = ignoredPercentage_ * total;\n> > + const unsigned int currentBlackIdx =\n> > + blackLevel_ / (256 / SwIspStats::kYHistogramSize);\n> \n> Should the newly introduced kRGBLookupSize constant be used instead of 256 here\n> and in other places in this patch?\n> \n> [...]\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 8ACFBBD1F1\n\tfor ;\n\tWed, 13 Mar 2024 10:58:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 277E062C8F;\n\tWed, 13 Mar 2024 11:58:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1583862973\n\tfor ;\n\tWed, 13 Mar 2024 11:58:49 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A553D899;\n\tWed, 13 Mar 2024 11:58:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jQu8Wfv8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710327506;\n\tbh=YwfruAJvxi9x5PZZQZmcMFICx0w53zfmNNaG04tdiho=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=jQu8Wfv85FJpT73ARCzQEdAYdYBoicCh5uLDzSp6eB6UDAiqNtUHLHgAVc0PDx/Gp\n\tnB7BmpQJ9R+nVZhkvg1da6fioTJhvdbO+DsJdLCJ/csiMJuxnoeL4QizrY6qchonB9\n\tT+nwKlzw4gmGhVPzUVAeJWU5maVYG0gsDw9QNhxs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<874jdb4l3l.fsf@redhat.com>","References":"<20240311141524.27192-1-hdegoede@redhat.com>\n\t<20240311141524.27192-18-hdegoede@redhat.com>\n\t<874jdb4l3l.fsf@redhat.com>","Subject":"Re: [PATCH v5 17/18] libcamera: software_isp: Apply black level\n\tcompensation","From":"Kieran Bingham ","To":"Hans de Goede , Milan Zamazal ","Date":"Wed, 13 Mar 2024 10:58:45 +0000","Message-ID":"<171032752593.3708697.16837380313470428748@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Cc":"Bryan O'Donoghue ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tPavel Machek ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]