[{"id":14806,"web_url":"https://patchwork.libcamera.org/comment/14806/","msgid":"","date":"2021-01-27T00:17:23","subject":"Re: [libcamera-devel] [PATCH v2 3/4] 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 Sun, Jan 24, 2021 at 02:05:05PM +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 AgcAlgorihtm class. The\n\ns/AgcAlgorihtm/AgcAlgorithm/\n\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 | 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> \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\nNot something to be addressed now, but would it make sense to express\ndurations using std::chrono::duration ? This would avoid the risk of\npassing a value expressed in the wrong unit. Another option would be to\nuse double across the code base, using the same unit everywhere (which\ncould be seconds, double should give us enough precision).\n\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..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> \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,23 @@ void Agc::SetFlickerPeriod(double flicker_period)\n> \tflicker_period_ = flicker_period;\n> }\n> \n> +static double clip_shutter(double shutter, double max_shutter)\n> +{\n> +\tif (max_shutter)\n> +\t\tshutter = std::min(shutter, max_shutter);\n> +\treturn shutter;\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 = clip_shutter(fixed_shutter_, max_shutter_);\n\nInstead of clipping every time the fixed shutter value is accessed,\nwouldn't it be simpler to store the clipped shutter in fixed_shutter_ ?\nThis would possibly cause the fixed shutter to be clipped based on the\ncurrent mode and not get increased on mode change, but is that a problem\n? An application setting the ExposureTime control should expect it to be\nclipped, but should it expect the original value to be remembered ?\n\nAn alternative would be to turn clip_shutter() into a GetFixedShutter()\nmember function that would use fixed_shutter_ and max_shutter_\ninternally and not take any argument. There's one caller that uses the\nfunction with a different set of arguments, so maybe we would need to\nkeep clip_shutter() and add a GetFixedShutter() wrapper.\n\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> \thousekeepConfig();\n> \n> -\tif (fixed_shutter_ != 0.0 && fixed_analogue_gain_ != 0.0) {\n> +\tdouble fixed_shutter = clip_shutter(fixed_shutter_, max_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 +282,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 +303,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 +416,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 = clip_shutter(fixed_shutter_, max_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 +596,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> +\t\tmax_shutter = clip_shutter(max_shutter, 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 +687,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 = clip_shutter(shutter_time, max_shutter_);\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 +695,16 @@ 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\tclip_shutter(exposure_mode_->shutter[stage],\n> +\t\t\t\t\t\t max_shutter_);\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> 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> \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> @@ -126,6 +127,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 30ba9aef9d97..75c9e404dcc1 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -1008,6 +1008,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 3F021C0F2B\n\tfor ;\n\tWed, 27 Jan 2021 00:17:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5C6068339;\n\tWed, 27 Jan 2021 01:17:44 +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 E1D3C682D2\n\tfor ;\n\tWed, 27 Jan 2021 01:17:43 +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 4D6862C1;\n\tWed, 27 Jan 2021 01:17:43 +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=\"cYLFs+o/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611706663;\n\tbh=Y8FpXaVBf4gaSBmoQkXZHrWnzJEb+MTu3tW3LXhDWq8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cYLFs+o/zcfb7BJjpotwwH1gXGdmtUWd+wmFSipYfPmJSGp1IB042nI8hErACNEmC\n\t2dnVwDAp/ljWmIBtHwXrAuirv/3+gy69ENgMamjCgzdh3JkQzkbUgYD2CGevxz95Ty\n\t5Ls15eLUhnLqrBYtOp4kVsxt5pegm2wQBigkQGls=","Date":"Wed, 27 Jan 2021 02:17:23 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210124140506.786503-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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@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":14817,"web_url":"https://patchwork.libcamera.org/comment/14817/","msgid":"","date":"2021-01-27T09:33:44","subject":"Re: [libcamera-devel] [PATCH v2 3/4] 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 Wed, 27 Jan 2021 at 00:17, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Sun, Jan 24, 2021 at 02:05:05PM +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 AgcAlgorihtm class. The\n>\n> s/AgcAlgorihtm/AgcAlgorithm/\n>\n\nAck.\n\n\n>\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 | 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\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>\n> Not something to be addressed now, but would it make sense to express\n> durations using std::chrono::duration ? This would avoid the risk of\n> passing a value expressed in the wrong unit. Another option would be to\n> use double across the code base, using the same unit everywhere (which\n> could be seconds, double should give us enough precision).\n>\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..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> Instead of clipping every time the fixed shutter value is accessed,\n> wouldn't it be simpler to store the clipped shutter in fixed_shutter_ ?\n> This would possibly cause the fixed shutter to be clipped based on the\n> current mode and not get increased on mode change, but is that a problem\n> ? An application setting the ExposureTime control should expect it to be\n> clipped, but should it expect the original value to be remembered ?\n>\n\nWe can indeed store the clipped fixed_shutter_ directly. However,\nmax_shutter_ can change at runtime (based on the application request), and\nso we must re-clip fixed_shutter_ if that occurs. There is also another\nsimplification here, we always pass max_shutter_ as the second argument to\nclip_shutter. This can be removed, so clip_shutter takes only one argument.\n\nDavid, what are your thoughts?\n\nNaush\n\n\n>\n> An alternative would be to turn clip_shutter() into a GetFixedShutter()\n> member function that would use fixed_shutter_ and max_shutter_\n> internally and not take any argument. There's one caller that uses the\n> function with a different set of arguments, so maybe we would need to\n> keep clip_shutter() and add a GetFixedShutter() wrapper.\n>\n> > }\n> >\n> > void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> > @@ -261,7 +273,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 = 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\n> fixed values.\n> >\n> > fetchAwbStatus(metadata);\n> > @@ -269,14 +282,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 +303,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 +416,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 = 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 *\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> > + 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> > +\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 /\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> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> 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; //\n> microseconds\n> > void SetFixedAnalogueGain(double fixed_analogue_gain) override;\n> > void SetMeteringMode(std::string const &metering_mode_name)\n> 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\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 30ba9aef9d97..75c9e404dcc1 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -1008,6 +1008,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 7994CBD808\n\tfor ;\n\tWed, 27 Jan 2021 09:34:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF34E68356;\n\tWed, 27 Jan 2021 10:34:02 +0100 (CET)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D65A68341\n\tfor ;\n\tWed, 27 Jan 2021 10:34:01 +0100 (CET)","by mail-lf1-x132.google.com with SMTP id h12so1697159lfp.9\n\tfor ;\n\tWed, 27 Jan 2021 01:34:01 -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=\"nX7FxHJN\"; 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=nwuD86pTG9EvVlN99wpEOwWweI5xE4yFq+mQMz57leg=;\n\tb=nX7FxHJNIsjb1uqxqVO7qNYnkjaiimQnKBrjxGz7CH6xOH4JA1NX3PdNgwLr1KB3sw\n\tzHkJQEFpTquDL4RqI1qUuIbHayqzwWEjo0kq7RtpbqWcSo07028SMCeZHzEoDa5wq3Y1\n\t3AG7EusPYY6T1kre68MrVC9GoKFXAPio5yJbR2YDD6E8tgl2/S/TOfY9qmmS3rayzPnh\n\tYWH38ZX44f3Twg63FbSviuNR5DQpAqwgD6ZDyhGzqMpsTkQmyl1gWqBcl1hijJ2T7/J/\n\toZeC0QWn4u1O7qyEtmJfqdqHrh1dvFDBehxCfkLBHabReiydrGA0ddZ4lVl4ID0OKxlA\n\tESSA==","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=nwuD86pTG9EvVlN99wpEOwWweI5xE4yFq+mQMz57leg=;\n\tb=msd+XWU2Tz2zS/vtkuLbaPSVhaT555RJdlQx9xDIIXV8M80gkT4MD8OIGULsGXSUUf\n\tARBUNKRluqj192IBUjR6JO/J/H7WX20NM38qjzYccW6c5IDh0hXOlkiGXDcaks0fRhLD\n\t9ksePyD21WoOWn/rr86tTLrn+vQq5Jdzrl8od4N3QYR/izeofLxXFtRNAqx7BlfOi5pm\n\tqaEAjnDV2QKSolzIWNqaJ9AJc0LL1fTANnnTs85nZYYtiP/ZPbhXVXUFJ/NvZqz5cWET\n\tmyedOJq+GEY4aVlfWzTebYEkF6glpknXDhqmNc1M+gVuoRsJ+GGoTn1HlH0E58UEKw3l\n\t5Gfw==","X-Gm-Message-State":"AOAM532L8X4NGGuOCWxR/1ngvhbED0Y5zW5v/5AWxvfXpFeHxs5/OuHt\n\tcUmh730ePeOnAwpuzB2tQowA1fXQxYStD3ms273wzIi0+0/qRg==","X-Google-Smtp-Source":"ABdhPJzf/5RMiNnimoJzyTjLTWHM2K0g5LejVLMcBJ8LNxckCHj9PgOw1XqPHhl/XWBxtF2v/mbJAhYdTcOs/VTa0Zg=","X-Received":"by 2002:a19:ee09:: with SMTP id g9mr4846385lfb.272.1611740040668;\n\tWed, 27 Jan 2021 01:34:00 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 27 Jan 2021 09:33:44 +0000","Message-ID":"","To":"Laurent Pinchart , \n\tDavid Plowman ","Subject":"Re: [libcamera-devel] [PATCH v2 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":"multipart/mixed;\n\tboundary=\"===============4950996170540546869==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14818,"web_url":"https://patchwork.libcamera.org/comment/14818/","msgid":"","date":"2021-01-27T09:47:03","subject":"Re: [libcamera-devel] [PATCH v2 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, Laurent\n\nThanks for all the discussion on this.\n\nOn Wed, 27 Jan 2021 at 09:34, Naushir Patuck wrote:\n>\n> Hi Laurent,\n>\n> Thank you for your review feedback.\n>\n> On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart wrote:\n>>\n>> Hi Naush,\n>>\n>> Thank you for the patch.\n>>\n>> On Sun, Jan 24, 2021 at 02:05:05PM +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 AgcAlgorihtm class. The\n>>\n>> s/AgcAlgorihtm/AgcAlgorithm/\n>\n>\n> Ack.\n>\n>>\n>>\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 | 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>>\n>> Not something to be addressed now, but would it make sense to express\n>> durations using std::chrono::duration ? This would avoid the risk of\n>> passing a value expressed in the wrong unit. Another option would be to\n>> use double across the code base, using the same unit everywhere (which\n>> could be seconds, double should give us enough precision).\n>>\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>> Instead of clipping every time the fixed shutter value is accessed,\n>> wouldn't it be simpler to store the clipped shutter in fixed_shutter_ ?\n>> This would possibly cause the fixed shutter to be clipped based on the\n>> current mode and not get increased on mode change, but is that a problem\n>> ? An application setting the ExposureTime control should expect it to be\n>> clipped, but should it expect the original value to be remembered ?\n>\n>\n> We can indeed store the clipped fixed_shutter_ directly. However, max_shutter_ can change at runtime (based on the application request), and so we must re-clip fixed_shutter_ if that occurs. There is also another simplification here, we always pass max_shutter_ as the second argument to clip_shutter. This can be removed, so clip_shutter takes only one argument.\n>\n> David, what are your thoughts?\n\nI guess I don't feel very strongly, whatever is tidiest is good with\nme. Having a new field such as clipped_fixed_shutter_ seems\nreasonable, I think over-writing fixed_shutter_ itself would be\nundesirable for the reasons described. Generally I'm a bit nervous\nabout adding extra state variables that you have to remember to\nupdate, but I guess if only SetFixedShutter and SetMaxShutter have to\ndo that then it all seems tidy enough, right?\n\nThanks!\nDavid\n\n>\n> Naush\n>\n>>\n>>\n>> An alternative would be to turn clip_shutter() into a GetFixedShutter()\n>> member function that would use fixed_shutter_ and max_shutter_\n>> internally and not take any argument. There's one caller that uses the\n>> function with a different set of arguments, so maybe we would need to\n>> keep clip_shutter() and add a GetFixedShutter() wrapper.\n>>\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>> > }\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 30ba9aef9d97..75c9e404dcc1 100644\n>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > @@ -1008,6 +1008,18 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio\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>> > + agc->SetMaxShutter(maxShutter);\n>> > }\n>> >\n>> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart","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 107AEC0F2B\n\tfor ;\n\tWed, 27 Jan 2021 09:47:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7836568356;\n\tWed, 27 Jan 2021 10:47:15 +0100 (CET)","from mail-oo1-xc33.google.com (mail-oo1-xc33.google.com\n\t[IPv6:2607:f8b0:4864:20::c33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE75268353\n\tfor ;\n\tWed, 27 Jan 2021 10:47:14 +0100 (CET)","by mail-oo1-xc33.google.com with SMTP id y72so376340ooa.5\n\tfor ;\n\tWed, 27 Jan 2021 01:47:14 -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=\"ge4vRv2i\"; 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:content-transfer-encoding;\n\tbh=8CMXuOqoR3e070G/3fSvsqfsdQY9qEUuFCAAniSf1nU=;\n\tb=ge4vRv2iThjtJ0occJq71f4pXhSTQ3bI6l9nKqAoVIoqAC2JfEG5HHSFIzRAmrdhP9\n\tSOrhUQKBRliKJRte2bYXTBswmmZ1I9G+jndJXdtYtS07XJxmZCkPf3vK/vh+NBngHB67\n\td+OljfBg87xZO5H21Y2ef9ChBjgIHNwx9VNm1MbypZeoRi8O8k4VniKJDU4k55VIubD3\n\tQ1K7p1qD7H9B/0WwsqCEKB03YDgHBK2nvKcUMoq2G3EQ7JbwCj5U4WUjGE3fmaIOZy0A\n\tHCgalITQEqsKJlLLvleiDVN/buZaUbNxhMhnmlKyRmqKQLCgrzcUGaB0LM/xYHfpB3hC\n\tbkzA==","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:content-transfer-encoding;\n\tbh=8CMXuOqoR3e070G/3fSvsqfsdQY9qEUuFCAAniSf1nU=;\n\tb=qi8GACjug5crLHXCSo3cqiMMgr0o+tJhuLtfE9TZjRGEsx6/cP03bZwg632WP2eVm+\n\tBCK3OChRgaqotaHpQfKAzIP/a+ytxQfQn5bqIjH6gxVovjErKeH+Ejiym7QNzdUYwcOT\n\ta4XknOb8Km44Ie4Fyju5CGZHRI1hYcoxQWJIn89bkADXJVWumPh0tnCv7FJHIOqJSD2L\n\tPiLuKbBwPiY7ayW1zUmjbpOUuBY0rKEdk5dKhSrIlsxR8tOutfNj/YDcjfUE3H6GZEsW\n\t/uNGvV2AIpDdd/rnlPf2XUUa0L+Knm2Ihsu7+2wKAF1SDYo4OLeaSlFsq6/Jm9C2HEqh\n\tfnGg==","X-Gm-Message-State":"AOAM5316p8T4eITaFGwCVdsZV1Z3d/Z9IsLJdgF8bzq8wsrxmht/v5aj\n\t14fPzCiTdzsW8dOURWzhbhIUxXu6Ilj/rSOUfGJMFA==","X-Google-Smtp-Source":"ABdhPJwIsPGEcMG1771FfwUi6ahIFC2bGa5noyyImyCLcUB5Jf0r2dqHuJYUwCULxgU8nOfcz+CfSvzXWyufYlU+PD4=","X-Received":"by 2002:a4a:d891:: with SMTP id b17mr7017080oov.28.1611740833249;\n\tWed, 27 Jan 2021 01:47:13 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-3-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 27 Jan 2021 09:47:03 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}},{"id":14820,"web_url":"https://patchwork.libcamera.org/comment/14820/","msgid":"","date":"2021-01-27T09:58:38","subject":"Re: [libcamera-devel] [PATCH v2 3/4] 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":"On Wed, 27 Jan 2021 at 09:47, David Plowman \nwrote:\n\n> Hi Naush, Laurent\n>\n> Thanks for all the discussion on this.\n>\n> On Wed, 27 Jan 2021 at 09:34, Naushir Patuck \n> wrote:\n> >\n> > Hi Laurent,\n> >\n> > Thank you for your review feedback.\n> >\n> > On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n> >>\n> >> Hi Naush,\n> >>\n> >> Thank you for the patch.\n> >>\n> >> On Sun, Jan 24, 2021 at 02:05:05PM +0000, Naushir Patuck wrote:\n> >> > In order to provide an optimal split between shutter speed and gain,\n> 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> >>\n> >> s/AgcAlgorihtm/AgcAlgorithm/\n> >\n> >\n> > Ack.\n> >\n> >>\n> >>\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 | 49\n> +++++++++++++------\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\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; //\n> microseconds\n> >>\n> >> Not something to be addressed now, but would it make sense to express\n> >> durations using std::chrono::duration ? This would avoid the risk of\n> >> passing a value expressed in the wrong unit. Another option would be to\n> >> use double across the code base, using the same unit everywhere (which\n> >> could be seconds, double should give us enough precision).\n> >>\n> >> > virtual void SetFixedAnalogueGain(double fixed_analogue_gain) =\n> 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..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\n> tells us\n> >> > @@ -227,11 +227,23 @@ void Agc::SetFlickerPeriod(double\n> 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_,\n> max_shutter_);\n> >>\n> >> Instead of clipping every time the fixed shutter value is accessed,\n> >> wouldn't it be simpler to store the clipped shutter in fixed_shutter_ ?\n> >> This would possibly cause the fixed shutter to be clipped based on the\n> >> current mode and not get increased on mode change, but is that a problem\n> >> ? An application setting the ExposureTime control should expect it to be\n> >> clipped, but should it expect the original value to be remembered ?\n> >\n> >\n> > We can indeed store the clipped fixed_shutter_ directly. However,\n> max_shutter_ can change at runtime (based on the application request), and\n> so we must re-clip fixed_shutter_ if that occurs. There is also another\n> simplification here, we always pass max_shutter_ as the second argument to\n> clip_shutter. This can be removed, so clip_shutter takes only one argument.\n> >\n> > David, what are your thoughts?\n>\n> I guess I don't feel very strongly, whatever is tidiest is good with\n> me. Having a new field such as clipped_fixed_shutter_ seems\n> reasonable, I think over-writing fixed_shutter_ itself would be\n> undesirable for the reasons described. Generally I'm a bit nervous\n> about adding extra state variables that you have to remember to\n> update, but I guess if only SetFixedShutter and SetMaxShutter have to\n> do that then it all seems tidy enough, right?\n>\n> Thanks!\n> David\n>\n\nMy personal preference would be:\n\n- Do not store a clipped_fixed_shutter_, instead do what we currently do,\ni.e. clip on use. From my scan through, it only happens once per frame and\non a switch mode. So it will only add a few cycles overall, but will make\nthe code more readable imo (by avoiding additional state variables, one\nclipped, one not).\n- clip_shutter becomes a private member function, so it can access\nmax_shutter_ and we do not pass the second argument into the function.\n\nHappy to go with the consensus though.\n\n\n>\n> >\n> > Naush\n> >\n> >>\n> >>\n> >> An alternative would be to turn clip_shutter() into a GetFixedShutter()\n> >> member function that would use fixed_shutter_ and max_shutter_\n> >> internally and not take any argument. There's one caller that uses the\n> >> function with a different set of arguments, so maybe we would need to\n> >> keep clip_shutter() and add a GetFixedShutter() wrapper.\n> >>\n> >> > }\n> >> >\n> >> > void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> >> > @@ -261,7 +273,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 = clip_shutter(fixed_shutter_,\n> max_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 +282,14 @@ void Agc::SwitchMode([[maybe_unused]]\n> CameraMode 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\n> \"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 +303,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 +416,8 @@ void Agc::housekeepConfig()\n> >> > {\n> >> > // First fetch all the up-to-date settings, so no one else has\n> to do it.\n> >> > status_.ev = ev_;\n> >> > - status_.fixed_shutter = fixed_shutter_;\n> >> > + double fixed_shutter = clip_shutter(fixed_shutter_,\n> 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\n> * gain;\n> >> > // The final target exposure is also limited to what\n> the exposure\n> >> > // mode allows.\n> >> > + double max_shutter = status_.fixed_shutter != 0.0\n> >> > + ? status_.fixed_shutter\n> >> > + :\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 =\n> 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> >> > +\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 /\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> >> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> 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; //\n> microseconds\n> >> > void SetFixedAnalogueGain(double fixed_analogue_gain) override;\n> >> > void SetMeteringMode(std::string const &metering_mode_name)\n> 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\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > index 30ba9aef9d97..75c9e404dcc1 100644\n> >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > @@ -1008,6 +1008,18 @@ void IPARPi::applyFrameDurations(double\n> minFrameDuration, double maxFrameDuratio\n> >> > libcameraMetadata_.set(controls::FrameDurations,\n> >> > {\n> static_cast(minFrameDuration_),\n> >> >\n> static_cast(maxFrameDuration_) });\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> >> > + 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 EFD57C0F2B\n\tfor ;\n\tWed, 27 Jan 2021 09:58:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7575168359;\n\tWed, 27 Jan 2021 10:58:57 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B535768353\n\tfor ;\n\tWed, 27 Jan 2021 10:58:55 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id q12so1777082lfo.12\n\tfor ;\n\tWed, 27 Jan 2021 01:58:55 -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=\"XoUHjkl+\"; 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=99IXc941XzhCKIK5h4zI2dTrqmzRDw/+ywiyfkzx4EU=;\n\tb=XoUHjkl+CS5M2USD2vJ2zs2SVfMWa1TL5ic3qgV/dUyRLcHiOEL1BsyHDHQZ2oWfcM\n\tMkOAJi/0GqqER2oVeDSHRJpyY7aE1X7699rEGgD7471TWZ+US9l8/AqMzW+4PXZ9DG2V\n\tG8S4G6DtzWvsKZtVZaWL71ZAg4aK/7NBfxDo1HeOHPofn+g6m3kq6SobDuJMtg0PzvBl\n\tk4YV4MKxHC5VUQfs/h84jwoL2Q/+L7wPSm6T/zmHFdx1BxHw2x+65c+lVdobt9Dc1EuK\n\to6Klfq15JqliCi3XlxQOY6YvKD5kRQ7/6nyt5CG/0vX6J19SWU7O2jxxEmE0zoyTrQEi\n\ttiJw==","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=99IXc941XzhCKIK5h4zI2dTrqmzRDw/+ywiyfkzx4EU=;\n\tb=WzAJdd0senj9I5E1LUGrMEayNE/Xi026KB20GBxXts+mB8zr5uvR+KxQU7p7/NSPSs\n\tP8o84pHRk+0ygY4JRpq4cB84UumUEmNut8QBB6oCWINqaVB1l3qRclbogvJ2h41EjW/u\n\tgSnSqH0VuBE1M2YQiMZPuZLo/4PfM2UAAg6wR58JAG2+oFa5OPQt4ojE4npBCFX+SvQx\n\tJid0c9hFm+GDwZptjzH8juS2hjykUgjF99vBrhq+RUjTae362e1LdQb7jYqxLyH/J7cx\n\tAJ6opBrg/J4tbH+3guS9KrfnmOUm1TgOJVZqkiviHg+AfbOfHjCK/ILrkthme7CEIR79\n\tALQg==","X-Gm-Message-State":"AOAM531ZkVn3X0rQSCSBdmCmRyXe75cTWCNwIkvayqgLmM54p4n4549v\n\t4XNuxakmzR2HaPbOwx/yH7InQzDK8M580TIjm6HpjQ==","X-Google-Smtp-Source":"ABdhPJw7FzbPe2MJH2EhvOA01zxYqKZG4cE53R8bl3668TI1URq3vnya+ro5InSI4STymi35K0goTH9i7yj9hMW3KBM=","X-Received":"by 2002:a19:8805:: with SMTP id k5mr4562166lfd.567.1611741535075;\n\tWed, 27 Jan 2021 01:58:55 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-3-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 27 Jan 2021 09:58:38 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v2 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":"multipart/mixed;\n\tboundary=\"===============0758650805793372781==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14821,"web_url":"https://patchwork.libcamera.org/comment/14821/","msgid":"","date":"2021-01-27T10:01:22","subject":"Re: [libcamera-devel] [PATCH v2 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\nOn Wed, 27 Jan 2021 at 09:58, Naushir Patuck wrote:\n>\n>\n> On Wed, 27 Jan 2021 at 09:47, David Plowman wrote:\n>>\n>> Hi Naush, Laurent\n>>\n>> Thanks for all the discussion on this.\n>>\n>> On Wed, 27 Jan 2021 at 09:34, Naushir Patuck wrote:\n>> >\n>> > Hi Laurent,\n>> >\n>> > Thank you for your review feedback.\n>> >\n>> > On Wed, 27 Jan 2021 at 00:17, Laurent Pinchart wrote:\n>> >>\n>> >> Hi Naush,\n>> >>\n>> >> Thank you for the patch.\n>> >>\n>> >> On Sun, Jan 24, 2021 at 02:05:05PM +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 AgcAlgorihtm class. The\n>> >>\n>> >> s/AgcAlgorihtm/AgcAlgorithm/\n>> >\n>> >\n>> > Ack.\n>> >\n>> >>\n>> >>\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 | 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>> >>\n>> >> Not something to be addressed now, but would it make sense to express\n>> >> durations using std::chrono::duration ? This would avoid the risk of\n>> >> passing a value expressed in the wrong unit. Another option would be to\n>> >> use double across the code base, using the same unit everywhere (which\n>> >> could be seconds, double should give us enough precision).\n>> >>\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>> >> Instead of clipping every time the fixed shutter value is accessed,\n>> >> wouldn't it be simpler to store the clipped shutter in fixed_shutter_ ?\n>> >> This would possibly cause the fixed shutter to be clipped based on the\n>> >> current mode and not get increased on mode change, but is that a problem\n>> >> ? An application setting the ExposureTime control should expect it to be\n>> >> clipped, but should it expect the original value to be remembered ?\n>> >\n>> >\n>> > We can indeed store the clipped fixed_shutter_ directly. However, max_shutter_ can change at runtime (based on the application request), and so we must re-clip fixed_shutter_ if that occurs. There is also another simplification here, we always pass max_shutter_ as the second argument to clip_shutter. This can be removed, so clip_shutter takes only one argument.\n>> >\n>> > David, what are your thoughts?\n>>\n>> I guess I don't feel very strongly, whatever is tidiest is good with\n>> me. Having a new field such as clipped_fixed_shutter_ seems\n>> reasonable, I think over-writing fixed_shutter_ itself would be\n>> undesirable for the reasons described. Generally I'm a bit nervous\n>> about adding extra state variables that you have to remember to\n>> update, but I guess if only SetFixedShutter and SetMaxShutter have to\n>> do that then it all seems tidy enough, right?\n>>\n>> Thanks!\n>> David\n>\n>\n> My personal preference would be:\n>\n> - Do not store a clipped_fixed_shutter_, instead do what we currently do, i.e. clip on use. From my scan through, it only happens once per frame and on a switch mode. So it will only add a few cycles overall, but will make the code more readable imo (by avoiding additional state variables, one clipped, one not).\n> - clip_shutter becomes a private member function, so it can access max_shutter_ and we do not pass the second argument into the function.\n>\n> Happy to go with the consensus though.\n\nThumbs up from me!\n\nDavid\n\n>\n>>\n>>\n>> >\n>> > Naush\n>> >\n>> >>\n>> >>\n>> >> An alternative would be to turn clip_shutter() into a GetFixedShutter()\n>> >> member function that would use fixed_shutter_ and max_shutter_\n>> >> internally and not take any argument. There's one caller that uses the\n>> >> function with a different set of arguments, so maybe we would need to\n>> >> keep clip_shutter() and add a GetFixedShutter() wrapper.\n>> >>\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>> >> > }\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 30ba9aef9d97..75c9e404dcc1 100644\n>> >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> >> > @@ -1008,6 +1008,18 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio\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>> >> > + agc->SetMaxShutter(maxShutter);\n>> >> > }\n>> >> >\n>> >> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n>> >>\n>> >> --\n>> >> Regards,\n>> >>\n>> >> Laurent Pinchart","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 B8F18BD808\n\tfor ;\n\tWed, 27 Jan 2021 10:01:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31CCA6835B;\n\tWed, 27 Jan 2021 11:01:36 +0100 (CET)","from mail-oi1-x22a.google.com (mail-oi1-x22a.google.com\n\t[IPv6:2607:f8b0:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A165768357\n\tfor ;\n\tWed, 27 Jan 2021 11:01:34 +0100 (CET)","by mail-oi1-x22a.google.com with SMTP id d18so1580714oic.3\n\tfor ;\n\tWed, 27 Jan 2021 02:01:34 -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=\"fVBQOdJF\"; 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:content-transfer-encoding;\n\tbh=53AYQowTPsOcWh2Cc1fjAF1qh/ef/u5gQJ5MBJue5HI=;\n\tb=fVBQOdJFFxcLQg3pPRkUkFBHK4jcELT0qjmSZw5CTuuosNxIXQ36lJ3ErkzyXuCNSd\n\tar1tGLU1kfTNd8GDfDGElhDi9NyuA9LkkXoimEB4M3Cg+XSag8jvP7Z+lSa9ZGQfrizO\n\t0jq45gIYzm3fx90FF+x4UMl8VqpeHmY4G7nyUDM03/bTohKeI9y0eqqLKOwtMu7hoNSv\n\t1KJ3pFXFVMV2iGM3ovhPPb/AMdONTzyNuik9XVtJsZWGiF+JIGaqeRBE4Cmm18uLaOTE\n\t4CYRKP1b72pYsr4M4uDCoZnY+XSouzfB+rUEyagAZDOyQK02aEoSqGD57oxZmGYpoqxR\n\tMy3g==","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:content-transfer-encoding;\n\tbh=53AYQowTPsOcWh2Cc1fjAF1qh/ef/u5gQJ5MBJue5HI=;\n\tb=iBULq6NToQiiFrl7TT2Mq/RlqUuKZoZbviMXV0rjJuxXHOHxU8ozWZ0Kf4It9viZu5\n\t6CMcIqJePWVaDDK7NrO9BjZ8M9BnBZpxChXXjBY5vBYrm76fz0ZQTNUjVz+kVjOHNd3W\n\tPfeWwDO1fny+6m/z94UzlDBPWH6z/fwPVN4iaAuwoXMipkRtJ6aNTLAvtmp834A82JeN\n\tbRN6JmkwlJbo6JqzMlipqG22nvZbt6ojSYJdvLQvXAhnPsLq2yFKO1auj+RqbI8A52ci\n\t//+CJuCLTJ3G5DFbHj3P/uS2stHIDQ2RChQMTGPbxQCReno58HoWMdSXLLMSNvv2H9Xb\n\t8XAw==","X-Gm-Message-State":"AOAM530qALxQyeQ1bTKeA06x7mXdV1NYu/PWdJ3M4b4ecW1cKRYjKUu7\n\tJ+cbg+DevsZ8nCAPpNhoF6J4MXCtnbQ2qkm7YwHRWw==","X-Google-Smtp-Source":"ABdhPJyV2zaQbzmzjxWnDT6R8hry4q985DkPGvuFTX/27ff4ZgLp9uqNvMZvwQxbPMl6Y3u9ojk3fNpfhsQa/8NiRk8=","X-Received":"by 2002:aca:5c0a:: with SMTP id q10mr2703295oib.55.1611741693111;\n\tWed, 27 Jan 2021 02:01:33 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-3-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 27 Jan 2021 10:01:22 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}},{"id":14822,"web_url":"https://patchwork.libcamera.org/comment/14822/","msgid":"","date":"2021-01-27T10:07:58","subject":"Re: [libcamera-devel] [PATCH v2 3/4] 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\nOn Wed, 27 Jan 2021 at 00:17, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Sun, Jan 24, 2021 at 02:05:05PM +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 AgcAlgorihtm class. The\n>\n> s/AgcAlgorihtm/AgcAlgorithm/\n>\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 | 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\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>\n> Not something to be addressed now, but would it make sense to express\n> durations using std::chrono::duration ? This would avoid the risk of\n> passing a value expressed in the wrong unit. Another option would be to\n> use double across the code base, using the same unit everywhere (which\n> could be seconds, double should give us enough precision).\n>\n\nSorry, forgot to comment about this. I do think it's a great idea to use\nstd::chrono::duration throughout for time based variables. It reduces so\nmuch of the ambiguity over time bases. When I get a chance, perhaps I'll\nhave a go within our IPA and controller.\n\nRegards,\nNaush\n\n\n\n>\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..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> Instead of clipping every time the fixed shutter value is accessed,\n> wouldn't it be simpler to store the clipped shutter in fixed_shutter_ ?\n> This would possibly cause the fixed shutter to be clipped based on the\n> current mode and not get increased on mode change, but is that a problem\n> ? An application setting the ExposureTime control should expect it to be\n> clipped, but should it expect the original value to be remembered ?\n>\n> An alternative would be to turn clip_shutter() into a GetFixedShutter()\n> member function that would use fixed_shutter_ and max_shutter_\n> internally and not take any argument. There's one caller that uses the\n> function with a different set of arguments, so maybe we would need to\n> keep clip_shutter() and add a GetFixedShutter() wrapper.\n>\n> > }\n> >\n> > void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> > @@ -261,7 +273,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 = 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\n> fixed values.\n> >\n> > fetchAwbStatus(metadata);\n> > @@ -269,14 +282,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 +303,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 +416,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 = 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 *\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> > + 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> > +\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 /\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> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> 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; //\n> microseconds\n> > void SetFixedAnalogueGain(double fixed_analogue_gain) override;\n> > void SetMeteringMode(std::string const &metering_mode_name)\n> 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\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 30ba9aef9d97..75c9e404dcc1 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -1008,6 +1008,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 8B3ACC0F2B\n\tfor ;\n\tWed, 27 Jan 2021 10:08:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 015F568363;\n\tWed, 27 Jan 2021 11:08:17 +0100 (CET)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9034068357\n\tfor ;\n\tWed, 27 Jan 2021 11:08:15 +0100 (CET)","by mail-lj1-x232.google.com with SMTP id 3so1401923ljc.4\n\tfor ;\n\tWed, 27 Jan 2021 02:08:15 -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=\"i0VlCFwa\"; 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=euwvbg3JMn7QR7eQ862cEfseaPAS3Wx3A24/3zd7Ekw=;\n\tb=i0VlCFwa0Sz87SsRCw/ueXI/ZwjahVpQqpkNcunfYxlFGSu0w7GYZgAsduWBPZZABS\n\tp37A9vV0Z/RUWTAyQ4Z0xqLu37Qh+cxditWGhuafzj2sYRwm+M/uflYHZXYW/z6tOh0K\n\tAPKWu1KuCG4TUKF57RW6os4Q0wvv/yiQ+Tyf8nlRnGd/1EvUZnd88fAC4JRc1fdXmGRD\n\ta4VTQtbzoyoln+QvYl38ttxzxyFA0p0lNPzXpRUs3NK/Tb8rvfzjL3nQf7JkIntBBFjI\n\tKvs2+FU3DFyZS8/gXstGb6EWQZhy59MUXmUN2kL/8ayOpM8pmAXUmhI4CRZvDWS13mtX\n\tbQww==","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=euwvbg3JMn7QR7eQ862cEfseaPAS3Wx3A24/3zd7Ekw=;\n\tb=i2KUyAo4HvfZnzAHMgjXI2vOVCTgp0hGVK7IzlIsE0H9sgKueyQHCPk+EQd/9J1/Qj\n\tV88KDNbFFkaN03rByekGQN0VvqpWAcqyrozt8s4od1wIEOTQVuzqoU2vckteEJXr+qyR\n\tyZtYyfs767pkmRybDGrYtI5PJoz2765Vi/FyRYed1bOfLowIxiP1YRhnhLpQymHm80zB\n\tFgbyBRXnr0R8OZvmmjOwlft6zB+HBLmIdmM2ligcwUE8i17ewoduxCrL9MXMrFkYsndk\n\tHVJ6U5xnRPVOmmjPVN2D6GsbnhmAWGgQnUSk6VRX71ixJT6/vVjyWmSuEcvdTbLOVw/8\n\tDJbQ==","X-Gm-Message-State":"AOAM532WYp2QQ0ORdO0GEKuoYxHl96XRkTnxDXEC3sxpZHpc17UfwunU\n\tF5aPztTMkRx8McaM1XG0Epb1oYi2tgc6mr93e0nUN5uItBpquw==","X-Google-Smtp-Source":"ABdhPJxRndVl/V4utAwvU1f7WZB1NclHhzOQ7QlBHaJbJuF8X+yCr589oMqxrJ1uAbdtwHkNI4Xqj1nlv83ApADuQ2I=","X-Received":"by 2002:a2e:9992:: with SMTP id\n\tw18mr5464449lji.169.1611742095004; \n\tWed, 27 Jan 2021 02:08:15 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 27 Jan 2021 10:07:58 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 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":"multipart/mixed;\n\tboundary=\"===============4108334934685478989==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]