[{"id":32404,"web_url":"https://patchwork.libcamera.org/comment/32404/","msgid":"<20241127071538.GT5461@pendragon.ideasonboard.com>","date":"2024-11-27T07:15:38","subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","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, Nov 26, 2024 at 10:15:04AM +0100, Milan Zamazal wrote:\n> Software ISP is currently fully automatic and doesn't allow image\n> modifications by explicitly set control values. The user has no means\n> to make the image looking better.\n> \n> This patch introduces support for contrast control, which can improve\n> e.g. a flat looking image. Based on the provided contrast value with a\n> range 0..infinity and 1.0 being the normal value, it applies a simple\n> S-curve modification to the image. The contrast algorithm just handles\n> the provided values, while the S-curve is applied in the gamma algorithm\n> on the computed gamma curve whenever the contrast value changes. Since\n> the algorithm is applied only on the lookup table already present, its\n> overhead is negligible.\n\nIsn't contrast defined as a multiplier ? Applying a different luminance\ntransformation and calling it contrast will make different cameras\nbehave in different ways.\n\n> This is a preparation patch without actually providing the control\n> itself, which is done in the following patch.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----\n> src/ipa/simple/algorithms/lut.h | 5 ++++\n> src/ipa/simple/ipa_context.h | 7 ++++++\n> 3 files changed, 45 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> index 9744e773a..ffded0594 100644\n> --- a/src/ipa/simple/algorithms/lut.cpp\n> +++ b/src/ipa/simple/algorithms/lut.cpp\n> @@ -9,14 +9,19 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> \n> #include \"simple/ipa_context.h\"\n> \n> +#include \"control_ids.h\"\n> +\n> namespace libcamera {\n> \n> +LOG_DEFINE_CATEGORY(IPASoftLut)\n> +\n> namespace ipa::soft::algorithms {\n> \n> int Lut::configure(IPAContext &context,\n> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,\n> {\n> \t/* Gamma value is fixed */\n> \tcontext.configuration.gamma = 0.5;\n> +\tcontext.activeState.knobs.contrast = std::optional();\n> \tupdateGammaTable(context);\n> \n> \treturn 0;\n> }\n> \n> +void Lut::queueRequest(typename Module::Context &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t [[maybe_unused]] typename Module::FrameContext &frameContext,\n> +\t\t const ControlList &controls)\n> +{\n> +\tconst auto &contrast = controls.get(controls::Contrast);\n> +\tif (contrast.has_value()) {\n> +\t\tcontext.activeState.knobs.contrast = contrast;\n> +\t\tLOG(IPASoftLut, Debug) << \"Setting contrast to \" << contrast.value();\n> +\t}\n> +}\n> +\n> void Lut::updateGammaTable(IPAContext &context)\n> {\n> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n> -\tauto blackLevel = context.activeState.blc.level;\n> +\tconst auto blackLevel = context.activeState.blc.level;\n> \tconst unsigned int blackIndex = blackLevel * gammaTable.size() / 256;\n> +\tconst auto contrast = context.activeState.knobs.contrast.value_or(1.0);\n> \n> \tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n> \tconst float divisor = gammaTable.size() - blackIndex - 1.0;\n> -\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++)\n> -\t\tgammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,\n> -\t\t\t\t\t\t context.configuration.gamma);\n> +\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++) {\n> +\t\tdouble normalized = (i - blackIndex) / divisor;\n> +\t\t/* Apply simple S-curve */\n> +\t\tif (normalized < 0.5)\n> +\t\t\tnormalized = 0.5 * std::pow(normalized / 0.5, contrast);\n> +\t\telse\n> +\t\t\tnormalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);\n> +\t\tgammaTable[i] = UINT8_MAX *\n> +\t\t\t\tstd::pow(normalized, context.configuration.gamma);\n> +\t}\n> \n> \tcontext.activeState.gamma.blackLevel = blackLevel;\n> +\tcontext.activeState.gamma.contrast = contrast;\n> }\n> \n> void Lut::prepare(IPAContext &context,\n> @@ -55,7 +82,8 @@ void Lut::prepare(IPAContext &context,\n> \t * observed, it's not permanently prone to minor fluctuations or\n> \t * rounding errors.\n> \t */\n> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n> +\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level ||\n> +\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n> \t\tupdateGammaTable(context);\n> \n> \tauto &gains = context.activeState.gains;\n> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n> index b635987d0..ef2df147c 100644\n> --- a/src/ipa/simple/algorithms/lut.h\n> +++ b/src/ipa/simple/algorithms/lut.h\n> @@ -20,6 +20,11 @@ public:\n> \t~Lut() = default;\n> \n> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> +\tvoid queueRequest(typename Module::Context &context,\n> +\t\t\t const uint32_t frame,\n> +\t\t\t typename Module::FrameContext &frameContext,\n> +\t\t\t const ControlList &controls)\n> +\t\toverride;\n> \tvoid prepare(IPAContext &context,\n> \t\t const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index fd7343e91..148052207 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -11,6 +11,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \n> \n> namespace libcamera {\n> @@ -48,7 +50,12 @@ struct IPAActiveState {\n> \tstruct {\n> \t\tstd::array gammaTable;\n> \t\tuint8_t blackLevel;\n> +\t\tdouble contrast;\n> \t} gamma;\n> +\tstruct {\n> +\t\t/* 0..inf range, 1.0 = normal */\n> +\t\tstd::optional contrast;\n> +\t} knobs;\n> };\n> \n> struct IPAFrameContext : public FrameContext {","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 26570C3200\n\tfor ;\n\tWed, 27 Nov 2024 07:15:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C6256609A;\n\tWed, 27 Nov 2024 08:15:51 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7A3465FD6\n\tfor ;\n\tWed, 27 Nov 2024 08:15:49 +0100 (CET)","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 849FF78C;\n\tWed, 27 Nov 2024 08:15: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=\"BPUrMEUF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732691726;\n\tbh=bKJ5ZUbT7y2t/HwZt9Dxfn2bb7Ji7WNCF+VJzVIOBGQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BPUrMEUFGDr6OYqR0xgcTDtYJThVV6LVa3HnSFRiB4KPb1M1vg4VlqAX8kNO+H67c\n\tYhRXmBMO0GPmt/Tglv8OLaexcPWrv1EF2HRTN0JczgVsj5Uy73Hg8zmqqhjMNqMFla\n\t4a7KOU12IeW8s3LKTXIOCZw7QhgGlu7txfoiYaLo=","Date":"Wed, 27 Nov 2024 09:15:38 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","Message-ID":"<20241127071538.GT5461@pendragon.ideasonboard.com>","References":"<20241126091509.89677-1-mzamazal@redhat.com>\n\t<20241126091509.89677-4-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241126091509.89677-4-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":32413,"web_url":"https://patchwork.libcamera.org/comment/32413/","msgid":"<878qt5qafy.fsf@redhat.com>","date":"2024-11-27T09:42:57","subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for\n\tcontrast control","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Tue, Nov 26, 2024 at 10:15:04AM +0100, Milan Zamazal wrote:\n>> Software ISP is currently fully automatic and doesn't allow image\n>> modifications by explicitly set control values. The user has no means\n>> to make the image looking better.\n>> \n>> This patch introduces support for contrast control, which can improve\n>> e.g. a flat looking image. Based on the provided contrast value with a\n>> range 0..infinity and 1.0 being the normal value, it applies a simple\n>> S-curve modification to the image. The contrast algorithm just handles\n>> the provided values, while the S-curve is applied in the gamma algorithm\n>> on the computed gamma curve whenever the contrast value changes. Since\n>> the algorithm is applied only on the lookup table already present, its\n>> overhead is negligible.\n>\n> Isn't contrast defined as a multiplier ? Applying a different luminance\n> transformation and calling it contrast will make different cameras\n> behave in different ways.\n\ncontrol_ids_core.yaml says:\n\n - Contrast:\n type: float\n description: |\n Specify a fixed contrast parameter.\n\n Normal contrast is given by the value 1.0; larger values produce images\n with more contrast.\n\nAnd V4L2:\n\n V4L2_CID_CONTRAST (integer)\n Picture contrast or luma gain.\n\nSo nothing specific. I don't know what hardware ISPs usually do with\nthis setting but simply multiplying with clipping (if this is what you\nmean) wouldn't be very useful regarding image quality.\n\n>> This is a preparation patch without actually providing the control\n>> itself, which is done in the following patch.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----\n>> src/ipa/simple/algorithms/lut.h | 5 ++++\n>> src/ipa/simple/ipa_context.h | 7 ++++++\n>> 3 files changed, 45 insertions(+), 5 deletions(-)\n>> \n>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n>> index 9744e773a..ffded0594 100644\n>> --- a/src/ipa/simple/algorithms/lut.cpp\n>> +++ b/src/ipa/simple/algorithms/lut.cpp\n>> @@ -9,14 +9,19 @@\n>> \n>> #include \n>> #include \n>> +#include \n>> #include \n>> \n>> #include \n>> \n>> #include \"simple/ipa_context.h\"\n>> \n>> +#include \"control_ids.h\"\n>> +\n>> namespace libcamera {\n>> \n>> +LOG_DEFINE_CATEGORY(IPASoftLut)\n>> +\n>> namespace ipa::soft::algorithms {\n>> \n>> int Lut::configure(IPAContext &context,\n>> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,\n>> {\n>> \t/* Gamma value is fixed */\n>> \tcontext.configuration.gamma = 0.5;\n>> +\tcontext.activeState.knobs.contrast = std::optional();\n>> \tupdateGammaTable(context);\n>> \n>> \treturn 0;\n>> }\n>> \n>> +void Lut::queueRequest(typename Module::Context &context,\n>> +\t\t [[maybe_unused]] const uint32_t frame,\n>> +\t\t [[maybe_unused]] typename Module::FrameContext &frameContext,\n>> +\t\t const ControlList &controls)\n>> +{\n>> +\tconst auto &contrast = controls.get(controls::Contrast);\n>> +\tif (contrast.has_value()) {\n>> +\t\tcontext.activeState.knobs.contrast = contrast;\n>> +\t\tLOG(IPASoftLut, Debug) << \"Setting contrast to \" << contrast.value();\n>> +\t}\n>> +}\n>> +\n>> void Lut::updateGammaTable(IPAContext &context)\n>> {\n>> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n>> -\tauto blackLevel = context.activeState.blc.level;\n>> +\tconst auto blackLevel = context.activeState.blc.level;\n>> \tconst unsigned int blackIndex = blackLevel * gammaTable.size() / 256;\n>> +\tconst auto contrast = context.activeState.knobs.contrast.value_or(1.0);\n>> \n>> \tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n>> \tconst float divisor = gammaTable.size() - blackIndex - 1.0;\n>> -\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++)\n>> -\t\tgammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,\n>> -\t\t\t\t\t\t context.configuration.gamma);\n>> +\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++) {\n>> +\t\tdouble normalized = (i - blackIndex) / divisor;\n>> +\t\t/* Apply simple S-curve */\n>> +\t\tif (normalized < 0.5)\n>> +\t\t\tnormalized = 0.5 * std::pow(normalized / 0.5, contrast);\n>> +\t\telse\n>> +\t\t\tnormalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);\n>> +\t\tgammaTable[i] = UINT8_MAX *\n>> +\t\t\t\tstd::pow(normalized, context.configuration.gamma);\n>> +\t}\n>> \n>> \tcontext.activeState.gamma.blackLevel = blackLevel;\n>> +\tcontext.activeState.gamma.contrast = contrast;\n>> }\n>> \n>> void Lut::prepare(IPAContext &context,\n>> @@ -55,7 +82,8 @@ void Lut::prepare(IPAContext &context,\n>> \t * observed, it's not permanently prone to minor fluctuations or\n>> \t * rounding errors.\n>> \t */\n>> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n>> +\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level ||\n>> +\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n>> \t\tupdateGammaTable(context);\n>> \n>> \tauto &gains = context.activeState.gains;\n>> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n>> index b635987d0..ef2df147c 100644\n>> --- a/src/ipa/simple/algorithms/lut.h\n>> +++ b/src/ipa/simple/algorithms/lut.h\n>> @@ -20,6 +20,11 @@ public:\n>> \t~Lut() = default;\n>> \n>> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>> +\tvoid queueRequest(typename Module::Context &context,\n>> +\t\t\t const uint32_t frame,\n>> +\t\t\t typename Module::FrameContext &frameContext,\n>> +\t\t\t const ControlList &controls)\n>> +\t\toverride;\n>> \tvoid prepare(IPAContext &context,\n>> \t\t const uint32_t frame,\n>> \t\t IPAFrameContext &frameContext,\n>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> index fd7343e91..148052207 100644\n>> --- a/src/ipa/simple/ipa_context.h\n>> +++ b/src/ipa/simple/ipa_context.h\n>> @@ -11,6 +11,8 @@\n>> #include \n>> #include \n>> \n>> +#include \n>> +\n>> #include \n>> \n>> namespace libcamera {\n>> @@ -48,7 +50,12 @@ struct IPAActiveState {\n>> \tstruct {\n>> \t\tstd::array gammaTable;\n>> \t\tuint8_t blackLevel;\n>> +\t\tdouble contrast;\n>> \t} gamma;\n>> +\tstruct {\n>> +\t\t/* 0..inf range, 1.0 = normal */\n>> +\t\tstd::optional contrast;\n>> +\t} knobs;\n>> };\n>> \n>> struct IPAFrameContext : public FrameContext {","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 12899C3213\n\tfor ;\n\tWed, 27 Nov 2024 09:43:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 127FD660C6;\n\tWed, 27 Nov 2024 10:43:05 +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 587D1660B5\n\tfor ;\n\tWed, 27 Nov 2024 10:43:03 +0100 (CET)","from mail-wr1-f69.google.com (mail-wr1-f69.google.com\n\t[209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-368-H3XQVobiPj-2r7ZkwTxVgA-1; Wed, 27 Nov 2024 04:43:00 -0500","by mail-wr1-f69.google.com with SMTP id\n\tffacd0b85a97d-3825a721afaso3880096f8f.0\n\tfor ;\n\tWed, 27 Nov 2024 01:43:00 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-3825fbc4130sm15690642f8f.65.2024.11.27.01.42.57\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 27 Nov 2024 01:42:58 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Pijur6+Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1732700582;\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=L6ydqZhfBSZfd6Kkc77VMpxSOOMQdIsOsHtawfFLQVs=;\n\tb=Pijur6+QFxixuPkHH1+4RUgoLa1Nmk01oHXjd1FUye0GzNGBXhpD7GudT2AoSt46lKgBys\n\tQcVNOBG+Kh8K5vhOlF9pesV59DZoGdPDaVq4zf/8Q3nmRdSND0pphLMO/hK4aQxHYxSfyS\n\tKL65qHFvoFqekxk9xAS4JXD6srSPvRw=","X-MC-Unique":"H3XQVobiPj-2r7ZkwTxVgA-1","X-Mimecast-MFC-AGG-ID":"H3XQVobiPj-2r7ZkwTxVgA","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732700579; x=1733305379;\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=L6ydqZhfBSZfd6Kkc77VMpxSOOMQdIsOsHtawfFLQVs=;\n\tb=YKdLaYS84CDWzFnw0qJWS8+HXPOl7yMvBlwV2WemJIy8mQ5o9MnMWEB3aOK9VpLZmO\n\tUT45Vl8BCJS0LpcLOLi8gePpJ26XhMaNxvBFpXumoUwjRZ+nVDnLEfLbUcDvj4vwLrSE\n\twamR62U23Dx2VMe2s/QUGGo6mBVISPveBAbxf74VcgNWCAlgOvQpVc97zaox0H1jmxNS\n\teGerLauDW+7Rm5ZT/I+pmLqwGlQEM4qRMg/B5BsbfAjlBgOM1qW+e7UJUwyd66efvWQR\n\t5SL3PsIugwjzKbu3NO9L42/P4bZJi0qNUrdwb2IZvZdY8Ap7TrXRcwltd/Ysl9k+2K/p\n\tmXjg==","X-Gm-Message-State":"AOJu0Ywu1b2GNEcNvzbJP5VocINjD8EVgNWHkzpyuX6NUl9bLGDjNNPR\n\txnPQtBGUws6HqwB8U/XW7uikTX00/vR+U0PQdqXVBIE0qJLauBH7ohY0OBJayXNEfoIKjX8ISKP\n\tFHg5myuBdysacmpPAXV3h/2O1knjQTsarbL3J4IWkl2M3an4+h6554AHp5N33B8WriQ2Y7J8=","X-Gm-Gg":"ASbGncu/dm0UvdyzKxW0MozwU+z5aip0dlqDpPAspx8Oj+dDupoVi9wbRcF3Zc8Mj+S\n\tq1Uz2f1uoIUV0e3RNbOkWRKCXpLswR8k+UWDXH7WDixajh5pb43ovXyx+z+j1XfQdMxnHTtUnJx\n\tz5d+h0Il6q1FRqS/rTYht9bmfVbeV1Mdd8WulkJ/sEIol9a6inZTZzAV/Yjp9hcT4XMoLPCF/ei\n\trxFSex8ytQh1kvytAB7p/6P3p9AUNcTc2OT1WMfaHEOz8y4k12+Z4BpCXI4rAnMhua80Zg=","X-Received":["by 2002:a5d:588f:0:b0:382:4aeb:91c7 with SMTP id\n\tffacd0b85a97d-385c6eb6911mr1711071f8f.18.1732700579173; \n\tWed, 27 Nov 2024 01:42:59 -0800 (PST)","by 2002:a5d:588f:0:b0:382:4aeb:91c7 with SMTP id\n\tffacd0b85a97d-385c6eb6911mr1711055f8f.18.1732700578815; \n\tWed, 27 Nov 2024 01:42:58 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFQooxpzuuzINCegKqWKGMt8GhbpSS9HPUysSnFeW1+4oyDhBxDjtV7Fk9HuMX//bGK4gdafw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t","Subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for\n\tcontrast control","In-Reply-To":"<20241127071538.GT5461@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 27 Nov 2024 09:15:38 +0200\")","References":"<20241126091509.89677-1-mzamazal@redhat.com>\n\t<20241126091509.89677-4-mzamazal@redhat.com>\n\t<20241127071538.GT5461@pendragon.ideasonboard.com>","Date":"Wed, 27 Nov 2024 10:42:57 +0100","Message-ID":"<878qt5qafy.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"stZDTGq1h6Rbc4vIr4tOYp3DzCoujqnUQYQPWiZ3dq4_1732700579","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\" "}},{"id":32414,"web_url":"https://patchwork.libcamera.org/comment/32414/","msgid":"","date":"2024-11-27T13:36:24","subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Milan, hi Laurent,\n\nOn Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:\n> Hi Laurent,\n> \n> Laurent Pinchart writes:\n> \n> > Hi Milan,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Nov 26, 2024 at 10:15:04AM +0100, Milan Zamazal wrote:\n> >> Software ISP is currently fully automatic and doesn't allow image\n> >> modifications by explicitly set control values. The user has no means\n> >> to make the image looking better.\n> >> \n> >> This patch introduces support for contrast control, which can improve\n> >> e.g. a flat looking image. Based on the provided contrast value with a\n> >> range 0..infinity and 1.0 being the normal value, it applies a simple\n> >> S-curve modification to the image. The contrast algorithm just handles\n> >> the provided values, while the S-curve is applied in the gamma algorithm\n> >> on the computed gamma curve whenever the contrast value changes. Since\n> >> the algorithm is applied only on the lookup table already present, its\n> >> overhead is negligible.\n> >\n> > Isn't contrast defined as a multiplier ? Applying a different luminance\n> > transformation and calling it contrast will make different cameras\n> > behave in different ways.\n> \n> control_ids_core.yaml says:\n> \n> - Contrast:\n> type: float\n> description: |\n> Specify a fixed contrast parameter.\n> \n> Normal contrast is given by the value 1.0; larger values produce images\n> with more contrast.\n> \n> And V4L2:\n> \n> V4L2_CID_CONTRAST (integer)\n> Picture contrast or luma gain.\n> \n> So nothing specific. I don't know what hardware ISPs usually do with\n> this setting but simply multiplying with clipping (if this is what you\n> mean) wouldn't be very useful regarding image quality.\n\nI agree that a linear contrast curve wouldn't serve image quality\npurposes but is merely for scientific purposes. We could implement\nsomething like a contrast mode (linear|s-curve), but I believe an\nS-curve is the most useful one to the users.\n\n> \n> >> This is a preparation patch without actually providing the control\n> >> itself, which is done in the following patch.\n> >> \n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----\n> >> src/ipa/simple/algorithms/lut.h | 5 ++++\n> >> src/ipa/simple/ipa_context.h | 7 ++++++\n> >> 3 files changed, 45 insertions(+), 5 deletions(-)\n> >> \n> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> >> index 9744e773a..ffded0594 100644\n> >> --- a/src/ipa/simple/algorithms/lut.cpp\n> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n> >> @@ -9,14 +9,19 @@\n> >> \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> \n> >> #include \n> >> \n> >> #include \"simple/ipa_context.h\"\n> >> \n> >> +#include \"control_ids.h\"\n> >> +\n> >> namespace libcamera {\n> >> \n> >> +LOG_DEFINE_CATEGORY(IPASoftLut)\n> >> +\n> >> namespace ipa::soft::algorithms {\n> >> \n> >> int Lut::configure(IPAContext &context,\n> >> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,\n> >> {\n> >> \t/* Gamma value is fixed */\n> >> \tcontext.configuration.gamma = 0.5;\n> >> +\tcontext.activeState.knobs.contrast = std::optional();\n> >> \tupdateGammaTable(context);\n> >> \n> >> \treturn 0;\n> >> }\n> >> \n> >> +void Lut::queueRequest(typename Module::Context &context,\n> >> +\t\t [[maybe_unused]] const uint32_t frame,\n> >> +\t\t [[maybe_unused]] typename Module::FrameContext &frameContext,\n> >> +\t\t const ControlList &controls)\n> >> +{\n> >> +\tconst auto &contrast = controls.get(controls::Contrast);\n> >> +\tif (contrast.has_value()) {\n> >> +\t\tcontext.activeState.knobs.contrast = contrast;\n> >> +\t\tLOG(IPASoftLut, Debug) << \"Setting contrast to \" << contrast.value();\n> >> +\t}\n> >> +}\n> >> +\n> >> void Lut::updateGammaTable(IPAContext &context)\n> >> {\n> >> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n> >> -\tauto blackLevel = context.activeState.blc.level;\n> >> +\tconst auto blackLevel = context.activeState.blc.level;\n> >> \tconst unsigned int blackIndex = blackLevel * gammaTable.size() / 256;\n> >> +\tconst auto contrast = context.activeState.knobs.contrast.value_or(1.0);\n> >> \n> >> \tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n> >> \tconst float divisor = gammaTable.size() - blackIndex - 1.0;\n> >> -\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++)\n> >> -\t\tgammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,\n> >> -\t\t\t\t\t\t context.configuration.gamma);\n> >> +\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++) {\n> >> +\t\tdouble normalized = (i - blackIndex) / divisor;\n> >> +\t\t/* Apply simple S-curve */\n> >> +\t\tif (normalized < 0.5)\n> >> +\t\t\tnormalized = 0.5 * std::pow(normalized / 0.5, contrast);\n> >> +\t\telse\n> >> +\t\t\tnormalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);\n\nOne thing that bothers me on that curve is the asymmetry with regards to\nthe contrast value. So a contrast value of 2 provides some contrast\nincrease while a contrast value of 0 creates a completely flat response.\n\nI believe a value range of -n to n would be easier to understand (0\nequals no change in contrast, -1 decrease, 1 increase) but that is a\ndifferent topic.\n\nI played around with the formula a bit:\nhttps://www.desmos.com/calculator/5zknbyjpln\n\nThe red curve is the one from this patch, the green one is a symmetric\nversion. The steepness is something that might need attention as it goes\nto infinity for a contrast value of 2.\n\nTo my knowledge there is no \"official\" s-curve with a corresponding\ndefined behavior of the contrast value. We might lookup how gimp does\nit.\n\nBest regards,\nStefan\n\n> >> +\t\tgammaTable[i] = UINT8_MAX *\n> >> +\t\t\t\tstd::pow(normalized, context.configuration.gamma);\n> >> +\t}\n> >> \n> >> \tcontext.activeState.gamma.blackLevel = blackLevel;\n> >> +\tcontext.activeState.gamma.contrast = contrast;\n> >> }\n> >> \n> >> void Lut::prepare(IPAContext &context,\n> >> @@ -55,7 +82,8 @@ void Lut::prepare(IPAContext &context,\n> >> \t * observed, it's not permanently prone to minor fluctuations or\n> >> \t * rounding errors.\n> >> \t */\n> >> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n> >> +\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level ||\n> >> +\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n> >> \t\tupdateGammaTable(context);\n> >> \n> >> \tauto &gains = context.activeState.gains;\n> >> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n> >> index b635987d0..ef2df147c 100644\n> >> --- a/src/ipa/simple/algorithms/lut.h\n> >> +++ b/src/ipa/simple/algorithms/lut.h\n> >> @@ -20,6 +20,11 @@ public:\n> >> \t~Lut() = default;\n> >> \n> >> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> >> +\tvoid queueRequest(typename Module::Context &context,\n> >> +\t\t\t const uint32_t frame,\n> >> +\t\t\t typename Module::FrameContext &frameContext,\n> >> +\t\t\t const ControlList &controls)\n> >> +\t\toverride;\n> >> \tvoid prepare(IPAContext &context,\n> >> \t\t const uint32_t frame,\n> >> \t\t IPAFrameContext &frameContext,\n> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> >> index fd7343e91..148052207 100644\n> >> --- a/src/ipa/simple/ipa_context.h\n> >> +++ b/src/ipa/simple/ipa_context.h\n> >> @@ -11,6 +11,8 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \n> >> +\n> >> #include \n> >> \n> >> namespace libcamera {\n> >> @@ -48,7 +50,12 @@ struct IPAActiveState {\n> >> \tstruct {\n> >> \t\tstd::array gammaTable;\n> >> \t\tuint8_t blackLevel;\n> >> +\t\tdouble contrast;\n> >> \t} gamma;\n> >> +\tstruct {\n> >> +\t\t/* 0..inf range, 1.0 = normal */\n> >> +\t\tstd::optional contrast;\n> >> +\t} knobs;\n> >> };\n> >> \n> >> struct IPAFrameContext : public FrameContext {\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 2AC9CC3200\n\tfor ;\n\tWed, 27 Nov 2024 13:36:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C904660D2;\n\tWed, 27 Nov 2024 14:36:29 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B87B5660C4\n\tfor ;\n\tWed, 27 Nov 2024 14:36:27 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:cf8b:d681:2f3e:129c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71ADD78C;\n\tWed, 27 Nov 2024 14:36:04 +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=\"m53p4LtN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732714564;\n\tbh=mHh63S3FqDG0GE6G999XLYXsuKeFZwUabc43iAWzlus=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m53p4LtNrnj7Hj1Cj9bUhrTB+oQqFTsMuwyPGfiCahRN3gxt9muD+Qh90yFJ3JRgi\n\tlQAQm/YnxC60zHXLgDV1qr4uArxRK/OitQR9fNx0ZF8a8Pd0toVtumDNVqfNaxutWy\n\t1bnmmTViBqMPOpT4AtmgIfenr38uYoBPbpVyO4yo=","Date":"Wed, 27 Nov 2024 14:36:24 +0100","From":"Stefan Klug ","To":"Milan Zamazal ","Cc":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","Message-ID":"","References":"<20241126091509.89677-1-mzamazal@redhat.com>\n\t<20241126091509.89677-4-mzamazal@redhat.com>\n\t<20241127071538.GT5461@pendragon.ideasonboard.com>\n\t<878qt5qafy.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<878qt5qafy.fsf@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":32416,"web_url":"https://patchwork.libcamera.org/comment/32416/","msgid":"<871pywr7yy.fsf@redhat.com>","date":"2024-11-27T15:51:01","subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for\n\tcontrast control","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Stefan,\n\nStefan Klug writes:\n\n> Hi Milan, hi Laurent,\n>\n> On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:\n>> Hi Laurent,\n>> \n>> Laurent Pinchart writes:\n>> \n>> > Hi Milan,\n>> >\n>> > Thank you for the patch.\n>> >\n>> > On Tue, Nov 26, 2024 at 10:15:04AM +0100, Milan Zamazal wrote:\n>> >> Software ISP is currently fully automatic and doesn't allow image\n>> >> modifications by explicitly set control values. The user has no means\n>> >> to make the image looking better.\n>> >> \n>> >> This patch introduces support for contrast control, which can improve\n>> >> e.g. a flat looking image. Based on the provided contrast value with a\n>> >> range 0..infinity and 1.0 being the normal value, it applies a simple\n>> >> S-curve modification to the image. The contrast algorithm just handles\n>> >> the provided values, while the S-curve is applied in the gamma algorithm\n>> >> on the computed gamma curve whenever the contrast value changes. Since\n>> >> the algorithm is applied only on the lookup table already present, its\n>> >> overhead is negligible.\n>> >\n>> > Isn't contrast defined as a multiplier ? Applying a different luminance\n>> > transformation and calling it contrast will make different cameras\n>> > behave in different ways.\n>> \n>> control_ids_core.yaml says:\n>> \n>> - Contrast:\n>> type: float\n>> description: |\n>> Specify a fixed contrast parameter.\n>> \n>> Normal contrast is given by the value 1.0; larger values produce images\n>> with more contrast.\n>> \n>> And V4L2:\n>> \n>> V4L2_CID_CONTRAST (integer)\n>> Picture contrast or luma gain.\n>> \n>> So nothing specific. I don't know what hardware ISPs usually do with\n>> this setting but simply multiplying with clipping (if this is what you\n>> mean) wouldn't be very useful regarding image quality.\n>\n> I agree that a linear contrast curve wouldn't serve image quality\n> purposes but is merely for scientific purposes. We could implement\n> something like a contrast mode (linear|s-curve), but I believe an\n> S-curve is the most useful one to the users.\n\nOK, let's stick with an S-curve for now.\n\n>> >> This is a preparation patch without actually providing the control\n>> >> itself, which is done in the following patch.\n>> >> \n>> >> Signed-off-by: Milan Zamazal \n>> >> ---\n>> >> src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----\n>> >> src/ipa/simple/algorithms/lut.h | 5 ++++\n>> >> src/ipa/simple/ipa_context.h | 7 ++++++\n>> >> 3 files changed, 45 insertions(+), 5 deletions(-)\n>> >> \n>> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n>> >> index 9744e773a..ffded0594 100644\n>> >> --- a/src/ipa/simple/algorithms/lut.cpp\n>> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n>> >> @@ -9,14 +9,19 @@\n>> >> \n>> >> #include \n>> >> #include \n>> >> +#include \n>> >> #include \n>> >> \n>> >> #include \n>> >> \n>> >> #include \"simple/ipa_context.h\"\n>> >> \n>> >> +#include \"control_ids.h\"\n>> >> +\n>> >> namespace libcamera {\n>> >> \n>> >> +LOG_DEFINE_CATEGORY(IPASoftLut)\n>> >> +\n>> >> namespace ipa::soft::algorithms {\n>> >> \n>> >> int Lut::configure(IPAContext &context,\n>> >> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,\n>> >> {\n>> >> \t/* Gamma value is fixed */\n>> >> \tcontext.configuration.gamma = 0.5;\n>> >> +\tcontext.activeState.knobs.contrast = std::optional();\n>> >> \tupdateGammaTable(context);\n>> >> \n>> >> \treturn 0;\n>> >> }\n>> >> \n>> >> +void Lut::queueRequest(typename Module::Context &context,\n>> >> +\t\t [[maybe_unused]] const uint32_t frame,\n>> >> +\t\t [[maybe_unused]] typename Module::FrameContext &frameContext,\n>> >> +\t\t const ControlList &controls)\n>> >> +{\n>> >> +\tconst auto &contrast = controls.get(controls::Contrast);\n>> >> +\tif (contrast.has_value()) {\n>> >> +\t\tcontext.activeState.knobs.contrast = contrast;\n>> >> +\t\tLOG(IPASoftLut, Debug) << \"Setting contrast to \" << contrast.value();\n>> >> +\t}\n>> >> +}\n>> >> +\n>> >> void Lut::updateGammaTable(IPAContext &context)\n>> >> {\n>> >> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n>> >> -\tauto blackLevel = context.activeState.blc.level;\n>> >> +\tconst auto blackLevel = context.activeState.blc.level;\n>> >> \tconst unsigned int blackIndex = blackLevel * gammaTable.size() / 256;\n>> >> +\tconst auto contrast = context.activeState.knobs.contrast.value_or(1.0);\n>> >> \n>> >> \tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n>> >> \tconst float divisor = gammaTable.size() - blackIndex - 1.0;\n>> >> -\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++)\n>> >> -\t\tgammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,\n>> >> -\t\t\t\t\t\t context.configuration.gamma);\n>> >> +\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++) {\n>> >> +\t\tdouble normalized = (i - blackIndex) / divisor;\n>> >> +\t\t/* Apply simple S-curve */\n>> >> +\t\tif (normalized < 0.5)\n>> >> +\t\t\tnormalized = 0.5 * std::pow(normalized / 0.5, contrast);\n>> >> +\t\telse\n>> >> +\t\t\tnormalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);\n>\n> One thing that bothers me on that curve is the asymmetry with regards to\n> the contrast value. So a contrast value of 2 provides some contrast\n> increase while a contrast value of 0 creates a completely flat response.\n\nWell, the fact that the counterpart of 2 is 1/2 is not exactly asymmetry\nbut it's true that it doesn't play well with linear sliders.\n\n> I believe a value range of -n to n would be easier to understand (0\n> equals no change in contrast, -1 decrease, 1 increase) but that is a\n> different topic.\n\nYes, this would violate the requirement of 1.0 being a normal contrast.\n\nI can see that rpi uses [0, 32] range, while rkisp1 uses [0, 1.993], so\nwe have precedents for both the ways. But considering that rpi is\ngenerally special and GUI sliders are usually linear, I'm in favor of\n[0, 2] range as you suggest below.\n\n> I played around with the formula a bit:\n> https://www.desmos.com/calculator/5zknbyjpln\n>\n> The red curve is the one from this patch, the green one is a symmetric\n> version. \n\nThe modified formula looks nice to linearize the scale, I can use it.\nThank you for this demonstration.\n\n> The steepness is something that might need attention as it goes to\n> infinity for a contrast value of 2.\n\nI wonder whether something similar could be the reason why rkisp1 uses\nthe mysterious value 1.993 as the upper bound (the message of the commit\nintroducing it doesn't explain it). I'd prefer using 2 in my code for\nclarity, perhaps with some internal check (g++/glibc++ seems to be fine\nwith tan(M_PI/2) but better to prevent possible portability issues).\n\n> To my knowledge there is no \"official\" s-curve with a corresponding\n> defined behavior of the contrast value. We might lookup how gimp does\n> it.\n\nAt a quick look, GIMP and photo editors (Darktable, Rawtherapee) seem to\ntransform contrast to curve points. GIMP computes\n\n slant = tan ((config->contrast + 1) * G_PI_4);\n\n(looks familiar, OK) that is then used to set some low and high output\npoints of the curve.\n\nIn our much simpler case, I think we wouldn't be wrong with your\n\"symmetric\" formula.\n\n> Best regards,\n> Stefan\n>\n>> >> +\t\tgammaTable[i] = UINT8_MAX *\n>> >> +\t\t\t\tstd::pow(normalized, context.configuration.gamma);\n>> >> +\t}\n>> >> \n>> >> \tcontext.activeState.gamma.blackLevel = blackLevel;\n>> >> +\tcontext.activeState.gamma.contrast = contrast;\n>> >> }\n>> >> \n>> >> void Lut::prepare(IPAContext &context,\n>> >> @@ -55,7 +82,8 @@ void Lut::prepare(IPAContext &context,\n>> >> \t * observed, it's not permanently prone to minor fluctuations or\n>> >> \t * rounding errors.\n>> >> \t */\n>> >> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n>> >> +\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level ||\n>> >> +\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n>> >> \t\tupdateGammaTable(context);\n>> >> \n>> >> \tauto &gains = context.activeState.gains;\n>> >> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n>> >> index b635987d0..ef2df147c 100644\n>> >> --- a/src/ipa/simple/algorithms/lut.h\n>> >> +++ b/src/ipa/simple/algorithms/lut.h\n>> >> @@ -20,6 +20,11 @@ public:\n>> >> \t~Lut() = default;\n>> >> \n>> >> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>> >> +\tvoid queueRequest(typename Module::Context &context,\n>> >> +\t\t\t const uint32_t frame,\n>> >> +\t\t\t typename Module::FrameContext &frameContext,\n>> >> +\t\t\t const ControlList &controls)\n>> >> +\t\toverride;\n>> >> \tvoid prepare(IPAContext &context,\n>> >> \t\t const uint32_t frame,\n>> >> \t\t IPAFrameContext &frameContext,\n>> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> >> index fd7343e91..148052207 100644\n>> >> --- a/src/ipa/simple/ipa_context.h\n>> >> +++ b/src/ipa/simple/ipa_context.h\n>> >> @@ -11,6 +11,8 @@\n>> >> #include \n>> >> #include \n>> >> \n>> >> +#include \n>> >> +\n>> >> #include \n>> >> \n>> >> namespace libcamera {\n>> >> @@ -48,7 +50,12 @@ struct IPAActiveState {\n>> >> \tstruct {\n>> >> \t\tstd::array gammaTable;\n>> >> \t\tuint8_t blackLevel;\n>> >> +\t\tdouble contrast;\n>> >> \t} gamma;\n>> >> +\tstruct {\n>> >> +\t\t/* 0..inf range, 1.0 = normal */\n>> >> +\t\tstd::optional contrast;\n>> >> +\t} knobs;\n>> >> };\n>> >> \n>> >> struct IPAFrameContext : public FrameContext {\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 46E70C3213\n\tfor ;\n\tWed, 27 Nov 2024 15:51:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E8FD660E2;\n\tWed, 27 Nov 2024 16:51:10 +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 4E9CB65FE0\n\tfor ;\n\tWed, 27 Nov 2024 16:51:08 +0100 (CET)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-468-zaY1d1WAMqmJ0A3tz_eJ6A-1; Wed, 27 Nov 2024 10:51:05 -0500","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-3822f550027so3417285f8f.1\n\tfor ;\n\tWed, 27 Nov 2024 07:51:04 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-3825fbc4123sm17016156f8f.79.2024.11.27.07.51.01\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 27 Nov 2024 07:51:02 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"D171YGPu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1732722667;\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=XGd5Dc4KOYYFSewBwfdZtEcSEzz4qSXufS4lF9Z7d5Q=;\n\tb=D171YGPu0PdALgTezAqMvEGOGnzHJlGJeQA+anlR2HP64xZywnJbzer+OvImmmz8YXZc9H\n\tf+IRVtVE4ih77RpSpKbYT6VIFtuSh7BVceST70+EwBDdDb9XdRVkRbsOann9ORWMAfPi38\n\t7GMXjJb6Fp+ijs/8AqljgHgT3Fm54mY=","X-MC-Unique":"zaY1d1WAMqmJ0A3tz_eJ6A-1","X-Mimecast-MFC-AGG-ID":"zaY1d1WAMqmJ0A3tz_eJ6A","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732722663; x=1733327463;\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=XGd5Dc4KOYYFSewBwfdZtEcSEzz4qSXufS4lF9Z7d5Q=;\n\tb=mTplXZ/oZ5h+58+YEN54/g/+OVRyyITXvN0H8qb9mCYr0za7woqjH9F4EYB497O7Rd\n\tUX6XVhNYmQFfCSWyiRxIk1g59H1iojPmWT0O1K7n44jZFQzL6msjvGmDWq4oNZhVYNUk\n\tp7101FBfPdtgmmgXA3BifsD0A6ZR3Wv/rHhnY9zy0BbNPiPFrBBqS67h2dYo/QJvD8Jp\n\tBNVjXkhCjjvmBHn7u4yJN/qkx+6K/qUDN5OZC1klqrVS9XkM3XUNf74bFdctf61ibz2J\n\tbIRz+2jLI316OzrH6VwCJ2KlpAeT8aMpdJiBcDolVYOaFDEtmhuyrQ0HialPho1nMjIx\n\tPqXg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXrjDdzkln/Yxfcz14IvAcOe7O836yQMKkvWmxpjeWdgS5RhHHIoOeuOJVH1R1uascKI8YbsLP04BVtzEkG0aM=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YynEcI4yZTDYYNlxlXCHbz5d0vlMU1t/Y2JZDWedEClJWr6NAeu\n\ti3iivYz1RXRRt6Zl63cGqamH6ar1K8E1SGDIIvVtZ4dCRuHBH+wBJEsA6b1dGmc7HWg20JF3bWh\n\ttTv0NY+DFQJoiq7l20an6IzH3ldMFjQDRD05OITS7kpPTq8D3nsKuOIit0Hp2nsGm3B8tNIvtcm\n\t3JOdM=","X-Gm-Gg":"ASbGncu2Q4SWF0M0iMJKdzdzz6IYblc9UuJXGDSOquu1wRKl6sZT94Qt/x+GN57Uoue\n\tfMOnrIlneZLWHaxFBnm/kC6aPkPebnWkQdDqqhQLGdsxBFW1ox6hYMRRKWMKjf9l4N3cH279czZ\n\tw9sKZy6RbcZv2wyymIDN3W0t153DEXE52a8wco7CruwY7bpU0dkyDsiT59jLD53KyUQA/Rai8ok\n\tsgc76sX2j2AGOJO9AuH1ZHCWHnIQkImKz4vGacMAQEpyeIs2W1mW6THCKLEVxm0StmqL0g=","X-Received":["by 2002:a5d:5888:0:b0:382:4378:4652 with SMTP id\n\tffacd0b85a97d-385c6edd47bmr2635318f8f.45.1732722663401; \n\tWed, 27 Nov 2024 07:51:03 -0800 (PST)","by 2002:a5d:5888:0:b0:382:4378:4652 with SMTP id\n\tffacd0b85a97d-385c6edd47bmr2635301f8f.45.1732722662967; \n\tWed, 27 Nov 2024 07:51:02 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IEKTp4FsBswzR8bPgDMkl6SDSJfYH7qVDvppAkI43mZaCl+XGDak0WorR9bEqyXKw1aM7OlbA==","From":"Milan Zamazal ","To":"Stefan Klug ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, Kieran Bingham\n\t","Subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for\n\tcontrast control","In-Reply-To":"\n\t(Stefan Klug's message of \"Wed, 27 Nov 2024 14:36:24 +0100\")","References":"<20241126091509.89677-1-mzamazal@redhat.com>\n\t<20241126091509.89677-4-mzamazal@redhat.com>\n\t<20241127071538.GT5461@pendragon.ideasonboard.com>\n\t<878qt5qafy.fsf@redhat.com>\n\t","Date":"Wed, 27 Nov 2024 16:51:01 +0100","Message-ID":"<871pywr7yy.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"4_C81KckjOAmLNR3-DlV8xCCXs0dxx4WVNsk7XsHg6k_1732722664","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\" "}},{"id":32417,"web_url":"https://patchwork.libcamera.org/comment/32417/","msgid":"<4xkxparew4yins2roqtpeptswkkzvvrnyik5fp5ixuaeiv3zxe@qttcxqfklmwg>","date":"2024-11-27T17:01:21","subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Milan,\n\nOn Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:\n> Hi Stefan,\n> \n> Stefan Klug writes:\n> \n> > Hi Milan, hi Laurent,\n> >\n> > On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:\n> >> Hi Laurent,\n> >> \n> >> Laurent Pinchart writes:\n> >> \n> >> > Hi Milan,\n> >> >\n> >> > Thank you for the patch.\n> >> >\n> >> > On Tue, Nov 26, 2024 at 10:15:04AM +0100, Milan Zamazal wrote:\n> >> >> Software ISP is currently fully automatic and doesn't allow image\n> >> >> modifications by explicitly set control values. The user has no means\n> >> >> to make the image looking better.\n> >> >> \n> >> >> This patch introduces support for contrast control, which can improve\n> >> >> e.g. a flat looking image. Based on the provided contrast value with a\n> >> >> range 0..infinity and 1.0 being the normal value, it applies a simple\n> >> >> S-curve modification to the image. The contrast algorithm just handles\n> >> >> the provided values, while the S-curve is applied in the gamma algorithm\n> >> >> on the computed gamma curve whenever the contrast value changes. Since\n> >> >> the algorithm is applied only on the lookup table already present, its\n> >> >> overhead is negligible.\n> >> >\n> >> > Isn't contrast defined as a multiplier ? Applying a different luminance\n> >> > transformation and calling it contrast will make different cameras\n> >> > behave in different ways.\n> >> \n> >> control_ids_core.yaml says:\n> >> \n> >> - Contrast:\n> >> type: float\n> >> description: |\n> >> Specify a fixed contrast parameter.\n> >> \n> >> Normal contrast is given by the value 1.0; larger values produce images\n> >> with more contrast.\n> >> \n> >> And V4L2:\n> >> \n> >> V4L2_CID_CONTRAST (integer)\n> >> Picture contrast or luma gain.\n> >> \n> >> So nothing specific. I don't know what hardware ISPs usually do with\n> >> this setting but simply multiplying with clipping (if this is what you\n> >> mean) wouldn't be very useful regarding image quality.\n> >\n> > I agree that a linear contrast curve wouldn't serve image quality\n> > purposes but is merely for scientific purposes. We could implement\n> > something like a contrast mode (linear|s-curve), but I believe an\n> > S-curve is the most useful one to the users.\n> \n> OK, let's stick with an S-curve for now.\n> \n> >> >> This is a preparation patch without actually providing the control\n> >> >> itself, which is done in the following patch.\n> >> >> \n> >> >> Signed-off-by: Milan Zamazal \n> >> >> ---\n> >> >> src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----\n> >> >> src/ipa/simple/algorithms/lut.h | 5 ++++\n> >> >> src/ipa/simple/ipa_context.h | 7 ++++++\n> >> >> 3 files changed, 45 insertions(+), 5 deletions(-)\n> >> >> \n> >> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> >> >> index 9744e773a..ffded0594 100644\n> >> >> --- a/src/ipa/simple/algorithms/lut.cpp\n> >> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n> >> >> @@ -9,14 +9,19 @@\n> >> >> \n> >> >> #include \n> >> >> #include \n> >> >> +#include \n> >> >> #include \n> >> >> \n> >> >> #include \n> >> >> \n> >> >> #include \"simple/ipa_context.h\"\n> >> >> \n> >> >> +#include \"control_ids.h\"\n> >> >> +\n> >> >> namespace libcamera {\n> >> >> \n> >> >> +LOG_DEFINE_CATEGORY(IPASoftLut)\n> >> >> +\n> >> >> namespace ipa::soft::algorithms {\n> >> >> \n> >> >> int Lut::configure(IPAContext &context,\n> >> >> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,\n> >> >> {\n> >> >> \t/* Gamma value is fixed */\n> >> >> \tcontext.configuration.gamma = 0.5;\n> >> >> +\tcontext.activeState.knobs.contrast = std::optional();\n> >> >> \tupdateGammaTable(context);\n> >> >> \n> >> >> \treturn 0;\n> >> >> }\n> >> >> \n> >> >> +void Lut::queueRequest(typename Module::Context &context,\n> >> >> +\t\t [[maybe_unused]] const uint32_t frame,\n> >> >> +\t\t [[maybe_unused]] typename Module::FrameContext &frameContext,\n> >> >> +\t\t const ControlList &controls)\n> >> >> +{\n> >> >> +\tconst auto &contrast = controls.get(controls::Contrast);\n> >> >> +\tif (contrast.has_value()) {\n> >> >> +\t\tcontext.activeState.knobs.contrast = contrast;\n> >> >> +\t\tLOG(IPASoftLut, Debug) << \"Setting contrast to \" << contrast.value();\n> >> >> +\t}\n> >> >> +}\n> >> >> +\n> >> >> void Lut::updateGammaTable(IPAContext &context)\n> >> >> {\n> >> >> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n> >> >> -\tauto blackLevel = context.activeState.blc.level;\n> >> >> +\tconst auto blackLevel = context.activeState.blc.level;\n> >> >> \tconst unsigned int blackIndex = blackLevel * gammaTable.size() / 256;\n> >> >> +\tconst auto contrast = context.activeState.knobs.contrast.value_or(1.0);\n> >> >> \n> >> >> \tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n> >> >> \tconst float divisor = gammaTable.size() - blackIndex - 1.0;\n> >> >> -\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++)\n> >> >> -\t\tgammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,\n> >> >> -\t\t\t\t\t\t context.configuration.gamma);\n> >> >> +\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++) {\n> >> >> +\t\tdouble normalized = (i - blackIndex) / divisor;\n> >> >> +\t\t/* Apply simple S-curve */\n> >> >> +\t\tif (normalized < 0.5)\n> >> >> +\t\t\tnormalized = 0.5 * std::pow(normalized / 0.5, contrast);\n> >> >> +\t\telse\n> >> >> +\t\t\tnormalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);\n> >\n> > One thing that bothers me on that curve is the asymmetry with regards to\n> > the contrast value. So a contrast value of 2 provides some contrast\n> > increase while a contrast value of 0 creates a completely flat response.\n> \n> Well, the fact that the counterpart of 2 is 1/2 is not exactly asymmetry\n> but it's true that it doesn't play well with linear sliders.\n> \n> > I believe a value range of -n to n would be easier to understand (0\n> > equals no change in contrast, -1 decrease, 1 increase) but that is a\n> > different topic.\n> \n> Yes, this would violate the requirement of 1.0 being a normal contrast.\n> \n> I can see that rpi uses [0, 32] range, while rkisp1 uses [0, 1.993], so\n> we have precedents for both the ways. But considering that rpi is\n> generally special and GUI sliders are usually linear, I'm in favor of\n> [0, 2] range as you suggest below.\n> \n> > I played around with the formula a bit:\n> > https://www.desmos.com/calculator/5zknbyjpln\n> >\n> > The red curve is the one from this patch, the green one is a symmetric\n> > version. \n> \n> The modified formula looks nice to linearize the scale, I can use it.\n> Thank you for this demonstration.\n> \n> > The steepness is something that might need attention as it goes to\n> > infinity for a contrast value of 2.\n> \n> I wonder whether something similar could be the reason why rkisp1 uses\n> the mysterious value 1.993 as the upper bound (the message of the commit\n> introducing it doesn't explain it). I'd prefer using 2 in my code for\n> clarity, perhaps with some internal check (g++/glibc++ seems to be fine\n> with tan(M_PI/2) but better to prevent possible portability issues).\n\nI don't exactly know what the internal formula of the rkisp1 is, but as\nthe image doesn't turn to the full extremes, I don't think a value of 2\nequals a vertical curve here. But maybe we just need to try it out and\ncompare them. Given the different hardware implementations I suspect it\nwill not be easy to come up with a scale that behaves similarly on each\nplatform. But we can try. The 1.993 is a simple hardware limitation.\nThey use a 1.7 fixed point format to represent the value. And with 7\nbits the biggest value you can represent after the dot is 127/128 ==\n0,99218\n\n> \n> > To my knowledge there is no \"official\" s-curve with a corresponding\n> > defined behavior of the contrast value. We might lookup how gimp does\n> > it.\n> \n> At a quick look, GIMP and photo editors (Darktable, Rawtherapee) seem to\n> transform contrast to curve points. GIMP computes\n> \n> slant = tan ((config->contrast + 1) * G_PI_4);\n\nOh funny. I didn't expect that :-)\n\nCheers,\nStefan\n\n> \n> (looks familiar, OK) that is then used to set some low and high output\n> points of the curve.\n> \n> In our much simpler case, I think we wouldn't be wrong with your\n> \"symmetric\" formula.\n> \n> > Best regards,\n> > Stefan\n> >\n> >> >> +\t\tgammaTable[i] = UINT8_MAX *\n> >> >> +\t\t\t\tstd::pow(normalized, context.configuration.gamma);\n> >> >> +\t}\n> >> >> \n> >> >> \tcontext.activeState.gamma.blackLevel = blackLevel;\n> >> >> +\tcontext.activeState.gamma.contrast = contrast;\n> >> >> }\n> >> >> \n> >> >> void Lut::prepare(IPAContext &context,\n> >> >> @@ -55,7 +82,8 @@ void Lut::prepare(IPAContext &context,\n> >> >> \t * observed, it's not permanently prone to minor fluctuations or\n> >> >> \t * rounding errors.\n> >> >> \t */\n> >> >> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n> >> >> +\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level ||\n> >> >> +\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n> >> >> \t\tupdateGammaTable(context);\n> >> >> \n> >> >> \tauto &gains = context.activeState.gains;\n> >> >> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n> >> >> index b635987d0..ef2df147c 100644\n> >> >> --- a/src/ipa/simple/algorithms/lut.h\n> >> >> +++ b/src/ipa/simple/algorithms/lut.h\n> >> >> @@ -20,6 +20,11 @@ public:\n> >> >> \t~Lut() = default;\n> >> >> \n> >> >> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> >> >> +\tvoid queueRequest(typename Module::Context &context,\n> >> >> +\t\t\t const uint32_t frame,\n> >> >> +\t\t\t typename Module::FrameContext &frameContext,\n> >> >> +\t\t\t const ControlList &controls)\n> >> >> +\t\toverride;\n> >> >> \tvoid prepare(IPAContext &context,\n> >> >> \t\t const uint32_t frame,\n> >> >> \t\t IPAFrameContext &frameContext,\n> >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> >> >> index fd7343e91..148052207 100644\n> >> >> --- a/src/ipa/simple/ipa_context.h\n> >> >> +++ b/src/ipa/simple/ipa_context.h\n> >> >> @@ -11,6 +11,8 @@\n> >> >> #include \n> >> >> #include \n> >> >> \n> >> >> +#include \n> >> >> +\n> >> >> #include \n> >> >> \n> >> >> namespace libcamera {\n> >> >> @@ -48,7 +50,12 @@ struct IPAActiveState {\n> >> >> \tstruct {\n> >> >> \t\tstd::array gammaTable;\n> >> >> \t\tuint8_t blackLevel;\n> >> >> +\t\tdouble contrast;\n> >> >> \t} gamma;\n> >> >> +\tstruct {\n> >> >> +\t\t/* 0..inf range, 1.0 = normal */\n> >> >> +\t\tstd::optional contrast;\n> >> >> +\t} knobs;\n> >> >> };\n> >> >> \n> >> >> struct IPAFrameContext : public FrameContext {\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 DDA22C3200\n\tfor ;\n\tWed, 27 Nov 2024 17:01:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 02C0A660E2;\n\tWed, 27 Nov 2024 18:01:26 +0100 (CET)","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 9087B660C4\n\tfor ;\n\tWed, 27 Nov 2024 18:01:24 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:cf8b:d681:2f3e:129c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 300A7842;\n\tWed, 27 Nov 2024 18:01:01 +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=\"q2CZ/E6t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732726861;\n\tbh=NINoNGL4VgNj6f/LliDARVzc5CNsmvwx3QzLDDnO5rA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=q2CZ/E6tyzXcj3fAbN3cLzImrv8F+Cxxe+nFz7wHgij2vBEiNZEc5dXtMzZ29VbGW\n\trIhw8Ok/7LAZcnygeUdC4giK2m18IZ59phQ5MboEbZRjZUZzBZPKXhmkqA3PCNDm72\n\tNcxLOf4oy9T3zzKYI+Kb1caj/p7PetKzRQaLMMI8=","Date":"Wed, 27 Nov 2024 18:01:21 +0100","From":"Stefan Klug ","To":"Milan Zamazal ","Cc":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","Message-ID":"<4xkxparew4yins2roqtpeptswkkzvvrnyik5fp5ixuaeiv3zxe@qttcxqfklmwg>","References":"<20241126091509.89677-1-mzamazal@redhat.com>\n\t<20241126091509.89677-4-mzamazal@redhat.com>\n\t<20241127071538.GT5461@pendragon.ideasonboard.com>\n\t<878qt5qafy.fsf@redhat.com>\n\t\n\t<871pywr7yy.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<871pywr7yy.fsf@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":32418,"web_url":"https://patchwork.libcamera.org/comment/32418/","msgid":"<87wmgopkbh.fsf@redhat.com>","date":"2024-11-27T19:07:14","subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for\n\tcontrast control","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Stefan,\n\nStefan Klug writes:\n\n> Hi Milan,\n>\n> On Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:\n>> Hi Stefan,\n>> \n>> Stefan Klug writes:\n>> \n>> > Hi Milan, hi Laurent,\n>> >\n>> > On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:\n>> >> Hi Laurent,\n>> >> \n>> >> Laurent Pinchart writes:\n>> >> \n>> >> > Hi Milan,\n>> >> >\n>> >> > Thank you for the patch.\n>> >> >\n>> >> > On Tue, Nov 26, 2024 at 10:15:04AM +0100, Milan Zamazal wrote:\n>> >> >> Software ISP is currently fully automatic and doesn't allow image\n>> >> >> modifications by explicitly set control values. The user has no means\n>> >> >> to make the image looking better.\n>> >> >> \n>> >> >> This patch introduces support for contrast control, which can improve\n>> >> >> e.g. a flat looking image. Based on the provided contrast value with a\n>> >> >> range 0..infinity and 1.0 being the normal value, it applies a simple\n>> >> >> S-curve modification to the image. The contrast algorithm just handles\n>> >> >> the provided values, while the S-curve is applied in the gamma algorithm\n>> >> >> on the computed gamma curve whenever the contrast value changes. Since\n>> >> >> the algorithm is applied only on the lookup table already present, its\n>> >> >> overhead is negligible.\n>> >> >\n>> >> > Isn't contrast defined as a multiplier ? Applying a different luminance\n>> >> > transformation and calling it contrast will make different cameras\n>> >> > behave in different ways.\n>> >> \n>> >> control_ids_core.yaml says:\n>> >> \n>> >> - Contrast:\n>> >> type: float\n>> >> description: |\n>> >> Specify a fixed contrast parameter.\n>> >> \n>> >> Normal contrast is given by the value 1.0; larger values produce images\n>> >> with more contrast.\n>> >> \n>> >> And V4L2:\n>> >> \n>> >> V4L2_CID_CONTRAST (integer)\n>> >> Picture contrast or luma gain.\n>> >> \n>> >> So nothing specific. I don't know what hardware ISPs usually do with\n>> >> this setting but simply multiplying with clipping (if this is what you\n>> >> mean) wouldn't be very useful regarding image quality.\n>> >\n>> > I agree that a linear contrast curve wouldn't serve image quality\n>> > purposes but is merely for scientific purposes. We could implement\n>> > something like a contrast mode (linear|s-curve), but I believe an\n>> > S-curve is the most useful one to the users.\n>> \n>> OK, let's stick with an S-curve for now.\n>> \n>> >> >> This is a preparation patch without actually providing the control\n>> >> >> itself, which is done in the following patch.\n>> >> >> \n>> >> >> Signed-off-by: Milan Zamazal \n>> >> >> ---\n>> >> >> src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----\n>> >> >> src/ipa/simple/algorithms/lut.h | 5 ++++\n>> >> >> src/ipa/simple/ipa_context.h | 7 ++++++\n>> >> >> 3 files changed, 45 insertions(+), 5 deletions(-)\n>> >> >> \n>> >> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n>> >> >> index 9744e773a..ffded0594 100644\n>> >> >> --- a/src/ipa/simple/algorithms/lut.cpp\n>> >> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n>> >> >> @@ -9,14 +9,19 @@\n>> >> >> \n>> >> >> #include \n>> >> >> #include \n>> >> >> +#include \n>> >> >> #include \n>> >> >> \n>> >> >> #include \n>> >> >> \n>> >> >> #include \"simple/ipa_context.h\"\n>> >> >> \n>> >> >> +#include \"control_ids.h\"\n>> >> >> +\n>> >> >> namespace libcamera {\n>> >> >> \n>> >> >> +LOG_DEFINE_CATEGORY(IPASoftLut)\n>> >> >> +\n>> >> >> namespace ipa::soft::algorithms {\n>> >> >> \n>> >> >> int Lut::configure(IPAContext &context,\n>> >> >> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,\n>> >> >> {\n>> >> >> \t/* Gamma value is fixed */\n>> >> >> \tcontext.configuration.gamma = 0.5;\n>> >> >> +\tcontext.activeState.knobs.contrast = std::optional();\n>> >> >> \tupdateGammaTable(context);\n>> >> >> \n>> >> >> \treturn 0;\n>> >> >> }\n>> >> >> \n>> >> >> +void Lut::queueRequest(typename Module::Context &context,\n>> >> >> +\t\t [[maybe_unused]] const uint32_t frame,\n>> >> >> +\t\t [[maybe_unused]] typename Module::FrameContext &frameContext,\n>> >> >> +\t\t const ControlList &controls)\n>> >> >> +{\n>> >> >> +\tconst auto &contrast = controls.get(controls::Contrast);\n>> >> >> +\tif (contrast.has_value()) {\n>> >> >> +\t\tcontext.activeState.knobs.contrast = contrast;\n>> >> >> +\t\tLOG(IPASoftLut, Debug) << \"Setting contrast to \" << contrast.value();\n>> >> >> +\t}\n>> >> >> +}\n>> >> >> +\n>> >> >> void Lut::updateGammaTable(IPAContext &context)\n>> >> >> {\n>> >> >> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n>> >> >> -\tauto blackLevel = context.activeState.blc.level;\n>> >> >> +\tconst auto blackLevel = context.activeState.blc.level;\n>> >> >> \tconst unsigned int blackIndex = blackLevel * gammaTable.size() / 256;\n>> >> >> +\tconst auto contrast = context.activeState.knobs.contrast.value_or(1.0);\n>> >> >> \n>> >> >> \tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n>> >> >> \tconst float divisor = gammaTable.size() - blackIndex - 1.0;\n>> >> >> -\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++)\n>> >> >> -\t\tgammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,\n>> >> >> -\t\t\t\t\t\t context.configuration.gamma);\n>> >> >> +\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++) {\n>> >> >> +\t\tdouble normalized = (i - blackIndex) / divisor;\n>> >> >> +\t\t/* Apply simple S-curve */\n>> >> >> +\t\tif (normalized < 0.5)\n>> >> >> +\t\t\tnormalized = 0.5 * std::pow(normalized / 0.5, contrast);\n>> >> >> +\t\telse\n>> >> >> +\t\t\tnormalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);\n>> >\n>> > One thing that bothers me on that curve is the asymmetry with regards to\n>> > the contrast value. So a contrast value of 2 provides some contrast\n>> > increase while a contrast value of 0 creates a completely flat response.\n>> \n>> Well, the fact that the counterpart of 2 is 1/2 is not exactly asymmetry\n>> but it's true that it doesn't play well with linear sliders.\n>> \n>> > I believe a value range of -n to n would be easier to understand (0\n>> > equals no change in contrast, -1 decrease, 1 increase) but that is a\n>> > different topic.\n>> \n>> Yes, this would violate the requirement of 1.0 being a normal contrast.\n>> \n>> I can see that rpi uses [0, 32] range, while rkisp1 uses [0, 1.993], so\n>> we have precedents for both the ways. But considering that rpi is\n>> generally special and GUI sliders are usually linear, I'm in favor of\n>> [0, 2] range as you suggest below.\n>> \n>> > I played around with the formula a bit:\n>> > https://www.desmos.com/calculator/5zknbyjpln\n>> >\n>> > The red curve is the one from this patch, the green one is a symmetric\n>> > version. \n>> \n>> The modified formula looks nice to linearize the scale, I can use it.\n>> Thank you for this demonstration.\n>> \n>> > The steepness is something that might need attention as it goes to\n>> > infinity for a contrast value of 2.\n>> \n>> I wonder whether something similar could be the reason why rkisp1 uses\n>> the mysterious value 1.993 as the upper bound (the message of the commit\n>> introducing it doesn't explain it). I'd prefer using 2 in my code for\n>> clarity, perhaps with some internal check (g++/glibc++ seems to be fine\n>> with tan(M_PI/2) but better to prevent possible portability issues).\n>\n> I don't exactly know what the internal formula of the rkisp1 is, but as\n> the image doesn't turn to the full extremes, I don't think a value of 2\n> equals a vertical curve here. But maybe we just need to try it out and\n> compare them. Given the different hardware implementations I suspect it\n> will not be easy to come up with a scale that behaves similarly on each\n> platform. \n\nNo problem, I don't think such a unification is necessary. I'm happy as\nlong as it behaves sanely in camshark. :-)\n\n> But we can try. The 1.993 is a simple hardware limitation. They use a\n> 1.7 fixed point format to represent the value. And with 7 bits the\n> biggest value you can represent after the dot is 127/128 == 0,99218\n\nI see, thank you for explanation.\n\n>> > To my knowledge there is no \"official\" s-curve with a corresponding\n>> > defined behavior of the contrast value. We might lookup how gimp does\n>> > it.\n>> \n>> At a quick look, GIMP and photo editors (Darktable, Rawtherapee) seem to\n>> transform contrast to curve points. GIMP computes\n>> \n>> slant = tan ((config->contrast + 1) * G_PI_4);\n>\n> Oh funny. I didn't expect that :-)\n>\n> Cheers,\n> Stefan\n>\n>> \n>> (looks familiar, OK) that is then used to set some low and high output\n>> points of the curve.\n>> \n>> In our much simpler case, I think we wouldn't be wrong with your\n>> \"symmetric\" formula.\n>> \n>> > Best regards,\n>> > Stefan\n>> >\n>> >> >> +\t\tgammaTable[i] = UINT8_MAX *\n>> >> >> +\t\t\t\tstd::pow(normalized, context.configuration.gamma);\n>> >> >> +\t}\n>> >> >> \n>> >> >> \tcontext.activeState.gamma.blackLevel = blackLevel;\n>> >> >> +\tcontext.activeState.gamma.contrast = contrast;\n>> >> >> }\n>> >> >> \n>> >> >> void Lut::prepare(IPAContext &context,\n>> >> >> @@ -55,7 +82,8 @@ void Lut::prepare(IPAContext &context,\n>> >> >> \t * observed, it's not permanently prone to minor fluctuations or\n>> >> >> \t * rounding errors.\n>> >> >> \t */\n>> >> >> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n>> >> >> +\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level ||\n>> >> >> +\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n>> >> >> \t\tupdateGammaTable(context);\n>> >> >> \n>> >> >> \tauto &gains = context.activeState.gains;\n>> >> >> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n>> >> >> index b635987d0..ef2df147c 100644\n>> >> >> --- a/src/ipa/simple/algorithms/lut.h\n>> >> >> +++ b/src/ipa/simple/algorithms/lut.h\n>> >> >> @@ -20,6 +20,11 @@ public:\n>> >> >> \t~Lut() = default;\n>> >> >> \n>> >> >> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>> >> >> +\tvoid queueRequest(typename Module::Context &context,\n>> >> >> +\t\t\t const uint32_t frame,\n>> >> >> +\t\t\t typename Module::FrameContext &frameContext,\n>> >> >> +\t\t\t const ControlList &controls)\n>> >> >> +\t\toverride;\n>> >> >> \tvoid prepare(IPAContext &context,\n>> >> >> \t\t const uint32_t frame,\n>> >> >> \t\t IPAFrameContext &frameContext,\n>> >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> >> >> index fd7343e91..148052207 100644\n>> >> >> --- a/src/ipa/simple/ipa_context.h\n>> >> >> +++ b/src/ipa/simple/ipa_context.h\n>> >> >> @@ -11,6 +11,8 @@\n>> >> >> #include \n>> >> >> #include \n>> >> >> \n>> >> >> +#include \n>> >> >> +\n>> >> >> #include \n>> >> >> \n>> >> >> namespace libcamera {\n>> >> >> @@ -48,7 +50,12 @@ struct IPAActiveState {\n>> >> >> \tstruct {\n>> >> >> \t\tstd::array gammaTable;\n>> >> >> \t\tuint8_t blackLevel;\n>> >> >> +\t\tdouble contrast;\n>> >> >> \t} gamma;\n>> >> >> +\tstruct {\n>> >> >> +\t\t/* 0..inf range, 1.0 = normal */\n>> >> >> +\t\tstd::optional contrast;\n>> >> >> +\t} knobs;\n>> >> >> };\n>> >> >> \n>> >> >> struct IPAFrameContext : public FrameContext {\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 29DF6C3226\n\tfor ;\n\tWed, 27 Nov 2024 19:07:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64CA2660C4;\n\tWed, 27 Nov 2024 20:07:23 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83034660C4\n\tfor ;\n\tWed, 27 Nov 2024 20:07:21 +0100 (CET)","from mail-ed1-f70.google.com (mail-ed1-f70.google.com\n\t[209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-343-Qjtv3A7ON02EYjeHWVn5rA-1; Wed, 27 Nov 2024 14:07:18 -0500","by mail-ed1-f70.google.com with SMTP id\n\t4fb4d7f45d1cf-5cfc26d02e6so1155352a12.0\n\tfor ;\n\tWed, 27 Nov 2024 11:07:18 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5d01d3a28fdsm6456080a12.8.2024.11.27.11.07.14\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 27 Nov 2024 11:07:15 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"OqDJ49ig\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1732734440;\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=vf0EHd/MpYoxvtt9XNAKWfHpaMozuLHfPZBxZrS9h1o=;\n\tb=OqDJ49ign6ydPbf6lg7ozoFe8Q4LWZnv4sSPugUrRLzbiUDwDqBW1x7M1dRxESE+zUk997\n\tdPqomhL+5aeQY22TGILcxE29HSA8ZXXW44b3Lsc5AUiIxUtQUM3V1aJqBCpnHrVcmY1F4T\n\tPSW/PtdQjrUR+VNQbhDPfkuQEmkFKyY=","X-MC-Unique":"Qjtv3A7ON02EYjeHWVn5rA-1","X-Mimecast-MFC-AGG-ID":"Qjtv3A7ON02EYjeHWVn5rA","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732734437; x=1733339237;\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=vf0EHd/MpYoxvtt9XNAKWfHpaMozuLHfPZBxZrS9h1o=;\n\tb=wy/bVf7YvtJ57RtnWbEsXpCMgJOqBOSXA9jyNRouvBVWFCBQSwKRR7yHmHpsSa/yqU\n\t+TrFfE77bWR/EDDvRKk/Kw6N3VgFTL71tCmBTri+q/OhLJuv3rpdYJk6cmsO2AlpCmuM\n\tMB5ZayzRW8Za3bML3wo8EvS92JnMh9HXExHXn+CWXsDaEMdD1E9C6y1rdSXY8HHoBDEN\n\tmKf4t6dwGe6G5HYyne7WQ77oQO5cfo+lVKRqrgLHzXZqrNOOE+ifyJyKzCQbo04dQ7dl\n\t+nLyWLzBkkInEb5xWhrwEdkVehbPFF6vw9aQHSiUlnZrJkAMcYPpAMePWa2Cc/XYmP29\n\twE5g==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWzontIjLwHrWbx56HbwFx83648XgLox1oVxtPrL5qPFz36+OPfFqNfDWRV3h+F/66BO1V68yE9m+ghhI1mNPE=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yy2R0RqmTbxQEIvKH4tsF7yBpG4oHkJkupZIj2ahs9Ofkh1vHgT\n\tD4mYETkaSdSlP9CRnX5PGJsZ/5STcSs6AESSjy94wU5QWZ2R4wJMcDfTjCLv9HNmo0leaoqxg6l\n\taAK7Eoz2wCdrt+P0t+43wVc6LExmA+4myl636pxYOhZjFSV3yPs1g3yNZYoajzJkF2f3zxJ/eVH\n\txYlX0=","X-Gm-Gg":"ASbGnctMVT/86N9sPyxIM+eqRf5TWAH2mNz1+HVGFzC0DFLZ0aJEnmFaUSRpMnznjvc\n\tERPVWrKMMbM28/dAyATygHLyUMzEuGwjYEV0SSnnMmV+WWxKAvqeoosTEqo2qxOXGq4zi2f93Ma\n\tCRQwmmJIoD0Ad+x0jTxsYc18yi4BWIPvAT5fvAO5PXwE34bGwgvFUch9TiXUjxrtFfWkMtiyfmj\n\tfoK+rYyOTOot7SmqkHh0zo1iMc3RGAHTJdgpGBioADa1qu8ZFyMiifQvpj8QplLw0/oPbg=","X-Received":["by 2002:a05:6402:3591:b0:5d0:6bbf:2da8 with SMTP id\n\t4fb4d7f45d1cf-5d0950d404fmr676652a12.2.1732734437205; \n\tWed, 27 Nov 2024 11:07:17 -0800 (PST)","by 2002:a05:6402:3591:b0:5d0:6bbf:2da8 with SMTP id\n\t4fb4d7f45d1cf-5d0950d404fmr676614a12.2.1732734436684; \n\tWed, 27 Nov 2024 11:07:16 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHifbnDb/tYNqPGqaUbUmFAoxsb1TG96wapT0vil8aH+EuCAGrfM76vKV7lEjc8M356fJsaew==","From":"Milan Zamazal ","To":"Stefan Klug ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, Kieran Bingham\n\t","Subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for\n\tcontrast control","In-Reply-To":"<4xkxparew4yins2roqtpeptswkkzvvrnyik5fp5ixuaeiv3zxe@qttcxqfklmwg>\n\t(Stefan Klug's message of \"Wed, 27 Nov 2024 18:01:21 +0100\")","References":"<20241126091509.89677-1-mzamazal@redhat.com>\n\t<20241126091509.89677-4-mzamazal@redhat.com>\n\t<20241127071538.GT5461@pendragon.ideasonboard.com>\n\t<878qt5qafy.fsf@redhat.com>\n\t\n\t<871pywr7yy.fsf@redhat.com>\n\t<4xkxparew4yins2roqtpeptswkkzvvrnyik5fp5ixuaeiv3zxe@qttcxqfklmwg>","Date":"Wed, 27 Nov 2024 20:07:14 +0100","Message-ID":"<87wmgopkbh.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"lDIE1QBEIY_O3YmWCI33erWmS5fSrudSgSS5Ai5AcSE_1732734438","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\" "}},{"id":32431,"web_url":"https://patchwork.libcamera.org/comment/32431/","msgid":"<20241128132211.GA25839@pendragon.ideasonboard.com>","date":"2024-11-28T13:22:11","subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"(CC'ing David and Naush)\n\nOn Wed, Nov 27, 2024 at 08:07:14PM +0100, Milan Zamazal wrote:\n> Stefan Klug writes:\n> > On Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:\n> >> Stefan Klug writes:\n> >> > On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:\n> >> >> Laurent Pinchart writes:\n> >> >> > On Tue, Nov 26, 2024 at 10:15:04AM +0100, Milan Zamazal wrote:\n> >> >> >> Software ISP is currently fully automatic and doesn't allow image\n> >> >> >> modifications by explicitly set control values. The user has no means\n> >> >> >> to make the image looking better.\n> >> >> >> \n> >> >> >> This patch introduces support for contrast control, which can improve\n> >> >> >> e.g. a flat looking image. Based on the provided contrast value with a\n> >> >> >> range 0..infinity and 1.0 being the normal value, it applies a simple\n> >> >> >> S-curve modification to the image. The contrast algorithm just handles\n> >> >> >> the provided values, while the S-curve is applied in the gamma algorithm\n> >> >> >> on the computed gamma curve whenever the contrast value changes. Since\n> >> >> >> the algorithm is applied only on the lookup table already present, its\n> >> >> >> overhead is negligible.\n> >> >> >\n> >> >> > Isn't contrast defined as a multiplier ? Applying a different luminance\n> >> >> > transformation and calling it contrast will make different cameras\n> >> >> > behave in different ways.\n> >> >> \n> >> >> control_ids_core.yaml says:\n> >> >> \n> >> >> - Contrast:\n> >> >> type: float\n> >> >> description: |\n> >> >> Specify a fixed contrast parameter.\n> >> >> \n> >> >> Normal contrast is given by the value 1.0; larger values produce images\n> >> >> with more contrast.\n> >> >> \n> >> >> And V4L2:\n> >> >> \n> >> >> V4L2_CID_CONTRAST (integer)\n> >> >> Picture contrast or luma gain.\n> >> >> \n> >> >> So nothing specific. I don't know what hardware ISPs usually do with\n> >> >> this setting but simply multiplying with clipping (if this is what you\n> >> >> mean) wouldn't be very useful regarding image quality.\n> >> >\n> >> > I agree that a linear contrast curve wouldn't serve image quality\n> >> > purposes but is merely for scientific purposes. We could implement\n> >> > something like a contrast mode (linear|s-curve), but I believe an\n> >> > S-curve is the most useful one to the users.\n> >> \n> >> OK, let's stick with an S-curve for now.\n> >> \n> >> >> >> This is a preparation patch without actually providing the control\n> >> >> >> itself, which is done in the following patch.\n> >> >> >> \n> >> >> >> Signed-off-by: Milan Zamazal \n> >> >> >> ---\n> >> >> >> src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----\n> >> >> >> src/ipa/simple/algorithms/lut.h | 5 ++++\n> >> >> >> src/ipa/simple/ipa_context.h | 7 ++++++\n> >> >> >> 3 files changed, 45 insertions(+), 5 deletions(-)\n> >> >> >> \n> >> >> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> >> >> >> index 9744e773a..ffded0594 100644\n> >> >> >> --- a/src/ipa/simple/algorithms/lut.cpp\n> >> >> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n> >> >> >> @@ -9,14 +9,19 @@\n> >> >> >> \n> >> >> >> #include \n> >> >> >> #include \n> >> >> >> +#include \n> >> >> >> #include \n> >> >> >> \n> >> >> >> #include \n> >> >> >> \n> >> >> >> #include \"simple/ipa_context.h\"\n> >> >> >> \n> >> >> >> +#include \"control_ids.h\"\n> >> >> >> +\n> >> >> >> namespace libcamera {\n> >> >> >> \n> >> >> >> +LOG_DEFINE_CATEGORY(IPASoftLut)\n> >> >> >> +\n> >> >> >> namespace ipa::soft::algorithms {\n> >> >> >> \n> >> >> >> int Lut::configure(IPAContext &context,\n> >> >> >> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,\n> >> >> >> {\n> >> >> >> \t/* Gamma value is fixed */\n> >> >> >> \tcontext.configuration.gamma = 0.5;\n> >> >> >> +\tcontext.activeState.knobs.contrast = std::optional();\n> >> >> >> \tupdateGammaTable(context);\n> >> >> >> \n> >> >> >> \treturn 0;\n> >> >> >> }\n> >> >> >> \n> >> >> >> +void Lut::queueRequest(typename Module::Context &context,\n> >> >> >> +\t\t [[maybe_unused]] const uint32_t frame,\n> >> >> >> +\t\t [[maybe_unused]] typename Module::FrameContext &frameContext,\n> >> >> >> +\t\t const ControlList &controls)\n> >> >> >> +{\n> >> >> >> +\tconst auto &contrast = controls.get(controls::Contrast);\n> >> >> >> +\tif (contrast.has_value()) {\n> >> >> >> +\t\tcontext.activeState.knobs.contrast = contrast;\n> >> >> >> +\t\tLOG(IPASoftLut, Debug) << \"Setting contrast to \" << contrast.value();\n> >> >> >> +\t}\n> >> >> >> +}\n> >> >> >> +\n> >> >> >> void Lut::updateGammaTable(IPAContext &context)\n> >> >> >> {\n> >> >> >> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n> >> >> >> -\tauto blackLevel = context.activeState.blc.level;\n> >> >> >> +\tconst auto blackLevel = context.activeState.blc.level;\n> >> >> >> \tconst unsigned int blackIndex = blackLevel * gammaTable.size() / 256;\n> >> >> >> +\tconst auto contrast = context.activeState.knobs.contrast.value_or(1.0);\n> >> >> >> \n> >> >> >> \tstd::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n> >> >> >> \tconst float divisor = gammaTable.size() - blackIndex - 1.0;\n> >> >> >> -\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++)\n> >> >> >> -\t\tgammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,\n> >> >> >> -\t\t\t\t\t\t context.configuration.gamma);\n> >> >> >> +\tfor (unsigned int i = blackIndex; i < gammaTable.size(); i++) {\n> >> >> >> +\t\tdouble normalized = (i - blackIndex) / divisor;\n> >> >> >> +\t\t/* Apply simple S-curve */\n> >> >> >> +\t\tif (normalized < 0.5)\n> >> >> >> +\t\t\tnormalized = 0.5 * std::pow(normalized / 0.5, contrast);\n> >> >> >> +\t\telse\n> >> >> >> +\t\t\tnormalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);\n> >> >\n> >> > One thing that bothers me on that curve is the asymmetry with regards to\n> >> > the contrast value. So a contrast value of 2 provides some contrast\n> >> > increase while a contrast value of 0 creates a completely flat response.\n> >> \n> >> Well, the fact that the counterpart of 2 is 1/2 is not exactly asymmetry\n> >> but it's true that it doesn't play well with linear sliders.\n> >> \n> >> > I believe a value range of -n to n would be easier to understand (0\n> >> > equals no change in contrast, -1 decrease, 1 increase) but that is a\n> >> > different topic.\n> >> \n> >> Yes, this would violate the requirement of 1.0 being a normal contrast.\n> >> \n> >> I can see that rpi uses [0, 32] range, while rkisp1 uses [0, 1.993], so\n> >> we have precedents for both the ways. But considering that rpi is\n> >> generally special and GUI sliders are usually linear, I'm in favor of\n> >> [0, 2] range as you suggest below.\n\nDavid, Naush, how does this work for Raspberry Pi ? As far as I can see,\ncontrast is applied through the gamma LUT, and on both the Pi 4 and Pi 5\nit's a linear multiplier centered around the middle of the range. Is\nthat right ? Have you considered other types of contrast adjustments,\nlike an S curve as done in this patch ?\n\n> >> > I played around with the formula a bit:\n> >> > https://www.desmos.com/calculator/5zknbyjpln\n> >> >\n> >> > The red curve is the one from this patch, the green one is a symmetric\n> >> > version. \n> >> \n> >> The modified formula looks nice to linearize the scale, I can use it.\n> >> Thank you for this demonstration.\n> >> \n> >> > The steepness is something that might need attention as it goes to\n> >> > infinity for a contrast value of 2.\n> >> \n> >> I wonder whether something similar could be the reason why rkisp1 uses\n> >> the mysterious value 1.993 as the upper bound (the message of the commit\n> >> introducing it doesn't explain it). I'd prefer using 2 in my code for\n\nThat's just the range of the hardware contrast register, which stores a\nQ1.7 value. 0x00 maps to 0.0, 0x80 to 1.0, and Oxff to 1.9921875\n(255/128).\n\n> >> clarity, perhaps with some internal check (g++/glibc++ seems to be fine\n> >> with tan(M_PI/2) but better to prevent possible portability issues).\n> >\n> > I don't exactly know what the internal formula of the rkisp1 is, but as\n> > the image doesn't turn to the full extremes, I don't think a value of 2\n> > equals a vertical curve here. But maybe we just need to try it out and\n\nAs far as I know, the hardware uses this value as a linear multiplier\nfor the luma component (contrast is applied in YUV space).\n\n> > compare them. Given the different hardware implementations I suspect it\n> > will not be easy to come up with a scale that behaves similarly on each\n> > platform. \n> \n> No problem, I don't think such a unification is necessary. I'm happy as\n> long as it behaves sanely in camshark. :-)\n> \n> > But we can try. The 1.993 is a simple hardware limitation. They use a\n> > 1.7 fixed point format to represent the value. And with 7 bits the\n> > biggest value you can represent after the dot is 127/128 == 0,99218\n\nI should have read the full e-mail before writing the above :-)\n\n> I see, thank you for explanation.\n> \n> >> > To my knowledge there is no \"official\" s-curve with a corresponding\n> >> > defined behavior of the contrast value. We might lookup how gimp does\n> >> > it.\n> >> \n> >> At a quick look, GIMP and photo editors (Darktable, Rawtherapee) seem to\n> >> transform contrast to curve points. GIMP computes\n> >> \n> >> slant = tan ((config->contrast + 1) * G_PI_4);\n> >\n> > Oh funny. I didn't expect that :-)\n> >\n> >> (looks familiar, OK) that is then used to set some low and high output\n> >> points of the curve.\n> >> \n> >> In our much simpler case, I think we wouldn't be wrong with your\n> >> \"symmetric\" formula.\n> >> \n> >> >> >> +\t\tgammaTable[i] = UINT8_MAX *\n> >> >> >> +\t\t\t\tstd::pow(normalized, context.configuration.gamma);\n> >> >> >> +\t}\n> >> >> >> \n> >> >> >> \tcontext.activeState.gamma.blackLevel = blackLevel;\n> >> >> >> +\tcontext.activeState.gamma.contrast = contrast;\n> >> >> >> }\n> >> >> >> \n> >> >> >> void Lut::prepare(IPAContext &context,\n> >> >> >> @@ -55,7 +82,8 @@ void Lut::prepare(IPAContext &context,\n> >> >> >> \t * observed, it's not permanently prone to minor fluctuations or\n> >> >> >> \t * rounding errors.\n> >> >> >> \t */\n> >> >> >> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n> >> >> >> +\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level ||\n> >> >> >> +\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n> >> >> >> \t\tupdateGammaTable(context);\n> >> >> >> \n> >> >> >> \tauto &gains = context.activeState.gains;\n> >> >> >> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n> >> >> >> index b635987d0..ef2df147c 100644\n> >> >> >> --- a/src/ipa/simple/algorithms/lut.h\n> >> >> >> +++ b/src/ipa/simple/algorithms/lut.h\n> >> >> >> @@ -20,6 +20,11 @@ public:\n> >> >> >> \t~Lut() = default;\n> >> >> >> \n> >> >> >> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> >> >> >> +\tvoid queueRequest(typename Module::Context &context,\n> >> >> >> +\t\t\t const uint32_t frame,\n> >> >> >> +\t\t\t typename Module::FrameContext &frameContext,\n> >> >> >> +\t\t\t const ControlList &controls)\n> >> >> >> +\t\toverride;\n> >> >> >> \tvoid prepare(IPAContext &context,\n> >> >> >> \t\t const uint32_t frame,\n> >> >> >> \t\t IPAFrameContext &frameContext,\n> >> >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> >> >> >> index fd7343e91..148052207 100644\n> >> >> >> --- a/src/ipa/simple/ipa_context.h\n> >> >> >> +++ b/src/ipa/simple/ipa_context.h\n> >> >> >> @@ -11,6 +11,8 @@\n> >> >> >> #include \n> >> >> >> #include \n> >> >> >> \n> >> >> >> +#include \n> >> >> >> +\n> >> >> >> #include \n> >> >> >> \n> >> >> >> namespace libcamera {\n> >> >> >> @@ -48,7 +50,12 @@ struct IPAActiveState {\n> >> >> >> \tstruct {\n> >> >> >> \t\tstd::array gammaTable;\n> >> >> >> \t\tuint8_t blackLevel;\n> >> >> >> +\t\tdouble contrast;\n> >> >> >> \t} gamma;\n> >> >> >> +\tstruct {\n> >> >> >> +\t\t/* 0..inf range, 1.0 = normal */\n> >> >> >> +\t\tstd::optional contrast;\n> >> >> >> +\t} knobs;\n> >> >> >> };\n> >> >> >> \n> >> >> >> struct IPAFrameContext : public FrameContext {","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 AB218BDE6B\n\tfor ;\n\tThu, 28 Nov 2024 13:22:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4406265FA2;\n\tThu, 28 Nov 2024 14:22:23 +0100 (CET)","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 8504665898\n\tfor ;\n\tThu, 28 Nov 2024 14:22:21 +0100 (CET)","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 7CB7E526;\n\tThu, 28 Nov 2024 14:21:57 +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=\"WISIx72o\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732800117;\n\tbh=8bgqzwuKgwRMt+lYbxHcDbk1QycpfYdu56/H2/AioeQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WISIx72odh+Hcu6tC+QQNVIzrTQU7xpJlOXMaKxxWLB6+nJPgxQ3DKwyBm1Ozn9bw\n\taluqsfmYYwDQ/e+LBzkQBGREPYvMgNXqcdeeegxp9A567fkZaiThk66j2z/S5MWKB+\n\tte7bJ4H50rHc38J64GPeyMBKUszAIvkjqr5C6A9o=","Date":"Thu, 28 Nov 2024 15:22:11 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tDavid Plowman ","Subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","Message-ID":"<20241128132211.GA25839@pendragon.ideasonboard.com>","References":"<20241126091509.89677-1-mzamazal@redhat.com>\n\t<20241126091509.89677-4-mzamazal@redhat.com>\n\t<20241127071538.GT5461@pendragon.ideasonboard.com>\n\t<878qt5qafy.fsf@redhat.com>\n\t\n\t<871pywr7yy.fsf@redhat.com>\n\t<4xkxparew4yins2roqtpeptswkkzvvrnyik5fp5ixuaeiv3zxe@qttcxqfklmwg>\n\t<87wmgopkbh.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87wmgopkbh.fsf@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":32433,"web_url":"https://patchwork.libcamera.org/comment/32433/","msgid":"","date":"2024-11-28T13:47:20","subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nOn Thu, 28 Nov 2024 at 13:22, Laurent Pinchart\n wrote:\n>\n> (CC'ing David and Naush)\n>\n> On Wed, Nov 27, 2024 at 08:07:14PM +0100, Milan Zamazal wrote:\n> > Stefan Klug writes:\n> > > On Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:\n> > >> Stefan Klug writes:\n> > >> > On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:\n> > >> >> Laurent Pinchart writes:\n> > >> >> > On Tue, Nov 26, 2024 at 10:15:04AM +0100, Milan Zamazal wrote:\n> > >> >> >> Software ISP is currently fully automatic and doesn't allow image\n> > >> >> >> modifications by explicitly set control values. The user has no means\n> > >> >> >> to make the image looking better.\n> > >> >> >>\n> > >> >> >> This patch introduces support for contrast control, which can improve\n> > >> >> >> e.g. a flat looking image. Based on the provided contrast value with a\n> > >> >> >> range 0..infinity and 1.0 being the normal value, it applies a simple\n> > >> >> >> S-curve modification to the image. The contrast algorithm just handles\n> > >> >> >> the provided values, while the S-curve is applied in the gamma algorithm\n> > >> >> >> on the computed gamma curve whenever the contrast value changes. Since\n> > >> >> >> the algorithm is applied only on the lookup table already present, its\n> > >> >> >> overhead is negligible.\n> > >> >> >\n> > >> >> > Isn't contrast defined as a multiplier ? Applying a different luminance\n> > >> >> > transformation and calling it contrast will make different cameras\n> > >> >> > behave in different ways.\n> > >> >>\n> > >> >> control_ids_core.yaml says:\n> > >> >>\n> > >> >> - Contrast:\n> > >> >> type: float\n> > >> >> description: |\n> > >> >> Specify a fixed contrast parameter.\n> > >> >>\n> > >> >> Normal contrast is given by the value 1.0; larger values produce images\n> > >> >> with more contrast.\n> > >> >>\n> > >> >> And V4L2:\n> > >> >>\n> > >> >> V4L2_CID_CONTRAST (integer)\n> > >> >> Picture contrast or luma gain.\n> > >> >>\n> > >> >> So nothing specific. I don't know what hardware ISPs usually do with\n> > >> >> this setting but simply multiplying with clipping (if this is what you\n> > >> >> mean) wouldn't be very useful regarding image quality.\n> > >> >\n> > >> > I agree that a linear contrast curve wouldn't serve image quality\n> > >> > purposes but is merely for scientific purposes. We could implement\n> > >> > something like a contrast mode (linear|s-curve), but I believe an\n> > >> > S-curve is the most useful one to the users.\n> > >>\n> > >> OK, let's stick with an S-curve for now.\n> > >>\n> > >> >> >> This is a preparation patch without actually providing the control\n> > >> >> >> itself, which is done in the following patch.\n> > >> >> >>\n> > >> >> >> Signed-off-by: Milan Zamazal \n> > >> >> >> ---\n> > >> >> >> src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----\n> > >> >> >> src/ipa/simple/algorithms/lut.h | 5 ++++\n> > >> >> >> src/ipa/simple/ipa_context.h | 7 ++++++\n> > >> >> >> 3 files changed, 45 insertions(+), 5 deletions(-)\n> > >> >> >>\n> > >> >> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> > >> >> >> index 9744e773a..ffded0594 100644\n> > >> >> >> --- a/src/ipa/simple/algorithms/lut.cpp\n> > >> >> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n> > >> >> >> @@ -9,14 +9,19 @@\n> > >> >> >>\n> > >> >> >> #include \n> > >> >> >> #include \n> > >> >> >> +#include \n> > >> >> >> #include \n> > >> >> >>\n> > >> >> >> #include \n> > >> >> >>\n> > >> >> >> #include \"simple/ipa_context.h\"\n> > >> >> >>\n> > >> >> >> +#include \"control_ids.h\"\n> > >> >> >> +\n> > >> >> >> namespace libcamera {\n> > >> >> >>\n> > >> >> >> +LOG_DEFINE_CATEGORY(IPASoftLut)\n> > >> >> >> +\n> > >> >> >> namespace ipa::soft::algorithms {\n> > >> >> >>\n> > >> >> >> int Lut::configure(IPAContext &context,\n> > >> >> >> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,\n> > >> >> >> {\n> > >> >> >> /* Gamma value is fixed */\n> > >> >> >> context.configuration.gamma = 0.5;\n> > >> >> >> + context.activeState.knobs.contrast = std::optional();\n> > >> >> >> updateGammaTable(context);\n> > >> >> >>\n> > >> >> >> return 0;\n> > >> >> >> }\n> > >> >> >>\n> > >> >> >> +void Lut::queueRequest(typename Module::Context &context,\n> > >> >> >> + [[maybe_unused]] const uint32_t frame,\n> > >> >> >> + [[maybe_unused]] typename Module::FrameContext &frameContext,\n> > >> >> >> + const ControlList &controls)\n> > >> >> >> +{\n> > >> >> >> + const auto &contrast = controls.get(controls::Contrast);\n> > >> >> >> + if (contrast.has_value()) {\n> > >> >> >> + context.activeState.knobs.contrast = contrast;\n> > >> >> >> + LOG(IPASoftLut, Debug) << \"Setting contrast to \" << contrast.value();\n> > >> >> >> + }\n> > >> >> >> +}\n> > >> >> >> +\n> > >> >> >> void Lut::updateGammaTable(IPAContext &context)\n> > >> >> >> {\n> > >> >> >> auto &gammaTable = context.activeState.gamma.gammaTable;\n> > >> >> >> - auto blackLevel = context.activeState.blc.level;\n> > >> >> >> + const auto blackLevel = context.activeState.blc.level;\n> > >> >> >> const unsigned int blackIndex = blackLevel * gammaTable.size() / 256;\n> > >> >> >> + const auto contrast = context.activeState.knobs.contrast.value_or(1.0);\n> > >> >> >>\n> > >> >> >> std::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);\n> > >> >> >> const float divisor = gammaTable.size() - blackIndex - 1.0;\n> > >> >> >> - for (unsigned int i = blackIndex; i < gammaTable.size(); i++)\n> > >> >> >> - gammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,\n> > >> >> >> - context.configuration.gamma);\n> > >> >> >> + for (unsigned int i = blackIndex; i < gammaTable.size(); i++) {\n> > >> >> >> + double normalized = (i - blackIndex) / divisor;\n> > >> >> >> + /* Apply simple S-curve */\n> > >> >> >> + if (normalized < 0.5)\n> > >> >> >> + normalized = 0.5 * std::pow(normalized / 0.5, contrast);\n> > >> >> >> + else\n> > >> >> >> + normalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);\n> > >> >\n> > >> > One thing that bothers me on that curve is the asymmetry with regards to\n> > >> > the contrast value. So a contrast value of 2 provides some contrast\n> > >> > increase while a contrast value of 0 creates a completely flat response.\n> > >>\n> > >> Well, the fact that the counterpart of 2 is 1/2 is not exactly asymmetry\n> > >> but it's true that it doesn't play well with linear sliders.\n> > >>\n> > >> > I believe a value range of -n to n would be easier to understand (0\n> > >> > equals no change in contrast, -1 decrease, 1 increase) but that is a\n> > >> > different topic.\n> > >>\n> > >> Yes, this would violate the requirement of 1.0 being a normal contrast.\n> > >>\n> > >> I can see that rpi uses [0, 32] range, while rkisp1 uses [0, 1.993], so\n> > >> we have precedents for both the ways. But considering that rpi is\n> > >> generally special and GUI sliders are usually linear, I'm in favor of\n> > >> [0, 2] range as you suggest below.\n>\n> David, Naush, how does this work for Raspberry Pi ? As far as I can see,\n> contrast is applied through the gamma LUT, and on both the Pi 4 and Pi 5\n> it's a linear multiplier centered around the middle of the range. Is\n> that right ? Have you considered other types of contrast adjustments,\n> like an S curve as done in this patch ?\n\nYes, that's what it does. Historically, it's what we've always done,\nand none of our users seem to ask about it. To be honest, I've always\nfelt that this control is only there because people \"expect\" it, it's\nalmost never the right thing to use.\n\nFor example, if you're changing the values at runtime to try and\nimprove the images, well, then the tuning/control algorithms should\nprobably be working better. If you're always applying the same values\nbecause that seems better, well, then the tuning is probably still\nwrong!!\n\nSo I don't think we really see any need to revisit this for the moment.\n\nDavid\n\n>\n> > >> > I played around with the formula a bit:\n> > >> > https://www.desmos.com/calculator/5zknbyjpln\n> > >> >\n> > >> > The red curve is the one from this patch, the green one is a symmetric\n> > >> > version.\n> > >>\n> > >> The modified formula looks nice to linearize the scale, I can use it.\n> > >> Thank you for this demonstration.\n> > >>\n> > >> > The steepness is something that might need attention as it goes to\n> > >> > infinity for a contrast value of 2.\n> > >>\n> > >> I wonder whether something similar could be the reason why rkisp1 uses\n> > >> the mysterious value 1.993 as the upper bound (the message of the commit\n> > >> introducing it doesn't explain it). I'd prefer using 2 in my code for\n>\n> That's just the range of the hardware contrast register, which stores a\n> Q1.7 value. 0x00 maps to 0.0, 0x80 to 1.0, and Oxff to 1.9921875\n> (255/128).\n>\n> > >> clarity, perhaps with some internal check (g++/glibc++ seems to be fine\n> > >> with tan(M_PI/2) but better to prevent possible portability issues).\n> > >\n> > > I don't exactly know what the internal formula of the rkisp1 is, but as\n> > > the image doesn't turn to the full extremes, I don't think a value of 2\n> > > equals a vertical curve here. But maybe we just need to try it out and\n>\n> As far as I know, the hardware uses this value as a linear multiplier\n> for the luma component (contrast is applied in YUV space).\n>\n> > > compare them. Given the different hardware implementations I suspect it\n> > > will not be easy to come up with a scale that behaves similarly on each\n> > > platform.\n> >\n> > No problem, I don't think such a unification is necessary. I'm happy as\n> > long as it behaves sanely in camshark. :-)\n> >\n> > > But we can try. The 1.993 is a simple hardware limitation. They use a\n> > > 1.7 fixed point format to represent the value. And with 7 bits the\n> > > biggest value you can represent after the dot is 127/128 == 0,99218\n>\n> I should have read the full e-mail before writing the above :-)\n>\n> > I see, thank you for explanation.\n> >\n> > >> > To my knowledge there is no \"official\" s-curve with a corresponding\n> > >> > defined behavior of the contrast value. We might lookup how gimp does\n> > >> > it.\n> > >>\n> > >> At a quick look, GIMP and photo editors (Darktable, Rawtherapee) seem to\n> > >> transform contrast to curve points. GIMP computes\n> > >>\n> > >> slant = tan ((config->contrast + 1) * G_PI_4);\n> > >\n> > > Oh funny. I didn't expect that :-)\n> > >\n> > >> (looks familiar, OK) that is then used to set some low and high output\n> > >> points of the curve.\n> > >>\n> > >> In our much simpler case, I think we wouldn't be wrong with your\n> > >> \"symmetric\" formula.\n> > >>\n> > >> >> >> + gammaTable[i] = UINT8_MAX *\n> > >> >> >> + std::pow(normalized, context.configuration.gamma);\n> > >> >> >> + }\n> > >> >> >>\n> > >> >> >> context.activeState.gamma.blackLevel = blackLevel;\n> > >> >> >> + context.activeState.gamma.contrast = contrast;\n> > >> >> >> }\n> > >> >> >>\n> > >> >> >> void Lut::prepare(IPAContext &context,\n> > >> >> >> @@ -55,7 +82,8 @@ void Lut::prepare(IPAContext &context,\n> > >> >> >> * observed, it's not permanently prone to minor fluctuations or\n> > >> >> >> * rounding errors.\n> > >> >> >> */\n> > >> >> >> - if (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n> > >> >> >> + if (context.activeState.gamma.blackLevel != context.activeState.blc.level ||\n> > >> >> >> + context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n> > >> >> >> updateGammaTable(context);\n> > >> >> >>\n> > >> >> >> auto &gains = context.activeState.gains;\n> > >> >> >> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n> > >> >> >> index b635987d0..ef2df147c 100644\n> > >> >> >> --- a/src/ipa/simple/algorithms/lut.h\n> > >> >> >> +++ b/src/ipa/simple/algorithms/lut.h\n> > >> >> >> @@ -20,6 +20,11 @@ public:\n> > >> >> >> ~Lut() = default;\n> > >> >> >>\n> > >> >> >> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> > >> >> >> + void queueRequest(typename Module::Context &context,\n> > >> >> >> + const uint32_t frame,\n> > >> >> >> + typename Module::FrameContext &frameContext,\n> > >> >> >> + const ControlList &controls)\n> > >> >> >> + override;\n> > >> >> >> void prepare(IPAContext &context,\n> > >> >> >> const uint32_t frame,\n> > >> >> >> IPAFrameContext &frameContext,\n> > >> >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> > >> >> >> index fd7343e91..148052207 100644\n> > >> >> >> --- a/src/ipa/simple/ipa_context.h\n> > >> >> >> +++ b/src/ipa/simple/ipa_context.h\n> > >> >> >> @@ -11,6 +11,8 @@\n> > >> >> >> #include \n> > >> >> >> #include \n> > >> >> >>\n> > >> >> >> +#include \n> > >> >> >> +\n> > >> >> >> #include \n> > >> >> >>\n> > >> >> >> namespace libcamera {\n> > >> >> >> @@ -48,7 +50,12 @@ struct IPAActiveState {\n> > >> >> >> struct {\n> > >> >> >> std::array gammaTable;\n> > >> >> >> uint8_t blackLevel;\n> > >> >> >> + double contrast;\n> > >> >> >> } gamma;\n> > >> >> >> + struct {\n> > >> >> >> + /* 0..inf range, 1.0 = normal */\n> > >> >> >> + std::optional contrast;\n> > >> >> >> + } knobs;\n> > >> >> >> };\n> > >> >> >>\n> > >> >> >> struct IPAFrameContext : public FrameContext {\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 7D63DBDE6B\n\tfor ;\n\tThu, 28 Nov 2024 13:47:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76F6765FA5;\n\tThu, 28 Nov 2024 14:47:35 +0100 (CET)","from mail-qv1-xf33.google.com (mail-qv1-xf33.google.com\n\t[IPv6:2607:f8b0:4864:20::f33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1A6B65898\n\tfor ;\n\tThu, 28 Nov 2024 14:47:32 +0100 (CET)","by mail-qv1-xf33.google.com with SMTP id\n\t6a1803df08f44-6d87d82622fso1348406d6.0\n\tfor ;\n\tThu, 28 Nov 2024 05:47:32 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"FolYuidx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1732801651; x=1733406451;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=EzS8ldKX0VwGRH5hA+CoDQyeqO5qYZjmQPpKkPBW4hc=;\n\tb=FolYuidxg8BYxQER6pJj/BPdRQZzPFxqMFBcE2TRKCgljsTZUQsPjrbBRktHFMesuK\n\teZmFjgvs0X2bgTWTGCFSrV+YxzrWrNYvi/L5+TbT/QXNQ4q5EOdvflvTYotbmLC8NROR\n\tQnIqWtXBM+6382ApGNEwiiypyDsqPToO+WUO07J09YuqdSeEkBRqftWZejRuhfBDIbK4\n\tz6C0Mm1xtwNKvcRV9CllU/kR4jqkoMnno8wxL+aX6MNURZhDNRODmzkAyuxNWNFfSz4J\n\tmDskSc2eqO6FBic7L8ZqAlZ08kfLo6Gy6adBtjUhLfCytYkVw/olUO4Q0KHIPcwvze3x\n\tGyWQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732801651; x=1733406451;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=EzS8ldKX0VwGRH5hA+CoDQyeqO5qYZjmQPpKkPBW4hc=;\n\tb=uoA9F1YdlTcAa8MMb+x9GMNvcwDxzy4Lf3+0mP5sswKEPVe8+vfVfKK8s+8qwD6Op7\n\tvaT8z/T0P72V9q4dr6xB6T7ysxV1H/g+IchlD6X3Eu7ivymTSHV3LevdH8FPHXHd+Plu\n\tE/Rqb3i+uUWBtPfRy3CpdQE6cv1hreiljKki8wlrXTaLla/JHNQ68G1gKkI0dYx1UfNn\n\tSXYR3OPQFRFixydQjGrRsq3FNfxQCGSf+RR6Xz9V8CI7sTYNSRa3OgbtyNp92KHSpHrH\n\tcygekz+03N2yIVPZKEe3b0qAJnOnCZHTIeqiIm3oJIDItqCsr86/wEKdnYHdI5ojBRhd\n\tFxIQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXnDCjR9piuHQpUNi6eunv0OIY87APZ8T58SycHCV6nwqGDqxkwvgB2xXlt7o/nEFKmuY2GGTslFVeGkYSOKN8=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Ywi2HZmQBhgsrYe92hlrpol76EBGY+H/owW4NlHdvCliB/JQMpe\n\t5nyclmDrBltWE5ddAynLs/sq7Le+vzY0AMU+uTfOjBYKUFoUfB9Lbn6G0We/ZybeP4oFzawXLW0\n\tLkRmZfuDn+yvTexdY06NsxVbnKubEn1JaYTvqByOSED56eWR9","X-Gm-Gg":"ASbGncsNnhcLpDIG/s5FIFg7Q/t+XFcTrN53mqcVXNGM6zIqZOTQYV8fQ5gJoZWXTeM\n\tQWv7evbMy7xxqPnVN/mOOdi8SAC6HddfL2oQ6uG6AP2lt5R8E01eqekoIfZRC5/c=","X-Google-Smtp-Source":"AGHT+IHlpyplMJvk95BZg2l+7VgHxhcoeGTk6NAPg4kkw8uLiKumZPqdfR0oUvzbY3aw/YwfgEijmkuNtBnoL3FNNu4=","X-Received":"by 2002:a05:6214:21e1:b0:6cb:f40c:b868 with SMTP id\n\t6a1803df08f44-6d864df7541mr88347266d6.46.1732801651536;\n\tThu, 28 Nov 2024 05:47:31 -0800 (PST)","MIME-Version":"1.0","References":"<20241126091509.89677-1-mzamazal@redhat.com>\n\t<20241126091509.89677-4-mzamazal@redhat.com>\n\t<20241127071538.GT5461@pendragon.ideasonboard.com>\n\t<878qt5qafy.fsf@redhat.com>\n\t\n\t<871pywr7yy.fsf@redhat.com>\n\t<4xkxparew4yins2roqtpeptswkkzvvrnyik5fp5ixuaeiv3zxe@qttcxqfklmwg>\n\t<87wmgopkbh.fsf@redhat.com>\n\t<20241128132211.GA25839@pendragon.ideasonboard.com>","In-Reply-To":"<20241128132211.GA25839@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Thu, 28 Nov 2024 13:47:20 +0000","Message-ID":"","Subject":"Re: [PATCH v4 3/4] libcamera: software_isp: Add support for contrast\n\tcontrol","To":"Laurent Pinchart ","Cc":"Milan Zamazal ,\n\tStefan Klug , \n\tlibcamera-devel@lists.libcamera.org, \n\tKieran Bingham ,\n\tNaushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","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\" "}}]