Patch Detail
Show a patch.
GET /api/1.1/patches/20683/?format=api
{ "id": 20683, "url": "https://patchwork.libcamera.org/api/1.1/patches/20683/?format=api", "web_url": "https://patchwork.libcamera.org/patch/20683/", "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": "<20240717085444.289997-22-mzamazal@redhat.com>", "date": "2024-07-17T08:54:42", "name": "[v3,21/23] libcamera: software_isp: Use DelayedControls", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "e5f2b17ffecbe81c260efd98e9b5f708943831ef", "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/20683/mbox/", "series": [ { "id": 4454, "url": "https://patchwork.libcamera.org/api/1.1/series/4454/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4454", "date": "2024-07-17T08:54:21", "name": "Software ISP refactoring", "version": 3, "mbox": "https://patchwork.libcamera.org/series/4454/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/20683/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/20683/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 2B11BC323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 17 Jul 2024 08:55:53 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B073633C8;\n\tWed, 17 Jul 2024 10:55:52 +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 DFB2F633B4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 17 Jul 2024 10:55:33 +0200 (CEST)", "from mx-prod-mc-04.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-515-p2UddAOyPgCUxd-hgcuECA-1;\n\tWed, 17 Jul 2024 04:55:29 -0400", "from mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com\n\t(mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com\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-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTPS id CA68F1955D5A; Wed, 17 Jul 2024 08:55:28 +0000 (UTC)", "from nuthatch.brq.redhat.com (unknown [10.43.17.37])\n\tby mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTP id AC10C1955F40; Wed, 17 Jul 2024 08:55:27 +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=\"hH08ifrY\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1721206533;\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=NlI/8UQzI8ODAdu9AhG5m75aZ3WVlOZ188xKeb87McE=;\n\tb=hH08ifrYNawJQoHCxkn6EF2OxrBZlnYHUE/oMYx3H+LBEtsDh75M6a+6gDX2JwB2GYN/ho\n\tzy0GDlgz5ebDVdFLVB7p6aWxZjLciQ3RCT86/x7g1dagFMsZkNJS//h6YVmqZOy50APXed\n\t6e/wZFLftkbvRjUZY/khUtxfzBT7Eso=", "X-MC-Unique": "p2UddAOyPgCUxd-hgcuECA-1", "From": "Milan Zamazal <mzamazal@redhat.com>", "To": "libcamera-devel@lists.libcamera.org", "Cc": "Milan Zamazal <mzamazal@redhat.com>,\n\tUmang Jain <umang.jain@ideasonboard.com>", "Subject": "[PATCH v3 21/23] libcamera: software_isp: Use DelayedControls", "Date": "Wed, 17 Jul 2024 10:54:42 +0200", "Message-ID": "<20240717085444.289997-22-mzamazal@redhat.com>", "In-Reply-To": "<20240717085444.289997-1-mzamazal@redhat.com>", "References": "<20240717085444.289997-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": "<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 2658e359..cf2d628d 100644\n--- a/src/ipa/simple/soft_simple.cpp\n+++ b/src/ipa/simple/soft_simple.cpp\n@@ -58,8 +58,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@@ -97,7 +96,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(\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(\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 984c3a77..0932f808 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<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<const Stream *, FrameBuffer *>> conversionQueue_;\n \tbool useConversion_;\n@@ -903,12 +906,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@@ -1289,6 +1292,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": [ "v3", "21/23" ] }