Patch Detail
Show a patch.
GET /api/1.1/patches/21072/?format=api
{ "id": 21072, "url": "https://patchwork.libcamera.org/api/1.1/patches/21072/?format=api", "web_url": "https://patchwork.libcamera.org/patch/21072/", "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": "<20240830072554.180672-17-mzamazal@redhat.com>", "date": "2024-08-30T07:25:52", "name": "[v5,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=api", "name": "Milan Zamazal", "email": "mzamazal@redhat.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/21072/mbox/", "series": [ { "id": 4548, "url": "https://patchwork.libcamera.org/api/1.1/series/4548/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4548", "date": "2024-08-30T07:25:36", "name": "Software ISP refactoring", "version": 5, "mbox": "https://patchwork.libcamera.org/series/4548/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/21072/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/21072/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 EBABCC32D7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 30 Aug 2024 07:26:57 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4300F634EA;\n\tFri, 30 Aug 2024 09:26:57 +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 760D4634C8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 30 Aug 2024 09:26:51 +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-644-JcISMMQzPFWGmEGws7071Q-1;\n\tFri, 30 Aug 2024 03:26:47 -0400", "from mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com\n\t(mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com\n\t[10.30.177.40])\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 207191956048; Fri, 30 Aug 2024 07:26:46 +0000 (UTC)", "from nuthatch.redhat.com (unknown [10.45.225.65])\n\tby mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTP id E8A6D19560A3; Fri, 30 Aug 2024 07:26:43 +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=\"hqnilink\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1725002810;\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=zqi62KONqOYAkH9I2+EWHCyIrENNhFJ60R7QakIJ3pA=;\n\tb=hqnilink0p3J8aOH17jwMzSzo4lOA3Vx69ermPbpHPhcP0jd4sJvA6GCGEpRzkAWXLyIRG\n\tzSF36ivCWkBTDa6FuZta+Z4ENm27+QHqyMIL+Lah0UiAMS0QxNHn6qT39LpTGzsxlTtF+8\n\tDAdNnffx0oSc6VuH6h2wEzj86owhqf0=", "X-MC-Unique": "JcISMMQzPFWGmEGws7071Q-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>,\n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tDaniel Scally <dan.scally@ideasonboard.com>", "Subject": "[PATCH v5 16/18] libcamera: software_isp: Use DelayedControls", "Date": "Fri, 30 Aug 2024 09:25:52 +0200", "Message-ID": "<20240830072554.180672-17-mzamazal@redhat.com>", "In-Reply-To": "<20240830072554.180672-1-mzamazal@redhat.com>", "References": "<20240830072554.180672-1-mzamazal@redhat.com>", "MIME-Version": "1.0", "X-Scanned-By": "MIMEDefang 3.0 on 10.30.177.40", "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 | 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 8f01bf7d..0e47b9c5 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<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 ed50ab52..9a778994 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@@ -900,14 +903,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@@ -1288,6 +1290,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 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": [ "v5", "16/18" ] }