[{"id":14987,"web_url":"https://patchwork.libcamera.org/comment/14987/","msgid":"","date":"2021-02-04T20:59:06","subject":"Re: [libcamera-devel] [PATCH v4 4/5] ipa: raspberrypi: Pass the\n\tmaximum allowable shutter speed into the AGC","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 Fri, Jan 29, 2021 at 11:16:15AM +0000, Naushir Patuck wrote:\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 AgcAlgorithm class. The\n> IPA provides the the maximum shutter speed for AGC calculations.\n\ns/the the/the/\n\n> This applies to both the manual and auto AGC modes.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> ---\n> .../raspberrypi/controller/agc_algorithm.hpp | 1 +\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 48 +++++++++++++------\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 3 ++\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> \tvirtual void SetEv(double ev) = 0;\n> \tvirtual void SetFlickerPeriod(double flicker_period) = 0;\n> \tvirtual void SetFixedShutter(double fixed_shutter) = 0; // microseconds\n> +\tvirtual void SetMaxShutter(double max_shutter) = 0; // microseconds\n> \tvirtual void SetFixedAnalogueGain(double fixed_analogue_gain) = 0;\n> \tvirtual void SetMeteringMode(std::string const &metering_mode_name) = 0;\n> \tvirtual void SetExposureMode(std::string const &exposure_mode_name) = 0;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index eddd1684da12..0023d50029f1 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> \t frame_count_(0), lock_count_(0),\n> \t last_target_exposure_(0.0),\n> \t ev_(1.0), flicker_period_(0.0),\n> -\t fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> +\t max_shutter_(0), fixed_shutter_(0), fixed_analogue_gain_(0.0)\n> {\n> \tmemset(&awb_, 0, sizeof(awb_));\n> \t// Setting status_.total_exposure_value_ to zero initially tells us\n> @@ -227,11 +227,16 @@ void Agc::SetFlickerPeriod(double flicker_period)\n> \tflicker_period_ = flicker_period;\n> }\n> \n> +void Agc::SetMaxShutter(double max_shutter)\n> +{\n> +\tmax_shutter_ = max_shutter;\n> +}\n> +\n> void Agc::SetFixedShutter(double fixed_shutter)\n> {\n> \tfixed_shutter_ = fixed_shutter;\n> \t// Set this in case someone calls Pause() straight after.\n> -\tstatus_.shutter_time = fixed_shutter;\n> +\tstatus_.shutter_time = clipShutter(fixed_shutter_);\n> }\n> \n> void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> @@ -261,7 +266,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> {\n> \thousekeepConfig();\n> \n> -\tif (fixed_shutter_ != 0.0 && fixed_analogue_gain_ != 0.0) {\n> +\tdouble fixed_shutter = clipShutter(fixed_shutter_);\n> +\tif (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n> \t\t// We're going to reset the algorithm here with these fixed values.\n> \n> \t\tfetchAwbStatus(metadata);\n> @@ -269,14 +275,14 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> \t\tASSERT(min_colour_gain != 0.0);\n> \n> \t\t// This is the equivalent of computeTargetExposure and applyDigitalGain.\n> -\t\ttarget_.total_exposure_no_dg = fixed_shutter_ * fixed_analogue_gain_;\n> +\t\ttarget_.total_exposure_no_dg = fixed_shutter * fixed_analogue_gain_;\n> \t\ttarget_.total_exposure = target_.total_exposure_no_dg / min_colour_gain;\n> \n> \t\t// Equivalent of filterExposure. This resets any \"history\".\n> \t\tfiltered_ = target_;\n> \n> \t\t// Equivalent of divideUpExposure.\n> -\t\tfiltered_.shutter = fixed_shutter_;\n> +\t\tfiltered_.shutter = fixed_shutter;\n> \t\tfiltered_.analogue_gain = fixed_analogue_gain_;\n> \t} else if (status_.total_exposure_value) {\n> \t\t// On a mode switch, it's possible the exposure profile could change,\n> @@ -290,7 +296,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> \t\t// for any that weren't set.\n> \n> \t\t// Equivalent of divideUpExposure.\n> -\t\tfiltered_.shutter = fixed_shutter_ ? fixed_shutter_ : config_.default_exposure_time;\n> +\t\tfiltered_.shutter = fixed_shutter ? fixed_shutter : config_.default_exposure_time;\n> \t\tfiltered_.analogue_gain = fixed_analogue_gain_ ? fixed_analogue_gain_ : config_.default_analogue_gain;\n> \t}\n> \n> @@ -403,7 +409,8 @@ void Agc::housekeepConfig()\n> {\n> \t// First fetch all the up-to-date settings, so no one else has to do it.\n> \tstatus_.ev = ev_;\n> -\tstatus_.fixed_shutter = fixed_shutter_;\n> +\tdouble fixed_shutter = clipShutter(fixed_shutter_);\n> +\tstatus_.fixed_shutter = fixed_shutter;\n\nYou could write\n\n\tstatus_.fixed_shutter = clipShutter(fixed_shutter_);\n\n> \tstatus_.fixed_analogue_gain = fixed_analogue_gain_;\n> \tstatus_.flicker_period = flicker_period_;\n> \tLOG(RPiAgc, Debug) << \"ev \" << status_.ev << \" fixed_shutter \"\n> @@ -582,13 +589,15 @@ void Agc::computeTargetExposure(double gain)\n> \t\ttarget_.total_exposure = current_.total_exposure_no_dg * gain;\n> \t\t// The final target exposure is also limited to what the exposure\n> \t\t// mode allows.\n> +\t\tdouble max_shutter = status_.fixed_shutter != 0.0\n> +\t\t\t\t\t ? status_.fixed_shutter\n> +\t\t\t\t\t : exposure_mode_->shutter.back();\n\nThe indentation is a bit weird, how about this ?\n\n\t\tdouble max_shutter = status_.fixed_shutter != 0.0\n\t\t\t\t ? status_.fixed_shutter\n\t\t\t\t : exposure_mode_->shutter.back();\n\nThose are minor issues,\n\nReviewed-by: Laurent Pinchart \n\nIf you're fine with these changes, there's no need to resent, I'll fix\nwhen applying.\n\n> +\t\tmax_shutter = clipShutter(max_shutter);\n> \t\tdouble max_total_exposure =\n> -\t\t\t(status_.fixed_shutter != 0.0\n> -\t\t\t ? status_.fixed_shutter\n> -\t\t\t : exposure_mode_->shutter.back()) *\n> +\t\t\tmax_shutter *\n> \t\t\t(status_.fixed_analogue_gain != 0.0\n> -\t\t\t ? status_.fixed_analogue_gain\n> -\t\t\t : exposure_mode_->gain.back());\n> +\t\t\t\t ? status_.fixed_analogue_gain\n> +\t\t\t\t : exposure_mode_->gain.back());\n> \t\ttarget_.total_exposure = std::min(target_.total_exposure,\n> \t\t\t\t\t\t max_total_exposure);\n> \t}\n> @@ -671,6 +680,7 @@ void Agc::divideUpExposure()\n> \tshutter_time = status_.fixed_shutter != 0.0\n> \t\t\t ? status_.fixed_shutter\n> \t\t\t : exposure_mode_->shutter[0];\n> +\tshutter_time = clipShutter(shutter_time);\n> \tanalogue_gain = status_.fixed_analogue_gain != 0.0\n> \t\t\t\t? status_.fixed_analogue_gain\n> \t\t\t\t: exposure_mode_->gain[0];\n> @@ -678,14 +688,15 @@ void Agc::divideUpExposure()\n> \t\tfor (unsigned int stage = 1;\n> \t\t stage < exposure_mode_->gain.size(); stage++) {\n> \t\t\tif (status_.fixed_shutter == 0.0) {\n> -\t\t\t\tif (exposure_mode_->shutter[stage] *\n> -\t\t\t\t\t analogue_gain >=\n> +\t\t\t\tdouble stage_shutter =\n> +\t\t\t\t\tclipShutter(exposure_mode_->shutter[stage]);\n> +\t\t\t\tif (stage_shutter * analogue_gain >=\n> \t\t\t\t exposure_value) {\n> \t\t\t\t\tshutter_time =\n> \t\t\t\t\t\texposure_value / analogue_gain;\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> -\t\t\t\tshutter_time = exposure_mode_->shutter[stage];\n> +\t\t\t\tshutter_time = stage_shutter;\n> \t\t\t}\n> \t\t\tif (status_.fixed_analogue_gain == 0.0) {\n> \t\t\t\tif (exposure_mode_->gain[stage] *\n> @@ -740,6 +751,13 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)\n> \t\t\t << \" analogue gain \" << filtered_.analogue_gain;\n> }\n> \n> +double Agc::clipShutter(double shutter)\n> +{\n> +\tif (max_shutter_)\n> +\t\tshutter = std::min(shutter, max_shutter_);\n> +\treturn shutter;\n> +}\n> +\n> // Register algorithm with the system.\n> static Algorithm *Create(Controller *controller)\n> {\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 05c334e4a1de..0427fb59ec1b 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> \tunsigned int GetConvergenceFrames() const override;\n> \tvoid SetEv(double ev) override;\n> \tvoid SetFlickerPeriod(double flicker_period) override;\n> +\tvoid SetMaxShutter(double max_shutter) override; // microseconds\n> \tvoid SetFixedShutter(double fixed_shutter) override; // microseconds\n> \tvoid SetFixedAnalogueGain(double fixed_analogue_gain) override;\n> \tvoid SetMeteringMode(std::string const &metering_mode_name) override;\n> @@ -100,6 +101,7 @@ private:\n> \tvoid filterExposure(bool desaturate);\n> \tvoid divideUpExposure();\n> \tvoid writeAndFinish(Metadata *image_metadata, bool desaturate);\n> +\tdouble clipShutter(double shutter);\n> \tAgcMeteringMode *metering_mode_;\n> \tAgcExposureMode *exposure_mode_;\n> \tAgcConstraintMode *constraint_mode_;\n> @@ -126,6 +128,7 @@ private:\n> \tstd::string constraint_mode_name_;\n> \tdouble ev_;\n> \tdouble flicker_period_;\n> +\tdouble max_shutter_;\n> \tdouble fixed_shutter_;\n> \tdouble fixed_analogue_gain_;\n> };\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index e4911b734e20..8c0e699184f6 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -1006,6 +1006,18 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio\n> \tlibcameraMetadata_.set(controls::FrameDurations,\n> \t\t\t { static_cast(minFrameDuration_),\n> \t\t\t\t static_cast(maxFrameDuration_) });\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> +\thelper_->GetVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);\n> +\n> +\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> +\t\tcontroller_.GetAlgorithm(\"agc\"));\n> +\tagc->SetMaxShutter(maxShutter);\n> }\n> \n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)","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 331D4BD162\n\tfor ;\n\tThu, 4 Feb 2021 20:59:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA1E56145F;\n\tThu, 4 Feb 2021 21:59:30 +0100 (CET)","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 B951861430\n\tfor ;\n\tThu, 4 Feb 2021 21:59:29 +0100 (CET)","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 26DBF45D;\n\tThu, 4 Feb 2021 21:59:29 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YNr/i0zh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612472369;\n\tbh=1AqDLabwYq/R9WdG530STucjyvkE1pRqFXe6JlL/j8k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YNr/i0zhIwJZJa1zTyLzydl0FbCoFQkHo6JHFJRe0EYCyehm2z/u2sHbP4E9fLfUs\n\txHryKolFXh2nqLwB/naaxYtJ1gG2ibwLeBbxFM9+K49PbtwS4YLMeqFXpW6dhr0/Cu\n\t3NEgDg8RFKloVO9qj1JoGcLrX7wFZCjmFbLDesLA=","Date":"Thu, 4 Feb 2021 22:59:06 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210129111616.1047483-1-naush@raspberrypi.com>\n\t<20210129111616.1047483-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210129111616.1047483-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/5] 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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15011,"web_url":"https://patchwork.libcamera.org/comment/15011/","msgid":"","date":"2021-02-04T22:20:31","subject":"Re: [libcamera-devel] [PATCH v4 4/5] ipa: raspberrypi: Pass the\n\tmaximum allowable shutter speed into the AGC","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 Thu, 4 Feb 2021 at 20:59, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Jan 29, 2021 at 11:16:15AM +0000, Naushir Patuck wrote:\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 AgcAlgorithm class. The\n> > IPA provides the the maximum shutter speed for AGC calculations.\n>\n> s/the the/the/\n>\n> > This applies to both the manual and auto AGC modes.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > .../raspberrypi/controller/agc_algorithm.hpp | 1 +\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 48 +++++++++++++------\n> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 3 ++\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\n> 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; //\n> 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\n> &metering_mode_name) = 0;\n> > virtual void SetExposureMode(std::string const\n> &exposure_mode_name) = 0;\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index eddd1684da12..0023d50029f1 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,16 @@ void Agc::SetFlickerPeriod(double flicker_period)\n> > flicker_period_ = flicker_period;\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 = clipShutter(fixed_shutter_);\n> > }\n> >\n> > void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> > @@ -261,7 +266,8 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode\n> const &camera_mode,\n> > {\n> > housekeepConfig();\n> >\n> > - if (fixed_shutter_ != 0.0 && fixed_analogue_gain_ != 0.0) {\n> > + double fixed_shutter = clipShutter(fixed_shutter_);\n> > + if (fixed_shutter != 0.0 && fixed_analogue_gain_ != 0.0) {\n> > // We're going to reset the algorithm here with these\n> fixed values.\n> >\n> > fetchAwbStatus(metadata);\n> > @@ -269,14 +275,14 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode\n> const &camera_mode,\n> > ASSERT(min_colour_gain != 0.0);\n> >\n> > // This is the equivalent of computeTargetExposure and\n> applyDigitalGain.\n> > - target_.total_exposure_no_dg = fixed_shutter_ *\n> fixed_analogue_gain_;\n> > + target_.total_exposure_no_dg = fixed_shutter *\n> fixed_analogue_gain_;\n> > target_.total_exposure = target_.total_exposure_no_dg /\n> 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\n> could change,\n> > @@ -290,7 +296,7 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode\n> const &camera_mode,\n> > // for any that weren't set.\n> >\n> > // Equivalent of divideUpExposure.\n> > - filtered_.shutter = fixed_shutter_ ? fixed_shutter_ :\n> config_.default_exposure_time;\n> > + filtered_.shutter = fixed_shutter ? fixed_shutter :\n> config_.default_exposure_time;\n> > filtered_.analogue_gain = fixed_analogue_gain_ ?\n> fixed_analogue_gain_ : config_.default_analogue_gain;\n> > }\n> >\n> > @@ -403,7 +409,8 @@ void Agc::housekeepConfig()\n> > {\n> > // First fetch all the up-to-date settings, so no one else has to\n> do it.\n> > status_.ev = ev_;\n> > - status_.fixed_shutter = fixed_shutter_;\n> > + double fixed_shutter = clipShutter(fixed_shutter_);\n> > + status_.fixed_shutter = fixed_shutter;\n>\n> You could write\n>\n> status_.fixed_shutter = clipShutter(fixed_shutter_);\n>\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 +589,15 @@ void Agc::computeTargetExposure(double gain)\n> > target_.total_exposure = current_.total_exposure_no_dg *\n> gain;\n> > // The final target exposure is also limited to what the\n> exposure\n> > // mode allows.\n> > + double max_shutter = status_.fixed_shutter != 0.0\n> > + ? status_.fixed_shutter\n> > + :\n> exposure_mode_->shutter.back();\n>\n> The indentation is a bit weird, how about this ?\n>\n> double max_shutter = status_.fixed_shutter != 0.0\n> ? status_.fixed_shutter\n> : exposure_mode_->shutter.back();\n>\n> Those are minor issues,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> If you're fine with these changes, there's no need to resent, I'll fix\n> when applying.\n>\n\nAck all the minors. Please do fix when applying if it is not too much\ntrouble.\n\nRegards,\nNaush\n\n\n>\n> > + max_shutter = clipShutter(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 +680,7 @@ void Agc::divideUpExposure()\n> > shutter_time = status_.fixed_shutter != 0.0\n> > ? status_.fixed_shutter\n> > : exposure_mode_->shutter[0];\n> > + shutter_time = clipShutter(shutter_time);\n> > analogue_gain = status_.fixed_analogue_gain != 0.0\n> > ? status_.fixed_analogue_gain\n> > : exposure_mode_->gain[0];\n> > @@ -678,14 +688,15 @@ 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> > +\n> clipShutter(exposure_mode_->shutter[stage]);\n> > + if (stage_shutter * analogue_gain >=\n> > exposure_value) {\n> > shutter_time =\n> > exposure_value /\n> analogue_gain;\n> > break;\n> > }\n> > - shutter_time =\n> exposure_mode_->shutter[stage];\n> > + shutter_time = stage_shutter;\n> > }\n> > if (status_.fixed_analogue_gain == 0.0) {\n> > if (exposure_mode_->gain[stage] *\n> > @@ -740,6 +751,13 @@ void Agc::writeAndFinish(Metadata *image_metadata,\n> bool desaturate)\n> > << \" analogue gain \" << filtered_.analogue_gain;\n> > }\n> >\n> > +double Agc::clipShutter(double shutter)\n> > +{\n> > + if (max_shutter_)\n> > + shutter = std::min(shutter, max_shutter_);\n> > + return shutter;\n> > +}\n> > +\n> > // Register algorithm with the system.\n> > static Algorithm *Create(Controller *controller)\n> > {\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > index 05c334e4a1de..0427fb59ec1b 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; //\n> microseconds\n> > void SetFixedAnalogueGain(double fixed_analogue_gain) override;\n> > void SetMeteringMode(std::string const &metering_mode_name)\n> override;\n> > @@ -100,6 +101,7 @@ private:\n> > void filterExposure(bool desaturate);\n> > void divideUpExposure();\n> > void writeAndFinish(Metadata *image_metadata, bool desaturate);\n> > + double clipShutter(double shutter);\n> > AgcMeteringMode *metering_mode_;\n> > AgcExposureMode *exposure_mode_;\n> > AgcConstraintMode *constraint_mode_;\n> > @@ -126,6 +128,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\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index e4911b734e20..8c0e699184f6 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -1006,6 +1006,18 @@ void IPARPi::applyFrameDurations(double\n> minFrameDuration, double maxFrameDuratio\n> > libcameraMetadata_.set(controls::FrameDurations,\n> > { static_cast(minFrameDuration_),\n> > static_cast(maxFrameDuration_)\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> > + helper_->GetVBlanking(maxShutter, minFrameDuration_,\n> maxFrameDuration_);\n> > +\n> > + RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> > + controller_.GetAlgorithm(\"agc\"));\n> > + agc->SetMaxShutter(maxShutter);\n> > }\n> >\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\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 4ED9DBD162\n\tfor ;\n\tThu, 4 Feb 2021 22:20:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19FDA61492;\n\tThu, 4 Feb 2021 23:20:53 +0100 (CET)","from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A4A861486\n\tfor ;\n\tThu, 4 Feb 2021 23:20:51 +0100 (CET)","by mail-lf1-x12b.google.com with SMTP id q12so6899237lfo.12\n\tfor ;\n\tThu, 04 Feb 2021 14:20:51 -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=\"K0O5sgXi\"; 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=NLGpHoQywhYK8Qi/0e46+7+juxNBmQjGU3jQ6Ni7Ft0=;\n\tb=K0O5sgXi7sKXPh+MXcrKEayGXQjnQ6MosT/3fTtaJYr0DuyrbUmmc2zLl9xSPPAO0B\n\tVkrewpqlpoSHyQRsfoej/yI2H2cwZ4C231ok2LZZBDSdeLVRc4WQDA1JioEFcwy2ktnP\n\tTXn98LX1redHZbOldAZjMD9R+Nt8gchKdnTfd0sXTw4cM4pM68fs4MUxHgpADJS0v9NR\n\tPfc+oy3l94tLLIe1I6lpbb+L9+GNws4jfp+pJoXl2A0wWerg0WS6G/Z66f0urE+HjYmw\n\tbFvA/cmK3jTt1dWkrUWn2x9hNJS8SXJ52OgkkfzQFxuy0o/5pmXkN/9rlcvHcaohdL8t\n\tpcVQ==","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=NLGpHoQywhYK8Qi/0e46+7+juxNBmQjGU3jQ6Ni7Ft0=;\n\tb=k5wdtkTw7cGxfv63Ttx26JL5Hs8Zsxteg70M7FRGZDgjmg187juTgMFXnmT3Rtro7U\n\tITyZ0QInzqQ3lB0cFqctqqtKPGPsNow+uh9PyorigVY6q0wpANqtzxSiUZnbAraFl6qv\n\tRmTpCNdo5zZJbhLi5jpow8NIFVtK6dfIURgXgPRp5J9AUUgrvUzUojyeBVaKjFZARXpp\n\tpnnG1DBb0xKa8ELitfVO/KQaWOdsZlXOmHtuvJ0n/R3b1TsYaIUUSSqvgnclqx9v99Z/\n\tI3UkCE7QMjALz6/gUstziAxIH9KKsAI84DHErSSw0nHL+Q0tzY0GMa27N1X6PMxorZEz\n\tVhAQ==","X-Gm-Message-State":"AOAM5312P287oEyXo9ETsrZLY5gDAKt2Z2hU8ISLU1+YokeJaEqQRvcY\n\tByvoZGUCBbMkOq/2jYJ0RUj2vguzjJY4AZ0HqZ+jJdyIT5WHhw==","X-Google-Smtp-Source":"ABdhPJwVTjdVj7wwQVqwll0UvjY86vNWqQ1VDHkWdzz9l3z15KJPSjT5xUgqPE5DvqQoPdkgX4bNzOVExz09oOFmjhs=","X-Received":"by 2002:ac2:592c:: with SMTP id v12mr778976lfi.133.1612477248628;\n\tThu, 04 Feb 2021 14:20:48 -0800 (PST)","MIME-Version":"1.0","References":"<20210129111616.1047483-1-naush@raspberrypi.com>\n\t<20210129111616.1047483-5-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 4 Feb 2021 22:20:31 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 4/5] 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":"multipart/mixed;\n\tboundary=\"===============2548832784179296209==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]