[{"id":17459,"web_url":"https://patchwork.libcamera.org/comment/17459/","msgid":"","date":"2021-06-08T01:11:48","subject":"Re: [libcamera-devel] [PATCH v5 3/4] ipa: raspberrypi: Switch\n\tAgcAlgorithm API to use utils::Duration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, May 25, 2021 at 12:42:40PM +0100, Naushir Patuck wrote:\n> Switch the AgcAlgorithm API functions to use utils::Duration for all\n> time based variables.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> ---\n> src/ipa/raspberrypi/controller/agc_algorithm.hpp | 7 ++++---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 13 +++++++------\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +++---\n> src/ipa/raspberrypi/raspberrypi.cpp | 6 +++---\n> 4 files changed, 17 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> index f97deb0fca59..579ee2c91fa8 100644\n> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> @@ -6,6 +6,7 @@\n> */\n> #pragma once\n> \n> +#include \"libcamera/internal/utils.h\"\n> #include \"algorithm.hpp\"\n> \n> namespace RPiController {\n> @@ -17,9 +18,9 @@ public:\n> \t// An AGC algorithm must provide the following:\n> \tvirtual unsigned int GetConvergenceFrames() const = 0;\n> \tvirtual void SetEv(double ev) = 0;\n> -\tvirtual void SetFlickerPeriod(double flicker_period) = 0;\n> -\tvirtual void SetFixedShutter(double fixed_shutter) = 0; // microseconds\n> -\tvirtual void SetMaxShutter(double max_shutter) = 0; // microseconds\n> +\tvirtual void SetFlickerPeriod(const libcamera::utils::Duration &flicker_period) = 0;\n> +\tvirtual void SetFixedShutter(const libcamera::utils::Duration &fixed_shutter) = 0;\n> +\tvirtual void SetMaxShutter(const libcamera::utils::Duration &max_shutter) = 0;\n> \tvirtual void SetFixedAnalogueGain(double fixed_analogue_gain) = 0;\n> \tvirtual void SetMeteringMode(std::string const &metering_mode_name) = 0;\n> \tvirtual void SetExposureMode(std::string const &exposure_mode_name) = 0;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index f4cd5d26fb4e..4e3318445cf3 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -21,6 +21,7 @@\n> \n> using namespace RPiController;\n> using namespace libcamera;\n> +using libcamera::utils::Duration;\n\nI would have used utils::Duration instead of Duration below, but I don't\nmind.\n\nReviewed-by: Laurent Pinchart \n\n> \n> LOG_DEFINE_CATEGORY(RPiAgc)\n> \n> @@ -222,19 +223,19 @@ void Agc::SetEv(double ev)\n> \tev_ = ev;\n> }\n> \n> -void Agc::SetFlickerPeriod(double flicker_period)\n> +void Agc::SetFlickerPeriod(const Duration &flicker_period)\n> {\n> -\tflicker_period_ = flicker_period;\n> +\tflicker_period_ = flicker_period.get();\n> }\n> \n> -void Agc::SetMaxShutter(double max_shutter)\n> +void Agc::SetMaxShutter(const Duration &max_shutter)\n> {\n> -\tmax_shutter_ = max_shutter;\n> +\tmax_shutter_ = max_shutter.get();\n> }\n> \n> -void Agc::SetFixedShutter(double fixed_shutter)\n> +void Agc::SetFixedShutter(const Duration &fixed_shutter)\n> {\n> -\tfixed_shutter_ = fixed_shutter;\n> +\tfixed_shutter_ = fixed_shutter.get();\n> \t// Set this in case someone calls Pause() straight after.\n> \tstatus_.shutter_time = clipShutter(fixed_shutter_);\n> }\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 0427fb59ec1b..2b39fcabada3 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -77,9 +77,9 @@ public:\n> \tvoid Resume() override;\n> \tunsigned int GetConvergenceFrames() const override;\n> \tvoid SetEv(double ev) override;\n> -\tvoid SetFlickerPeriod(double flicker_period) override;\n> -\tvoid SetMaxShutter(double max_shutter) override; // microseconds\n> -\tvoid SetFixedShutter(double fixed_shutter) override; // microseconds\n> +\tvoid SetFlickerPeriod(const libcamera::utils::Duration &flicker_period) override;\n> +\tvoid SetMaxShutter(const libcamera::utils::Duration &max_shutter) override;\n> +\tvoid SetFixedShutter(const libcamera::utils::Duration &fixed_shutter) override;\n> \tvoid SetFixedAnalogueGain(double fixed_analogue_gain) override;\n> \tvoid SetMeteringMode(std::string const &metering_mode_name) override;\n> \tvoid SetExposureMode(std::string const &exposure_mode_name) override;\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 4e02e94084f7..e083f6c254cc 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -640,8 +640,8 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\t\tbreak;\n> \t\t\t}\n> \n> -\t\t\t/* This expects units of micro-seconds. */\n> -\t\t\tagc->SetFixedShutter(ctrl.second.get());\n> +\t\t\t/* The control provides units of microseconds. */\n> +\t\t\tagc->SetFixedShutter(ctrl.second.get() * 1.0us);\n> \n> \t\t\tlibcameraMetadata_.set(controls::ExposureTime, ctrl.second.get());\n> \t\t\tbreak;\n> @@ -1094,7 +1094,7 @@ void IPARPi::applyFrameDurations(const Duration &minFrameDuration,\n> \n> \tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\tagc->SetMaxShutter(maxShutter.get());\n> +\tagc->SetMaxShutter(maxShutter);\n> }\n> \n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)","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 55507BD22E\n\tfor ;\n\tTue, 8 Jun 2021 01:12:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AEB2D6892D;\n\tTue, 8 Jun 2021 03:12:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 075896891C\n\tfor ;\n\tTue, 8 Jun 2021 03:12:04 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 713EA4AD;\n\tTue, 8 Jun 2021 03:12:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ouloQwM7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623114723;\n\tbh=RIDqaJ2xv4o8bg4ULVG+4QeZOiTmnqKXdZL9dffyGws=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ouloQwM7vZWVwqixECzRWP20IbMFO6IBblXSydbNc6ChTH7MQZe0qOg1cfv7fqats\n\tzlEN3d8zrqSxxmeJckFCMJh+nzhjTseygZfUeDFyD0M/Y7mNwB0ya97YisImmOmBRe\n\tNyMp2XVwsnv+3cNmwzb44+yuX0siQrfRrOAt0/eg=","Date":"Tue, 8 Jun 2021 04:11:48 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210525114241.906280-1-naush@raspberrypi.com>\n\t<20210525114241.906280-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210525114241.906280-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 3/4] ipa: raspberrypi: Switch\n\tAgcAlgorithm API to use utils::Duration","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]