{"id":21321,"url":"https://patchwork.libcamera.org/api/1.1/patches/21321/?format=json","web_url":"https://patchwork.libcamera.org/patch/21321/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20240920183143.1674117-17-mzamazal@redhat.com>","date":"2024-09-20T18:31:41","name":"[v7,16/18] libcamera: software_isp: Use DelayedControls","commit_ref":null,"pull_url":null,"state":"superseded","archived":false,"hash":"af5150ada64fda9695729e6e1398606f8589e347","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/1.1/people/177/?format=json","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/21321/mbox/","series":[{"id":4610,"url":"https://patchwork.libcamera.org/api/1.1/series/4610/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=4610","date":"2024-09-20T18:31:25","name":"Software ISP refactoring","version":7,"mbox":"https://patchwork.libcamera.org/series/4610/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/21321/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/21321/checks/","tags":{},"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 BEE3CC32D9\n\tfor ;\n\tFri, 20 Sep 2024 18:32:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61EAC6351A;\n\tFri, 20 Sep 2024 20:32:54 +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 4BC0363517\n\tfor ;\n\tFri, 20 Sep 2024 20:32:49 +0200 (CEST)","from mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com\n\t(ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63])\n\tby relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3,\n\tcipher=TLS_AES_256_GCM_SHA384) id us-mta-684-YEKDJJuUPtCJWiYL9QRCLg-1;\n\tFri, 20 Sep 2024 14:32:45 -0400","from mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (unknown\n\t[10.30.177.17])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (2048 bits)\n\tserver-digest SHA256) (No client certificate requested)\n\tby mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTPS id 1A92F192DE23; Fri, 20 Sep 2024 18:32:44 +0000 (UTC)","from nuthatch.redhat.com (unknown [10.45.224.12])\n\tby mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTP id A6AA51956052; Fri, 20 Sep 2024 18:32:41 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Fn5u/YW8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1726857168;\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\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Olk1RosT+bQYkTrXEbxCy+Fx8rfXSlDgwzpfYj9az20=;\n\tb=Fn5u/YW8UPEod4yWor67OfzjyopZRMJgyKRkxr5JpmS+HoPvDssQjt0CCmpPMVxETpKLAg\n\tnVxVYTpz4Lv9vkec9JFbvDFM0DQ5ZGJNC4qeS8aPJZ2C/Vv6igWyfA+fMillVFf2aa87Rs\n\tyP3O/UxrYAfTX4xKSF/7dQIhDQQTbIQ=","X-MC-Unique":"YEKDJJuUPtCJWiYL9QRCLg-1","From":"Milan Zamazal ","To":"libcamera-devel@lists.libcamera.org","Cc":"Milan Zamazal ,\n\tUmang Jain ,\n\tLaurent Pinchart ,\n\tDaniel Scally ,\n\tKieran Bingham ","Subject":"[PATCH v7 16/18] libcamera: software_isp: Use DelayedControls","Date":"Fri, 20 Sep 2024 20:31:41 +0200","Message-ID":"<20240920183143.1674117-17-mzamazal@redhat.com>","In-Reply-To":"<20240920183143.1674117-1-mzamazal@redhat.com>","References":"<20240920183143.1674117-1-mzamazal@redhat.com>","MIME-Version":"1.0","X-Scanned-By":"MIMEDefang 3.0 on 10.30.177.17","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Transfer-Encoding":"8bit","Content-Type":"text/plain; charset=\"US-ASCII\"; x-default=true","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\" "},"content":"Use the standard libcamera mechanism to report the \"current\" controls\nrather than delaying updates by counting from the last update.\n\nA problem is that with software ISP we cannot be sure about the sensor\ndelay. The original implementation simply skips exposure updates for 2\nframes, which should be enough in most cases. After this change, we\nassume the delay being exactly 2 frames, which may or may not be correct\nand may work with outdated values if the delay is shorter.\n\nAccording to Kieran, the wrong parts are also wrong on the\nIPU3/RKISP1/Mali pipelines and only RPi have this correct. We need to\nfix this, by correctly specifying the gains in the libipa camera sensor\nhelpers. The sooner the better because this change could introduce a\nrisk of increasing oscillations.\n\nThis patch also prepares moving exposure+gain to an algorithm module\nwhere the original delay mechanism would be a (possibly unnecessary)\ncomplication.\n\nResolves software ISP TODO #11 + #12.\n\nSigned-off-by: Milan Zamazal \nReviewed-by: Kieran Bingham \n---\n src/ipa/simple/soft_simple.cpp | 16 +------------\n src/libcamera/pipeline/simple/simple.cpp | 18 ++++++++++++---\n src/libcamera/software_isp/TODO | 29 ------------------------\n 3 files changed, 16 insertions(+), 47 deletions(-)","diff":"diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\nindex f8d923c5..60310a8e 100644\n--- a/src/ipa/simple/soft_simple.cpp\n+++ b/src/ipa/simple/soft_simple.cpp\n@@ -59,8 +59,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module\n public:\n \tIPASoftSimple()\n \t\t: params_(nullptr), stats_(nullptr),\n-\t\t context_({ {}, {}, { kMaxFrameContexts } }),\n-\t\t ignoreUpdates_(0)\n+\t\t context_({ {}, {}, { kMaxFrameContexts } })\n \t{\n \t}\n \n@@ -98,7 +97,6 @@ private:\n \tint32_t exposure_;\n \tdouble againMin_, againMax_, againMinStep_;\n \tdouble again_;\n-\tunsigned int ignoreUpdates_;\n };\n \n IPASoftSimple::~IPASoftSimple()\n@@ -298,16 +296,6 @@ void IPASoftSimple::processStats(const uint32_t frame,\n \n \t/* \\todo Switch to the libipa/algorithm.h API someday. */\n \n-\t/*\n-\t * AE / AGC, use 2 frames delay to make sure that the exposure and\n-\t * the gain set have applied to the camera sensor.\n-\t * \\todo This could be handled better with DelayedControls.\n-\t */\n-\tif (ignoreUpdates_ > 0) {\n-\t\t--ignoreUpdates_;\n-\t\treturn;\n-\t}\n-\n \t/*\n \t * Calculate Mean Sample Value (MSV) according to formula from:\n \t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n@@ -356,8 +344,6 @@ void IPASoftSimple::processStats(const uint32_t frame,\n \tctrls.set(V4L2_CID_ANALOGUE_GAIN,\n \t\t static_cast(camHelper_ ? camHelper_->gainCode(again_) : again_));\n \n-\tignoreUpdates_ = 2;\n-\n \tsetSensorControls.emit(ctrls);\n \n \tLOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\ndiff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\nindex 61157fe6..1e13bd74 100644\n--- a/src/libcamera/pipeline/simple/simple.cpp\n+++ b/src/libcamera/pipeline/simple/simple.cpp\n@@ -32,6 +32,7 @@\n #include \"libcamera/internal/camera.h\"\n #include \"libcamera/internal/camera_sensor.h\"\n #include \"libcamera/internal/converter.h\"\n+#include \"libcamera/internal/delayed_controls.h\"\n #include \"libcamera/internal/device_enumerator.h\"\n #include \"libcamera/internal/media_device.h\"\n #include \"libcamera/internal/pipeline_handler.h\"\n@@ -278,6 +279,8 @@ public:\n \tstd::vector configs_;\n \tstd::map> formats_;\n \n+\tstd::unique_ptr delayedCtrls_;\n+\n \tstd::vector> conversionBuffers_;\n \tstd::queue> conversionQueue_;\n \tbool useConversion_;\n@@ -896,14 +899,13 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n \n void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n {\n-\t/* \\todo Use the DelayedControls class */\n \tswIsp_->processStats(frame, bufferId,\n-\t\t\t sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,\n-\t\t\t\t\t\t V4L2_CID_EXPOSURE }));\n+\t\t\t delayedCtrls_->get(frame));\n }\n \n void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n {\n+\tdelayedCtrls_->push(sensorControls);\n \tControlList ctrls(sensorControls);\n \tsensor_->setControls(&ctrls);\n }\n@@ -1284,6 +1286,16 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n \tif (outputCfgs.empty())\n \t\treturn 0;\n \n+\tstd::unordered_map params = {\n+\t\t{ V4L2_CID_ANALOGUE_GAIN, { 2, false } },\n+\t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n+\t};\n+\tdata->delayedCtrls_ =\n+\t\tstd::make_unique(data->sensor_->device(),\n+\t\t\t\t\t\t params);\n+\tdata->video_->frameStart.connect(data->delayedCtrls_.get(),\n+\t\t\t\t\t &DelayedControls::applyControls);\n+\n \tStreamConfiguration inputCfg;\n \tinputCfg.pixelFormat = pipeConfig->captureFormat;\n \tinputCfg.size = pipeConfig->captureSize;\ndiff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\nindex 9978afc0..8eeede46 100644\n--- a/src/libcamera/software_isp/TODO\n+++ b/src/libcamera/software_isp/TODO\n@@ -209,35 +209,6 @@ At some point, yes.\n \n ---\n \n-11. Improve handling the sensor controls which take effect with a delay\n-\n-> void IPASoftSimple::processStats(const ControlList &sensorControls)\n-> {\n-> ...\n->\t/*\n->\t * AE / AGC, use 2 frames delay to make sure that the exposure and\n->\t * the gain set have applied to the camera sensor.\n->\t */\n->\tif (ignore_updates_ > 0) {\n->\t\t--ignore_updates_;\n->\t\treturn;\n->\t}\n-\n-This could be handled better with DelayedControls.\n-\n----\n-\n-12. Use DelayedControls class in ispStatsReady()\n-\n-> void SimpleCameraData::ispStatsReady()\n-> {\n-> \tswIsp_->processStats(sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,\n-> \t\t\t\t\t\t V4L2_CID_EXPOSURE }));\n-\n-You should use the DelayedControls class.\n-\n----\n-\n 13. Improve black level and colour gains application\n \n I think the black level should eventually be moved before debayering, and\n","prefixes":["v7","16/18"]}