[{"id":29029,"web_url":"https://patchwork.libcamera.org/comment/29029/","msgid":"","date":"2024-03-21T11:54:11","subject":"Re: [PATCH v3 12/16] libcamera: delayed_controls: Ignore delayed\n\trequest, if there is a newer one","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Tue, Mar 19, 2024 at 01:05:13PM +0100, Stefan Klug wrote:\n> Assume for frame 10 a ExposureTime=42 is queued. We have a delay of 2. After\n> receiving stats for frame 8, the isp tries to queue for frame 9. As it's too\n> lae for that frame, delayed controls schedules the update for frame 11. But as\n> frame 10 was already queued, the request should be discarded.\n\nI think this use case is the main reason for the whole delayed control\nrework, isn't it ?\n\nSo please let's try to first clarify if it is valid. Let's expand the\nabove text:\n\n> Assume for frame 10 a ExposureTime=42 is queued. We have a delay of 2. After\n\nAt what time is the request queued ? During the exposure of which\nframe ?\n\n> receiving stats for frame 8, the isp tries to queue for frame 9. As it's too\n\ns/isp/IPA ? Otherwise I can't make a sense out of this phrase.\n\nIf so, frame 8 has been processed by the ISP. If frame 8 has been\nprocessed, it means (at best) frame 9 is already exposing. Whatever\nsettings we apply at this time, they will get realized at (10 +\nmax_delay).\n\n> lae for that frame, delayed controls schedules the update for frame 11. But as\n\nWhy frame 11 ?\n\n> frame 10 was already queued, the request should be discarded.\n\nframe 10 is queued to what ? To the ISP ?\n\nLet's calrify this use case first then let's discuss the changes to\ndelayed controls.\n\nThanks\n j\n\n> ---\n> include/libcamera/internal/delayed_controls.h | 2 +\n> src/libcamera/delayed_controls.cpp | 19 +++++-\n> test/delayed_controls.cpp | 66 +++++++++++++++++++\n> 3 files changed, 86 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> index e2decbcc..91c9415a 100644\n> --- a/include/libcamera/internal/delayed_controls.h\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -67,6 +67,8 @@ private:\n> \t\t{\n> \t\t\treturn std::array::operator[](index % listSize);\n> \t\t}\n> +\n> +\t\tunsigned int largestValidIndex;\n> \t};\n>\n> \tbool controlsAreQueued(unsigned int frame, const ControlList &controls);\n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> index 3fd5a0f7..7f2bb479 100644\n> --- a/src/libcamera/delayed_controls.cpp\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -151,6 +151,7 @@ void DelayedControls::reset(ControlList *controls)\n> \t\t * to be written to to device on startup.\n> \t\t */\n> \t\tvalues_[id][0] = Info(ctrl.second, 0, false);\n> +\t\tvalues_[id].largestValidIndex = 0;\n> \t}\n>\n> \t/* Propagate initial state */\n> @@ -304,18 +305,34 @@ bool DelayedControls::push(const ControlList &controls, std::optional\n> \t\t\tcontinue;\n> \t\t}\n>\n> -\t\tInfo &info = values_[id][updateIndex];\n> +\t\tControlRingBuffer &ring = values_[id];\n> +\t\tInfo &info = ring[updateIndex];\n> \t\t/*\n> \t\t * Update the control only if the already existing value stems from a\n> \t\t * request with a sequence number smaller or equal to the current one\n> \t\t */\n> \t\tif (info.sourceSequence <= sequence) {\n> \t\t\tinfo = Info(control.second, sequence);\n> +\t\t\tif (updateIndex > ring.largestValidIndex)\n> +\t\t\t\tring.largestValidIndex = updateIndex;\n>\n> \t\t\tLOG(DelayedControls, Debug)\n> \t\t\t\t<< \"Queuing \" << id->name()\n> \t\t\t\t<< \" to \" << info.toString()\n> \t\t\t\t<< \" at index \" << updateIndex;\n> +\n> +\t\t\t/* fill up the next indices with the new information */\n> +\t\t\tunsigned int i = updateIndex + 1;\n> +\t\t\twhile (i <= ring.largestValidIndex) {\n> +\t\t\t\tLOG(DelayedControls, Error) << \"update \" << i;\n> +\t\t\t\tInfo &next = ring[i];\n> +\t\t\t\tif (next.sourceSequence <= sequence)\n> +\t\t\t\t\tnext = info;\n> +\t\t\t\telse\n> +\t\t\t\t\tbreak;\n> +\n> +\t\t\t\ti++;\n> +\t\t\t}\n> \t\t} else {\n> \t\t\tLOG(DelayedControls, Warning)\n> \t\t\t\t<< \"Skipped update \" << id->name()\n> diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp\n> index 481334e7..7d671a0e 100644\n> --- a/test/delayed_controls.cpp\n> +++ b/test/delayed_controls.cpp\n> @@ -271,6 +271,68 @@ protected:\n> \t\treturn TestPass;\n> \t}\n>\n> +\tint updateTooLateMustSometimesBeIgnored()\n> +\t{\n> +\t\tstd::unordered_map delays = {\n> +\t\t\t{ V4L2_CID_BRIGHTNESS, { 2, false } },\n> +\t\t};\n> +\t\tstd::unique_ptr delayed =\n> +\t\t\tstd::make_unique(dev_.get(), delays);\n> +\t\tControlList ctrls;\n> +\n> +\t\t/* Reset control to value that will be first in test. */\n> +\t\tint32_t initial = 4;\n> +\t\tctrls.set(V4L2_CID_BRIGHTNESS, initial);\n> +\t\tdev_->setControls(&ctrls);\n> +\t\tdelayed->reset();\n> +\n> +\t\tint32_t expected = 10;\n> +\n> +\t\tdelayed->push({}, 0);\n> +\t\tdelayed->push({}, 1);\n> +\t\tctrls.set(V4L2_CID_BRIGHTNESS, expected);\n> +\t\tdelayed->push(ctrls, 2);\n> +\t\tdelayed->applyControls(0); /* puts 10 on the bus */\n> +\n> +\t\t/*\n> +\t\t * Post an update for frame 1. It's too late to fulfill that request,\n> +\t\t * delayed controls will therefore try to delay it to frame 3. But as\n> +\t\t * frame 2 is already queued, the update must be dropped.\n> +\t\t */\n> +\t\tctrls.set(V4L2_CID_BRIGHTNESS, 20);\n> +\t\tdelayed->push(ctrls, 1);\n> +\t\tdelayed->applyControls(1);\n> +\t\tdelayed->applyControls(2);\n> +\t\tdelayed->applyControls(3);\n> +\n> +\t\tint frame = 3;\n> +\n> +\t\tControlList result = delayed->get(frame);\n> +\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n> +\t\tControlList ctrlsV4L = dev_->getControls({ V4L2_CID_BRIGHTNESS });\n> +\t\tint32_t brightnessV4L = ctrlsV4L.get(V4L2_CID_BRIGHTNESS).get();\n> +\n> +\t\tif (brightness != expected) {\n> +\t\t\tcerr << \"Failed \" << __func__\n> +\t\t\t << \" frame \" << frame\n> +\t\t\t << \" expected \" << expected\n> +\t\t\t << \" got \" << brightness\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (brightnessV4L != expected) {\n> +\t\t\tcerr << \"Failed \" << __func__\n> +\t\t\t << \" frame \" << frame\n> +\t\t\t << \" expected V4L \" << expected\n> +\t\t\t << \" got \" << brightnessV4L\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> \tint updateTooLateGetsDelayed()\n> \t{\n> \t\tstd::unordered_map delays = {\n> @@ -503,6 +565,10 @@ protected:\n> \t\tif (ret)\n> \t\t\tfailed = true;\n>\n> +\t\tret = updateTooLateMustSometimesBeIgnored();\n> +\t\tif (ret)\n> +\t\t\tfailed = true;\n> +\n> \t\tret = updateTooLateGetsDelayed();\n> \t\tif (ret)\n> \t\t\tfailed = true;\n> --\n> 2.40.1\n>","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 647E9C3272\n\tfor ;\n\tThu, 21 Mar 2024 11:54:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E63163055;\n\tThu, 21 Mar 2024 12:54:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA72262827\n\tfor ;\n\tThu, 21 Mar 2024 12:54:15 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 76E427E9;\n\tThu, 21 Mar 2024 12:53:47 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lhfcgjwG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711022027;\n\tbh=btdyCviuoVpErktEjfxw9fvPdNOOOcaAdgs+bBIZbAI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lhfcgjwGaQzW6VNvKGcymGgxVQCNNEZGk4obXMXFQQFRf/RofFgVRbQuhs0hmWGIX\n\tVjhfAKKyNBbeBr/17GBazL9OaWO7Qz2N0+AVr/CS8gxzoOFcJTZ3LzIxnpNcQuioqG\n\tGK5AY70RQmafKEVSmBPM7rHTS4lVSueLWbbZ37XI=","Date":"Thu, 21 Mar 2024 12:54:11 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 12/16] libcamera: delayed_controls: Ignore delayed\n\trequest, if there is a newer one","Message-ID":"","References":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>\n\t<20240319120517.362082-13-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319120517.362082-13-stefan.klug@ideasonboard.com>","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\" "}}]