[{"id":17199,"web_url":"https://patchwork.libcamera.org/comment/17199/","msgid":"<20210524124435.xuhjmebjihywt4q7@uno.localdomain>","date":"2021-05-24T12:44:35","subject":"Re: [libcamera-devel] [PATCH v4 3/4] ipa: raspberrypi: Switch\n\tAgcAlgorithm API to use utils::Duration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Mon, May 24, 2021 at 09:48:21AM +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> ---\n> src/ipa/raspberrypi/controller/agc_algorithm.hpp | 9 ++++++---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++------\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +++---\n> src/ipa/raspberrypi/raspberrypi.cpp | 6 +++---\n> 4 files changed, 18 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..dabb814ea3dd 100644\n> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> @@ -6,10 +6,13 @@\n> */\n> #pragma once\n>\n> +#include \"libcamera/internal/utils.h\"\n> #include \"algorithm.hpp\"\n>\n> namespace RPiController {\n>\n> +using libcamera::utils::Duration;\n> +\n\nSame comment about importing namespaces in header files. At least for\nthe HAL those are spelled out in full. IPA are kind of weird beasts,\npart of the core library but not really, so the rule might not apply\nhere.\n\n> class AgcAlgorithm : public Algorithm\n> {\n> public:\n> @@ -17,9 +20,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 Duration &flicker_period) = 0;\n> +\tvirtual void SetFixedShutter(const Duration &fixed_shutter) = 0;\n> +\tvirtual void SetMaxShutter(const 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..482eb3ef962d 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -222,19 +222,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..16a65959d90e 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 Duration &flicker_period) override;\n> +\tvoid SetMaxShutter(const Duration &max_shutter) override;\n> +\tvoid SetFixedShutter(const 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\nLooks good\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> }\n>\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> --\n> 2.25.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 604BDC3201\n\tfor ;\n\tMon, 24 May 2021 12:43:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB4946891C;\n\tMon, 24 May 2021 14:43:52 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8EC29601AA\n\tfor ;\n\tMon, 24 May 2021 14:43:51 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 1A299200004;\n\tMon, 24 May 2021 12:43:49 +0000 (UTC)"],"Date":"Mon, 24 May 2021 14:44:35 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210524124435.xuhjmebjihywt4q7@uno.localdomain>","References":"<20210524084822.3690690-1-naush@raspberrypi.com>\n\t<20210524084822.3690690-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210524084822.3690690-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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\" "}}]