[{"id":29359,"web_url":"https://patchwork.libcamera.org/comment/29359/","msgid":"<20240428231255.GB4950@pendragon.ideasonboard.com>","date":"2024-04-28T23:12:55","subject":"Re: [PATCH 5/5] libcamera: software_isp: Remove\n\tDebayerParams::kGain10","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Tue, Apr 23, 2024 at 08:20:00PM +0200, Milan Zamazal wrote:\n> The constant is used in a single place internally and doesn't belong to\n> DebayerParams anymore. Let's use 256 directly.\n> \n> Completes software ISP TODO #4.\n\nThe change doesn't address the TODO item.\n\n> Signed-off-by: Milan Zamazal \n> ---\n> .../internal/software_isp/debayer_params.h | 1 -\n> src/ipa/simple/soft_simple.cpp | 3 +--\n> src/libcamera/software_isp/TODO | 13 -------------\n> src/libcamera/software_isp/debayer.cpp | 5 -----\n> 4 files changed, 1 insertion(+), 21 deletions(-)\n> \n> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n> index 09f4ff00..6aaa7d4c 100644\n> --- a/include/libcamera/internal/software_isp/debayer_params.h\n> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> @@ -16,7 +16,6 @@\n> namespace libcamera {\n> \n> struct DebayerParams {\n> -\tstatic constexpr unsigned int kGain10 = 256;\n> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n> \n> \tusing ColorLookupTable = std::array;\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index b2f4d308..5298836c 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -300,8 +300,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \n> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> \t\tconstexpr unsigned int div =\n> -\t\t\tDebayerParams::kRGBLookupSize * DebayerParams::kGain10 /\n> -\t\t\tkGammaLookupSize;\n> +\t\t\tDebayerParams::kRGBLookupSize * 256 / kGammaLookupSize;\n> \t\tunsigned int idx;\n> \n> \t\t/* Apply gamma after gain! */\n> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\n> index fcb02588..f8f00139 100644\n> --- a/src/libcamera/software_isp/TODO\n> +++ b/src/libcamera/software_isp/TODO\n> @@ -72,19 +72,6 @@ stats in hardware, such as the i.MX7), but please keep it on your radar.\n> \n> ---\n> \n> -4. Hide internal representation of gains from callers\n> -\n> -> struct DebayerParams {\n> -> \tstatic constexpr unsigned int kGain10 = 256;\n> -\n> -Forcing the caller to deal with the internal representation of gains\n> -isn't nice, especially given that it precludes implementing gains of\n> -different precisions in different backend. Wouldn't it be better to pass\n> -the values as floating point numbers, and convert them to the internal\n> -representation in the implementation of process() before using them ?\n> -\n> ----\n> -\n> 5. Store ISP parameters in per-frame buffers\n> \n> > /**\n> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n> index ac438a33..548c2ccd 100644\n> --- a/src/libcamera/software_isp/debayer.cpp\n> +++ b/src/libcamera/software_isp/debayer.cpp\n> @@ -18,11 +18,6 @@ namespace libcamera {\n> * \\brief Struct to hold the debayer parameters.\n> */\n> \n> -/**\n> - * \\var DebayerParams::kGain10\n> - * \\brief const value for 1.0 gain\n> - */\n> -\n> /**\n> * \\var DebayerParams::kRGBLookupSize\n> * \\brief Size of a color lookup table","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 88C85BE08B\n\tfor ;\n\tSun, 28 Apr 2024 23:13:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 191F763418;\n\tMon, 29 Apr 2024 01:13:04 +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 12FAB61A90\n\tfor ;\n\tMon, 29 Apr 2024 01:13:02 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.130.69.237])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 16CB3720;\n\tMon, 29 Apr 2024 01:12:07 +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=\"TWG4IRLN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714345927;\n\tbh=STXrO164rj9vR5TXqF15iuDjUbHYF/fk8oi7MCVllps=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TWG4IRLNnqL+NKPz7iLyNhuZYIgyDQ1nWEhnIj4P5HGsgUnkEMpN7n2igV/Rtr55N\n\t5QPrtExjgxCOpsQqYrn6r0m3VRsLzB0RraCncIOtCz8eiKw03ppWfAOnY41BrB/27z\n\tqe5gOUB3mOkjQ6TqETWrrDn/KnP7NDIPY61wQWOc=","Date":"Mon, 29 Apr 2024 02:12:55 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 5/5] libcamera: software_isp: Remove\n\tDebayerParams::kGain10","Message-ID":"<20240428231255.GB4950@pendragon.ideasonboard.com>","References":"<20240423182000.1527425-1-mzamazal@redhat.com>\n\t<20240423182000.1527425-6-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240423182000.1527425-6-mzamazal@redhat.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":29361,"web_url":"https://patchwork.libcamera.org/comment/29361/","msgid":"<87le4wzhg0.fsf@redhat.com>","date":"2024-04-29T08:44:31","subject":"Re: [PATCH 5/5] libcamera: software_isp: Remove\n\tDebayerParams::kGain10","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 23, 2024 at 08:20:00PM +0200, Milan Zamazal wrote:\n>> The constant is used in a single place internally and doesn't belong to\n>> DebayerParams anymore. Let's use 256 directly.\n>> \n>> Completes software ISP TODO #4.\n>\n> The change doesn't address the TODO item.\n\nHi Laurent,\n\nso is the remaining step to change the item type of ColorLookupTable's from\nuint8_t (0..255) to float (0.0..1.0) and converting the tables just before\ndebayering to uint8_t for the sake of separation? Anything else?\n\nThanks,\nMilan\n\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> .../internal/software_isp/debayer_params.h | 1 -\n>> src/ipa/simple/soft_simple.cpp | 3 +--\n>> src/libcamera/software_isp/TODO | 13 -------------\n>> src/libcamera/software_isp/debayer.cpp | 5 -----\n>> 4 files changed, 1 insertion(+), 21 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n>> index 09f4ff00..6aaa7d4c 100644\n>> --- a/include/libcamera/internal/software_isp/debayer_params.h\n>> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n>> @@ -16,7 +16,6 @@\n>> namespace libcamera {\n>> \n>> struct DebayerParams {\n>> -\tstatic constexpr unsigned int kGain10 = 256;\n>> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> \n>> \tusing ColorLookupTable = std::array;\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index b2f4d308..5298836c 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -300,8 +300,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> \n>> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> \t\tconstexpr unsigned int div =\n>> -\t\t\tDebayerParams::kRGBLookupSize * DebayerParams::kGain10 /\n>> -\t\t\tkGammaLookupSize;\n>> +\t\t\tDebayerParams::kRGBLookupSize * 256 / kGammaLookupSize;\n>> \t\tunsigned int idx;\n>> \n>> \t\t/* Apply gamma after gain! */\n>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\n>> index fcb02588..f8f00139 100644\n>> --- a/src/libcamera/software_isp/TODO\n>> +++ b/src/libcamera/software_isp/TODO\n>> @@ -72,19 +72,6 @@ stats in hardware, such as the i.MX7), but please keep it on your radar.\n>> \n>> ---\n>> \n>> -4. Hide internal representation of gains from callers\n>> -\n>> -> struct DebayerParams {\n>> -> \tstatic constexpr unsigned int kGain10 = 256;\n>> -\n>> -Forcing the caller to deal with the internal representation of gains\n>> -isn't nice, especially given that it precludes implementing gains of\n>> -different precisions in different backend. Wouldn't it be better to pass\n>> -the values as floating point numbers, and convert them to the internal\n>> -representation in the implementation of process() before using them ?\n>> -\n>> ----\n>> -\n>> 5. Store ISP parameters in per-frame buffers\n>> \n>> > /**\n>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n>> index ac438a33..548c2ccd 100644\n>> --- a/src/libcamera/software_isp/debayer.cpp\n>> +++ b/src/libcamera/software_isp/debayer.cpp\n>> @@ -18,11 +18,6 @@ namespace libcamera {\n>> * \\brief Struct to hold the debayer parameters.\n>> */\n>> \n>> -/**\n>> - * \\var DebayerParams::kGain10\n>> - * \\brief const value for 1.0 gain\n>> - */\n>> -\n>> /**\n>> * \\var DebayerParams::kRGBLookupSize\n>> * \\brief Size of a color lookup table","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 6BF72C3220\n\tfor ;\n\tMon, 29 Apr 2024 08:44:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A51563418;\n\tMon, 29 Apr 2024 10:44:39 +0200 (CEST)","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 B652A633ED\n\tfor ;\n\tMon, 29 Apr 2024 10:44:37 +0200 (CEST)","from mail-ej1-f71.google.com (mail-ej1-f71.google.com\n\t[209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-312-2-ScIq_GPkK2_rfEZ9pNIg-1; Mon, 29 Apr 2024 04:44:34 -0400","by mail-ej1-f71.google.com with SMTP id\n\ta640c23a62f3a-a5741ee352bso229512166b.1\n\tfor ;\n\tMon, 29 Apr 2024 01:44:34 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tf20-20020a17090624d400b00a5575cde7cdsm13319128ejb.220.2024.04.29.01.44.31\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 29 Apr 2024 01:44:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"JYnKWPG2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1714380276;\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=75fQi0zBdzpTDZ8X8jStIutdEGetvqE/gOrdfPx9xd4=;\n\tb=JYnKWPG2AO28ispw1vw4KhjNZtn9izECyAMJ4JupkLSPsBn3CdxaXlgjDQ3CZs2E/M1Ri5\n\tevWbXgw3stTjoBUkBLEcis7H7OJqtWWcJgcJsb4EvP9hxAgu2nAkjqpj2sKPOjVycg3n39\n\t76ao/Xgb6YekFiWdkfBSmRr0w8o3NtA=","X-MC-Unique":"2-ScIq_GPkK2_rfEZ9pNIg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1714380273; x=1714985073;\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=75fQi0zBdzpTDZ8X8jStIutdEGetvqE/gOrdfPx9xd4=;\n\tb=oQbowbiyKXWZbWVm/adfQAyOd4+/FKAeLlIljdCv4V4RwLJW+PzjvYBXJsK0Cb+neC\n\t1cKfrncRGCTFB77i+itNeubQ8mku2iB875Mvzw9zSg4UnAI3u8r2YIu5S879+8gQrBx/\n\tQNi0NQ+iC79AIdL4mTHvZNGmIP0taNpBRD/e7StXrvEPJvwhBk6iyfJT3HTinyyFtzze\n\tg3fp3DtB3unLbLlDWBsRttPXH4Z/pAVH81D76JNvijJ3u8zWO9bIUCqKjeXgvjuyS4kO\n\tmemNCZ4GvjRblloeOCH6ua40LsvG5a1t+ISqBSzL+fiFPMeLwVzKHFTYms1bOb4yKtq4\n\t67Aw==","X-Gm-Message-State":"AOJu0Yzo/VpAktFknZi28ECDeiyjMa5ECTbFQ9S9A7TH6hEYwCahaqi0\n\tO4LwybkBfeNJv+At2C1+4FHmivj5P0LfmNBAuwqgCFNThTOiQSubu3ea1S7kKtK0A4/laDAIVqH\n\tujr9foVUzzmCElGzhLhszb3A11v3JJZVZXJ29d3/FHbkQpvfOJhQ4q1+cVt6yw1xc5KIfY0Y/sW\n\tE4mlNKJZwacaHlZwfX9vYyULm+lh/px7MCKtFcNxVLu8Z/C0/OrlqWBjc=","X-Received":["by 2002:a17:906:5fd2:b0:a58:eaca:db4c with SMTP id\n\tk18-20020a1709065fd200b00a58eacadb4cmr3511211ejv.61.1714380272885; \n\tMon, 29 Apr 2024 01:44:32 -0700 (PDT)","by 2002:a17:906:5fd2:b0:a58:eaca:db4c with SMTP id\n\tk18-20020a1709065fd200b00a58eacadb4cmr3511199ejv.61.1714380272482; \n\tMon, 29 Apr 2024 01:44:32 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEemokyXNWlj0v59VtWXlsfu3QSrSAlIBHyE+mrxzt19qMMo37vc+9OioaLrRr37uXaPrjD6g==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 5/5] libcamera: software_isp: Remove\n\tDebayerParams::kGain10","In-Reply-To":"<20240428231255.GB4950@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 29 Apr 2024 02:12:55 +0300\")","References":"<20240423182000.1527425-1-mzamazal@redhat.com>\n\t<20240423182000.1527425-6-mzamazal@redhat.com>\n\t<20240428231255.GB4950@pendragon.ideasonboard.com>","Date":"Mon, 29 Apr 2024 10:44:31 +0200","Message-ID":"<87le4wzhg0.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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]