Patch Detail
Show a patch.
GET /api/patches/20558/?format=api
{ "id": 20558, "url": "https://patchwork.libcamera.org/api/patches/20558/?format=api", "web_url": "https://patchwork.libcamera.org/patch/20558/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/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": "<20240703175119.1872585-18-mzamazal@redhat.com>", "date": "2024-07-03T17:51:17", "name": "[v2,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/people/177/?format=api", "name": "Milan Zamazal", "email": "mzamazal@redhat.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/20558/mbox/", "series": [ { "id": 4440, "url": "https://patchwork.libcamera.org/api/series/4440/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4440", "date": "2024-07-03T17:51:00", "name": "Software ISP refactoring", "version": 2, "mbox": "https://patchwork.libcamera.org/series/4440/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/20558/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/20558/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 706CBBEFBE\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 3 Jul 2024 17:52:16 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 841B66337C;\n\tWed, 3 Jul 2024 19:52:15 +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 425DE63372\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 3 Jul 2024 19:52:03 +0200 (CEST)", "from mx-prod-mc-01.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-232-RnaJU6CSPve27GWGjPxd_w-1;\n\tWed, 03 Jul 2024 13:52:00 -0400", "from mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com\n\t(mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com\n\t[10.30.177.12])\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-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTPS id A9D5A1955BC1; Wed, 3 Jul 2024 17:51:59 +0000 (UTC)", "from nuthatch.redhat.com (unknown [10.45.224.13])\n\tby mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTP id 479E31954B0D; Wed, 3 Jul 2024 17:51:58 +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=\"iAjHKxO2\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1720029122;\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=L7MlOh+qw73mQOEyoWAqoUxlB5WmS0x01cRTOrcozbI=;\n\tb=iAjHKxO2zSNMQBvA61OU9riukEG9nyiHvCzLpiVCDnWR6AlJ8oTL3YI+H6hb/5GGEtj3Fj\n\tO95qQAngq3XbxzOd7Jft8P8H9GyY4fWy1krfzGAPH1QdnG5z8JlLF8iDSq1MRcWUmWQbbq\n\tuxqfhLm9g9bwdskBLvheMNeyv3Qi3KU=", "X-MC-Unique": "RnaJU6CSPve27GWGjPxd_w-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 v2 17/19] libcamera: software_isp: Use DelayedControls", "Date": "Wed, 3 Jul 2024 19:51:17 +0200", "Message-ID": "<20240703175119.1872585-18-mzamazal@redhat.com>", "In-Reply-To": "<20240703175119.1872585-1-mzamazal@redhat.com>", "References": "<20240703175119.1872585-1-mzamazal@redhat.com>", "MIME-Version": "1.0", "X-Scanned-By": "MIMEDefang 3.0 on 10.30.177.12", "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 6fb80209..68f650b4 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@@ -902,12 +905,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@@ -1288,6 +1291,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": [ "v2", "17/19" ] }