[{"id":17019,"web_url":"https://patchwork.libcamera.org/comment/17019/","msgid":"","date":"2021-05-19T14:09:17","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Switch\n\tipa/cam_helper to use RPiController::Duration","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for this patch.\n\nOn Tue, 18 May 2021 at 11:09, Naushir Patuck wrote:\n>\n> Switch the ipa and cam_helper code to use RPiController::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> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 19 +++---\n> src/ipa/raspberrypi/cam_helper.hpp | 10 +--\n> src/ipa/raspberrypi/controller/camera_mode.h | 5 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 67 +++++++++++---------\n> 4 files changed, 56 insertions(+), 45 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 09917f3cc079..e2b6c8eb8e03 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -61,20 +61,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(Duration exposure) const\n\nI take it we're happy to pass these by value. I assume they're not\nbig, just a double plus some kind of time unit?\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\nYes, I do prefer this!\n\n> }\n>\n> -uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> - double maxFrameDuration) const\n> +uint32_t CamHelper::GetVBlanking(Duration &exposure,\n> + Duration minFrameDuration,\n> + Duration maxFrameDuration) const\n> {\n> uint32_t frameLengthMin, frameLengthMax, vblank;\n> uint32_t exposureLines = ExposureLines(exposure);\n> @@ -85,8 +86,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 +183,7 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> return;\n> }\n>\n> - deviceStatus.shutter_speed = Exposure(exposureLines);\n> + deviceStatus.shutter_speed = DurationValue(Exposure(exposureLines));\n\nAh yes, so this is one of those \"in between\" changes which will get\nbetter again in a later patch...\n\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..a91afaa59909 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -13,6 +13,7 @@\n> #include \"camera_mode.h\"\n> #include \"controller/controller.hpp\"\n> #include \"controller/metadata.hpp\"\n> +#include \"duration.hpp\"\n> #include \"md_parser.hpp\"\n>\n> #include \"libcamera/internal/v4l2_videodevice.h\"\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(Duration exposure) const;\n> + Duration Exposure(uint32_t exposure_lines) const;\n> + virtual uint32_t GetVBlanking(Duration &exposure,\n> + Duration minFrameDuration,\n> + 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..ab7cf2912a06 100644\n> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> @@ -6,6 +6,7 @@\n> */\n> #pragma once\n>\n> +#include \"duration.hpp\"\n> #include \n>\n> // Description of a \"camera mode\", holding enough information for control\n> @@ -33,8 +34,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> + RPiController::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..994fb7e057a9 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -45,6 +45,7 @@\n> #include \"denoise_algorithm.hpp\"\n> #include \"denoise_status.h\"\n> #include \"dpc_status.h\"\n> +#include \"duration.hpp\"\n> #include \"focus_status.h\"\n> #include \"geq_status.h\"\n> #include \"lux_status.h\"\n> @@ -55,19 +56,24 @@\n>\n> namespace libcamera {\n>\n> +using namespace std::literals::chrono_literals;\n> +using RPiController::Duration;\n> +using RPiController::DurationValue;\n> +using RPiController::operator<<;\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 +117,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\nSome pass-by-reference, I notice. Do we want to be consistent? Are we\nbothered (I'm not...)?\n\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 +174,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> @@ -314,7 +321,7 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> * Calculate the line length in nanoseconds as the ratio between\n\nElsewhere you've tended to remove the mention of units, so does it\nwant the same here?\n\n> * the line length in 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 +394,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 = DurationValue(DefaultExposureTime);\n> agcStatus.analogue_gain = DefaultAnalogueGain;\n> applyAGC(&agcStatus, ctrls);\n>\n> @@ -862,7 +869,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 +944,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 +1019,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 = DurationValue(helper_->Exposure(exposureLines));\n> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>\n> LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> @@ -1057,17 +1064,18 @@ 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> * The values may be clamped based on the sensor mode capabilities as well.\n> */\n> - minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;\n> - maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;\n> + minFrameDuration_ = (minFrameDuration > 0.0s) ? minFrameDuration : defaultMaxFrameDuration;\n> + maxFrameDuration_ = (maxFrameDuration > 0.0s) ? maxFrameDuration : defaultMinFrameDuration;\n> minFrameDuration_ = std::clamp(minFrameDuration_,\n> minSensorFrameDuration, maxSensorFrameDuration);\n> maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> @@ -1076,20 +1084,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(DurationValue(minFrameDuration_)),\n> + static_cast(DurationValue(minFrameDuration_)) });\n\nAnd this will look nicer again once the whole of libcamera adopts\nstd::chrono! :)\n\nSo no significant comments really from me, thus:\n\nReviewed-by: David Plowman \n\nThanks\nDavid\n\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(DurationValue(maxShutter));\n> }\n>\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> @@ -1097,9 +1105,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\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 60D25C31FB\n\tfor ;\n\tWed, 19 May 2021 14:09:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9547F68919;\n\tWed, 19 May 2021 16:09:33 +0200 (CEST)","from mail-wm1-x336.google.com (mail-wm1-x336.google.com\n\t[IPv6:2a00:1450:4864:20::336])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 58213602B2\n\tfor ;\n\tWed, 19 May 2021 16:09:31 +0200 (CEST)","by mail-wm1-x336.google.com with SMTP id\n\tl11-20020a05600c4f0bb029017a7cd488f5so773415wmq.0\n\tfor ;\n\tWed, 19 May 2021 07:09:31 -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=\"dELqWUnJ\"; 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=rrzgliefPWoP8yEDoi27TKxklwuh3W1s1IocyDfL0q0=;\n\tb=dELqWUnJttGz7gHT9mPJJssvkZYpm5gGKr26pmqpLnf/oU9KFnF65JQ3AbiSgjKm5a\n\tl62iHq+m+xFq5IC3eXzjPYSH74vdMNfCSdvutltBc4d1s3NwnCeM9Lmuh4InQAXOp5mz\n\t14NOAUIWWoCw+8Pehc1MeldTWTtfayQPVpflQAvdkaUHIbkBB/5qpwrFYRhPqi5zOjro\n\teVOUCNcP5hnbrsf6m7KWjJSM204UP+XL5CFvDtgQGEEHYNoHjqNk38gI63W+adg/A1Nx\n\taRfgVcZOoFfVUwvdHoZIR67VrpmAC7W6z106IaDNLx5NdSyNNyPRRGl52PlS1une7DOo\n\tYjig==","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=rrzgliefPWoP8yEDoi27TKxklwuh3W1s1IocyDfL0q0=;\n\tb=fvp0iv4IBtrBJD+X2nyq1i2ANmLLqLKp4s5YGYgXsDCZxQZ3Kkj9iwt7jMMevvpy/p\n\ty258IE7PziGB5jFcRvMNpql17pK7wWFSs266i56icnWGSJNYvxjoH1WYC2j/XSsTjcOA\n\tES6gWJlMv/U4Q1b3yf1qcPrDl5JXG61e1F7MYMoGA2/5rjs7XDvewwEwC2eM+0gY80O5\n\toQaeRzvRG8jxMy4GX5+AUwJ6DmnV+ps4is/G/lfJ3UvEGFvCxwfBMiIMbz6wFztCFjpz\n\tjlNUAlfwz5gnYT3a9Zg7iS2eVgoqHSS1dxmSF0JQxp7f/a0DwuCXSrrSr453bhOe4glF\n\t/siw==","X-Gm-Message-State":"AOAM533Cb3xi6MeJLcq6ta9JGtL4xkflAG+By1nGxsbnSUSLDDvUnJmt\n\tUABehEdrDdiDHmxcFCXOavaTLxLFtOA5hSKwnr5iwTWuez8=","X-Google-Smtp-Source":"ABdhPJwfdV2sne/Tt7rv1m6zrXjUYFZIKBQQjVUiu010kDV4coQMxBwl/JNeFd71KQjDvqvRfkMP2GQotj3j+dcddMM=","X-Received":"by 2002:a05:600c:4a23:: with SMTP id\n\tc35mr11354199wmp.130.1621433370406; \n\tWed, 19 May 2021 07:09:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20210518100706.578526-1-naush@raspberrypi.com>\n\t<20210518100706.578526-3-naush@raspberrypi.com>","In-Reply-To":"<20210518100706.578526-3-naush@raspberrypi.com>","From":"David Plowman ","Date":"Wed, 19 May 2021 15:09:17 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Switch\n\tipa/cam_helper to use RPiController::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":17022,"web_url":"https://patchwork.libcamera.org/comment/17022/","msgid":"","date":"2021-05-19T14:25:11","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Switch\n\tipa/cam_helper to use RPiController::Duration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your review feedback.\n\nOn Wed, 19 May 2021 at 15:09, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for this patch.\n>\n> On Tue, 18 May 2021 at 11:09, Naushir Patuck \n> wrote:\n> >\n> > Switch the ipa and cam_helper code to use RPiController::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> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 19 +++---\n> > src/ipa/raspberrypi/cam_helper.hpp | 10 +--\n> > src/ipa/raspberrypi/controller/camera_mode.h | 5 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 67 +++++++++++---------\n> > 4 files changed, 56 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..e2b6c8eb8e03 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -61,20 +61,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(Duration exposure) const\n>\n> I take it we're happy to pass these by value. I assume they're not\n> big, just a double plus some kind of time unit?\n>\n\nThere is minimal overhead, but perhaps I should change to a const\nreference to be consistent.\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> Yes, I do prefer this!\n>\n> > }\n> >\n> > -uint32_t CamHelper::GetVBlanking(double &exposure, double\n> minFrameDuration,\n> > - double maxFrameDuration) const\n> > +uint32_t CamHelper::GetVBlanking(Duration &exposure,\n> > + Duration minFrameDuration,\n> > + Duration maxFrameDuration) const\n> > {\n> > uint32_t frameLengthMin, frameLengthMax, vblank;\n> > uint32_t exposureLines = ExposureLines(exposure);\n> > @@ -85,8 +86,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 +183,7 @@ void CamHelper::parseEmbeddedData(Span uint8_t> buffer,\n> > return;\n> > }\n> >\n> > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > + deviceStatus.shutter_speed =\n> DurationValue(Exposure(exposureLines));\n>\n> Ah yes, so this is one of those \"in between\" changes which will get\n> better again in a later patch...\n>\n\nYes, this will be reverted in the later patch.\n\n\n>\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..a91afaa59909 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -13,6 +13,7 @@\n> > #include \"camera_mode.h\"\n> > #include \"controller/controller.hpp\"\n> > #include \"controller/metadata.hpp\"\n> > +#include \"duration.hpp\"\n> > #include \"md_parser.hpp\"\n> >\n> > #include \"libcamera/internal/v4l2_videodevice.h\"\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(Duration exposure) const;\n> > + Duration Exposure(uint32_t exposure_lines) const;\n> > + virtual uint32_t GetVBlanking(Duration &exposure,\n> > + Duration minFrameDuration,\n> > + 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\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> > index 256438c931d9..ab7cf2912a06 100644\n> > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > @@ -6,6 +6,7 @@\n> > */\n> > #pragma once\n> >\n> > +#include \"duration.hpp\"\n> > #include \n> >\n> > // Description of a \"camera mode\", holding enough information for\n> control\n> > @@ -33,8 +34,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> > + RPiController::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..994fb7e057a9 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -45,6 +45,7 @@\n> > #include \"denoise_algorithm.hpp\"\n> > #include \"denoise_status.h\"\n> > #include \"dpc_status.h\"\n> > +#include \"duration.hpp\"\n> > #include \"focus_status.h\"\n> > #include \"geq_status.h\"\n> > #include \"lux_status.h\"\n> > @@ -55,19 +56,24 @@\n> >\n> > namespace libcamera {\n> >\n> > +using namespace std::literals::chrono_literals;\n> > +using RPiController::Duration;\n> > +using RPiController::DurationValue;\n> > +using RPiController::operator<<;\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 +117,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>\n> Some pass-by-reference, I notice. Do we want to be consistent? Are we\n> bothered (I'm not...)?\n>\n\nI will change to using const references in the other places to be\nconsistent.\n\n\n>\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 +174,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> > @@ -314,7 +321,7 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> > * Calculate the line length in nanoseconds as the ratio between\n>\n> Elsewhere you've tended to remove the mention of units, so does it\n> want the same here?\n>\n\nOops, that one I missed.\n\n\n>\n> > * the line length in 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 +394,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> DurationValue(DefaultExposureTime);\n> > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > applyAGC(&agcStatus, ctrls);\n> >\n> > @@ -862,7 +869,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 +944,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\n> the current\n> > * frame. This will not overwrite exposure/gain values\n> for the\n> > @@ -1012,7 +1019,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> DurationValue(helper_->Exposure(exposureLines));\n> > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> >\n> > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > @@ -1057,17 +1064,18 @@ 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> > * The values may be clamped based on the sensor mode\n> capabilities as well.\n> > */\n> > - minFrameDuration_ = minFrameDuration ? minFrameDuration :\n> defaultMaxFrameDuration;\n> > - maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :\n> defaultMinFrameDuration;\n> > + minFrameDuration_ = (minFrameDuration > 0.0s) ? minFrameDuration\n> : defaultMaxFrameDuration;\n> > + maxFrameDuration_ = (maxFrameDuration > 0.0s) ? maxFrameDuration\n> : defaultMinFrameDuration;\n> > minFrameDuration_ = std::clamp(minFrameDuration_,\n> > minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > @@ -1076,20 +1084,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(DurationValue(minFrameDuration_)),\n> > +\n> static_cast(DurationValue(minFrameDuration_)) });\n>\n> And this will look nicer again once the whole of libcamera adopts\n> std::chrono! :)\n>\n\nYes, but that comes later :-)\n\n\n>\n> So no significant comments really from me, thus:\n>\n> Reviewed-by: David Plowman \n>\n> Thanks\n> David\n>\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(DurationValue(maxShutter));\n> > }\n> >\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> > @@ -1097,9 +1105,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 533C4C31FF\n\tfor ;\n\tWed, 19 May 2021 14:25:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A3F66891A;\n\tWed, 19 May 2021 16:25:30 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE9BA68915\n\tfor ;\n\tWed, 19 May 2021 16:25:27 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id z13so19382053lft.1\n\tfor ;\n\tWed, 19 May 2021 07:25:27 -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=\"F29SP2N/\"; 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=1dy1f2SAq4w3c/IQvsBrtH4nTP9ACDgpo20Mo5QCZOU=;\n\tb=F29SP2N/G0BtGVrql98n+Efx/aY6MTCEe85Gj0dG5EXM24OnmISiRbtWIugPaKW1gz\n\tMr/o+F6jNrUXzx3sL4IDOT/+rVVIYlSukvVJOhbrYtje1B4KLkTUnJ3F7b/cF+rH2dy8\n\tfQx4fbFqR/qeFIdEE/zyOMJapnjloKJNk+pTqx84gsY5LZ4aKmcKxcUR/VgD5Lukv+4y\n\tf9kmldwoXc0B1pK98/FHj+tuF1piuasubZyns1u4r062BLjRKOtw44HMgFdL1QiPkPBe\n\t11YRUKnV7dGTz3GeaGRERFmoVwmmZ9ljltSEQH37co+W5ftKDBSjGkzavsaLE6fwpXQG\n\tTAFg==","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=1dy1f2SAq4w3c/IQvsBrtH4nTP9ACDgpo20Mo5QCZOU=;\n\tb=OoTtZng1VkCaW1leXm3NEdzHhHxYoLdPxa43N1uycBnc2HmOFE1MWUoWHVahc+Z6kE\n\ttwoqSfyWIOeTcuKhb1zewmdo7aAL9d1mBV2Yrs55NMkEl0r6fdCrwVG/lwkW7+el788u\n\tcmcu4kBLx55MYouWwz0ErEmljTyOgpG/25k4zuyUyAbPdzZGg9OWFwq32kyFLP2/pHT5\n\t/0DV1mF33Tvl4A/lkGKrWPff0hhviYfo3B2xAGPf0p2GNt4nJhDr/MXTn7Ldb+gx/1Iw\n\tox99g3oR9wa40Kh7o/ilouoYWRRKWKV5RdEq4vD8Qc96tuaRIeWm1CfprsdlmkYFI7sc\n\tVJpw==","X-Gm-Message-State":"AOAM530RlmP28d7kNvbUx+nKTJMsjm6Dqgk6/muPGowYG1D2JMki3fqj\n\txScPupIoRuSaY4vd2OJf9uK8TRZ7eGAInpyq8O9gUQ==","X-Google-Smtp-Source":"ABdhPJwRMiwKMrMBd7UKp3apgp4RwiBjHMbRlD7OaqmQKq3g24QxsNbpUpSQjh2eBRTkxSmeqxetfW6i5UAuoa4G4r4=","X-Received":"by 2002:a05:6512:2215:: with SMTP id\n\th21mr8675911lfu.375.1621434327099; \n\tWed, 19 May 2021 07:25:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20210518100706.578526-1-naush@raspberrypi.com>\n\t<20210518100706.578526-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 19 May 2021 15:25:11 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"00000000000018074d05c2af9927\"","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Switch\n\tipa/cam_helper to use RPiController::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\" "}}]