[{"id":14850,"web_url":"https://patchwork.libcamera.org/comment/14850/","msgid":"<20210129095144.wiujbvhogtc43jpy@uno.localdomain>","date":"2021-01-29T09:51:44","subject":"Re: [libcamera-devel] [PATCH v3 4/5] ipa: raspberrypi: Pass the\n\tmaximum allowable shutter speed into the AGC","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n I'm not expert of this part, so I trust your and David's review\nacks\n\nOn Thu, Jan 28, 2021 at 09:10:49AM +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> This applies to both the manual and auto AGC modes.\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \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> \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\nNit: I would align ? below =, unless checkstyle says otherwise.\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\nDo you need to intialize this, as GetVBlanking will unconditionally\nupdate it, unless assert() fails, but we've bigger troubles in that\ncase.\n\nRest looks good\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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)\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 9E532BD808\n\tfor ;\n\tFri, 29 Jan 2021 09:51:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38D106839C;\n\tFri, 29 Jan 2021 10:51:25 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 422F568393\n\tfor ;\n\tFri, 29 Jan 2021 10:51:24 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id C1AB11C0002;\n\tFri, 29 Jan 2021 09:51:23 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 29 Jan 2021 10:51:44 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210129095144.wiujbvhogtc43jpy@uno.localdomain>","References":"<20210128091050.881815-1-naush@raspberrypi.com>\n\t<20210128091050.881815-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210128091050.881815-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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":14851,"web_url":"https://patchwork.libcamera.org/comment/14851/","msgid":"","date":"2021-01-29T10:00:06","subject":"Re: [libcamera-devel] [PATCH v3 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 Jacopo,\n\nThank you for your review feedback.\n\nOn Fri, 29 Jan 2021 at 09:51, Jacopo Mondi wrote:\n\n> Hi Naush,\n> I'm not expert of this part, so I trust your and David's review\n> acks\n>\n> On Thu, Jan 28, 2021 at 09:10:49AM +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> > This applies to both the manual and auto AGC modes.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \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> > 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> Nit: I would align ? below =, unless checkstyle says otherwise.\n>\n\nAck.\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>\n> Do you need to intialize this, as GetVBlanking will unconditionally\n> update it, unless assert() fails, but we've bigger troubles in that\n> case.\n>\n\nI do need to initialize this as GetVBlanking() will clip the passed in\nexposure value if it needs to.\nSo, I pass in std::numeric_limits::max() to return out the largest\npossible exposure achievable.\n\nRegards,\nNaush\n\n\n>\n> Rest looks good\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\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> > 2.25.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 A344EC33BB\n\tfor ;\n\tFri, 29 Jan 2021 10:00:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 235C2683A2;\n\tFri, 29 Jan 2021 11:00:26 +0100 (CET)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0BD0D68393\n\tfor ;\n\tFri, 29 Jan 2021 11:00:24 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id b2so11724255lfq.0\n\tfor ;\n\tFri, 29 Jan 2021 02:00:23 -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=\"jeIdsbp0\"; 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=P2BFz09IJoJVxRmCgTo8ipAJAhwgL/VewzUmiAPF1Yk=;\n\tb=jeIdsbp0Z4bk/vvzh8QZajvJpPG916Sg/Fq3y40MSQ+rvXvuAEmNPh2/FmZRoJJVJm\n\tjIMHo5ep7Mwpc/sXjy1Kyr+eT5KxPi0uFGDVwotC8o5NH+l//N3WVsNX5i01gF/6mO+g\n\tSHtB8dx9xz6RTmUnkXgomwPR7XV8D88HpdJSqVAByhkQuFWfaX4Y32dQkPLJJQoX+kWx\n\tPktrdjHI4120IxBWK7deVp0+jctez3Lkr6D7clt4b4EnsZQ/hZ6hxAfBAGasoG57+aH3\n\tj0urMy1nuQwH3IM/VqIq9wGOITCRLv+BzMmUFgiMzug3Uyk2JaA7sL3f/vnn5fhmXQdi\n\tYK4w==","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=P2BFz09IJoJVxRmCgTo8ipAJAhwgL/VewzUmiAPF1Yk=;\n\tb=P3x3Tof7Ru2x3V8zmzrNMxvU/AdWAOe3AqxfQYXtSEnVv8iIxuRCNDf2AV/h0oYBWx\n\tV0Grvswfew3Fcfz1k9/dGwphN66drLRQPdzqsAOTxbNPjwuW5wn5Vk4RgLaRBc9R25Yc\n\tO3KjoPcvGZwAi+rMpu0D6B7WGMlLj72zq4mPBsFD2mBksHQFUwEOFO5JAdxa39r/Gp/Z\n\t8ZuO0tM1LqivVdWybU7Ysdm34hBkWOkhjn04oWstAWN1VNvJYWEOXQwkoW5FOmwUwyd4\n\tmpc8l18shS58wjlH3FOxR7nNwWi6LYSPboU45TEXQrwDv4vhqqxk6YIbhNPdz6Kkz1bt\n\tiCjA==","X-Gm-Message-State":"AOAM530AMFBKpBvai/E4pknEQO5BgfyjMQwRY3gDmRDml8t+AiN82593\n\t2MauBEARt9z3dLTP4qo3hxzBbj2wjgjQGseYHiRNptLHJX11Qg==","X-Google-Smtp-Source":"ABdhPJyRKaqqwVDTuOa90tR9nnpqZ2WC5RfPGaGvaOG1XGY60uGPD6H2Hjabf5m+WKtyydarrUtguFYV/1NsTLo9wSE=","X-Received":"by 2002:a05:6512:36cf:: with SMTP id\n\te15mr1749640lfs.617.1611914423346; \n\tFri, 29 Jan 2021 02:00:23 -0800 (PST)","MIME-Version":"1.0","References":"<20210128091050.881815-1-naush@raspberrypi.com>\n\t<20210128091050.881815-5-naush@raspberrypi.com>\n\t<20210129095144.wiujbvhogtc43jpy@uno.localdomain>","In-Reply-To":"<20210129095144.wiujbvhogtc43jpy@uno.localdomain>","From":"Naushir Patuck ","Date":"Fri, 29 Jan 2021 10:00:06 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 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=\"===============3076015698944760571==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]