[{"id":14655,"web_url":"https://patchwork.libcamera.org/comment/14655/","msgid":"","date":"2021-01-21T15:29:01","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: raspberrypi: Pass the\n\tmaximum allowable shutter speed into the AGC","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 submitting this... most of it looks rather familiar! As\nsuch I'm not likely to have too much to say, just one or two minor\nobservations.\n\nOn Thu, 21 Jan 2021 at 11:59, Naushir Patuck wrote:\n>\n> In order to provide an optimal split between shutter speed and gain, the\n> AGC must know the maximum allowable shutter speed, as limited by the\n> maximum frame duration (either application provided or the default).\n>\n> Add a new API function, SetMaxShutter, to the AgcAlgorihtm class. The\n> IPA provides the the maximum shutter speed for AGC calculations.\n> This applies to both the manual and auto AGC modes.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> .../raspberrypi/controller/agc_algorithm.hpp | 1 +\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 49 +++++++++++++------\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++\n> 4 files changed, 49 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> index 981f1de2f0e4..f97deb0fca59 100644\n> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> @@ -19,6 +19,7 @@ public:\n> virtual void SetEv(double ev) = 0;\n> virtual void SetFlickerPeriod(double flicker_period) = 0;\n> virtual void SetFixedShutter(double fixed_shutter) = 0; // microseconds\n> + virtual void SetMaxShutter(double max_shutter) = 0; // microseconds\n> virtual void SetFixedAnalogueGain(double fixed_analogue_gain) = 0;\n> virtual void SetMeteringMode(std::string const &metering_mode_name) = 0;\n> virtual void SetExposureMode(std::string const &exposure_mode_name) = 0;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index eddd1684da12..937bb70ece67 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -157,7 +157,7 @@ Agc::Agc(Controller *controller)\n> frame_count_(0), lock_count_(0),\n> last_target_exposure_(0.0),\n> ev_(1.0), flicker_period_(0.0),\n> - fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> + max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> {\n> memset(&awb_, 0, sizeof(awb_));\n> // Setting status_.total_exposure_value_ to zero initially tells us\n> @@ -227,11 +227,23 @@ void Agc::SetFlickerPeriod(double flicker_period)\n> flicker_period_ = flicker_period;\n> }\n>\n> +static double clip_shutter(double shutter, double max_shutter)\n> +{\n> + if (max_shutter)\n> + shutter = std::min(shutter, max_shutter);\n> + return shutter;\n> +}\n> +\n> +void Agc::SetMaxShutter(double max_shutter)\n> +{\n> + max_shutter_ = max_shutter;\n> +}\n> +\n> void Agc::SetFixedShutter(double fixed_shutter)\n> {\n> fixed_shutter_ = fixed_shutter;\n> // Set this in case someone calls Pause() straight after.\n> - status_.shutter_time = fixed_shutter;\n> + status_.shutter_time = clip_shutter(fixed_shutter_, max_shutter_);\n> }\n>\n> void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> @@ -261,7 +273,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> {\n> housekeepConfig();\n>\n> - if (fixed_shutter_ != 0.0 && fixed_analogue_gain_ != 0.0) {\n> + double fixed_shutter = clip_shutter(fixed_shutter_, max_shutter_);\n> + if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n> // We're going to reset the algorithm here with these fixed values.\n>\n> fetchAwbStatus(metadata);\n> @@ -269,14 +282,14 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> ASSERT(min_colour_gain != 0.0);\n>\n> // This is the equivalent of computeTargetExposure and applyDigitalGain.\n> - target_.total_exposure_no_dg = fixed_shutter_ * fixed_analogue_gain_;\n> + target_.total_exposure_no_dg = fixed_shutter * fixed_analogue_gain_;\n> target_.total_exposure = target_.total_exposure_no_dg / min_colour_gain;\n>\n> // Equivalent of filterExposure. This resets any \"history\".\n> filtered_ = target_;\n>\n> // Equivalent of divideUpExposure.\n> - filtered_.shutter = fixed_shutter_;\n> + filtered_.shutter = fixed_shutter;\n> filtered_.analogue_gain = fixed_analogue_gain_;\n> } else if (status_.total_exposure_value) {\n> // On a mode switch, it's possible the exposure profile could change,\n> @@ -290,7 +303,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> // for any that weren't set.\n>\n> // Equivalent of divideUpExposure.\n> - filtered_.shutter = fixed_shutter_ ? fixed_shutter_ : config_.default_exposure_time;\n> + filtered_.shutter = fixed_shutter ? fixed_shutter : config_.default_exposure_time;\n> filtered_.analogue_gain = fixed_analogue_gain_ ? fixed_analogue_gain_ : config_.default_analogue_gain;\n> }\n>\n> @@ -403,7 +416,8 @@ void Agc::housekeepConfig()\n> {\n> // First fetch all the up-to-date settings, so no one else has to do it.\n> status_.ev = ev_;\n> - status_.fixed_shutter = fixed_shutter_;\n> + double fixed_shutter = clip_shutter(fixed_shutter_, max_shutter_);\n> + status_.fixed_shutter = fixed_shutter;\n> status_.fixed_analogue_gain = fixed_analogue_gain_;\n> status_.flicker_period = flicker_period_;\n> LOG(RPiAgc, Debug) << \"ev \" << status_.ev << \" fixed_shutter \"\n> @@ -582,13 +596,15 @@ void Agc::computeTargetExposure(double gain)\n> target_.total_exposure = current_.total_exposure_no_dg * gain;\n> // The final target exposure is also limited to what the exposure\n> // mode allows.\n> + double max_shutter = status_.fixed_shutter != 0.0\n> + ? status_.fixed_shutter\n> + : exposure_mode_->shutter.back();\n> + max_shutter = clip_shutter(max_shutter, max_shutter_);\n> double max_total_exposure =\n> - (status_.fixed_shutter != 0.0\n> - ? status_.fixed_shutter\n> - : exposure_mode_->shutter.back()) *\n> + max_shutter *\n> (status_.fixed_analogue_gain != 0.0\n> - ? status_.fixed_analogue_gain\n> - : exposure_mode_->gain.back());\n> + ? status_.fixed_analogue_gain\n> + : exposure_mode_->gain.back());\n> target_.total_exposure = std::min(target_.total_exposure,\n> max_total_exposure);\n> }\n> @@ -671,6 +687,7 @@ void Agc::divideUpExposure()\n> shutter_time = status_.fixed_shutter != 0.0\n> ? status_.fixed_shutter\n> : exposure_mode_->shutter[0];\n> + shutter_time = clip_shutter(shutter_time, max_shutter_);\n> analogue_gain = status_.fixed_analogue_gain != 0.0\n> ? status_.fixed_analogue_gain\n> : exposure_mode_->gain[0];\n> @@ -678,14 +695,16 @@ void Agc::divideUpExposure()\n> for (unsigned int stage = 1;\n> stage < exposure_mode_->gain.size(); stage++) {\n> if (status_.fixed_shutter == 0.0) {\n> - if (exposure_mode_->shutter[stage] *\n> - analogue_gain >=\n> + double stage_shutter =\n> + clip_shutter(exposure_mode_->shutter[stage],\n> + max_shutter_);\n> + if (stage_shutter * analogue_gain >=\n> exposure_value) {\n> shutter_time =\n> exposure_value / analogue_gain;\n> break;\n> }\n> - shutter_time = exposure_mode_->shutter[stage];\n> + shutter_time = stage_shutter;\n\nIt's not a subject for this patch set, but I do rather feel here that\nthe (my) attempts to placate the line-length-gods have rendered\neverything a bit unreadable. Maybe worth revisiting at some point.\n\n> }\n> if (status_.fixed_analogue_gain == 0.0) {\n> if (exposure_mode_->gain[stage] *\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 05c334e4a1de..2ce3b1a1700a 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -78,6 +78,7 @@ public:\n> unsigned int GetConvergenceFrames() const override;\n> void SetEv(double ev) override;\n> void SetFlickerPeriod(double flicker_period) override;\n> + void SetMaxShutter(double max_shutter) override; // microseconds\n> void SetFixedShutter(double fixed_shutter) override; // microseconds\n> void SetFixedAnalogueGain(double fixed_analogue_gain) override;\n> void SetMeteringMode(std::string const &metering_mode_name) override;\n> @@ -126,6 +127,7 @@ private:\n> std::string constraint_mode_name_;\n> double ev_;\n> double flicker_period_;\n> + double max_shutter_;\n> double fixed_shutter_;\n> double fixed_analogue_gain_;\n> };\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 9e6e030c4997..2b71efc63f10 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -1008,6 +1008,18 @@ void IPARPi::applyFrameDurations(double min, double max)\n> libcameraMetadata_.set(controls::FrameDurations,\n> { static_cast(minFrameDuration_),\n> static_cast(maxFrameDuration_) });\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> + helper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);\n> +\n> + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> + controller_.GetAlgorithm(\"agc\"));\n\nI notice we don't check for NULL here. It's true that without AGC,\nrealistically, what could you actually do? Again, it may be a topic\nfor another time, but we should probably revisit exactly which\nalgorithms we mandate and which could be left out (for example,\nsomeone should probably be able to omit the \"sdn\" algorithm without\neverything blowing up).\n\nBut as my comments are really just notes to look at other stuff:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n> + agc->SetMaxShutter(maxShutter);\n> }\n>\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 333A7BD808\n\tfor ;\n\tThu, 21 Jan 2021 15:29:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B190B681F0;\n\tThu, 21 Jan 2021 16:29:15 +0100 (CET)","from mail-ot1-x32d.google.com (mail-ot1-x32d.google.com\n\t[IPv6:2607:f8b0:4864:20::32d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D4051681DB\n\tfor ;\n\tThu, 21 Jan 2021 16:29:13 +0100 (CET)","by mail-ot1-x32d.google.com with SMTP id 36so1961963otp.2\n\tfor ;\n\tThu, 21 Jan 2021 07:29:13 -0800 (PST)"],"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=\"dCCIEt0O\"; 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=f99r5kWJTRY8kKMXUMt/WtHsYxP8eXKS1henq1FY84s=;\n\tb=dCCIEt0O0TAWMpaxO1q7Aw5K2l7ZxAW4lzRiCi1TFuFZchkPr2omDzvmEc/9Fhm2CC\n\thBQPQpbmEtKRziYo33puDbfn+56WzqXmJ/Vd+V6l3UEWKeJ8vxfz5qzaUnlBR7AEqU6m\n\t6hKVMHIeOJZY/IKZBp2kNzx6wQoeHbRQbX3GRP3yJq0olWtnpjeOht4sQ2UM4OkswpV0\n\tXAeijShVgUN+88XA8vwCsrktKwVzOufV/XvMqPHwP/vZO0mlI1SA6NHZkISdyG6pZnpa\n\tWPHeXUTo1Lk9mKgQJiQKQKH1IxoT/Frzwux9can2JW8r/VZCUzoLPS2bfHeKJCPC/Kcv\n\tPrtA==","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=f99r5kWJTRY8kKMXUMt/WtHsYxP8eXKS1henq1FY84s=;\n\tb=Uk5q5Npb+B9qTjjV0L7H5/2rCbtYAyYuDNTWKQCo5B6FH6MjOWKSESNX/6Z9icm7HK\n\t22fKQddEaHic3psoohpVcMBqtW4xfY/RNZ2QPpcawwo0ictrA7wg0+O0Z4gzujYDwyK8\n\tcPunWp/QyUI3XlArCPgUgDhemBASTQYxFiRV7gPgRGCss9+heAPMtnud4cHn5eRusXm3\n\thuSy/3Rbe5KtdyiniWlkG2hXo82i7lvSpvr0UEmESh5M+1ysCyGFdQy/vrQEBsdBnbg2\n\txN1VzK6QhcBBAsLZ6b6NR+e3uwO6m5HNxAiE1J2bXpyRkB+i0EMqPl/8QxSKokqhrBEZ\n\tLDdQ==","X-Gm-Message-State":"AOAM5325gqpjGXLLgK5lEBtmw55IuHpcKfcm/Gmt8AxqtV3M5R+cQn6n\n\tb3B2eeo1tlrOz/aq0o4BZ6LoDOpPy6OQDyujW8Sqtw==","X-Google-Smtp-Source":"ABdhPJzQU/T3264Tbh1y6tY4UjRYmdO2bnyTy5lGy6t3h9qMCd9xXQYmS5mZf/OurJE2LP6mw3JfnfeGOZ2r31d+HTE=","X-Received":"by 2002:a9d:4812:: with SMTP id\n\tc18mr7278760otf.160.1611242952379; \n\tThu, 21 Jan 2021 07:29:12 -0800 (PST)","MIME-Version":"1.0","References":"<20210121115849.682130-1-naush@raspberrypi.com>\n\t<20210121115849.682130-4-naush@raspberrypi.com>","In-Reply-To":"<20210121115849.682130-4-naush@raspberrypi.com>","From":"David Plowman ","Date":"Thu, 21 Jan 2021 15:29:01 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: raspberrypi: Pass the\n\tmaximum allowable shutter speed into the AGC","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]