[{"id":14057,"web_url":"https://patchwork.libcamera.org/comment/14057/","msgid":"","date":"2020-12-04T15:57:25","subject":"Re: [libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add\n\tGetDropFrames method to AGC base class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your patch.\n\nOn Wed, 2 Dec 2020 at 11:53, David Plowman \nwrote:\n\n> We add a GetDropFrames method to the AgcAlgorithm class which can be\n> called when the AGC is started from scratch. It suggests how many\n> frames should be dropped before displaying any (while the AGC\n> converges).\n>\n> The Raspberry Pi specific implementation makes this customisable from\n> the tuning file.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/agc_algorithm.hpp | 1 +\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 11 +++++++++++\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 ++\n> 3 files changed, 14 insertions(+)\n>\n> diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> index b4ea54fb..bfc9743f 100644\n> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> @@ -15,6 +15,7 @@ class AgcAlgorithm : public Algorithm\n> public:\n> AgcAlgorithm(Controller *controller) : Algorithm(controller) {}\n> // An AGC algorithm must provide the following:\n> + virtual unsigned int GetDropFrames() const = 0;\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> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 9da18c31..94c02d47 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -142,6 +142,7 @@ void AgcConfig::Read(boost::property_tree::ptree const\n> ¶ms)\n> Y_target.Read(params.get_child(\"y_target\"));\n> speed = params.get(\"speed\", 0.2);\n> startup_frames = params.get(\"startup_frames\", 10);\n> + drop_frames = params.get(\"drop_frames\", 6);\n> fast_reduce_threshold =\n> params.get(\"fast_reduce_threshold\", 0.4);\n> base_ev = params.get(\"base_ev\", 1.0);\n> @@ -206,6 +207,16 @@ void Agc::Resume()\n> fixed_analogue_gain_ = 0;\n> }\n>\n> +unsigned int Agc::GetDropFrames() const\n> +{\n> + // If shutter and gain have been explicitly set, there is no\n> + // convergence to happen, so no need to drop any frames.\n> + if (fixed_shutter_ && fixed_analogue_gain_)\n> + return 0;\n> + else\n> + return config_.drop_frames;\n> +}\n> +\n>\n\n One little nit on this one. The method name, GetDropFrames, doesn't sit\nright with me. Could we rename this to something like, say,\nGetConvergenceFrames, or something. I feel this would better convey what\nthis number is. What do you think?\n\n void Agc::SetEv(double ev)\n> {\n> ev_ = ev;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 95db1812..1de4d505 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -52,6 +52,7 @@ struct AgcConfig {\n> Pwl Y_target;\n> double speed;\n> uint16_t startup_frames;\n> + unsigned int drop_frames;\n>\n\nSimilarly, instead of calling it drop_frames, we could call it\nconvergence_frames?\n\nRegards,\nNaush\n\n\n> double max_change;\n> double min_change;\n> double fast_reduce_threshold;\n> @@ -74,6 +75,7 @@ public:\n> bool IsPaused() const override;\n> void Pause() override;\n> void Resume() override;\n> + unsigned int GetDropFrames() const override;\n> void SetEv(double ev) override;\n> void SetFlickerPeriod(double flicker_period) override;\n> void SetFixedShutter(double fixed_shutter) override; //\n> microseconds\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1E5B6BE177\n\tfor ;\n\tFri, 4 Dec 2020 15:57:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0F9A635D6;\n\tFri, 4 Dec 2020 16:57:44 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDB5A635D0\n\tfor ;\n\tFri, 4 Dec 2020 16:57:42 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id y7so7144946lji.8\n\tfor ;\n\tFri, 04 Dec 2020 07:57:42 -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=\"bWy26s8+\"; 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=WIN0chrJadp5RoUjZgVf1o/Fjk0iGCcls7hrF/cUXss=;\n\tb=bWy26s8+RztQfvLrWH5YhAEWqtbpWhcMe3Vs4mTqmXHDthJVAIYPnJV3nfR9WN+6rE\n\tF45eLbPeSA0kTW9o4gvZXM8Ikkzu6kV0bqL7tGORQoi15bjnPUqOIWES9NQnxAJjzNz/\n\tDs3PC4wlg94udLMTd7zxGAANSdAqP1yVl6ZOQ1YMePZeGnLp0Ny4I4cXNla9CrCL9h+Q\n\toxsKL1yIeiRiXQVcyqVT0ucOXtA1pNHVIcA70obNDp5j/xXNCVpIW2CStTcyrHRw6bps\n\t//yjeyujtQG6QI2Tq/f5Vljn6Qpyh6+D0p0Y8C5q1ujasM4QJHb2GAu1iribXdOWatOF\n\tL8aw==","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=WIN0chrJadp5RoUjZgVf1o/Fjk0iGCcls7hrF/cUXss=;\n\tb=uEtvuMi7mqEbbgKg/hJhWnyRWqcBcoHPX1IltJOFwgb27ItHmivFQBiII0EPlfbqFg\n\tflk958SuZFI/Q3kchRrfnwzzwCfKX3+f3YkvbDnpbJ3/Aqyi2LiPR9cDCn28ZCv0I5xl\n\t7cJkeXw0PPs8nJWeIDJiYuoZrSUC8LqUo5Q+VdZJGiaezJAvZfLddQ4Vu3dhjFWCZ6QN\n\tEAng7RN76jX5YYtZSYFu9jow17iY/ARpo/AfYKSN4GqdL5x3bj/RSZWPmlqd2qApD5kF\n\tG2F/zkgAnxseHCWYWaPyx1Kx6RcTR41VfaftOploLe/QORx1BhcbS7T2IU3MAjZunska\n\tJBeg==","X-Gm-Message-State":"AOAM530MO20slCbVeGbPp85+psV4CYJD5/cy/JzqHTpEXB7WV813sMtr\n\tjq4ny7U7Y/bC7b4p+G80IGeL05Y9prYRFjx2fD4qbebOzih7VQ==","X-Google-Smtp-Source":"ABdhPJxxTTEQZRcUlPkP+EP6RRZqWUrkPz3/PMXh7G7ZrO/tvILNuXXnhUbA5pkwztAzJnn2vFneqxOknt6VmydIMBM=","X-Received":"by 2002:a2e:9ad2:: with SMTP id\n\tp18mr3740629ljj.415.1607097462235; \n\tFri, 04 Dec 2020 07:57:42 -0800 (PST)","MIME-Version":"1.0","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-4-david.plowman@raspberrypi.com>","In-Reply-To":"<20201202115253.14705-4-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Fri, 4 Dec 2020 15:57:25 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add\n\tGetDropFrames method to AGC base class","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=\"===============5764232959684984642==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14082,"web_url":"https://patchwork.libcamera.org/comment/14082/","msgid":"","date":"2020-12-05T20:02:58","subject":"Re: [libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add\n\tGetDropFrames method to AGC base class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David and Naush,\n\nOn Fri, Dec 04, 2020 at 03:57:25PM +0000, Naushir Patuck wrote:\n> On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:\n> \n> > We add a GetDropFrames method to the AgcAlgorithm class which can be\n> > called when the AGC is started from scratch. It suggests how many\n> > frames should be dropped before displaying any (while the AGC\n> > converges).\n> >\n> > The Raspberry Pi specific implementation makes this customisable from\n> > the tuning file.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/controller/agc_algorithm.hpp | 1 +\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 11 +++++++++++\n> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 ++\n> > 3 files changed, 14 insertions(+)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> > index b4ea54fb..bfc9743f 100644\n> > --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> > +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> > @@ -15,6 +15,7 @@ class AgcAlgorithm : public Algorithm\n> > public:\n> > AgcAlgorithm(Controller *controller) : Algorithm(controller) {}\n> > // An AGC algorithm must provide the following:\n> > + virtual unsigned int GetDropFrames() const = 0;\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> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 9da18c31..94c02d47 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -142,6 +142,7 @@ void AgcConfig::Read(boost::property_tree::ptree const ¶ms)\n> > Y_target.Read(params.get_child(\"y_target\"));\n> > speed = params.get(\"speed\", 0.2);\n> > startup_frames = params.get(\"startup_frames\", 10);\n> > + drop_frames = params.get(\"drop_frames\", 6);\n> > fast_reduce_threshold =\n> > params.get(\"fast_reduce_threshold\", 0.4);\n> > base_ev = params.get(\"base_ev\", 1.0);\n> > @@ -206,6 +207,16 @@ void Agc::Resume()\n> > fixed_analogue_gain_ = 0;\n> > }\n> >\n> > +unsigned int Agc::GetDropFrames() const\n> > +{\n> > + // If shutter and gain have been explicitly set, there is no\n> > + // convergence to happen, so no need to drop any frames.\n> > + if (fixed_shutter_ && fixed_analogue_gain_)\n> > + return 0;\n> > + else\n> > + return config_.drop_frames;\n> > +}\n> > +\n> \n> One little nit on this one. The method name, GetDropFrames, doesn't sit\n> right with me. Could we rename this to something like, say,\n> GetConvergenceFrames, or something. I feel this would better convey what\n> this number is. What do you think?\n\nI think that's better, but, as commented in another patch, I wonder if\nwe actually need this, or if we should base the decision on the AeLocked\nvalue.\n\nI'm also wondering how one would pick the right value to set in the\ntuning file. And don't we need a patch for CTT to set a value in the\nfile it generates ?\n\n> void Agc::SetEv(double ev)\n> > {\n> > ev_ = ev;\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > index 95db1812..1de4d505 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > @@ -52,6 +52,7 @@ struct AgcConfig {\n> > Pwl Y_target;\n> > double speed;\n> > uint16_t startup_frames;\n> > + unsigned int drop_frames;\n> >\n> \n> Similarly, instead of calling it drop_frames, we could call it\n> convergence_frames?\n> \n> > double max_change;\n> > double min_change;\n> > double fast_reduce_threshold;\n> > @@ -74,6 +75,7 @@ public:\n> > bool IsPaused() const override;\n> > void Pause() override;\n> > void Resume() override;\n> > + unsigned int GetDropFrames() const override;\n> > void SetEv(double ev) override;\n> > void SetFlickerPeriod(double flicker_period) override;\n> > void SetFixedShutter(double fixed_shutter) override; // microseconds","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 E728ABDB20\n\tfor ;\n\tSat, 5 Dec 2020 20:03:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 810E2635F0;\n\tSat, 5 Dec 2020 21:03:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A44860327\n\tfor ;\n\tSat, 5 Dec 2020 21:03:02 +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 31B68D2F;\n\tSat, 5 Dec 2020 21:03:01 +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=\"Ae4cFMWI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607198581;\n\tbh=2nl0mr/98wG+hfSMi9pymYpIHQo7KrG/ycU7Lf+pZeU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ae4cFMWIYmqIiF3KAeXq5/4XpG0jyx0pfYRR0AR78mo6U5XFZ2rPeNDQA8O2RYWe1\n\t4w7xABc48JCq8f7gVgR9X22qYlda2+RUBk0DHD8lhjwXw/BFLHur0CQu6Ti/Qsw4J/\n\tauTDaipFwDtcThGIk6263WHLr9z/3hQqptPIehYw=","Date":"Sat, 5 Dec 2020 22:02:58 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-4-david.plowman@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add\n\tGetDropFrames method to AGC base class","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":14090,"web_url":"https://patchwork.libcamera.org/comment/14090/","msgid":"","date":"2020-12-05T23:39:28","subject":"Re: [libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add\n\tGetDropFrames method to AGC base class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the review.\n\nOn Sat, 5 Dec 2020 at 20:03, Laurent Pinchart\n wrote:\n>\n> Hi David and Naush,\n>\n> On Fri, Dec 04, 2020 at 03:57:25PM +0000, Naushir Patuck wrote:\n> > On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:\n> >\n> > > We add a GetDropFrames method to the AgcAlgorithm class which can be\n> > > called when the AGC is started from scratch. It suggests how many\n> > > frames should be dropped before displaying any (while the AGC\n> > > converges).\n> > >\n> > > The Raspberry Pi specific implementation makes this customisable from\n> > > the tuning file.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/controller/agc_algorithm.hpp | 1 +\n> > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 11 +++++++++++\n> > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 ++\n> > > 3 files changed, 14 insertions(+)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> > > index b4ea54fb..bfc9743f 100644\n> > > --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> > > @@ -15,6 +15,7 @@ class AgcAlgorithm : public Algorithm\n> > > public:\n> > > AgcAlgorithm(Controller *controller) : Algorithm(controller) {}\n> > > // An AGC algorithm must provide the following:\n> > > + virtual unsigned int GetDropFrames() const = 0;\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> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > index 9da18c31..94c02d47 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > @@ -142,6 +142,7 @@ void AgcConfig::Read(boost::property_tree::ptree const ¶ms)\n> > > Y_target.Read(params.get_child(\"y_target\"));\n> > > speed = params.get(\"speed\", 0.2);\n> > > startup_frames = params.get(\"startup_frames\", 10);\n> > > + drop_frames = params.get(\"drop_frames\", 6);\n> > > fast_reduce_threshold =\n> > > params.get(\"fast_reduce_threshold\", 0.4);\n> > > base_ev = params.get(\"base_ev\", 1.0);\n> > > @@ -206,6 +207,16 @@ void Agc::Resume()\n> > > fixed_analogue_gain_ = 0;\n> > > }\n> > >\n> > > +unsigned int Agc::GetDropFrames() const\n> > > +{\n> > > + // If shutter and gain have been explicitly set, there is no\n> > > + // convergence to happen, so no need to drop any frames.\n> > > + if (fixed_shutter_ && fixed_analogue_gain_)\n> > > + return 0;\n> > > + else\n> > > + return config_.drop_frames;\n> > > +}\n> > > +\n> >\n> > One little nit on this one. The method name, GetDropFrames, doesn't sit\n> > right with me. Could we rename this to something like, say,\n> > GetConvergenceFrames, or something. I feel this would better convey what\n> > this number is. What do you think?\n>\n> I think that's better, but, as commented in another patch, I wonder if\n> we actually need this, or if we should base the decision on the AeLocked\n> value.\n>\n> I'm also wondering how one would pick the right value to set in the\n> tuning file. And don't we need a patch for CTT to set a value in the\n> file it generates ?\n\nThe value just gets assigned a default, according to the person who\nimplemented the algorithm. My advice would be to use that, so nothing\nextra is required in the json file (nor in the CTT therefore). Having\nsaid that, there are always people who like to fiddle (\"I want to see\nevery frame even while everything is still converging!\") which is why\nI have given them the option to customise it.\n\nBest regards\nDavid\n\n>\n> > void Agc::SetEv(double ev)\n> > > {\n> > > ev_ = ev;\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > index 95db1812..1de4d505 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > @@ -52,6 +52,7 @@ struct AgcConfig {\n> > > Pwl Y_target;\n> > > double speed;\n> > > uint16_t startup_frames;\n> > > + unsigned int drop_frames;\n> > >\n> >\n> > Similarly, instead of calling it drop_frames, we could call it\n> > convergence_frames?\n> >\n> > > double max_change;\n> > > double min_change;\n> > > double fast_reduce_threshold;\n> > > @@ -74,6 +75,7 @@ public:\n> > > bool IsPaused() const override;\n> > > void Pause() override;\n> > > void Resume() override;\n> > > + unsigned int GetDropFrames() const override;\n> > > void SetEv(double ev) override;\n> > > void SetFlickerPeriod(double flicker_period) override;\n> > > void SetFixedShutter(double fixed_shutter) override; // microseconds\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 C56B8BDB20\n\tfor ;\n\tSat, 5 Dec 2020 23:39:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 556E8635F6;\n\tSun, 6 Dec 2020 00:39:43 +0100 (CET)","from mail-oo1-xc41.google.com (mail-oo1-xc41.google.com\n\t[IPv6:2607:f8b0:4864:20::c41])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10899635D0\n\tfor ;\n\tSun, 6 Dec 2020 00:39:42 +0100 (CET)","by mail-oo1-xc41.google.com with SMTP id i13so2340432oou.11\n\tfor ;\n\tSat, 05 Dec 2020 15:39:41 -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=\"oTUhBYLG\"; 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=r5M6LSLERvCorhIF4//nB1wyaoB8SrxjkvwGT+q480A=;\n\tb=oTUhBYLGJBedF1lNKR9PVrSCzwVejvBwVmfGu3BiVp4ojfMEqHb5qz+zbabFRYj37S\n\tq2l7YDP1U4kwHopvFYJfpkWcYcsMWTAxHbK1gSywp25Ql3ZNCVrfP74QNUFSB2Wiq5yA\n\tVKniB8fbuy6jUBHmGpmvTYyZ0SWp+mWiBVV3sPNDZmHKR0D/IsKBldzT7xzT7kQfyXO0\n\tBCHy4YbgzsEgRB85aVG1p0RHMVYCosBXEhO9EPWxlG9XGS0FyVkI+djWYh+d0l8tGTMD\n\tNtQoUcZMZkzxuE0VDN/e3eTjjcYalLut4kOVBXyFRpvip6iIp6fFPjWVnzBTJzCkQ9Sx\n\tkyOg==","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=r5M6LSLERvCorhIF4//nB1wyaoB8SrxjkvwGT+q480A=;\n\tb=LWODVWo39badFJ1/zz8RbIaHmXJzfCd5d2E78rx7qKvDndd9fXjauChzBXKxoEG5Q1\n\tzYY79NRaIvheTTpcuD/DQCNtUnoui/JGBqbAhTrqS0LPM+z6StBHud0oVhlhWRjYJA9n\n\tT6aQ3HxOhpbY+OXz94UNDz93n6V+o5BK0ULdchWZOOlIogAxhtK9gzGz/OWy4nv8Tm6R\n\tvRaXntbPVoggH354AqP8dz+ATawfe1zkGU836pv9VFqnY47JPPJjzYTRyOnSvzZtuQfO\n\tIVdUztdxwASY9LmTGKE1IaLG52RskXWenFxkP7F6Lpgr8PB7V+83QshMcHmsO1AkkAAM\n\tj4eA==","X-Gm-Message-State":"AOAM532uyUcK3JKfy8X+apTGbqbZXbS4sPTTlkEr4dWXEvYCwF1eZ3N/\n\toerShha+9k04FzEK3hCWD3Cs71R6mzrycZNtfPOIeg==","X-Google-Smtp-Source":"ABdhPJxtsPRJZYzBfSd7SdLTeQO2hV8/BqvcycJ1o8GpuoLP0OUV30zP/rzSBxyBJfszfUIa5yMIkxJI6Av7K1zpomQ=","X-Received":"by 2002:a4a:84c9:: with SMTP id o9mr8182669oog.28.1607211580520; \n\tSat, 05 Dec 2020 15:39:40 -0800 (PST)","MIME-Version":"1.0","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-4-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Sat, 5 Dec 2020 23:39:28 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add\n\tGetDropFrames method to AGC base class","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":14148,"web_url":"https://patchwork.libcamera.org/comment/14148/","msgid":"","date":"2020-12-08T17:26:42","subject":"Re: [libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add\n\tGetDropFrames method to AGC base class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Sat, Dec 05, 2020 at 11:39:28PM +0000, David Plowman wrote:\n> On Sat, 5 Dec 2020 at 20:03, Laurent Pinchart wrote:\n> > On Fri, Dec 04, 2020 at 03:57:25PM +0000, Naushir Patuck wrote:\n> > > On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:\n> > >\n> > > > We add a GetDropFrames method to the AgcAlgorithm class which can be\n> > > > called when the AGC is started from scratch. It suggests how many\n> > > > frames should be dropped before displaying any (while the AGC\n> > > > converges).\n> > > >\n> > > > The Raspberry Pi specific implementation makes this customisable from\n> > > > the tuning file.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/controller/agc_algorithm.hpp | 1 +\n> > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 11 +++++++++++\n> > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 ++\n> > > > 3 files changed, 14 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> > > > index b4ea54fb..bfc9743f 100644\n> > > > --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp\n> > > > @@ -15,6 +15,7 @@ class AgcAlgorithm : public Algorithm\n> > > > public:\n> > > > AgcAlgorithm(Controller *controller) : Algorithm(controller) {}\n> > > > // An AGC algorithm must provide the following:\n> > > > + virtual unsigned int GetDropFrames() const = 0;\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> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > index 9da18c31..94c02d47 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > @@ -142,6 +142,7 @@ void AgcConfig::Read(boost::property_tree::ptree const ¶ms)\n> > > > Y_target.Read(params.get_child(\"y_target\"));\n> > > > speed = params.get(\"speed\", 0.2);\n> > > > startup_frames = params.get(\"startup_frames\", 10);\n> > > > + drop_frames = params.get(\"drop_frames\", 6);\n> > > > fast_reduce_threshold =\n> > > > params.get(\"fast_reduce_threshold\", 0.4);\n> > > > base_ev = params.get(\"base_ev\", 1.0);\n> > > > @@ -206,6 +207,16 @@ void Agc::Resume()\n> > > > fixed_analogue_gain_ = 0;\n> > > > }\n> > > >\n> > > > +unsigned int Agc::GetDropFrames() const\n> > > > +{\n> > > > + // If shutter and gain have been explicitly set, there is no\n> > > > + // convergence to happen, so no need to drop any frames.\n> > > > + if (fixed_shutter_ && fixed_analogue_gain_)\n> > > > + return 0;\n> > > > + else\n> > > > + return config_.drop_frames;\n> > > > +}\n> > > > +\n> > >\n> > > One little nit on this one. The method name, GetDropFrames, doesn't sit\n> > > right with me. Could we rename this to something like, say,\n> > > GetConvergenceFrames, or something. I feel this would better convey what\n> > > this number is. What do you think?\n> >\n> > I think that's better, but, as commented in another patch, I wonder if\n> > we actually need this, or if we should base the decision on the AeLocked\n> > value.\n> >\n> > I'm also wondering how one would pick the right value to set in the\n> > tuning file. And don't we need a patch for CTT to set a value in the\n> > file it generates ?\n> \n> The value just gets assigned a default, according to the person who\n> implemented the algorithm. My advice would be to use that, so nothing\n> extra is required in the json file (nor in the CTT therefore). Having\n> said that, there are always people who like to fiddle (\"I want to see\n> every frame even while everything is still converging!\") which is why\n> I have given them the option to customise it.\n\nSounds good to me. Maybe this could become a runtime API in the future\nif a need arises, but that's definitely for later.\n\n> > > void Agc::SetEv(double ev)\n> > > > {\n> > > > ev_ = ev;\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > index 95db1812..1de4d505 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > @@ -52,6 +52,7 @@ struct AgcConfig {\n> > > > Pwl Y_target;\n> > > > double speed;\n> > > > uint16_t startup_frames;\n> > > > + unsigned int drop_frames;\n> > > >\n> > >\n> > > Similarly, instead of calling it drop_frames, we could call it\n> > > convergence_frames?\n> > >\n> > > > double max_change;\n> > > > double min_change;\n> > > > double fast_reduce_threshold;\n> > > > @@ -74,6 +75,7 @@ public:\n> > > > bool IsPaused() const override;\n> > > > void Pause() override;\n> > > > void Resume() override;\n> > > > + unsigned int GetDropFrames() const override;\n> > > > void SetEv(double ev) override;\n> > > > void SetFlickerPeriod(double flicker_period) override;\n> > > > void SetFixedShutter(double fixed_shutter) override; // microseconds","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 78921BDB1F\n\tfor ;\n\tTue, 8 Dec 2020 17:26:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B05867F09;\n\tTue, 8 Dec 2020 18:26:47 +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 EAF0F67E4D\n\tfor ;\n\tTue, 8 Dec 2020 18:26:45 +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 78B22543;\n\tTue, 8 Dec 2020 18:26:45 +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=\"N4BHpfGg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607448405;\n\tbh=szfAT+LGyAXaWicDYuIlKERyv1YPVmvgOj82hpXjqrc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=N4BHpfGgcbitg556noLOHg/FNOIF5dKm1UMbTsZivZB1zfvEZup5mDcSQKmrK21Js\n\tZsUi1XvE96+lpJ5KyvTUGKi5xB5avUCgnz9vU7+uM78JEQy9AUul4ixGmQZETUxPcL\n\tpb2QmVWzCFF9eG/HV/J4tBQoYwhpGtw4VdVLK7ks=","Date":"Tue, 8 Dec 2020 19:26:42 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-4-david.plowman@raspberrypi.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/5] src: ipa: raspberrypi: agc: Add\n\tGetDropFrames method to AGC base class","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\" "}}]