[{"id":17388,"web_url":"https://patchwork.libcamera.org/comment/17388/","msgid":"","date":"2021-06-02T13:11:04","subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper to use utils::Duration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nGentle ping to have some feedback for this patch.\n\nAll other patches in this series have 2x R-b tags, so once this has another\nreview, I think\nthe series can be merged.\n\nRegards,\nNaush\n\n\nOn Tue, 25 May 2021 at 12:42, Naushir Patuck wrote:\n\n> Switch the ipa and cam_helper code to use libcamera::utils::Duration for\n> all time based variables. This improves code readability and avoids\n> possible errors when converting between time bases.\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 20 +++---\n> src/ipa/raspberrypi/cam_helper.hpp | 10 +--\n> src/ipa/raspberrypi/controller/camera_mode.h | 6 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 64 +++++++++++---------\n> 4 files changed, 55 insertions(+), 45 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> index 09917f3cc079..7b4331a87a42 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -18,6 +18,7 @@\n>\n> using namespace RPiController;\n> using namespace libcamera;\n> +using libcamera::utils::Duration;\n>\n> namespace libcamera {\n> LOG_DECLARE_CATEGORY(IPARPI)\n> @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr\n> &stats,\n> {\n> }\n>\n> -uint32_t CamHelper::ExposureLines(double exposure_us) const\n> +uint32_t CamHelper::ExposureLines(const Duration &exposure) const\n> {\n> assert(initialized_);\n> - return exposure_us * 1000.0 / mode_.line_length;\n> + return exposure / mode_.line_length;\n> }\n>\n> -double CamHelper::Exposure(uint32_t exposure_lines) const\n> +Duration CamHelper::Exposure(uint32_t exposure_lines) const\n> {\n> assert(initialized_);\n> - return exposure_lines * mode_.line_length / 1000.0;\n> + return exposure_lines * mode_.line_length;\n> }\n>\n> -uint32_t CamHelper::GetVBlanking(double &exposure, double\n> minFrameDuration,\n> - double maxFrameDuration) const\n> +uint32_t CamHelper::GetVBlanking(Duration &exposure,\n> + const Duration &minFrameDuration,\n> + const Duration &maxFrameDuration) const\n> {\n> uint32_t frameLengthMin, frameLengthMax, vblank;\n> uint32_t exposureLines = ExposureLines(exposure);\n> @@ -85,8 +87,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure,\n> double minFrameDuration,\n> * minFrameDuration and maxFrameDuration are clamped by the caller\n> * based on the limits for the active sensor mode.\n> */\n> - frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> - frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> + frameLengthMin = minFrameDuration / mode_.line_length;\n> + frameLengthMax = maxFrameDuration / mode_.line_length;\n>\n> /*\n> * Limit the exposure to the maximum frame duration requested, and\n> @@ -182,7 +184,7 @@ void CamHelper::parseEmbeddedData(Span\n> buffer,\n> return;\n> }\n>\n> - deviceStatus.shutter_speed = Exposure(exposureLines);\n> + deviceStatus.shutter_speed =\n> Exposure(exposureLines).get();\n> deviceStatus.analogue_gain = Gain(gainCode);\n>\n> LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> index a52f3f0b583c..07481e4174a7 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -15,6 +15,7 @@\n> #include \"controller/metadata.hpp\"\n> #include \"md_parser.hpp\"\n>\n> +#include \"libcamera/internal/utils.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n>\n> namespace RPiController {\n> @@ -72,10 +73,11 @@ public:\n> virtual void Prepare(libcamera::Span buffer,\n> Metadata &metadata);\n> virtual void Process(StatisticsPtr &stats, Metadata &metadata);\n> - uint32_t ExposureLines(double exposure_us) const;\n> - double Exposure(uint32_t exposure_lines) const; // in us\n> - virtual uint32_t GetVBlanking(double &exposure_us, double\n> minFrameDuration,\n> - double maxFrameDuration) const;\n> + uint32_t ExposureLines(const libcamera::utils::Duration &exposure)\n> const;\n> + libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;\n> + virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,\n> + const libcamera::utils::Duration\n> &minFrameDuration,\n> + const libcamera::utils::Duration\n> &maxFrameDuration) const;\n> virtual uint32_t GainCode(double gain) const = 0;\n> virtual double Gain(uint32_t gain_code) const = 0;\n> virtual void GetDelays(int &exposure_delay, int &gain_delay,\n> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> index 256438c931d9..2aa2335dcf90 100644\n> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> @@ -8,6 +8,8 @@\n>\n> #include \n>\n> +#include \"libcamera/internal/utils.h\"\n> +\n> // Description of a \"camera mode\", holding enough information for control\n> // algorithms to adapt their behaviour to the different modes of the\n> camera,\n> // including binning, scaling, cropping etc.\n> @@ -33,8 +35,8 @@ struct CameraMode {\n> double scale_x, scale_y;\n> // scaling of the noise compared to the native sensor mode\n> double noise_factor;\n> - // line time in nanoseconds\n> - double line_length;\n> + // line time\n> + libcamera::utils::Duration line_length;\n> // any camera transform *not* reflected already in the camera\n> tuning\n> libcamera::Transform transform;\n> // minimum and maximum fame lengths in units of lines\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 52d91db282ea..4e02e94084f7 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -55,19 +55,22 @@\n>\n> namespace libcamera {\n>\n> +using namespace std::literals::chrono_literals;\n> +using utils::Duration;\n> +\n> /* Configure the sensor with these values initially. */\n> constexpr double DefaultAnalogueGain = 1.0;\n> -constexpr unsigned int DefaultExposureTime = 20000;\n> -constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> +constexpr Duration DefaultExposureTime = 20.0ms;\n> +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;\n> +constexpr Duration defaultMaxFrameDuration = 100.0s;\n>\n> /*\n> - * Determine the minimum allowable inter-frame duration (in us) to run the\n> - * controller algorithms. If the pipeline handler provider frames at a\n> rate\n> - * higher than this, we rate-limit the controller Prepare() and Process()\n> calls\n> - * to lower than or equal to this rate.\n> + * Determine the minimum allowable inter-frame duration to run the\n> controller\n> + * algorithms. If the pipeline handler provider frames at a rate higher\n> than this,\n> + * we rate-limit the controller Prepare() and Process() calls to lower\n> than or\n> + * equal to this rate.\n> */\n> -constexpr double controllerMinFrameDuration = 1e6 / 60.0;\n> +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;\n>\n> LOG_DEFINE_CATEGORY(IPARPI)\n>\n> @@ -111,7 +114,8 @@ private:\n> void reportMetadata();\n> void fillDeviceStatus(const ControlList &sensorControls);\n> void processStats(unsigned int bufferId);\n> - void applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration);\n> + void applyFrameDurations(const Duration &minFrameDuration,\n> + const Duration &maxFrameDuration);\n> void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls);\n> void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> &ctrls);\n> void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> @@ -167,9 +171,9 @@ private:\n> /* Distinguish the first camera start from others. */\n> bool firstStart_;\n>\n> - /* Frame duration (1/fps) limits, given in microseconds. */\n> - double minFrameDuration_;\n> - double maxFrameDuration_;\n> + /* Frame duration (1/fps) limits. */\n> + Duration minFrameDuration_;\n> + Duration maxFrameDuration_;\n> };\n>\n> int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig\n> *sensorConfig)\n> @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);\n>\n> /*\n> - * Calculate the line length in nanoseconds as the ratio between\n> - * the line length in pixels and the pixel rate.\n> + * Calculate the line length as the ratio between the line length\n> in\n> + * pixels and the pixel rate.\n> */\n> - mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate;\n> + mode_.line_length = sensorInfo.lineLength * (1.0s /\n> sensorInfo.pixelRate);\n>\n> /*\n> * Set the frame length limits for the mode to ensure exposure and\n> @@ -387,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> /* Supply initial values for gain and exposure. */\n> ControlList ctrls(sensorCtrls_);\n> AgcStatus agcStatus;\n> - agcStatus.shutter_time = DefaultExposureTime;\n> + agcStatus.shutter_time =\n> DefaultExposureTime.get();\n> agcStatus.analogue_gain = DefaultAnalogueGain;\n> applyAGC(&agcStatus, ctrls);\n>\n> @@ -862,7 +866,7 @@ void IPARPi::queueRequest(const ControlList &controls)\n>\n> case controls::FRAME_DURATIONS: {\n> auto frameDurations = ctrl.second.get int64_t>>();\n> - applyFrameDurations(frameDurations[0],\n> frameDurations[1]);\n> + applyFrameDurations(frameDurations[0] * 1.0us,\n> frameDurations[1] * 1.0us);\n> break;\n> }\n>\n> @@ -937,9 +941,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig\n> &data)\n> returnEmbeddedBuffer(data.embeddedBufferId);\n>\n> /* Allow a 10% margin on the comparison below. */\n> - constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;\n> + Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> - frameTimestamp - lastRunTimestamp_ + eps <\n> controllerMinFrameDuration * 1e3) {\n> + delta < controllerMinFrameDuration * 0.9) {\n> /*\n> * Ensure we merge the previous frame's metadata with the\n> current\n> * frame. This will not overwrite exposure/gain values for\n> the\n> @@ -1012,7 +1016,7 @@ void IPARPi::fillDeviceStatus(const ControlList\n> &sensorControls)\n> int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>\n> - deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> + deviceStatus.shutter_speed =\n> helper_->Exposure(exposureLines).get();\n> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>\n> LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> @@ -1057,10 +1061,11 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> static_cast(awbStatus->gain_b * 1000));\n> }\n>\n> -void IPARPi::applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration)\n> +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,\n> + const Duration &maxFrameDuration)\n> {\n> - const double minSensorFrameDuration = 1e-3 *\n> mode_.min_frame_length * mode_.line_length;\n> - const double maxSensorFrameDuration = 1e-3 *\n> mode_.max_frame_length * mode_.line_length;\n> + const Duration minSensorFrameDuration = mode_.min_frame_length *\n> mode_.line_length;\n> + const Duration maxSensorFrameDuration = mode_.max_frame_length *\n> mode_.line_length;\n>\n> /*\n> * This will only be applied once AGC recalculations occur.\n> @@ -1076,20 +1081,20 @@ void IPARPi::applyFrameDurations(double\n> minFrameDuration, double maxFrameDuratio\n>\n> /* Return the validated limits via metadata. */\n> libcameraMetadata_.set(controls::FrameDurations,\n> - { static_cast(minFrameDuration_),\n> - static_cast(maxFrameDuration_)\n> });\n> + {\n> static_cast(minFrameDuration_.get()),\n> +\n> static_cast(maxFrameDuration_.get()) });\n>\n> /*\n> * Calculate the maximum exposure time possible for the AGC to use.\n> * GetVBlanking() will update maxShutter with the largest exposure\n> * value possible.\n> */\n> - double maxShutter = std::numeric_limits::max();\n> + Duration maxShutter = Duration::max();\n> helper_->GetVBlanking(maxShutter, minFrameDuration_,\n> maxFrameDuration_);\n>\n> RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> controller_.GetAlgorithm(\"agc\"));\n> - agc->SetMaxShutter(maxShutter);\n> + agc->SetMaxShutter(maxShutter.get());\n> }\n>\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> @@ -1097,9 +1102,8 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n>\n> /* GetVBlanking might clip exposure time to the fps limits. */\n> - double exposure = agcStatus->shutter_time;\n> - int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_,\n> - maxFrameDuration_);\n> + Duration exposure = agcStatus->shutter_time * 1.0us;\n> + int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_, maxFrameDuration_);\n> int32_t exposureLines = helper_->ExposureLines(exposure);\n>\n> LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> --\n> 2.25.1\n>\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 61135C3208\n\tfor ;\n\tWed, 2 Jun 2021 13:11:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DCE26050E;\n\tWed, 2 Jun 2021 15:11:23 +0200 (CEST)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98652602A4\n\tfor ;\n\tWed, 2 Jun 2021 15:11:21 +0200 (CEST)","by mail-lf1-x12a.google.com with SMTP id v8so3300165lft.8\n\tfor ;\n\tWed, 02 Jun 2021 06:11:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"WapAKtHP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=xP+cVuGVEurCq8iO/8hFPvP4BXlFtiAeaDKwFPtUzbQ=;\n\tb=WapAKtHP4XQNPZqmP08l/gfrCgR2DSUHbOaGK42rsInINa9CwxcFMhLkUav1waZbA5\n\tW/zeZw9caJsBipNZp+0nGhdPPm6oVwl7jyb+HMWL41vZzTXnqfcCC8NEG2PpfIpMYDtI\n\t6SIjk8pap/2FPlOgxtvkApfTcH3/hFLTOiDVkgF/XJu+NhSwHnxG6KqAhIhmupP7r+pN\n\thQJ3NQW4aSFPvDRRvmzWvC6rwvpwkHf4IwtKT5mkmUQiOq5jtQhdb/jCxwzS+JVMSbSy\n\tqu6l+Fk6XzrmBH+huoeIzRsTEyKaXZfbhixX3ADd9E7gso1oNfK6a6S4fEgpvXh/YW68\n\t033w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=xP+cVuGVEurCq8iO/8hFPvP4BXlFtiAeaDKwFPtUzbQ=;\n\tb=T11vez9HaBlxVbXnkvK4wMRp1flIYrM1ekTdnCyYsTKFVEt8kXH7u2fMmDm7Jo5T+M\n\tDUMRL2dDCSqAHaSq/JySMdReba9tHBm8o+B7hpiZDfWZO/0Y0CQJLJFJ/EVI9jbsSpWR\n\tfq8Tad2UftktednKQHx5os4qAFn2k+hKVjShZJTFYTW/2vdVwf3BLgumHfYJuxntpLHc\n\ttE2QsTG9sGJCgGLMuYuimVQ5/jioWhjNykT3/M7tfuvZqkEOrPAaDjwZLoyHrZ9U4S6Z\n\tGDm/Wpr0XgFKpzdfC86gvpKubtpLZg0K/w/UjWBVIwBJJhCjlEc7wWuXBNsxNT+hwRWL\n\tTw1Q==","X-Gm-Message-State":"AOAM530HauXX97rySC99nmGx6Koa53TiZncCM23IBN2MnsBZK3wKZWDw\n\t3ZHBWNeqphrrnyulirEc6BRh4FdQOdEtgGOLDOlhHcPkH0A=","X-Google-Smtp-Source":"ABdhPJwLF9uyG7rVSvCnDoc7PPdU4yXm3EbKCVAolmcvm9erYoaQpb+lgFFdDWp5/62YBQ5SvSqfcjZDzg+BXTQOD10=","X-Received":"by 2002:a05:6512:2215:: with SMTP id\n\th21mr22194364lfu.375.1622639480600; \n\tWed, 02 Jun 2021 06:11:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20210525114241.906280-1-naush@raspberrypi.com>\n\t<20210525114241.906280-3-naush@raspberrypi.com>","In-Reply-To":"<20210525114241.906280-3-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Wed, 2 Jun 2021 14:11:04 +0100","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"000000000000d70dfc05c3c83150\"","Subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17460,"web_url":"https://patchwork.libcamera.org/comment/17460/","msgid":"","date":"2021-06-08T01:16:58","subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper 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:39PM +0100, Naushir Patuck wrote:\n> Switch the ipa and cam_helper code to use libcamera::utils::Duration for\n> all time based variables. This improves code readability and avoids\n> possible errors when converting between time bases.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 20 +++---\n> src/ipa/raspberrypi/cam_helper.hpp | 10 +--\n> src/ipa/raspberrypi/controller/camera_mode.h | 6 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 64 +++++++++++---------\n> 4 files changed, 55 insertions(+), 45 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 09917f3cc079..7b4331a87a42 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -18,6 +18,7 @@\n> \n> using namespace RPiController;\n> using namespace libcamera;\n> +using libcamera::utils::Duration;\n\nI would have used utils::Duration below, but I don't mind.\n\n> \n> namespace libcamera {\n> LOG_DECLARE_CATEGORY(IPARPI)\n> @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,\n> {\n> }\n> \n> -uint32_t CamHelper::ExposureLines(double exposure_us) const\n> +uint32_t CamHelper::ExposureLines(const Duration &exposure) const\n\nGiven that the duration only stores a double value, you could pass it by\nvalue instead of by reference. The comment applies to the whole series.\nThat would be my preference, but it's up to you.\n\nOverall the code is really much nicer !\n\nReviewed-by: Laurent Pinchart \n\n> {\n> \tassert(initialized_);\n> -\treturn exposure_us * 1000.0 / mode_.line_length;\n> +\treturn exposure / mode_.line_length;\n> }\n> \n> -double CamHelper::Exposure(uint32_t exposure_lines) const\n> +Duration CamHelper::Exposure(uint32_t exposure_lines) const\n> {\n> \tassert(initialized_);\n> -\treturn exposure_lines * mode_.line_length / 1000.0;\n> +\treturn exposure_lines * mode_.line_length;\n> }\n> \n> -uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> -\t\t\t\t double maxFrameDuration) const\n> +uint32_t CamHelper::GetVBlanking(Duration &exposure,\n> +\t\t\t\t const Duration &minFrameDuration,\n> +\t\t\t\t const Duration &maxFrameDuration) const\n> {\n> \tuint32_t frameLengthMin, frameLengthMax, vblank;\n> \tuint32_t exposureLines = ExposureLines(exposure);\n> @@ -85,8 +87,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> \t * minFrameDuration and maxFrameDuration are clamped by the caller\n> \t * based on the limits for the active sensor mode.\n> \t */\n> -\tframeLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> -\tframeLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> +\tframeLengthMin = minFrameDuration / mode_.line_length;\n> +\tframeLengthMax = maxFrameDuration / mode_.line_length;\n> \n> \t/*\n> \t * Limit the exposure to the maximum frame duration requested, and\n> @@ -182,7 +184,7 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> \t\treturn;\n> \t}\n> \n> -\tdeviceStatus.shutter_speed = Exposure(exposureLines);\n> +\tdeviceStatus.shutter_speed = Exposure(exposureLines).get();\n> \tdeviceStatus.analogue_gain = Gain(gainCode);\n> \n> \tLOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index a52f3f0b583c..07481e4174a7 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -15,6 +15,7 @@\n> #include \"controller/metadata.hpp\"\n> #include \"md_parser.hpp\"\n> \n> +#include \"libcamera/internal/utils.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> \n> namespace RPiController {\n> @@ -72,10 +73,11 @@ public:\n> \tvirtual void Prepare(libcamera::Span buffer,\n> \t\t\t Metadata &metadata);\n> \tvirtual void Process(StatisticsPtr &stats, Metadata &metadata);\n> -\tuint32_t ExposureLines(double exposure_us) const;\n> -\tdouble Exposure(uint32_t exposure_lines) const; // in us\n> -\tvirtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,\n> -\t\t\t\t double maxFrameDuration) const;\n> +\tuint32_t ExposureLines(const libcamera::utils::Duration &exposure) const;\n> +\tlibcamera::utils::Duration Exposure(uint32_t exposure_lines) const;\n> +\tvirtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,\n> +\t\t\t\t const libcamera::utils::Duration &minFrameDuration,\n> +\t\t\t\t const libcamera::utils::Duration &maxFrameDuration) const;\n> \tvirtual uint32_t GainCode(double gain) const = 0;\n> \tvirtual double Gain(uint32_t gain_code) const = 0;\n> \tvirtual void GetDelays(int &exposure_delay, int &gain_delay,\n> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h\n> index 256438c931d9..2aa2335dcf90 100644\n> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> @@ -8,6 +8,8 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/utils.h\"\n> +\n> // Description of a \"camera mode\", holding enough information for control\n> // algorithms to adapt their behaviour to the different modes of the camera,\n> // including binning, scaling, cropping etc.\n> @@ -33,8 +35,8 @@ struct CameraMode {\n> \tdouble scale_x, scale_y;\n> \t// scaling of the noise compared to the native sensor mode\n> \tdouble noise_factor;\n> -\t// line time in nanoseconds\n> -\tdouble line_length;\n> +\t// line time\n> +\tlibcamera::utils::Duration line_length;\n> \t// any camera transform *not* reflected already in the camera tuning\n> \tlibcamera::Transform transform;\n> \t// minimum and maximum fame lengths in units of lines\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 52d91db282ea..4e02e94084f7 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -55,19 +55,22 @@\n> \n> namespace libcamera {\n> \n> +using namespace std::literals::chrono_literals;\n> +using utils::Duration;\n> +\n> /* Configure the sensor with these values initially. */\n> constexpr double DefaultAnalogueGain = 1.0;\n> -constexpr unsigned int DefaultExposureTime = 20000;\n> -constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> +constexpr Duration DefaultExposureTime = 20.0ms;\n> +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;\n> +constexpr Duration defaultMaxFrameDuration = 100.0s;\n> \n> /*\n> - * Determine the minimum allowable inter-frame duration (in us) to run the\n> - * controller algorithms. If the pipeline handler provider frames at a rate\n> - * higher than this, we rate-limit the controller Prepare() and Process() calls\n> - * to lower than or equal to this rate.\n> + * Determine the minimum allowable inter-frame duration to run the controller\n> + * algorithms. If the pipeline handler provider frames at a rate higher than this,\n> + * we rate-limit the controller Prepare() and Process() calls to lower than or\n> + * equal to this rate.\n> */\n> -constexpr double controllerMinFrameDuration = 1e6 / 60.0;\n> +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;\n> \n> LOG_DEFINE_CATEGORY(IPARPI)\n> \n> @@ -111,7 +114,8 @@ private:\n> \tvoid reportMetadata();\n> \tvoid fillDeviceStatus(const ControlList &sensorControls);\n> \tvoid processStats(unsigned int bufferId);\n> -\tvoid applyFrameDurations(double minFrameDuration, double maxFrameDuration);\n> +\tvoid applyFrameDurations(const Duration &minFrameDuration,\n> +\t\t\t\t const Duration &maxFrameDuration);\n> \tvoid applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> \tvoid applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> \tvoid applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> @@ -167,9 +171,9 @@ private:\n> \t/* Distinguish the first camera start from others. */\n> \tbool firstStart_;\n> \n> -\t/* Frame duration (1/fps) limits, given in microseconds. */\n> -\tdouble minFrameDuration_;\n> -\tdouble maxFrameDuration_;\n> +\t/* Frame duration (1/fps) limits. */\n> +\tDuration minFrameDuration_;\n> +\tDuration maxFrameDuration_;\n> };\n> \n> int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)\n> @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> \tmode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);\n> \n> \t/*\n> -\t * Calculate the line length in nanoseconds as the ratio between\n> -\t * the line length in pixels and the pixel rate.\n> +\t * Calculate the line length as the ratio between the line length in\n> +\t * pixels and the pixel rate.\n> \t */\n> -\tmode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;\n> +\tmode_.line_length = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate);\n> \n> \t/*\n> \t * Set the frame length limits for the mode to ensure exposure and\n> @@ -387,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\t/* Supply initial values for gain and exposure. */\n> \t\tControlList ctrls(sensorCtrls_);\n> \t\tAgcStatus agcStatus;\n> -\t\tagcStatus.shutter_time = DefaultExposureTime;\n> +\t\tagcStatus.shutter_time = DefaultExposureTime.get();\n> \t\tagcStatus.analogue_gain = DefaultAnalogueGain;\n> \t\tapplyAGC(&agcStatus, ctrls);\n> \n> @@ -862,7 +866,7 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \n> \t\tcase controls::FRAME_DURATIONS: {\n> \t\t\tauto frameDurations = ctrl.second.get>();\n> -\t\t\tapplyFrameDurations(frameDurations[0], frameDurations[1]);\n> +\t\t\tapplyFrameDurations(frameDurations[0] * 1.0us, frameDurations[1] * 1.0us);\n> \t\t\tbreak;\n> \t\t}\n> \n> @@ -937,9 +941,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> \t\treturnEmbeddedBuffer(data.embeddedBufferId);\n> \n> \t/* Allow a 10% margin on the comparison below. */\n> -\tconstexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;\n> +\tDuration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> \tif (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> -\t frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) {\n> +\t delta < controllerMinFrameDuration * 0.9) {\n> \t\t/*\n> \t\t * Ensure we merge the previous frame's metadata with the current\n> \t\t * frame. This will not overwrite exposure/gain values for the\n> @@ -1012,7 +1016,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n> \tint32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> \tint32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> \n> -\tdeviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> +\tdeviceStatus.shutter_speed = helper_->Exposure(exposureLines).get();\n> \tdeviceStatus.analogue_gain = helper_->Gain(gainCode);\n> \n> \tLOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> @@ -1057,10 +1061,11 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> \t\t static_cast(awbStatus->gain_b * 1000));\n> }\n> \n> -void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)\n> +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,\n> +\t\t\t\t const Duration &maxFrameDuration)\n> {\n> -\tconst double minSensorFrameDuration = 1e-3 * mode_.min_frame_length * mode_.line_length;\n> -\tconst double maxSensorFrameDuration = 1e-3 * mode_.max_frame_length * mode_.line_length;\n> +\tconst Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;\n> +\tconst Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;\n> \n> \t/*\n> \t * This will only be applied once AGC recalculations occur.\n> @@ -1076,20 +1081,20 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio\n> \n> \t/* Return the validated limits via metadata. */\n> \tlibcameraMetadata_.set(controls::FrameDurations,\n> -\t\t\t { static_cast(minFrameDuration_),\n> -\t\t\t\t static_cast(maxFrameDuration_) });\n> +\t\t\t { static_cast(minFrameDuration_.get()),\n> +\t\t\t\t static_cast(maxFrameDuration_.get()) });\n> \n> \t/*\n> \t * Calculate the maximum exposure time possible for the AGC to use.\n> \t * GetVBlanking() will update maxShutter with the largest exposure\n> \t * value possible.\n> \t */\n> -\tdouble maxShutter = std::numeric_limits::max();\n> +\tDuration maxShutter = Duration::max();\n> \thelper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);\n> \n> \tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\tagc->SetMaxShutter(maxShutter);\n> +\tagc->SetMaxShutter(maxShutter.get());\n> }\n> \n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> @@ -1097,9 +1102,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> \tint32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> \n> \t/* GetVBlanking might clip exposure time to the fps limits. */\n> -\tdouble exposure = agcStatus->shutter_time;\n> -\tint32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,\n> -\t\t\t\t\t\t maxFrameDuration_);\n> +\tDuration exposure = agcStatus->shutter_time * 1.0us;\n> +\tint32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);\n> \tint32_t exposureLines = helper_->ExposureLines(exposure);\n> \n> \tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure","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 22F72C3206\n\tfor ;\n\tTue, 8 Jun 2021 01:17:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D1EB6892D;\n\tTue, 8 Jun 2021 03:17:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 40FCA6891C\n\tfor ;\n\tTue, 8 Jun 2021 03:17:13 +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 B51BB4AD;\n\tTue, 8 Jun 2021 03:17:12 +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=\"PTRQTW70\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623115032;\n\tbh=Fi5BkrL9FPCOwZHu+qBTQnz09dMJLyqipu9dK4TCtrw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PTRQTW703yPK7wZx9H1aNaTymgchfSIylJRdp0wLUomVnW6Pp0xg0LDABeAD3xnV9\n\t70y8e0Jep8Wyj4BIvorjNIpRPqG8vTQnWG6xQP5uwGdqnyvMOVdoPbAnykkpbLS+ze\n\tZaszIvx11eSynT/ocFmJKCfumOHLFZZsb598z4r0=","Date":"Tue, 8 Jun 2021 04:16:58 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210525114241.906280-1-naush@raspberrypi.com>\n\t<20210525114241.906280-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210525114241.906280-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper 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\" "}},{"id":17475,"web_url":"https://patchwork.libcamera.org/comment/17475/","msgid":"","date":"2021-06-08T09:47:47","subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper to use utils::Duration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your review feedback.\n\nOn Tue, 8 Jun 2021 at 02:17, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, May 25, 2021 at 12:42:39PM +0100, Naushir Patuck wrote:\n> > Switch the ipa and cam_helper code to use libcamera::utils::Duration for\n> > all time based variables. This improves code readability and avoids\n> > possible errors when converting between time bases.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 20 +++---\n> > src/ipa/raspberrypi/cam_helper.hpp | 10 +--\n> > src/ipa/raspberrypi/controller/camera_mode.h | 6 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 64 +++++++++++---------\n> > 4 files changed, 55 insertions(+), 45 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index 09917f3cc079..7b4331a87a42 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -18,6 +18,7 @@\n> >\n> > using namespace RPiController;\n> > using namespace libcamera;\n> > +using libcamera::utils::Duration;\n>\n> I would have used utils::Duration below, but I don't mind.\n>\n> >\n> > namespace libcamera {\n> > LOG_DECLARE_CATEGORY(IPARPI)\n> > @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]]\n> StatisticsPtr &stats,\n> > {\n> > }\n> >\n> > -uint32_t CamHelper::ExposureLines(double exposure_us) const\n> > +uint32_t CamHelper::ExposureLines(const Duration &exposure) const\n>\n> Given that the duration only stores a double value, you could pass it by\n> value instead of by reference. The comment applies to the whole series.\n> That would be my preference, but it's up to you.\n>\n\nAgreed, I will change all functions to pass by value in the next revision.\n\n\n>\n> Overall the code is really much nicer !\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nThanks. Yes, this change does make time based variables so much easier\nto work with! At some point, we should change libcamera controls to use\nthis :-)\n\nRegards,\nNaush\n\n\n> > {\n> > assert(initialized_);\n> > - return exposure_us * 1000.0 / mode_.line_length;\n> > + return exposure / mode_.line_length;\n> > }\n> >\n> > -double CamHelper::Exposure(uint32_t exposure_lines) const\n> > +Duration CamHelper::Exposure(uint32_t exposure_lines) const\n> > {\n> > assert(initialized_);\n> > - return exposure_lines * mode_.line_length / 1000.0;\n> > + return exposure_lines * mode_.line_length;\n> > }\n> >\n> > -uint32_t CamHelper::GetVBlanking(double &exposure, double\n> minFrameDuration,\n> > - double maxFrameDuration) const\n> > +uint32_t CamHelper::GetVBlanking(Duration &exposure,\n> > + const Duration &minFrameDuration,\n> > + const Duration &maxFrameDuration) const\n> > {\n> > uint32_t frameLengthMin, frameLengthMax, vblank;\n> > uint32_t exposureLines = ExposureLines(exposure);\n> > @@ -85,8 +87,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure,\n> double minFrameDuration,\n> > * minFrameDuration and maxFrameDuration are clamped by the caller\n> > * based on the limits for the active sensor mode.\n> > */\n> > - frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> > - frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> > + frameLengthMin = minFrameDuration / mode_.line_length;\n> > + frameLengthMax = maxFrameDuration / mode_.line_length;\n> >\n> > /*\n> > * Limit the exposure to the maximum frame duration requested, and\n> > @@ -182,7 +184,7 @@ void CamHelper::parseEmbeddedData(Span uint8_t> buffer,\n> > return;\n> > }\n> >\n> > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > + deviceStatus.shutter_speed =\n> Exposure(exposureLines).get();\n> > deviceStatus.analogue_gain = Gain(gainCode);\n> >\n> > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > index a52f3f0b583c..07481e4174a7 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -15,6 +15,7 @@\n> > #include \"controller/metadata.hpp\"\n> > #include \"md_parser.hpp\"\n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > #include \"libcamera/internal/v4l2_videodevice.h\"\n> >\n> > namespace RPiController {\n> > @@ -72,10 +73,11 @@ public:\n> > virtual void Prepare(libcamera::Span buffer,\n> > Metadata &metadata);\n> > virtual void Process(StatisticsPtr &stats, Metadata &metadata);\n> > - uint32_t ExposureLines(double exposure_us) const;\n> > - double Exposure(uint32_t exposure_lines) const; // in us\n> > - virtual uint32_t GetVBlanking(double &exposure_us, double\n> minFrameDuration,\n> > - double maxFrameDuration) const;\n> > + uint32_t ExposureLines(const libcamera::utils::Duration &exposure)\n> const;\n> > + libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;\n> > + virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,\n> > + const libcamera::utils::Duration\n> &minFrameDuration,\n> > + const libcamera::utils::Duration\n> &maxFrameDuration) const;\n> > virtual uint32_t GainCode(double gain) const = 0;\n> > virtual double Gain(uint32_t gain_code) const = 0;\n> > virtual void GetDelays(int &exposure_delay, int &gain_delay,\n> > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> > index 256438c931d9..2aa2335dcf90 100644\n> > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > @@ -8,6 +8,8 @@\n> >\n> > #include \n> >\n> > +#include \"libcamera/internal/utils.h\"\n> > +\n> > // Description of a \"camera mode\", holding enough information for\n> control\n> > // algorithms to adapt their behaviour to the different modes of the\n> camera,\n> > // including binning, scaling, cropping etc.\n> > @@ -33,8 +35,8 @@ struct CameraMode {\n> > double scale_x, scale_y;\n> > // scaling of the noise compared to the native sensor mode\n> > double noise_factor;\n> > - // line time in nanoseconds\n> > - double line_length;\n> > + // line time\n> > + libcamera::utils::Duration line_length;\n> > // any camera transform *not* reflected already in the camera\n> tuning\n> > libcamera::Transform transform;\n> > // minimum and maximum fame lengths in units of lines\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 52d91db282ea..4e02e94084f7 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -55,19 +55,22 @@\n> >\n> > namespace libcamera {\n> >\n> > +using namespace std::literals::chrono_literals;\n> > +using utils::Duration;\n> > +\n> > /* Configure the sensor with these values initially. */\n> > constexpr double DefaultAnalogueGain = 1.0;\n> > -constexpr unsigned int DefaultExposureTime = 20000;\n> > -constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> > +constexpr Duration DefaultExposureTime = 20.0ms;\n> > +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;\n> > +constexpr Duration defaultMaxFrameDuration = 100.0s;\n> >\n> > /*\n> > - * Determine the minimum allowable inter-frame duration (in us) to run\n> the\n> > - * controller algorithms. If the pipeline handler provider frames at a\n> rate\n> > - * higher than this, we rate-limit the controller Prepare() and\n> Process() calls\n> > - * to lower than or equal to this rate.\n> > + * Determine the minimum allowable inter-frame duration to run the\n> controller\n> > + * algorithms. If the pipeline handler provider frames at a rate higher\n> than this,\n> > + * we rate-limit the controller Prepare() and Process() calls to lower\n> than or\n> > + * equal to this rate.\n> > */\n> > -constexpr double controllerMinFrameDuration = 1e6 / 60.0;\n> > +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;\n> >\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> > @@ -111,7 +114,8 @@ private:\n> > void reportMetadata();\n> > void fillDeviceStatus(const ControlList &sensorControls);\n> > void processStats(unsigned int bufferId);\n> > - void applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration);\n> > + void applyFrameDurations(const Duration &minFrameDuration,\n> > + const Duration &maxFrameDuration);\n> > void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls);\n> > void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> &ctrls);\n> > void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> > @@ -167,9 +171,9 @@ private:\n> > /* Distinguish the first camera start from others. */\n> > bool firstStart_;\n> >\n> > - /* Frame duration (1/fps) limits, given in microseconds. */\n> > - double minFrameDuration_;\n> > - double maxFrameDuration_;\n> > + /* Frame duration (1/fps) limits. */\n> > + Duration minFrameDuration_;\n> > + Duration maxFrameDuration_;\n> > };\n> >\n> > int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig\n> *sensorConfig)\n> > @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> > mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);\n> >\n> > /*\n> > - * Calculate the line length in nanoseconds as the ratio between\n> > - * the line length in pixels and the pixel rate.\n> > + * Calculate the line length as the ratio between the line length\n> in\n> > + * pixels and the pixel rate.\n> > */\n> > - mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate;\n> > + mode_.line_length = sensorInfo.lineLength * (1.0s /\n> sensorInfo.pixelRate);\n> >\n> > /*\n> > * Set the frame length limits for the mode to ensure exposure and\n> > @@ -387,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > /* Supply initial values for gain and exposure. */\n> > ControlList ctrls(sensorCtrls_);\n> > AgcStatus agcStatus;\n> > - agcStatus.shutter_time = DefaultExposureTime;\n> > + agcStatus.shutter_time =\n> DefaultExposureTime.get();\n> > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > applyAGC(&agcStatus, ctrls);\n> >\n> > @@ -862,7 +866,7 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> >\n> > case controls::FRAME_DURATIONS: {\n> > auto frameDurations = ctrl.second.get int64_t>>();\n> > - applyFrameDurations(frameDurations[0],\n> frameDurations[1]);\n> > + applyFrameDurations(frameDurations[0] * 1.0us,\n> frameDurations[1] * 1.0us);\n> > break;\n> > }\n> >\n> > @@ -937,9 +941,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig\n> &data)\n> > returnEmbeddedBuffer(data.embeddedBufferId);\n> >\n> > /* Allow a 10% margin on the comparison below. */\n> > - constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;\n> > + Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > - frameTimestamp - lastRunTimestamp_ + eps <\n> controllerMinFrameDuration * 1e3) {\n> > + delta < controllerMinFrameDuration * 0.9) {\n> > /*\n> > * Ensure we merge the previous frame's metadata with the\n> current\n> > * frame. This will not overwrite exposure/gain values for\n> the\n> > @@ -1012,7 +1016,7 @@ void IPARPi::fillDeviceStatus(const ControlList\n> &sensorControls)\n> > int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >\n> > - deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > + deviceStatus.shutter_speed =\n> helper_->Exposure(exposureLines).get();\n> > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> >\n> > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > @@ -1057,10 +1061,11 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > static_cast(awbStatus->gain_b * 1000));\n> > }\n> >\n> > -void IPARPi::applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration)\n> > +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,\n> > + const Duration &maxFrameDuration)\n> > {\n> > - const double minSensorFrameDuration = 1e-3 *\n> mode_.min_frame_length * mode_.line_length;\n> > - const double maxSensorFrameDuration = 1e-3 *\n> mode_.max_frame_length * mode_.line_length;\n> > + const Duration minSensorFrameDuration = mode_.min_frame_length *\n> mode_.line_length;\n> > + const Duration maxSensorFrameDuration = mode_.max_frame_length *\n> mode_.line_length;\n> >\n> > /*\n> > * This will only be applied once AGC recalculations occur.\n> > @@ -1076,20 +1081,20 @@ void IPARPi::applyFrameDurations(double\n> minFrameDuration, double maxFrameDuratio\n> >\n> > /* Return the validated limits via metadata. */\n> > libcameraMetadata_.set(controls::FrameDurations,\n> > - { static_cast(minFrameDuration_),\n> > - static_cast(maxFrameDuration_)\n> });\n> > + {\n> static_cast(minFrameDuration_.get()),\n> > +\n> static_cast(maxFrameDuration_.get()) });\n> >\n> > /*\n> > * Calculate the maximum exposure time possible for the AGC to use.\n> > * GetVBlanking() will update maxShutter with the largest exposure\n> > * value possible.\n> > */\n> > - double maxShutter = std::numeric_limits::max();\n> > + Duration maxShutter = Duration::max();\n> > helper_->GetVBlanking(maxShutter, minFrameDuration_,\n> maxFrameDuration_);\n> >\n> > RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> > controller_.GetAlgorithm(\"agc\"));\n> > - agc->SetMaxShutter(maxShutter);\n> > + agc->SetMaxShutter(maxShutter.get());\n> > }\n> >\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> > @@ -1097,9 +1102,8 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> >\n> > /* GetVBlanking might clip exposure time to the fps limits. */\n> > - double exposure = agcStatus->shutter_time;\n> > - int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_,\n> > - maxFrameDuration_);\n> > + Duration exposure = agcStatus->shutter_time * 1.0us;\n> > + int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_, maxFrameDuration_);\n> > int32_t exposureLines = helper_->ExposureLines(exposure);\n> >\n> > LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 F20C2BD22E\n\tfor ;\n\tTue, 8 Jun 2021 09:48:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 44DE46892F;\n\tTue, 8 Jun 2021 11:48:06 +0200 (CEST)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9453A6029E\n\tfor ;\n\tTue, 8 Jun 2021 11:48:04 +0200 (CEST)","by mail-lj1-x232.google.com with SMTP id x14so11834141ljp.7\n\tfor ;\n\tTue, 08 Jun 2021 02:48:04 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"V0iSC+1r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=MeMBj7dfSdL87PmkbT+QG1Tg4SRi8QqsvRzuflqeQFo=;\n\tb=V0iSC+1r1MRCAOg/tIXMuJ/DhtXNcp/hjP++pkMrhbCAGQNzaes/tCvLXUouvQ+9Dg\n\t/Q6Hf+E0ftWtJfrvqOLUdHc3xctprdguUNgg/hShRgQTbJzXUshVWKsR3sRF2q5bY6h3\n\t5TVxILfwT7NyeW4t4klsPoIy9oqUxKvqjLQBZAYQqaP3OdFAPHLgAIvhVz1auipch9SV\n\ta34xNAsO6k5JAbl15wC9FMkGAfSpbmt7MIUDEDLSE3R+OlbJAxhSRYBPceeiW9yOYVCM\n\tbK01Me8jsRPwQ6GdP+ScxDRhdv/XriP/2m3QhmE7pR26HkAV4OXFbM6EOAi+nWWm/D6y\n\tXagw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=MeMBj7dfSdL87PmkbT+QG1Tg4SRi8QqsvRzuflqeQFo=;\n\tb=GJn/1iLkxBybXmDjXopeZ0hYTrMmBCxq0YHhoHu6hiL/lpJSzqCVB9EYXEyiSwqCMG\n\taN67zOLak76ahi/FTehiPvGH2j7F1mTaLgTulg0sZ7xLCd+FWCAqCQavF4fevmMlpNyJ\n\tq3biFI4YRdVnuGI+A/DPDg22D7FjM43L2rMlvqziz1ws+AvY/Ej9ge24XAoRiEOUVCeB\n\tcp0oYuLKKEuCM2DYCeOg4RhlFjhrXdOHA5acegBWYYwb5osetXGkOMw2fP1Q2buq8wxm\n\twG1cHuhAZJIDQSyfmF/QF7X5S3CZSp60lftRn7qsnqJWpE2/9M0A2v3ydttizjOtDJra\n\tl0dg==","X-Gm-Message-State":"AOAM530b4BEXqCMGRkYqQSNkp0rpTNoUKSnTuMiOHB3BH3YiUMdtgK24\n\tehOvnP36iPoMneAa3Un0WwD2sat5R1lXOl7JvzF/SQ==","X-Google-Smtp-Source":"ABdhPJywjNwd70gaXDIaFsSjgzfK4Fk+s7WcyEhe+osRzukcUpVIqJaGXv2ihQsJ1Keo4VwVQp9zSqQxFzcCWjR5Fbs=","X-Received":"by 2002:a2e:8e78:: with SMTP id\n\tt24mr17701501ljk.499.1623145683918; \n\tTue, 08 Jun 2021 02:48:03 -0700 (PDT)","MIME-Version":"1.0","References":"<20210525114241.906280-1-naush@raspberrypi.com>\n\t<20210525114241.906280-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 8 Jun 2021 10:47:47 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000e8b5e105c43e0d51\"","Subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17476,"web_url":"https://patchwork.libcamera.org/comment/17476/","msgid":"","date":"2021-06-08T09:53:01","subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper 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\nOn Tue, Jun 08, 2021 at 10:47:47AM +0100, Naushir Patuck wrote:\n> On Tue, 8 Jun 2021 at 02:17, Laurent Pinchart wrote:\n> > On Tue, May 25, 2021 at 12:42:39PM +0100, Naushir Patuck wrote:\n> > > Switch the ipa and cam_helper code to use libcamera::utils::Duration for\n> > > all time based variables. This improves code readability and avoids\n> > > possible errors when converting between time bases.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 20 +++---\n> > > src/ipa/raspberrypi/cam_helper.hpp | 10 +--\n> > > src/ipa/raspberrypi/controller/camera_mode.h | 6 +-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 64 +++++++++++---------\n> > > 4 files changed, 55 insertions(+), 45 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index 09917f3cc079..7b4331a87a42 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -18,6 +18,7 @@\n> > >\n> > > using namespace RPiController;\n> > > using namespace libcamera;\n> > > +using libcamera::utils::Duration;\n> >\n> > I would have used utils::Duration below, but I don't mind.\n> >\n> > >\n> > > namespace libcamera {\n> > > LOG_DECLARE_CATEGORY(IPARPI)\n> > > @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,\n> > > {\n> > > }\n> > >\n> > > -uint32_t CamHelper::ExposureLines(double exposure_us) const\n> > > +uint32_t CamHelper::ExposureLines(const Duration &exposure) const\n> >\n> > Given that the duration only stores a double value, you could pass it by\n> > value instead of by reference. The comment applies to the whole series.\n> > That would be my preference, but it's up to you.\n> \n> Agreed, I will change all functions to pass by value in the next revision.\n> \n> > Overall the code is really much nicer !\n> >\n> > Reviewed-by: Laurent Pinchart \n> \n> Thanks. Yes, this change does make time based variables so much easier\n> to work with! At some point, we should change libcamera controls to use\n> this :-)\n\nThat's on my todo list :-)\n\nBy the way, would it be possible to add a test case for the Duration\nclass in the next version ?\n\n> > > {\n> > > assert(initialized_);\n> > > - return exposure_us * 1000.0 / mode_.line_length;\n> > > + return exposure / mode_.line_length;\n> > > }\n> > >\n> > > -double CamHelper::Exposure(uint32_t exposure_lines) const\n> > > +Duration CamHelper::Exposure(uint32_t exposure_lines) const\n> > > {\n> > > assert(initialized_);\n> > > - return exposure_lines * mode_.line_length / 1000.0;\n> > > + return exposure_lines * mode_.line_length;\n> > > }\n> > >\n> > > -uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> > > - double maxFrameDuration) const\n> > > +uint32_t CamHelper::GetVBlanking(Duration &exposure,\n> > > + const Duration &minFrameDuration,\n> > > + const Duration &maxFrameDuration) const\n> > > {\n> > > uint32_t frameLengthMin, frameLengthMax, vblank;\n> > > uint32_t exposureLines = ExposureLines(exposure);\n> > > @@ -85,8 +87,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> > > * minFrameDuration and maxFrameDuration are clamped by the caller\n> > > * based on the limits for the active sensor mode.\n> > > */\n> > > - frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> > > - frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> > > + frameLengthMin = minFrameDuration / mode_.line_length;\n> > > + frameLengthMax = maxFrameDuration / mode_.line_length;\n> > >\n> > > /*\n> > > * Limit the exposure to the maximum frame duration requested, and\n> > > @@ -182,7 +184,7 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> > > return;\n> > > }\n> > >\n> > > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > > + deviceStatus.shutter_speed = Exposure(exposureLines).get();\n> > > deviceStatus.analogue_gain = Gain(gainCode);\n> > >\n> > > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> > > index a52f3f0b583c..07481e4174a7 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > @@ -15,6 +15,7 @@\n> > > #include \"controller/metadata.hpp\"\n> > > #include \"md_parser.hpp\"\n> > >\n> > > +#include \"libcamera/internal/utils.h\"\n> > > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > >\n> > > namespace RPiController {\n> > > @@ -72,10 +73,11 @@ public:\n> > > virtual void Prepare(libcamera::Span buffer,\n> > > Metadata &metadata);\n> > > virtual void Process(StatisticsPtr &stats, Metadata &metadata);\n> > > - uint32_t ExposureLines(double exposure_us) const;\n> > > - double Exposure(uint32_t exposure_lines) const; // in us\n> > > - virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,\n> > > - double maxFrameDuration) const;\n> > > + uint32_t ExposureLines(const libcamera::utils::Duration &exposure) const;\n> > > + libcamera::utils::Duration Exposure(uint32_t exposure_lines) const;\n> > > + virtual uint32_t GetVBlanking(libcamera::utils::Duration &exposure,\n> > > + const libcamera::utils::Duration &minFrameDuration,\n> > > + const libcamera::utils::Duration &maxFrameDuration) const;\n> > > virtual uint32_t GainCode(double gain) const = 0;\n> > > virtual double Gain(uint32_t gain_code) const = 0;\n> > > virtual void GetDelays(int &exposure_delay, int &gain_delay,\n> > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > index 256438c931d9..2aa2335dcf90 100644\n> > > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > @@ -8,6 +8,8 @@\n> > >\n> > > #include \n> > >\n> > > +#include \"libcamera/internal/utils.h\"\n> > > +\n> > > // Description of a \"camera mode\", holding enough information for control\n> > > // algorithms to adapt their behaviour to the different modes of the camera,\n> > > // including binning, scaling, cropping etc.\n> > > @@ -33,8 +35,8 @@ struct CameraMode {\n> > > double scale_x, scale_y;\n> > > // scaling of the noise compared to the native sensor mode\n> > > double noise_factor;\n> > > - // line time in nanoseconds\n> > > - double line_length;\n> > > + // line time\n> > > + libcamera::utils::Duration line_length;\n> > > // any camera transform *not* reflected already in the camera tuning\n> > > libcamera::Transform transform;\n> > > // minimum and maximum fame lengths in units of lines\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 52d91db282ea..4e02e94084f7 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -55,19 +55,22 @@\n> > >\n> > > namespace libcamera {\n> > >\n> > > +using namespace std::literals::chrono_literals;\n> > > +using utils::Duration;\n> > > +\n> > > /* Configure the sensor with these values initially. */\n> > > constexpr double DefaultAnalogueGain = 1.0;\n> > > -constexpr unsigned int DefaultExposureTime = 20000;\n> > > -constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > > -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> > > +constexpr Duration DefaultExposureTime = 20.0ms;\n> > > +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;\n> > > +constexpr Duration defaultMaxFrameDuration = 100.0s;\n> > >\n> > > /*\n> > > - * Determine the minimum allowable inter-frame duration (in us) to run the\n> > > - * controller algorithms. If the pipeline handler provider frames at a rate\n> > > - * higher than this, we rate-limit the controller Prepare() and Process() calls\n> > > - * to lower than or equal to this rate.\n> > > + * Determine the minimum allowable inter-frame duration to run the controller\n> > > + * algorithms. If the pipeline handler provider frames at a rate higher than this,\n> > > + * we rate-limit the controller Prepare() and Process() calls to lower than or\n> > > + * equal to this rate.\n> > > */\n> > > -constexpr double controllerMinFrameDuration = 1e6 / 60.0;\n> > > +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;\n> > >\n> > > LOG_DEFINE_CATEGORY(IPARPI)\n> > >\n> > > @@ -111,7 +114,8 @@ private:\n> > > void reportMetadata();\n> > > void fillDeviceStatus(const ControlList &sensorControls);\n> > > void processStats(unsigned int bufferId);\n> > > - void applyFrameDurations(double minFrameDuration, double maxFrameDuration);\n> > > + void applyFrameDurations(const Duration &minFrameDuration,\n> > > + const Duration &maxFrameDuration);\n> > > void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > > void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> > > void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> > > @@ -167,9 +171,9 @@ private:\n> > > /* Distinguish the first camera start from others. */\n> > > bool firstStart_;\n> > >\n> > > - /* Frame duration (1/fps) limits, given in microseconds. */\n> > > - double minFrameDuration_;\n> > > - double maxFrameDuration_;\n> > > + /* Frame duration (1/fps) limits. */\n> > > + Duration minFrameDuration_;\n> > > + Duration maxFrameDuration_;\n> > > };\n> > >\n> > > int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)\n> > > @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> > > mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);\n> > >\n> > > /*\n> > > - * Calculate the line length in nanoseconds as the ratio between\n> > > - * the line length in pixels and the pixel rate.\n> > > + * Calculate the line length as the ratio between the line length in\n> > > + * pixels and the pixel rate.\n> > > */\n> > > - mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;\n> > > + mode_.line_length = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate);\n> > >\n> > > /*\n> > > * Set the frame length limits for the mode to ensure exposure and\n> > > @@ -387,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > /* Supply initial values for gain and exposure. */\n> > > ControlList ctrls(sensorCtrls_);\n> > > AgcStatus agcStatus;\n> > > - agcStatus.shutter_time = DefaultExposureTime;\n> > > + agcStatus.shutter_time = DefaultExposureTime.get();\n> > > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > > applyAGC(&agcStatus, ctrls);\n> > >\n> > > @@ -862,7 +866,7 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > >\n> > > case controls::FRAME_DURATIONS: {\n> > > auto frameDurations = ctrl.second.get>();\n> > > - applyFrameDurations(frameDurations[0], frameDurations[1]);\n> > > + applyFrameDurations(frameDurations[0] * 1.0us, frameDurations[1] * 1.0us);\n> > > break;\n> > > }\n> > >\n> > > @@ -937,9 +941,9 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> > > returnEmbeddedBuffer(data.embeddedBufferId);\n> > >\n> > > /* Allow a 10% margin on the comparison below. */\n> > > - constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;\n> > > + Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> > > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > > - frameTimestamp - lastRunTimestamp_ + eps < controllerMinFrameDuration * 1e3) {\n> > > + delta < controllerMinFrameDuration * 0.9) {\n> > > /*\n> > > * Ensure we merge the previous frame's metadata with the current\n> > > * frame. This will not overwrite exposure/gain values for the\n> > > @@ -1012,7 +1016,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n> > > int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> > >\n> > > - deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines).get();\n> > > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> > >\n> > > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > > @@ -1057,10 +1061,11 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > > static_cast(awbStatus->gain_b * 1000));\n> > > }\n> > >\n> > > -void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)\n> > > +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,\n> > > + const Duration &maxFrameDuration)\n> > > {\n> > > - const double minSensorFrameDuration = 1e-3 * mode_.min_frame_length * mode_.line_length;\n> > > - const double maxSensorFrameDuration = 1e-3 * mode_.max_frame_length * mode_.line_length;\n> > > + const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;\n> > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;\n> > >\n> > > /*\n> > > * This will only be applied once AGC recalculations occur.\n> > > @@ -1076,20 +1081,20 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio\n> > >\n> > > /* Return the validated limits via metadata. */\n> > > libcameraMetadata_.set(controls::FrameDurations,\n> > > - { static_cast(minFrameDuration_),\n> > > - static_cast(maxFrameDuration_) });\n> > > + { static_cast(minFrameDuration_.get()),\n> > > + static_cast(maxFrameDuration_.get()) });\n> > >\n> > > /*\n> > > * Calculate the maximum exposure time possible for the AGC to use.\n> > > * GetVBlanking() will update maxShutter with the largest exposure\n> > > * value possible.\n> > > */\n> > > - double maxShutter = std::numeric_limits::max();\n> > > + Duration maxShutter = Duration::max();\n> > > helper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);\n> > >\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - agc->SetMaxShutter(maxShutter);\n> > > + agc->SetMaxShutter(maxShutter.get());\n> > > }\n> > >\n> > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> > > @@ -1097,9 +1102,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > >\n> > > /* GetVBlanking might clip exposure time to the fps limits. */\n> > > - double exposure = agcStatus->shutter_time;\n> > > - int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,\n> > > - maxFrameDuration_);\n> > > + Duration exposure = agcStatus->shutter_time * 1.0us;\n> > > + int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);\n> > > int32_t exposureLines = helper_->ExposureLines(exposure);\n> > >\n> > > LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure","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 DC983C3206\n\tfor ;\n\tTue, 8 Jun 2021 09:53:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6558C6892C;\n\tTue, 8 Jun 2021 11:53:18 +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 7D5FE6029E\n\tfor ;\n\tTue, 8 Jun 2021 11:53:17 +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 EAFC73E6;\n\tTue, 8 Jun 2021 11:53:16 +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=\"pLvYnV1h\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623145997;\n\tbh=nJOWVeKQJAzNuFonecgZWpSTqOJKgrZa0KjZ8vgZRPM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pLvYnV1hz+drlBy/dKIEuQ2w3MjFRD3XwIbT/TUQtqzqCF/foDgO2af/+r7bHeDpq\n\twkL8uEQs7Ht+/+pvtB1N+0jnQSHITMFz9E9rm1tIkVCK4cVx2ipfw7HgxVmAkGA01P\n\tbe2WDbrGyWQdXWHKsbQ/dI9H80i04ShXraICqIXs=","Date":"Tue, 8 Jun 2021 12:53:01 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210525114241.906280-1-naush@raspberrypi.com>\n\t<20210525114241.906280-3-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17477,"web_url":"https://patchwork.libcamera.org/comment/17477/","msgid":"","date":"2021-06-08T09:58:00","subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper to use utils::Duration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Tue, 8 Jun 2021 at 10:53, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Tue, Jun 08, 2021 at 10:47:47AM +0100, Naushir Patuck wrote:\n> > On Tue, 8 Jun 2021 at 02:17, Laurent Pinchart wrote:\n> > > On Tue, May 25, 2021 at 12:42:39PM +0100, Naushir Patuck wrote:\n> > > > Switch the ipa and cam_helper code to use libcamera::utils::Duration\n> for\n> > > > all time based variables. This improves code readability and avoids\n> > > > possible errors when converting between time bases.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/cam_helper.cpp | 20 +++---\n> > > > src/ipa/raspberrypi/cam_helper.hpp | 10 +--\n> > > > src/ipa/raspberrypi/controller/camera_mode.h | 6 +-\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 64\n> +++++++++++---------\n> > > > 4 files changed, 55 insertions(+), 45 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > index 09917f3cc079..7b4331a87a42 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > @@ -18,6 +18,7 @@\n> > > >\n> > > > using namespace RPiController;\n> > > > using namespace libcamera;\n> > > > +using libcamera::utils::Duration;\n> > >\n> > > I would have used utils::Duration below, but I don't mind.\n> > >\n> > > >\n> > > > namespace libcamera {\n> > > > LOG_DECLARE_CATEGORY(IPARPI)\n> > > > @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]]\n> StatisticsPtr &stats,\n> > > > {\n> > > > }\n> > > >\n> > > > -uint32_t CamHelper::ExposureLines(double exposure_us) const\n> > > > +uint32_t CamHelper::ExposureLines(const Duration &exposure) const\n> > >\n> > > Given that the duration only stores a double value, you could pass it\n> by\n> > > value instead of by reference. The comment applies to the whole series.\n> > > That would be my preference, but it's up to you.\n> >\n> > Agreed, I will change all functions to pass by value in the next\n> revision.\n> >\n> > > Overall the code is really much nicer !\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> >\n> > Thanks. Yes, this change does make time based variables so much easier\n> > to work with! At some point, we should change libcamera controls to use\n> > this :-)\n>\n> That's on my todo list :-)\n>\n> By the way, would it be possible to add a test case for the Duration\n> class in the next version ?\n>\n\nSure, I will add some unit tests in the next revision.\n\nNaush\n\n\n\n>\n> > > > {\n> > > > assert(initialized_);\n> > > > - return exposure_us * 1000.0 / mode_.line_length;\n> > > > + return exposure / mode_.line_length;\n> > > > }\n> > > >\n> > > > -double CamHelper::Exposure(uint32_t exposure_lines) const\n> > > > +Duration CamHelper::Exposure(uint32_t exposure_lines) const\n> > > > {\n> > > > assert(initialized_);\n> > > > - return exposure_lines * mode_.line_length / 1000.0;\n> > > > + return exposure_lines * mode_.line_length;\n> > > > }\n> > > >\n> > > > -uint32_t CamHelper::GetVBlanking(double &exposure, double\n> minFrameDuration,\n> > > > - double maxFrameDuration) const\n> > > > +uint32_t CamHelper::GetVBlanking(Duration &exposure,\n> > > > + const Duration &minFrameDuration,\n> > > > + const Duration &maxFrameDuration)\n> const\n> > > > {\n> > > > uint32_t frameLengthMin, frameLengthMax, vblank;\n> > > > uint32_t exposureLines = ExposureLines(exposure);\n> > > > @@ -85,8 +87,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure,\n> double minFrameDuration,\n> > > > * minFrameDuration and maxFrameDuration are clamped by the\n> caller\n> > > > * based on the limits for the active sensor mode.\n> > > > */\n> > > > - frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> > > > - frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> > > > + frameLengthMin = minFrameDuration / mode_.line_length;\n> > > > + frameLengthMax = maxFrameDuration / mode_.line_length;\n> > > >\n> > > > /*\n> > > > * Limit the exposure to the maximum frame duration requested,\n> and\n> > > > @@ -182,7 +184,7 @@ void CamHelper::parseEmbeddedData(Span uint8_t> buffer,\n> > > > return;\n> > > > }\n> > > >\n> > > > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > > > + deviceStatus.shutter_speed =\n> Exposure(exposureLines).get();\n> > > > deviceStatus.analogue_gain = Gain(gainCode);\n> > > >\n> > > > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > > > index a52f3f0b583c..07481e4174a7 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > > @@ -15,6 +15,7 @@\n> > > > #include \"controller/metadata.hpp\"\n> > > > #include \"md_parser.hpp\"\n> > > >\n> > > > +#include \"libcamera/internal/utils.h\"\n> > > > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > > >\n> > > > namespace RPiController {\n> > > > @@ -72,10 +73,11 @@ public:\n> > > > virtual void Prepare(libcamera::Span buffer,\n> > > > Metadata &metadata);\n> > > > virtual void Process(StatisticsPtr &stats, Metadata &metadata);\n> > > > - uint32_t ExposureLines(double exposure_us) const;\n> > > > - double Exposure(uint32_t exposure_lines) const; // in us\n> > > > - virtual uint32_t GetVBlanking(double &exposure_us, double\n> minFrameDuration,\n> > > > - double maxFrameDuration) const;\n> > > > + uint32_t ExposureLines(const libcamera::utils::Duration\n> &exposure) const;\n> > > > + libcamera::utils::Duration Exposure(uint32_t exposure_lines)\n> const;\n> > > > + virtual uint32_t GetVBlanking(libcamera::utils::Duration\n> &exposure,\n> > > > + const libcamera::utils::Duration\n> &minFrameDuration,\n> > > > + const libcamera::utils::Duration\n> &maxFrameDuration) const;\n> > > > virtual uint32_t GainCode(double gain) const = 0;\n> > > > virtual double Gain(uint32_t gain_code) const = 0;\n> > > > virtual void GetDelays(int &exposure_delay, int &gain_delay,\n> > > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > > index 256438c931d9..2aa2335dcf90 100644\n> > > > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > > @@ -8,6 +8,8 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \"libcamera/internal/utils.h\"\n> > > > +\n> > > > // Description of a \"camera mode\", holding enough information for\n> control\n> > > > // algorithms to adapt their behaviour to the different modes of\n> the camera,\n> > > > // including binning, scaling, cropping etc.\n> > > > @@ -33,8 +35,8 @@ struct CameraMode {\n> > > > double scale_x, scale_y;\n> > > > // scaling of the noise compared to the native sensor mode\n> > > > double noise_factor;\n> > > > - // line time in nanoseconds\n> > > > - double line_length;\n> > > > + // line time\n> > > > + libcamera::utils::Duration line_length;\n> > > > // any camera transform *not* reflected already in the camera\n> tuning\n> > > > libcamera::Transform transform;\n> > > > // minimum and maximum fame lengths in units of lines\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 52d91db282ea..4e02e94084f7 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -55,19 +55,22 @@\n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > +using namespace std::literals::chrono_literals;\n> > > > +using utils::Duration;\n> > > > +\n> > > > /* Configure the sensor with these values initially. */\n> > > > constexpr double DefaultAnalogueGain = 1.0;\n> > > > -constexpr unsigned int DefaultExposureTime = 20000;\n> > > > -constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > > > -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> > > > +constexpr Duration DefaultExposureTime = 20.0ms;\n> > > > +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;\n> > > > +constexpr Duration defaultMaxFrameDuration = 100.0s;\n> > > >\n> > > > /*\n> > > > - * Determine the minimum allowable inter-frame duration (in us) to\n> run the\n> > > > - * controller algorithms. If the pipeline handler provider frames\n> at a rate\n> > > > - * higher than this, we rate-limit the controller Prepare() and\n> Process() calls\n> > > > - * to lower than or equal to this rate.\n> > > > + * Determine the minimum allowable inter-frame duration to run the\n> controller\n> > > > + * algorithms. If the pipeline handler provider frames at a rate\n> higher than this,\n> > > > + * we rate-limit the controller Prepare() and Process() calls to\n> lower than or\n> > > > + * equal to this rate.\n> > > > */\n> > > > -constexpr double controllerMinFrameDuration = 1e6 / 60.0;\n> > > > +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;\n> > > >\n> > > > LOG_DEFINE_CATEGORY(IPARPI)\n> > > >\n> > > > @@ -111,7 +114,8 @@ private:\n> > > > void reportMetadata();\n> > > > void fillDeviceStatus(const ControlList &sensorControls);\n> > > > void processStats(unsigned int bufferId);\n> > > > - void applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration);\n> > > > + void applyFrameDurations(const Duration &minFrameDuration,\n> > > > + const Duration &maxFrameDuration);\n> > > > void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls);\n> > > > void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> &ctrls);\n> > > > void applyDG(const struct AgcStatus *dgStatus, ControlList\n> &ctrls);\n> > > > @@ -167,9 +171,9 @@ private:\n> > > > /* Distinguish the first camera start from others. */\n> > > > bool firstStart_;\n> > > >\n> > > > - /* Frame duration (1/fps) limits, given in microseconds. */\n> > > > - double minFrameDuration_;\n> > > > - double maxFrameDuration_;\n> > > > + /* Frame duration (1/fps) limits. */\n> > > > + Duration minFrameDuration_;\n> > > > + Duration maxFrameDuration_;\n> > > > };\n> > > >\n> > > > int IPARPi::init(const IPASettings &settings,\n> ipa::RPi::SensorConfig *sensorConfig)\n> > > > @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> > > > mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);\n> > > >\n> > > > /*\n> > > > - * Calculate the line length in nanoseconds as the ratio\n> between\n> > > > - * the line length in pixels and the pixel rate.\n> > > > + * Calculate the line length as the ratio between the line\n> length in\n> > > > + * pixels and the pixel rate.\n> > > > */\n> > > > - mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate;\n> > > > + mode_.line_length = sensorInfo.lineLength * (1.0s /\n> sensorInfo.pixelRate);\n> > > >\n> > > > /*\n> > > > * Set the frame length limits for the mode to ensure exposure\n> and\n> > > > @@ -387,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > > > /* Supply initial values for gain and exposure. */\n> > > > ControlList ctrls(sensorCtrls_);\n> > > > AgcStatus agcStatus;\n> > > > - agcStatus.shutter_time = DefaultExposureTime;\n> > > > + agcStatus.shutter_time =\n> DefaultExposureTime.get();\n> > > > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > > > applyAGC(&agcStatus, ctrls);\n> > > >\n> > > > @@ -862,7 +866,7 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> > > >\n> > > > case controls::FRAME_DURATIONS: {\n> > > > auto frameDurations =\n> ctrl.second.get>();\n> > > > - applyFrameDurations(frameDurations[0],\n> frameDurations[1]);\n> > > > + applyFrameDurations(frameDurations[0] * 1.0us,\n> frameDurations[1] * 1.0us);\n> > > > break;\n> > > > }\n> > > >\n> > > > @@ -937,9 +941,9 @@ void IPARPi::prepareISP(const\n> ipa::RPi::ISPConfig &data)\n> > > > returnEmbeddedBuffer(data.embeddedBufferId);\n> > > >\n> > > > /* Allow a 10% margin on the comparison below. */\n> > > > - constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;\n> > > > + Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> > > > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > > > - frameTimestamp - lastRunTimestamp_ + eps <\n> controllerMinFrameDuration * 1e3) {\n> > > > + delta < controllerMinFrameDuration * 0.9) {\n> > > > /*\n> > > > * Ensure we merge the previous frame's metadata with\n> the current\n> > > > * frame. This will not overwrite exposure/gain values\n> for the\n> > > > @@ -1012,7 +1016,7 @@ void IPARPi::fillDeviceStatus(const\n> ControlList &sensorControls)\n> > > > int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > > int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> > > >\n> > > > - deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > > > + deviceStatus.shutter_speed =\n> helper_->Exposure(exposureLines).get();\n> > > > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> > > >\n> > > > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > > > @@ -1057,10 +1061,11 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > > > static_cast(awbStatus->gain_b * 1000));\n> > > > }\n> > > >\n> > > > -void IPARPi::applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration)\n> > > > +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,\n> > > > + const Duration &maxFrameDuration)\n> > > > {\n> > > > - const double minSensorFrameDuration = 1e-3 *\n> mode_.min_frame_length * mode_.line_length;\n> > > > - const double maxSensorFrameDuration = 1e-3 *\n> mode_.max_frame_length * mode_.line_length;\n> > > > + const Duration minSensorFrameDuration = mode_.min_frame_length\n> * mode_.line_length;\n> > > > + const Duration maxSensorFrameDuration = mode_.max_frame_length\n> * mode_.line_length;\n> > > >\n> > > > /*\n> > > > * This will only be applied once AGC recalculations occur.\n> > > > @@ -1076,20 +1081,20 @@ void IPARPi::applyFrameDurations(double\n> minFrameDuration, double maxFrameDuratio\n> > > >\n> > > > /* Return the validated limits via metadata. */\n> > > > libcameraMetadata_.set(controls::FrameDurations,\n> > > > - {\n> static_cast(minFrameDuration_),\n> > > > -\n> static_cast(maxFrameDuration_) });\n> > > > + {\n> static_cast(minFrameDuration_.get()),\n> > > > + static_cast(maxFrameDuration_.get()) });\n> > > >\n> > > > /*\n> > > > * Calculate the maximum exposure time possible for the AGC to\n> use.\n> > > > * GetVBlanking() will update maxShutter with the largest\n> exposure\n> > > > * value possible.\n> > > > */\n> > > > - double maxShutter = std::numeric_limits::max();\n> > > > + Duration maxShutter = Duration::max();\n> > > > helper_->GetVBlanking(maxShutter, minFrameDuration_,\n> maxFrameDuration_);\n> > > >\n> > > > RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - agc->SetMaxShutter(maxShutter);\n> > > > + agc->SetMaxShutter(maxShutter.get());\n> > > > }\n> > > >\n> > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus,\n> ControlList &ctrls)\n> > > > @@ -1097,9 +1102,8 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> > > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > > >\n> > > > /* GetVBlanking might clip exposure time to the fps limits. */\n> > > > - double exposure = agcStatus->shutter_time;\n> > > > - int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_,\n> > > > - maxFrameDuration_);\n> > > > + Duration exposure = agcStatus->shutter_time * 1.0us;\n> > > > + int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_, maxFrameDuration_);\n> > > > int32_t exposureLines = helper_->ExposureLines(exposure);\n> > > >\n> > > > LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 B0EA6BD22E\n\tfor ;\n\tTue, 8 Jun 2021 09:58:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 278AF6892F;\n\tTue, 8 Jun 2021 11:58:18 +0200 (CEST)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A98F6029E\n\tfor ;\n\tTue, 8 Jun 2021 11:58:16 +0200 (CEST)","by mail-lf1-x130.google.com with SMTP id p17so30374814lfc.6\n\tfor ;\n\tTue, 08 Jun 2021 02:58:16 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"F97Q7wGe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=3MucSRMRU6K1El4TXVExQGNso/9iYDc3ScH2RWkPc9M=;\n\tb=F97Q7wGeifn37vqUST+/OEH3DUm8ilGuJ2sKH7hPgBa6WWQLWb2BuaO5m9Wbnxiu4Q\n\tf9AjyJ6FOoUzvjmT0Av3vfwfpcbgQh/mCKtN+1qFQVmAxHyBQBmRjiHkpODjI70Q/MnQ\n\t3agXYPpfXx5duOXOtrFHddUMMnJBL/TtUajSrCjemXGruMrTJLbMIX0fBKC8F3xwR1FA\n\tcvUlBU4whrXkbRz+1lvWhn/DW5JpgjzEYpmxtRkIJf6DwnGioXdy1gN4lzV6vCvdgSP2\n\twckBijbbj2BrL8t68OMmWEs8G4XEB04UjoLejnsEHUkTS3/wjJuZqdatwZ2JaW0InNDG\n\tzp8Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=3MucSRMRU6K1El4TXVExQGNso/9iYDc3ScH2RWkPc9M=;\n\tb=OOqQ7F0ij5V6KDKXYR9d6Bg8ak9aOj3Js11UOSUCrA6mm+gA5l0HSWICO0YAbJRtzL\n\tMLVbwvq5FhexeMNrDgMPkNGnGrDszpr6g+AI1Uj/prMI2+cutzovRGOYHN0QeC6cPU/L\n\tztBDGtI4OtNp52uhJP8GLuoTE765B2DbdUoTRRouc8lxl5emVM55C8x/p13sqhmOiFgi\n\tB88wTlr1dwWiAAkjgPaly6gY4j78S61mJxNhDpuECilL8ivdGsJIwo6fuEwz0fMlEKet\n\tVEhZfmeQGTGzmcVfkKEJn8YT6WN40HC/3o+4sgMEDJanJFO63OjuAUpGYREzMHmhMB8f\n\tWuWg==","X-Gm-Message-State":"AOAM533qvz0LnRunfYB67ZShVcMYsCdwY34C9rtuw/TrCIWLJMGJK/yI\n\t/RkjL21t+3DvWkMFdESSXkeo43wx//lhbwBv9i/fxSIMCsc=","X-Google-Smtp-Source":"ABdhPJzC9/lT2MO4Fg3TIdk0q4JOYdhrd8L2VBpUCG9qE7AM5KG02osSKl2Oc0SpB225r2bCXtfW3DjJq2g/Sr1JRhY=","X-Received":"by 2002:a05:6512:2096:: with SMTP id\n\tt22mr15248956lfr.272.1623146295883; \n\tTue, 08 Jun 2021 02:58:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210525114241.906280-1-naush@raspberrypi.com>\n\t<20210525114241.906280-3-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 8 Jun 2021 10:58:00 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000628bfb05c43e32bf\"","Subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]