[{"id":29028,"web_url":"https://patchwork.libcamera.org/comment/29028/","msgid":"","date":"2024-03-21T11:36:50","subject":"Re: [PATCH v3 08/16] libcamera: delayed_controls: Add ctrls list\n\tparameter to reset() function","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:09PM +0100, Stefan Klug wrote:\n> This makes it easier for the caller. There is often this pattern\n>\n> sensor_->setControls(controls);\n> delayedControls_->reset();\n>\n> which can then be reduced to\n>\n> delayedControls_->reset(controls);\n>\n\nNit: I would then feel more confortable having the constructor accept\na CameraSensor &sensor and initialize device_ with sensor->device()\nso that we guarantee the device_ is the same.\n\nHowever, I wonder, to have controls applied immediately the sensor\nshould not be streaming and nothing enforces delayed controls to be\nreset only when the sensor is not streaming.\n\nI guess that's not different than what we have right now\n\n> Signed-off-by: Stefan Klug \n\nReviewed-by: Jacopo Mondi \n\n> ---\n> include/libcamera/internal/delayed_controls.h | 2 +-\n> src/libcamera/delayed_controls.cpp | 22 +++++++++++++++----\n> 2 files changed, 19 insertions(+), 5 deletions(-)\n>\n> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> index ccbe7239..224c1f7e 100644\n> --- a/include/libcamera/internal/delayed_controls.h\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -27,7 +27,7 @@ public:\n> \tDelayedControls(V4L2Device *device,\n> \t\t\tconst std::unordered_map &controlParams);\n>\n> -\tvoid reset();\n> +\tvoid reset(ControlList *controls = nullptr);\n>\n> \tbool push(const ControlList &controls);\n> \tControlList get(uint32_t sequence);\n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> index 6c766ede..6f06950e 100644\n> --- a/src/libcamera/delayed_controls.cpp\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -109,11 +109,13 @@ DelayedControls::DelayedControls(V4L2Device *device,\n>\n> /**\n> * \\brief Reset state machine\n> + * \\param[in,out] controls The controls to apply to the device\n> *\n> * Resets the state machine to a starting position based on control values\n> - * retrieved from the device.\n> + * retrieved from the device. If \\a controls is given, these controls are set\n> + * on the device before retrieving the reset values.\n> */\n> -void DelayedControls::reset()\n> +void DelayedControls::reset(ControlList *controls)\n> {\n> \tqueueIndex_ = 1;\n> \twriteIndex_ = 0;\n> @@ -123,11 +125,23 @@ void DelayedControls::reset()\n> \tfor (auto const ¶m : controlParams_)\n> \t\tids.push_back(param.first->id());\n>\n> -\tControlList controls = device_->getControls(ids);\n> +\tif (controls) {\n> +\t\tdevice_->setControls(controls);\n> +\n> +\t\tLOG(DelayedControls, Debug) << \"reset:\";\n> +\t\tauto idMap = controls->idMap();\n> +\t\tif (idMap) {\n> +\t\t\tfor (const auto &[id, value] : *controls)\n> +\t\t\t\tLOG(DelayedControls, Debug) << \" \" << idMap->at(id)->name()\n> +\t\t\t\t\t\t\t << \" : \" << value.toString();\n> +\t\t}\n> +\t}\n> +\n> +\tControlList ctrls = device_->getControls(ids);\n>\n> \t/* Seed the control queue with the controls reported by the device. */\n> \tvalues_.clear();\n> -\tfor (const auto &ctrl : controls) {\n> +\tfor (const auto &ctrl : ctrls) {\n> \t\tconst ControlId *id = device_->controls().idmap().at(ctrl.first);\n> \t\t/*\n> \t\t * Do not mark this control value as updated, it does not need\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 49D53BD160\n\tfor ;\n\tThu, 21 Mar 2024 11:36:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A24663055;\n\tThu, 21 Mar 2024 12:36:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1524A63037\n\tfor ;\n\tThu, 21 Mar 2024 12:36:54 +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 B549E7E9;\n\tThu, 21 Mar 2024 12:36:25 +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=\"dqQbBQoJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711020985;\n\tbh=/xVTgQj3JzOjr5npJsjxWaS4CVMmWswknghDi6xo9ks=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dqQbBQoJzQpYk9DKkB6OgAsWrIATiwK1BnxsW64Gtl5sV+MX0QtkdmMPgvRMli0B/\n\tIp7ZGTgkuj0yVbGgOYMKbNTJWMcgd3Id3DJZwMJDPf/Ebopd7eVjQ/ILn4CEQDLsNg\n\tDY4At+ACqDpPubgYqpYvWyxrpQqNsb7WnvcFrC+k=","Date":"Thu, 21 Mar 2024 12:36:50 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 08/16] libcamera: delayed_controls: Add ctrls list\n\tparameter to reset() function","Message-ID":"","References":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>\n\t<20240319120517.362082-9-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319120517.362082-9-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\" "}}]