Patch Detail
Show a patch.
GET /api/1.1/patches/20412/?format=api
{ "id": 20412, "url": "https://patchwork.libcamera.org/api/1.1/patches/20412/?format=api", "web_url": "https://patchwork.libcamera.org/patch/20412/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20240626072100.55497-18-mzamazal@redhat.com>", "date": "2024-06-26T07:20:58", "name": "[17/19] libcamera: software_isp: Use DelayedControls", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "8ca9c9904ed0560fe11a9efd406594d8052c4085", "submitter": { "id": 177, "url": "https://patchwork.libcamera.org/api/1.1/people/177/?format=api", "name": "Milan Zamazal", "email": "mzamazal@redhat.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/20412/mbox/", "series": [ { "id": 4419, "url": "https://patchwork.libcamera.org/api/1.1/series/4419/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4419", "date": "2024-06-26T07:20:41", "name": "Software ISP refactoring", "version": 1, "mbox": "https://patchwork.libcamera.org/series/4419/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/20412/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/20412/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 79238BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 26 Jun 2024 07:21:59 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01EF7654C2;\n\tWed, 26 Jun 2024 09:21:59 +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 4DD59654C6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Jun 2024 09:21:56 +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-543-6-WR7245P2avA6mNwpobag-1;\n\tWed, 26 Jun 2024 03:21:43 -0400", "from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com\n\t(mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.4])\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\n\tid DF39619560AD for <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Jun 2024 07:21:42 +0000 (UTC)", "from nuthatch.brq.redhat.com (unknown [10.43.17.159])\n\tby mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTP id E31ED300022B; Wed, 26 Jun 2024 07:21: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=\"OjsSMTN/\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1719386515;\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=wD8iiFHe98rl6wXwuDebVUALRRkeJSufmD9aCZg0Z5A=;\n\tb=OjsSMTN/+UaAinccbeLdGUt4EfqQRg8c27RxvdL33LBJw7cdFAMragic8RkdzkJQpSZszA\n\txUbv5VwJh1qdvVhoMT2r/hmRcJe2kbi71dYticxRTTTTz4qzhvYTpM//rJWwiRay/d7CwU\n\tGiaqtVdvRx4leCy4zJAoncroQIRP6X0=", "X-MC-Unique": "6-WR7245P2avA6mNwpobag-1", "From": "Milan Zamazal <mzamazal@redhat.com>", "To": "libcamera-devel@lists.libcamera.org", "Cc": "Milan Zamazal <mzamazal@redhat.com>", "Subject": "[PATCH 17/19] libcamera: software_isp: Use DelayedControls", "Date": "Wed, 26 Jun 2024 09:20:58 +0200", "Message-ID": "<20240626072100.55497-18-mzamazal@redhat.com>", "In-Reply-To": "<20240626072100.55497-1-mzamazal@redhat.com>", "References": "<20240626072100.55497-1-mzamazal@redhat.com>", "MIME-Version": "1.0", "X-Scanned-By": "MIMEDefang 3.4.1 on 10.30.177.4", "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": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "Use the standard libcamera mechanism to report the \"current\" controls\nrather than delaying updates by counting from the last update.\n\nMY SPECULATION -- valid or not?:\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\ncorrect and may work with outdated values if the delay is shorter.\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 <mzamazal@redhat.com>\n---\n src/ipa/simple/soft_simple.cpp | 16 +------------\n src/libcamera/pipeline/simple/simple.cpp | 17 ++++++++++++--\n src/libcamera/software_isp/TODO | 29 ------------------------\n 3 files changed, 16 insertions(+), 46 deletions(-)", "diff": "diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\nindex 25f6a71e..6b31fd08 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@@ -300,16 +298,6 @@ void IPASoftSimple::processStats(\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@@ -358,8 +346,6 @@ void IPASoftSimple::processStats(\n \tctrls.set(V4L2_CID_ANALOGUE_GAIN,\n \t\t static_cast<int32_t>(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 a82d05f4..7208de86 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@@ -277,6 +278,8 @@ public:\n \tstd::vector<Configuration> configs_;\n \tstd::map<PixelFormat, std::vector<const Configuration *>> formats_;\n \n+\tstd::unique_ptr<DelayedControls> delayedCtrls_;\n+\n \tstd::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;\n \tstd::queue<std::map<unsigned int, FrameBuffer *>> conversionQueue_;\n \tbool useConversion_;\n@@ -896,12 +899,12 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n \t/* \\todo Use the DelayedControls class */\n \tswIsp_->processStats(\n \t\tframe, bufferId,\n-\t\tsensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,\n-\t\t\t\t V4L2_CID_EXPOSURE }));\n+\t\tdelayedCtrls_->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@@ -1282,6 +1285,16 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n \tif (outputCfgs.empty())\n \t\treturn 0;\n \n+\tstd::unordered_map<uint32_t, DelayedControls::ControlParams> 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<DelayedControls>(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 6bdc5905..6b1cb57a 100644\n--- a/src/libcamera/software_isp/TODO\n+++ b/src/libcamera/software_isp/TODO\n@@ -228,35 +228,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": [ "17/19" ] }