[{"id":31155,"web_url":"https://patchwork.libcamera.org/comment/31155/","msgid":"<172599781077.412786.6337543108114769961@ping.linuxembedded.co.uk>","date":"2024-09-10T19:50:10","subject":"Re: [PATCH 2/2] libcamera: software_isp: Add contrast control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-09-04 08:44:59)\n> This patch introduces support for applying runtime controls to software\n> ISP. It enables the contrast algorithm as the first control that can be\n> used.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> .../libcamera/internal/software_isp/software_isp.h | 3 ++-\n> include/libcamera/ipa/soft.mojom | 2 +-\n> src/ipa/simple/data/uncalibrated.yaml | 1 +\n> src/ipa/simple/soft_simple.cpp | 13 +++++++++++--\n> src/libcamera/pipeline/simple/simple.cpp | 2 +-\n> src/libcamera/software_isp/software_isp.cpp | 8 ++++++--\n> 6 files changed, 22 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> index a3e3a9da..d51b03fd 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -46,7 +46,8 @@ LOG_DECLARE_CATEGORY(SoftwareIsp)\n> class SoftwareIsp\n> {\n> public:\n> - SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor);\n> + SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n> + ControlInfoMap *ipaControls);\n> ~SoftwareIsp();\n> \n> int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }\n> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> index bc70d4e4..3b8a72a2 100644\n> --- a/include/libcamera/ipa/soft.mojom\n> +++ b/include/libcamera/ipa/soft.mojom\n> @@ -17,7 +17,7 @@ interface IPASoftInterface {\n> libcamera.SharedFD fdStats,\n> libcamera.SharedFD fdParams,\n> libcamera.ControlInfoMap sensorCtrlInfoMap)\n> - => (int32 ret);\n> + => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> start() => (int32 ret);\n> stop();\n> configure(IPAConfigInfo configInfo)\n> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n> index 324f98f4..2ed24e8c 100644\n> --- a/src/ipa/simple/data/uncalibrated.yaml\n> +++ b/src/ipa/simple/data/uncalibrated.yaml\n> @@ -4,6 +4,7 @@\n> version: 1\n> algorithms:\n> - BlackLevel:\n> + - Contrast:\n> - Gamma:\n> - Awb:\n> - Lut:\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index ac7a22b7..d6c7ff7f 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -34,6 +34,10 @@ LOG_DEFINE_CATEGORY(IPASoft)\n> \n> namespace ipa::soft {\n> \n> +const ControlInfoMap::Map swispControls{\n> + { &controls::Contrast, ControlInfo(0.0f, 10.0f, 1.0f) },\n> +};\n\nI would prefer not to register all controls at the top level. Can the\ncontrol be registered by the algorithm that supports it?\n\nI'm not sure if 'contrast' is a full algorithm if it's all being handled\nthrough gamma ... but it's only a detail for now, I guess it's a way to\nenable or disable features if it's separated...\n\n> +\n> /* Maximum number of frame contexts to be held */\n> static constexpr uint32_t kMaxFrameContexts = 16;\n> \n> @@ -50,7 +54,8 @@ public:\n> int init(const IPASettings &settings,\n> const SharedFD &fdStats,\n> const SharedFD &fdParams,\n> - const ControlInfoMap &sensorInfoMap) override;\n> + const ControlInfoMap &sensorInfoMap,\n> + ControlInfoMap *ipaControls) override;\n> int configure(const IPAConfigInfo &configInfo) override;\n> \n> int start() override;\n> @@ -87,7 +92,8 @@ IPASoftSimple::~IPASoftSimple()\n> int IPASoftSimple::init(const IPASettings &settings,\n> const SharedFD &fdStats,\n> const SharedFD &fdParams,\n> - const ControlInfoMap &sensorInfoMap)\n> + const ControlInfoMap &sensorInfoMap,\n> + ControlInfoMap *ipaControls)\n> {\n> camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> if (!camHelper_) {\n> @@ -158,6 +164,9 @@ int IPASoftSimple::init(const IPASettings &settings,\n> stats_ = static_cast(mem);\n> }\n> \n> + ControlInfoMap::Map ctrlMap = swispControls;\n> + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> +\n> /*\n> * Check if the sensor driver supports the controls required by the\n> * Soft IPA.\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index fc80e665..86c4447e 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -531,7 +531,7 @@ int SimpleCameraData::init()\n> * Instantiate Soft ISP if this is enabled for the given driver and no converter is used.\n> */\n> if (!converter_ && pipe->swIspEnabled()) {\n> - swIsp_ = std::make_unique(pipe, sensor_.get());\n> + swIsp_ = std::make_unique(pipe, sensor_.get(), &controlInfo_);\n> if (!swIsp_->isValid()) {\n> LOG(SimplePipeline, Warning)\n> << \"Failed to create software ISP, disabling software debayering\";\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index dbf27f31..78b78bab 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -61,9 +62,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> * \\brief Constructs SoftwareIsp object\n> * \\param[in] pipe The pipeline handler in use\n> * \\param[in] sensor Pointer to the CameraSensor instance owned by the pipeline\n> + * \\param[out] ipaControls The IPA controls to update\n> * handler\n> */\n> -SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n> + ControlInfoMap *ipaControls)\n> : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n> DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n> @@ -125,7 +128,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> int ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },\n> debayer_->getStatsFD(),\n> sharedParams_.fd(),\n> - sensor->controls());\n> + sensor->controls(),\n> + ipaControls);\n> if (ret) {\n> LOG(SoftwareIsp, Error) << \"IPA init failed\";\n> debayer_.reset();\n> -- \n> 2.44.1\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 1F088BF415\n\tfor ;\n\tTue, 10 Sep 2024 19:50:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0AF9634FC;\n\tTue, 10 Sep 2024 21:50:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00C37634E4\n\tfor ;\n\tTue, 10 Sep 2024 21:50:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BDA2C3EA;\n\tTue, 10 Sep 2024 21:48:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VbJooHh5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725997736;\n\tbh=O6RIfb1RTFrS44Ck3gHZzQMXqrix5+frsRRQgPJEGDE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VbJooHh5GwZwsDm+HQwjgxR4mdC2WjbikjO4olapkvOI/ml9b+HOQhTDqSrInsrR5\n\tNJHAgtjeDqSTht1m4vXxW9ogvxFbxAPIRPOArjt4U87p+Whga5AlvV4A9VYw5Ac5aJ\n\tRGuFoq9RZ6VNzo7M6JMJX6MX1SAQd2KE6MUpkmjA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240904074500.106019-3-mzamazal@redhat.com>","References":"<20240904074500.106019-1-mzamazal@redhat.com>\n\t<20240904074500.106019-3-mzamazal@redhat.com>","Subject":"Re: [PATCH 2/2] libcamera: software_isp: Add contrast control","From":"Kieran Bingham ","Cc":"Milan Zamazal ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Tue, 10 Sep 2024 20:50:10 +0100","Message-ID":"<172599781077.412786.6337543108114769961@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31247,"web_url":"https://patchwork.libcamera.org/comment/31247/","msgid":"<87ttef8wem.fsf@redhat.com>","date":"2024-09-16T15:12:49","subject":"Re: [PATCH 2/2] libcamera: software_isp: Add contrast control","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Kieran,\n\nthank you for review.\n\nKieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-09-04 08:44:59)\n>> This patch introduces support for applying runtime controls to software\n>> ISP. It enables the contrast algorithm as the first control that can be\n>\n>> used.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> .../libcamera/internal/software_isp/software_isp.h | 3 ++-\n>> include/libcamera/ipa/soft.mojom | 2 +-\n>> src/ipa/simple/data/uncalibrated.yaml | 1 +\n>> src/ipa/simple/soft_simple.cpp | 13 +++++++++++--\n>> src/libcamera/pipeline/simple/simple.cpp | 2 +-\n>> src/libcamera/software_isp/software_isp.cpp | 8 ++++++--\n>> 6 files changed, 22 insertions(+), 7 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n>> index a3e3a9da..d51b03fd 100644\n>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>> @@ -46,7 +46,8 @@ LOG_DECLARE_CATEGORY(SoftwareIsp)\n>> class SoftwareIsp\n>> {\n>> public:\n>> - SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor);\n>> + SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>> + ControlInfoMap *ipaControls);\n>> ~SoftwareIsp();\n>> \n>> int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }\n>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n>> index bc70d4e4..3b8a72a2 100644\n>> --- a/include/libcamera/ipa/soft.mojom\n>> +++ b/include/libcamera/ipa/soft.mojom\n>> @@ -17,7 +17,7 @@ interface IPASoftInterface {\n>> libcamera.SharedFD fdStats,\n>> libcamera.SharedFD fdParams,\n>> libcamera.ControlInfoMap sensorCtrlInfoMap)\n>> - => (int32 ret);\n>> + => (int32 ret, libcamera.ControlInfoMap ipaControls);\n>> start() => (int32 ret);\n>> stop();\n>> configure(IPAConfigInfo configInfo)\n>> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n>> index 324f98f4..2ed24e8c 100644\n>> --- a/src/ipa/simple/data/uncalibrated.yaml\n>> +++ b/src/ipa/simple/data/uncalibrated.yaml\n>> @@ -4,6 +4,7 @@\n>> version: 1\n>> algorithms:\n>> - BlackLevel:\n>> + - Contrast:\n>> - Gamma:\n>> - Awb:\n>> - Lut:\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index ac7a22b7..d6c7ff7f 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -34,6 +34,10 @@ LOG_DEFINE_CATEGORY(IPASoft)\n>> \n>> namespace ipa::soft {\n>> \n>> +const ControlInfoMap::Map swispControls{\n>> + { &controls::Contrast, ControlInfo(0.0f, 10.0f, 1.0f) },\n>> +};\n>\n> I would prefer not to register all controls at the top level.\n\nWell, I followed what the other pipelines do.\n\n> Can the control be registered by the algorithm that supports it?\n\nI don't think Algorithm class API allows that. But I can look again at\nthe other pipelines for possibly more inspiration.\n\n> I'm not sure if 'contrast' is a full algorithm if it's all being handled\n> through gamma ... but it's only a detail for now, I guess it's a way to\n> enable or disable features if it's separated...\n\nI don't have strong opinion about this. On one hand it's better to not\nput many things on a single pile, on the other hand the separation may\nnot make much sense here. Another option is to modify the gamma table,\nor at least provide the S-curve, in the contrast algorithm.\n\n>> +\n>> /* Maximum number of frame contexts to be held */\n>> static constexpr uint32_t kMaxFrameContexts = 16;\n>> \n>> @@ -50,7 +54,8 @@ public:\n>> int init(const IPASettings &settings,\n>> const SharedFD &fdStats,\n>> const SharedFD &fdParams,\n>> - const ControlInfoMap &sensorInfoMap) override;\n>> + const ControlInfoMap &sensorInfoMap,\n>> + ControlInfoMap *ipaControls) override;\n>> int configure(const IPAConfigInfo &configInfo) override;\n>> \n>> int start() override;\n>> @@ -87,7 +92,8 @@ IPASoftSimple::~IPASoftSimple()\n>> int IPASoftSimple::init(const IPASettings &settings,\n>> const SharedFD &fdStats,\n>> const SharedFD &fdParams,\n>> - const ControlInfoMap &sensorInfoMap)\n>> + const ControlInfoMap &sensorInfoMap,\n>> + ControlInfoMap *ipaControls)\n>> {\n>> camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n>> if (!camHelper_) {\n>> @@ -158,6 +164,9 @@ int IPASoftSimple::init(const IPASettings &settings,\n>> stats_ = static_cast(mem);\n>> }\n>> \n>> + ControlInfoMap::Map ctrlMap = swispControls;\n>> + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>> +\n>> /*\n>> * Check if the sensor driver supports the controls required by the\n>> * Soft IPA.\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index fc80e665..86c4447e 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -531,7 +531,7 @@ int SimpleCameraData::init()\n>> * Instantiate Soft ISP if this is enabled for the given driver and no converter is used.\n>> */\n>> if (!converter_ && pipe->swIspEnabled()) {\n>> - swIsp_ = std::make_unique(pipe, sensor_.get());\n>> + swIsp_ = std::make_unique(pipe, sensor_.get(), &controlInfo_);\n>> if (!swIsp_->isValid()) {\n>> LOG(SimplePipeline, Warning)\n>> << \"Failed to create software ISP, disabling software debayering\";\n>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> index dbf27f31..78b78bab 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -13,6 +13,7 @@\n>> #include \n>> #include \n>> \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -61,9 +62,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n>> * \\brief Constructs SoftwareIsp object\n>> * \\param[in] pipe The pipeline handler in use\n>> * \\param[in] sensor Pointer to the CameraSensor instance owned by the pipeline\n>> + * \\param[out] ipaControls The IPA controls to update\n>> * handler\n>> */\n>> -SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n>> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>> + ControlInfoMap *ipaControls)\n>> : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n>> DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n>> DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n>> @@ -125,7 +128,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n>> int ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },\n>> debayer_->getStatsFD(),\n>> sharedParams_.fd(),\n>> - sensor->controls());\n>> + sensor->controls(),\n>> + ipaControls);\n>> if (ret) {\n>> LOG(SoftwareIsp, Error) << \"IPA init failed\";\n>> debayer_.reset();\n>> -- \n>> 2.44.1\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 E6550C3257\n\tfor ;\n\tMon, 16 Sep 2024 15:12:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EFF1634FD;\n\tMon, 16 Sep 2024 17:12:58 +0200 (CEST)","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 C790B634F5\n\tfor ;\n\tMon, 16 Sep 2024 17:12:55 +0200 (CEST)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-673-Yl6Zpe2_NEGe1b6afmAXPg-1; Mon, 16 Sep 2024 11:12:53 -0400","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-42cb0b0514bso35690415e9.1\n\tfor ;\n\tMon, 16 Sep 2024 08:12:53 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-378e73e81cbsm7417802f8f.28.2024.09.16.08.12.50\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 16 Sep 2024 08:12:50 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Mpu1lfut\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1726499574;\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=Px6pt3z2q9/FQzhI+zL6CLCJw7dCzGMQGbj8L0Wt4Ao=;\n\tb=Mpu1lfutXMhJ8hEGq13RkGeSNiKqKvLBC0n28RTw+HfrSGlMYXwTZ8iuTSRWVV9NJ6TeWi\n\tM5XFQ+EFT091eat9RhH5SegZaaq/2P6gURM8vMxxhT/pmV+wuauzb8vA54j/MSY2L6qsOg\n\to5KR5Am+ZtT3oWB5X5c7eZ4SVxOdZ3s=","X-MC-Unique":"Yl6Zpe2_NEGe1b6afmAXPg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726499572; x=1727104372;\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=Px6pt3z2q9/FQzhI+zL6CLCJw7dCzGMQGbj8L0Wt4Ao=;\n\tb=UhNImCMTkUyCZ8/ofxN2EokL0IMHUVsgHrPvIw4TxNgWOYR2Fhr1rE8PelCFEnWGpM\n\tRMdYMLdeWpcEfdMvcC8oHjnSiEy622IIIrbsr+ZXuWkmFCR93Dzp/8chBFtMY5brsYZB\n\tucuZ+RFBTP0lZDNWumBNNZg/pSDTgNCht+SwUsZZ2avBtjL/UcLczMpjmENeIMYVqpLk\n\tIP5n8p932AUSZgbAiqmow0D6EQ4bfZMZkEeJqPDAL8IdThd8uakGVt1YnsxZqikHEVFP\n\tOurappLsA275+rLqtnH1Y1r15K6Y+Janub7LZ5lnMBw/jMOaUeU7vszRmuPDSsPVrw0E\n\tPq8w==","X-Gm-Message-State":"AOJu0Yy6NTXj3Jqw2DshQlID4+5yNgSDB8BWII5yII0tP3QMVU9VbuCn\n\tLchtIj1dtNr6VjMSCabeTrXXlqva3zswpINcDWXapDCyANEt4r6e+j0S1pDj3shh5mAQkmGcPme\n\tV42exphst+4IHCxSHKBUDlYDrBlu2+cyet979g6ohopm8eY6QrY//wzLHnS+xwdu4qdz5y462DS\n\ta2+LN2LOOu6/sxuR3IBjCJz+ACO2hqbcN6RDCep6LhaDYnXIc03UuGgME=","X-Received":["by 2002:a5d:4811:0:b0:374:b9a7:5ed6 with SMTP id\n\tffacd0b85a97d-378c2d064c2mr9741798f8f.22.1726499572079; \n\tMon, 16 Sep 2024 08:12:52 -0700 (PDT)","by 2002:a5d:4811:0:b0:374:b9a7:5ed6 with SMTP id\n\tffacd0b85a97d-378c2d064c2mr9741757f8f.22.1726499571406; \n\tMon, 16 Sep 2024 08:12:51 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGjRr6gA9QzpkXuQE5oaT73H7olVKW3LaGue77GFYemhd5koBD6S573ZhC0TCuXCOOb9sJNGQ==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] libcamera: software_isp: Add contrast control","In-Reply-To":"<172599781077.412786.6337543108114769961@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Tue, 10 Sep 2024 20:50:10 +0100\")","References":"<20240904074500.106019-1-mzamazal@redhat.com>\n\t<20240904074500.106019-3-mzamazal@redhat.com>\n\t<172599781077.412786.6337543108114769961@ping.linuxembedded.co.uk>","Date":"Mon, 16 Sep 2024 17:12:49 +0200","Message-ID":"<87ttef8wem.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31251,"web_url":"https://patchwork.libcamera.org/comment/31251/","msgid":"<87h6af8tzi.fsf@redhat.com>","date":"2024-09-16T16:05:05","subject":"Re: [PATCH 2/2] libcamera: software_isp: Add contrast control","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Milan Zamazal writes:\n\n> Hi Kieran,\n>\n> thank you for review.\n>\n> Kieran Bingham writes:\n>\n>> Quoting Milan Zamazal (2024-09-04 08:44:59)\n>>> This patch introduces support for applying runtime controls to software\n>>> ISP. It enables the contrast algorithm as the first control that can be\n>>\n>>> used.\n>>> \n>>> Signed-off-by: Milan Zamazal \n>>> ---\n>>> .../libcamera/internal/software_isp/software_isp.h | 3 ++-\n>>> include/libcamera/ipa/soft.mojom | 2 +-\n>>> src/ipa/simple/data/uncalibrated.yaml | 1 +\n>>> src/ipa/simple/soft_simple.cpp | 13 +++++++++++--\n>>> src/libcamera/pipeline/simple/simple.cpp | 2 +-\n>>> src/libcamera/software_isp/software_isp.cpp | 8 ++++++--\n>>> 6 files changed, 22 insertions(+), 7 deletions(-)\n>>> \n>>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n>>> index a3e3a9da..d51b03fd 100644\n>>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>>> @@ -46,7 +46,8 @@ LOG_DECLARE_CATEGORY(SoftwareIsp)\n>>> class SoftwareIsp\n>>> {\n>>> public:\n>>> - SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor);\n>>> + SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>>> + ControlInfoMap *ipaControls);\n>>> ~SoftwareIsp();\n>>> \n>>> int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }\n>>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n>>> index bc70d4e4..3b8a72a2 100644\n>>> --- a/include/libcamera/ipa/soft.mojom\n>>> +++ b/include/libcamera/ipa/soft.mojom\n>>> @@ -17,7 +17,7 @@ interface IPASoftInterface {\n>>> libcamera.SharedFD fdStats,\n>>> libcamera.SharedFD fdParams,\n>>> libcamera.ControlInfoMap sensorCtrlInfoMap)\n>>> - => (int32 ret);\n>>> + => (int32 ret, libcamera.ControlInfoMap ipaControls);\n>>> start() => (int32 ret);\n>>> stop();\n>>> configure(IPAConfigInfo configInfo)\n>>> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n>>> index 324f98f4..2ed24e8c 100644\n>>> --- a/src/ipa/simple/data/uncalibrated.yaml\n>>> +++ b/src/ipa/simple/data/uncalibrated.yaml\n>>> @@ -4,6 +4,7 @@\n>>> version: 1\n>>> algorithms:\n>>> - BlackLevel:\n>>> + - Contrast:\n>>> - Gamma:\n>>> - Awb:\n>>> - Lut:\n>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>>> index ac7a22b7..d6c7ff7f 100644\n>>> --- a/src/ipa/simple/soft_simple.cpp\n>>> +++ b/src/ipa/simple/soft_simple.cpp\n>>> @@ -34,6 +34,10 @@ LOG_DEFINE_CATEGORY(IPASoft)\n>>> \n>>> namespace ipa::soft {\n>>> \n>>> +const ControlInfoMap::Map swispControls{\n>>> + { &controls::Contrast, ControlInfo(0.0f, 10.0f, 1.0f) },\n>>> +};\n>>\n>> I would prefer not to register all controls at the top level.\n>\n> Well, I followed what the other pipelines do.\n>\n>> Can the control be registered by the algorithm that supports it?\n>\n> I don't think Algorithm class API allows that. But I can look again at\n> the other pipelines for possibly more inspiration.\n\nI can see there has been a similar change posted for rkisp1, I'll try to\nfollow it.\n\n>> I'm not sure if 'contrast' is a full algorithm if it's all being handled\n>> through gamma ... but it's only a detail for now, I guess it's a way to\n>> enable or disable features if it's separated...\n>\n> I don't have strong opinion about this. On one hand it's better to not\n> put many things on a single pile, on the other hand the separation may\n> not make much sense here. Another option is to modify the gamma table,\n> or at least provide the S-curve, in the contrast algorithm.\n>\n>>> +\n>>> /* Maximum number of frame contexts to be held */\n>>> static constexpr uint32_t kMaxFrameContexts = 16;\n>>> \n>>> @@ -50,7 +54,8 @@ public:\n>>> int init(const IPASettings &settings,\n>>> const SharedFD &fdStats,\n>>> const SharedFD &fdParams,\n>>> - const ControlInfoMap &sensorInfoMap) override;\n>>> + const ControlInfoMap &sensorInfoMap,\n>>> + ControlInfoMap *ipaControls) override;\n>>> int configure(const IPAConfigInfo &configInfo) override;\n>>> \n>>> int start() override;\n>>> @@ -87,7 +92,8 @@ IPASoftSimple::~IPASoftSimple()\n>>> int IPASoftSimple::init(const IPASettings &settings,\n>>> const SharedFD &fdStats,\n>>> const SharedFD &fdParams,\n>>> - const ControlInfoMap &sensorInfoMap)\n>>> + const ControlInfoMap &sensorInfoMap,\n>>> + ControlInfoMap *ipaControls)\n>>> {\n>>> camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n>>> if (!camHelper_) {\n>>> @@ -158,6 +164,9 @@ int IPASoftSimple::init(const IPASettings &settings,\n>>> stats_ = static_cast(mem);\n>>> }\n>>> \n>>> + ControlInfoMap::Map ctrlMap = swispControls;\n>>> + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>>> +\n>>> /*\n>>> * Check if the sensor driver supports the controls required by the\n>>> * Soft IPA.\n>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>>> index fc80e665..86c4447e 100644\n>>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>>> @@ -531,7 +531,7 @@ int SimpleCameraData::init()\n>>> * Instantiate Soft ISP if this is enabled for the given driver and no converter is used.\n>>> */\n>>> if (!converter_ && pipe->swIspEnabled()) {\n>>> - swIsp_ = std::make_unique(pipe, sensor_.get());\n>>> + swIsp_ = std::make_unique(pipe, sensor_.get(), &controlInfo_);\n>>> if (!swIsp_->isValid()) {\n>>> LOG(SimplePipeline, Warning)\n>>> << \"Failed to create software ISP, disabling software debayering\";\n>>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>>> index dbf27f31..78b78bab 100644\n>>> --- a/src/libcamera/software_isp/software_isp.cpp\n>>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>>> @@ -13,6 +13,7 @@\n>>> #include \n>>> #include \n>>> \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -61,9 +62,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n>>> * \\brief Constructs SoftwareIsp object\n>>> * \\param[in] pipe The pipeline handler in use\n>>> * \\param[in] sensor Pointer to the CameraSensor instance owned by the pipeline\n>>> + * \\param[out] ipaControls The IPA controls to update\n>>> * handler\n>>> */\n>>> -SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n>>> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>>> + ControlInfoMap *ipaControls)\n>>> : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n>>> DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n>>> DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n>>> @@ -125,7 +128,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n>>> int ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },\n>>> debayer_->getStatsFD(),\n>>> sharedParams_.fd(),\n>>> - sensor->controls());\n>>> + sensor->controls(),\n>>> + ipaControls);\n>>> if (ret) {\n>>> LOG(SoftwareIsp, Error) << \"IPA init failed\";\n>>> debayer_.reset();\n>>> -- \n>>> 2.44.1\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 D9B62C3257\n\tfor ;\n\tMon, 16 Sep 2024 16:05:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF40A634FD;\n\tMon, 16 Sep 2024 18:05:14 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 089D3634F5\n\tfor ;\n\tMon, 16 Sep 2024 18:05:12 +0200 (CEST)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-556-6Z5s_02BNaikA9vM3HMjhQ-1; Mon, 16 Sep 2024 12:05:09 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-42cb050acc3so23821885e9.1\n\tfor ;\n\tMon, 16 Sep 2024 09:05:09 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-378e73f9b10sm7561744f8f.59.2024.09.16.09.05.06\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 16 Sep 2024 09:05:06 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Xh5BI/Si\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1726502711;\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=rn8kH9lXHrHbOYviw5PuhcDOEjv+EZ30fnBsBsEixKo=;\n\tb=Xh5BI/Si08DAT1quz0nUa5CktEy1XiAK25dlmyU0h6WVp0QQGF9tCpPFQFT8peQYsxwTNy\n\ts0hb8DCOoDznv5PszIUMUDqNpoyCnvE/NuYAVUIzihet1fyPLxB8+y3GuKbLgywyHi2QZh\n\tgvhgEQT1MngYpyV8aW60S6crqhiUYU0=","X-MC-Unique":"6Z5s_02BNaikA9vM3HMjhQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726502708; x=1727107508;\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=rn8kH9lXHrHbOYviw5PuhcDOEjv+EZ30fnBsBsEixKo=;\n\tb=XWdZFXx6+1GSTXzc/E+v787nCKJNpReavQRTzSx/ZB3OIYvQNbAahzEotv/fqIWSlG\n\tGJKQy7QWDBU8wZJdYu6yG7wYlHovkImyAVo4cVeEreRNtDZRiSTlPmqn8dnWB4wJks0c\n\t98RBdVDPSx3qSqkP65DcqkI/j+gB8T9kkFftq4vgGMphkb2MNR6FiQnWxU+QiJ+Rc/5D\n\tGY/I+mjbRZn6KRy7KWLO9yoX9tBmDHW3jJeTnbzHiMh3BmV0+NiC5SiVHBkcbohRtGsk\n\tgbhqsvz3tLllbDiUhPfvVCECUOmoncgjlQ31J0m2i/azWVvmqtDSi9jiUj3wzeiwk7T/\n\tjQ3w==","X-Gm-Message-State":"AOJu0Yx5Z01EnAblqmYpesnkIFXjDAcLTmu0/P9sFFqoymFnY7TwW9NE\n\tF7/FkJfi23GkjnpTO/IOWYiVJXKsROwHhbBgQroVXgyfP9XQRuRcPcbE+FpaGkI6tDGYYMNPtY2\n\tIpox3cw19u+TL+tbITMmARLm8UBoj6JSjbbUST2t4CyLQ+NoWujWCLf5+6nRFe3xqSnPfMVX/dx\n\teztItqgKG6djHvUTl4fhmZCZHKRDSCipuhzNNblsjtHiChTBH2sEYores=","X-Received":["by 2002:a05:600c:1d18:b0:426:63bc:f031 with SMTP id\n\t5b1f17b1804b1-42cdc87572fmr87500545e9.1.1726502707608; \n\tMon, 16 Sep 2024 09:05:07 -0700 (PDT)","by 2002:a05:600c:1d18:b0:426:63bc:f031 with SMTP id\n\t5b1f17b1804b1-42cdc87572fmr87500255e9.1.1726502707010; \n\tMon, 16 Sep 2024 09:05:07 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFjXipG9RtmBxeNsYiior2AVVkGnHbQmchD34WkCGfBI9cbg0AoZhR1w/3bvAUG2MYiRf+yVw==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] libcamera: software_isp: Add contrast control","In-Reply-To":"<87ttef8wem.fsf@redhat.com> (Milan Zamazal's message of \"Mon, 16\n\tSep 2024 17:12:49 +0200\")","References":"<20240904074500.106019-1-mzamazal@redhat.com>\n\t<20240904074500.106019-3-mzamazal@redhat.com>\n\t<172599781077.412786.6337543108114769961@ping.linuxembedded.co.uk>\n\t<87ttef8wem.fsf@redhat.com>","Date":"Mon, 16 Sep 2024 18:05:05 +0200","Message-ID":"<87h6af8tzi.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]