[{"id":17481,"web_url":"https://patchwork.libcamera.org/comment/17481/","msgid":"<20210608154632.oz2avj4azpabajnw@uno.localdomain>","date":"2021-06-08T15:46:32","subject":"Re: [libcamera-devel] [PATCH v6 2/4] ipa: raspberrypi: Switch ipa\n\tand cam_helper to use utils::Duration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Tue, Jun 08, 2021 at 12:03:33PM +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> Reviewed-by: Laurent Pinchart \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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 | 62 ++++++++++----------\n> 4 files changed, 53 insertions(+), 45 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 09917f3cc079..92a38007f038 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 &stats,\n> {\n> }\n>\n> -uint32_t CamHelper::ExposureLines(double exposure_us) const\n> +uint32_t CamHelper::ExposureLines(const Duration exposure) const\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 Duration minFrameDuration,\n> +\t\t\t\t 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..f53f5c39b01c 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(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 libcamera::utils::Duration minFrameDuration,\n> +\t\t\t\t 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 0c4752ecffb6..33b316b90d16 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -54,19 +54,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> @@ -110,7 +113,7 @@ 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(Duration minFrameDuration, 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> @@ -166,9 +169,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> @@ -310,10 +313,10 @@ void IPARPi::setMode(const IPACameraSensorInfo &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> @@ -386,7 +389,7 @@ int IPARPi::configure(const IPACameraSensorInfo &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> @@ -861,7 +864,7 @@ void IPARPi::queueRequest(const ControlList &controls)\n>\n> \t\tcase controls::FRAME_DURATION_LIMITS: {\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> @@ -936,9 +939,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> @@ -1011,7 +1014,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> @@ -1056,10 +1059,10 @@ 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(Duration minFrameDuration, 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> @@ -1075,20 +1078,20 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio\n>\n> \t/* Return the validated limits via metadata. */\n> \tlibcameraMetadata_.set(controls::FrameDurationLimits,\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> @@ -1096,9 +1099,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\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 7A452BD22E\n\tfor ;\n\tTue, 8 Jun 2021 15:45:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D95DF687EF;\n\tTue, 8 Jun 2021 17:45:44 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3451A602A0\n\tfor ;\n\tTue, 8 Jun 2021 17:45:43 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 87EF5FF80C;\n\tTue, 8 Jun 2021 15:45:42 +0000 (UTC)"],"Date":"Tue, 8 Jun 2021 17:46:32 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210608154632.oz2avj4azpabajnw@uno.localdomain>","References":"<20210608110335.4078551-1-naush@raspberrypi.com>\n\t<20210608110335.4078551-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210608110335.4078551-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 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\" "}}]