[{"id":13911,"web_url":"https://patchwork.libcamera.org/comment/13911/","msgid":"<20201126125618.GG3905@pendragon.ideasonboard.com>","date":"2020-11-26T12:56:18","subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Thu, Nov 26, 2020 at 12:32:01PM +0000, David Plowman wrote:\n> AGC, when paused, sets the last exposure/gain it wrote to be its\n> \"fixed\" values and will therefore continue to return them. When\n> resumed, we clear them so that both will float again.\n> \n> This approach is better because AGC can be paused and we can\n> subsequently change (for example) the exposure and the gain won't\n> float again.\n> \n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +++++\n> 2 files changed, 29 insertions(+)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 30a1c1c1..9da18c31 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -184,6 +184,26 @@ void Agc::Read(boost::property_tree::ptree const ¶ms)\n> \texposure_mode_ = &config_.exposure_modes[exposure_mode_name_];\n> \tconstraint_mode_name_ = config_.default_constraint_mode;\n> \tconstraint_mode_ = &config_.constraint_modes[constraint_mode_name_];\n> +\t// Set up the \"last shutter/gain\" values, in case AGC starts \"disabled\".\n> +\tstatus_.shutter_time = config_.default_exposure_time;\n> +\tstatus_.analogue_gain = config_.default_analogue_gain;\n> +}\n> +\n> +bool Agc::IsPaused() const\n> +{\n> +\treturn false;\n> +}\n> +\n> +void Agc::Pause()\n> +{\n> +\tfixed_shutter_ = status_.shutter_time;\n> +\tfixed_analogue_gain_ = status_.analogue_gain;\n> +}\n> +\n> +void Agc::Resume()\n> +{\n> +\tfixed_shutter_ = 0;\n> +\tfixed_analogue_gain_ = 0;\n> }\n> \n> void Agc::SetEv(double ev)\n> @@ -199,11 +219,15 @@ void Agc::SetFlickerPeriod(double flicker_period)\n> void Agc::SetFixedShutter(double fixed_shutter)\n> {\n> \tfixed_shutter_ = fixed_shutter;\n\nMaybe there's something I'm missing, but when a request is queued with\nan exposure time, Agc::SetFixedShutter is called regardless of whether\nAGC is enabled or disabled. fixed_shutter_ is thus set to a non-zero\nvalue, which will be copied to status.fixed_shutter in\nAgc::housekeepConfig(), called in Agc::Process(). As the core code\ndoesn't see the AGC being paused anymore, it will call Process() for\nevery frame. Won't the fixed shutter take precedence, even when AGC is\nenabled ?\n\n> +\t// Set this in case someone calls Pause() straight after.\n> +\tstatus_.shutter_time = fixed_shutter;\n> }\n> \n> void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> {\n> \tfixed_analogue_gain_ = fixed_analogue_gain;\n> +\t// Set this in case someone calls Pause() straight after.\n> +\tstatus_.analogue_gain = fixed_analogue_gain;\n> }\n> \n> void Agc::SetMeteringMode(std::string const &metering_mode_name)\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 47ebb324..8a1a20e6 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -70,6 +70,11 @@ public:\n> \tAgc(Controller *controller);\n> \tchar const *Name() const override;\n> \tvoid Read(boost::property_tree::ptree const ¶ms) override;\n> +\t// AGC handles \"pausing\" for itself.\n> +\tbool IsPaused() const;\n> +\tvoid Pause();\n> +\tvoid Resume();\n\nCould you add \"override\" for those three functions ?\n\n> +\tunsigned int GetFrameDrops() const override;\n> \tvoid SetEv(double ev) override;\n> \tvoid SetFlickerPeriod(double flicker_period) override;\n> \tvoid 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 B992DBE176\n\tfor ;\n\tThu, 26 Nov 2020 12:56:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5204363459;\n\tThu, 26 Nov 2020 13:56:29 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E06163408\n\tfor ;\n\tThu, 26 Nov 2020 13:56:28 +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 D0BAEA1B;\n\tThu, 26 Nov 2020 13:56:27 +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=\"em+M9/v8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606395388;\n\tbh=MckDaScbnhqaHSYYQ84cawW6ym5tZhPVyJrtjXRz4wg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=em+M9/v8Uvy8BsVkex4plBH8C0LGLt++facdjjGxWeWCW4LgRP4QGYvCPH4JvkCwM\n\twozZk1ZdGiMn88jk0Mt/5Q8w5upVYBVb3D+WMCZvVsHxMXtDGCEqm+pxDIeE8eEUed\n\tyE97SSJOU774m4Nute1vCGbjhkPLMYihhGGqC6gw=","Date":"Thu, 26 Nov 2020 14:56:18 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201126125618.GG3905@pendragon.ideasonboard.com>","References":"<20201126123203.19105-1-david.plowman@raspberrypi.com>\n\t<20201126123203.19105-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201126123203.19105-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","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":13918,"web_url":"https://patchwork.libcamera.org/comment/13918/","msgid":"","date":"2020-11-26T14:10:25","subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nYes, good comments.\n\nOn Thu, 26 Nov 2020 at 12:56, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Thu, Nov 26, 2020 at 12:32:01PM +0000, David Plowman wrote:\n> > AGC, when paused, sets the last exposure/gain it wrote to be its\n> > \"fixed\" values and will therefore continue to return them. When\n> > resumed, we clear them so that both will float again.\n> >\n> > This approach is better because AGC can be paused and we can\n> > subsequently change (for example) the exposure and the gain won't\n> > float again.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++\n> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +++++\n> > 2 files changed, 29 insertions(+)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 30a1c1c1..9da18c31 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -184,6 +184,26 @@ void Agc::Read(boost::property_tree::ptree const ¶ms)\n> > exposure_mode_ = &config_.exposure_modes[exposure_mode_name_];\n> > constraint_mode_name_ = config_.default_constraint_mode;\n> > constraint_mode_ = &config_.constraint_modes[constraint_mode_name_];\n> > + // Set up the \"last shutter/gain\" values, in case AGC starts \"disabled\".\n> > + status_.shutter_time = config_.default_exposure_time;\n> > + status_.analogue_gain = config_.default_analogue_gain;\n> > +}\n> > +\n> > +bool Agc::IsPaused() const\n> > +{\n> > + return false;\n> > +}\n> > +\n> > +void Agc::Pause()\n> > +{\n> > + fixed_shutter_ = status_.shutter_time;\n> > + fixed_analogue_gain_ = status_.analogue_gain;\n> > +}\n> > +\n> > +void Agc::Resume()\n> > +{\n> > + fixed_shutter_ = 0;\n> > + fixed_analogue_gain_ = 0;\n> > }\n> >\n> > void Agc::SetEv(double ev)\n> > @@ -199,11 +219,15 @@ void Agc::SetFlickerPeriod(double flicker_period)\n> > void Agc::SetFixedShutter(double fixed_shutter)\n> > {\n> > fixed_shutter_ = fixed_shutter;\n>\n> Maybe there's something I'm missing, but when a request is queued with\n> an exposure time, Agc::SetFixedShutter is called regardless of whether\n> AGC is enabled or disabled. fixed_shutter_ is thus set to a non-zero\n> value, which will be copied to status.fixed_shutter in\n> Agc::housekeepConfig(), called in Agc::Process(). As the core code\n> doesn't see the AGC being paused anymore, it will call Process() for\n> every frame. Won't the fixed shutter take precedence, even when AGC is\n> enabled ?\n\nThe AGC really has 4 modes of operation, depending on the various\ncombinations of fixed_shutter_ and fixed_analogue_gain_ being zero or\nnot. For example, fixed ISO (fixed gain, really) exposures are\naccomplished by fixing the analogue gain, and the exposure time can\nstill float.\n\nIn all cases except where both are non-zero, the AGC has to run. But\neven in this last case we still leave AGC running so that things work\nin the same way, but it actually does some useful things for us too:\n\n1. If you ask for a fixed shutter and gain, but the gain is bigger\nthan the sensor can deliver, the AGC will make up the rest with\ndigital gain.\n\n2. There's always this headache case if any of the colour gains go\nless than 1. The AGC takes care of that too by forcing the digital\ngain up to 1 / minimum_colour_gain.\n\nSo in short, yes, Prepare and Process keep running every frame - and\nthose \"fixed\" values take precedence if they are non-zero.\n\nI hope that makes it clearer... (but I'm not sure!)\n\n>\n> > + // Set this in case someone calls Pause() straight after.\n> > + status_.shutter_time = fixed_shutter;\n> > }\n> >\n> > void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> > {\n> > fixed_analogue_gain_ = fixed_analogue_gain;\n> > + // Set this in case someone calls Pause() straight after.\n> > + status_.analogue_gain = fixed_analogue_gain;\n> > }\n> >\n> > void Agc::SetMeteringMode(std::string const &metering_mode_name)\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > index 47ebb324..8a1a20e6 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > @@ -70,6 +70,11 @@ public:\n> > Agc(Controller *controller);\n> > char const *Name() const override;\n> > void Read(boost::property_tree::ptree const ¶ms) override;\n> > + // AGC handles \"pausing\" for itself.\n> > + bool IsPaused() const;\n> > + void Pause();\n> > + void Resume();\n>\n> Could you add \"override\" for those three functions ?\n\nOops, of course. Sorry about that!\n\nBest regards\nDavid\n\n>\n> > + unsigned int GetFrameDrops() 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 B790CBE176\n\tfor ;\n\tThu, 26 Nov 2020 14:10:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3ACDF6345A;\n\tThu, 26 Nov 2020 15:10:39 +0100 (CET)","from mail-oi1-x242.google.com (mail-oi1-x242.google.com\n\t[IPv6:2607:f8b0:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3E1063408\n\tfor ;\n\tThu, 26 Nov 2020 15:10:37 +0100 (CET)","by mail-oi1-x242.google.com with SMTP id f11so2396067oij.6\n\tfor ;\n\tThu, 26 Nov 2020 06:10:37 -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=\"l25d+6T+\"; 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=2PlqJ8+RE3u0ideeSnXaE5Z8njjHZCJvxSfF8DpdiHU=;\n\tb=l25d+6T+ZgxQYbYeTwDH6t7lCeLnFWdu2TIz6AdYfKRc9DVTH48S9GRpPCzWtoh6dZ\n\tLMbZvFcL4g+ga7k47OtOQ9Bz6kcmIhGdWGqIRYk+q2bHmUDNGKNYk++5WZb+sxdEQhuh\n\tHGeQQI3jC8mMh7na2qqtoAezGRDxf88EhHVkVxzY37jzefF5jRxuJP65z76PXj/G+T8z\n\tkb2vjk+Ei9z7zY/GrzzzM9y24Z/GcLE7Cdj+8tf7KmNsiP+DxSe/aCFShQLCzjEWDCqz\n\t1NIzX4/V782Xu5NCm29qws67ithpubg1kBbP/ty89B5vDWcA/cKIfZiP1DyEAc56ZdFM\n\tGclw==","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=2PlqJ8+RE3u0ideeSnXaE5Z8njjHZCJvxSfF8DpdiHU=;\n\tb=Vg418nInjcOB2+o5m8NaZGfn1Tlrvd9xuQz+xmYrv/gqMtbOP/wyYER8Q8HDSXejys\n\tY8EPVvQ+l8VNZnBrxiJ/htG+h4WNchCNfSvTJOFss7XZkPOB3LxBmH6xneKzcp5rztDu\n\tqmDsm1XLUB4x3IXQ4zL2/dTgmKARpLUSeoQ8thylBOXK0pH2DH4jfxyZDFi8PZeGFNQk\n\tk8ZXAKTo1bM0knwXo70F19GQne1aZUdFGHNC2I//dGnxGL+fqm33xiD9IIr5teAKH0YK\n\t6sbgMRe6c/pyGXc87T7VT3FpwYNskCkiC2XnL8NkjzMokOmMmLTsdZtGFDkCSfrORkIR\n\ti0qA==","X-Gm-Message-State":"AOAM532hJIRUVCDp2+gqV4uuj1/ngjAo/SLSXfQdMzjevQ1cD4DXMIq8\n\tm0OfLFwtlQFLvJImFE0G4OYfXGoXa+BDRvz1xNYgRO5wooXBGg==","X-Google-Smtp-Source":"ABdhPJzNdWT6+GmrgVi8D7p06Zg+diWjEvYCriqj5Z0RRkli1Se88q/QyHa97OSDetZk/eKm93x86e2e9Z1lt28wDB0=","X-Received":"by 2002:aca:de89:: with SMTP id\n\tv131mr2137671oig.55.1606399836720; \n\tThu, 26 Nov 2020 06:10:36 -0800 (PST)","MIME-Version":"1.0","References":"<20201126123203.19105-1-david.plowman@raspberrypi.com>\n\t<20201126123203.19105-3-david.plowman@raspberrypi.com>\n\t<20201126125618.GG3905@pendragon.ideasonboard.com>","In-Reply-To":"<20201126125618.GG3905@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Thu, 26 Nov 2020 14:10:25 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","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":13969,"web_url":"https://patchwork.libcamera.org/comment/13969/","msgid":"","date":"2020-11-30T10:56:33","subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 26/11/2020 14:10, David Plowman wrote:\n> Hi Laurent\n> \n> Yes, good comments.\n> \n> On Thu, 26 Nov 2020 at 12:56, Laurent Pinchart\n> wrote:\n>>\n>> Hi David,\n>>\n>> Thank you for the patch.\n>>\n>> On Thu, Nov 26, 2020 at 12:32:01PM +0000, David Plowman wrote:\n>>> AGC, when paused, sets the last exposure/gain it wrote to be its\n>>> \"fixed\" values and will therefore continue to return them. When\n>>> resumed, we clear them so that both will float again.\n>>>\n>>> This approach is better because AGC can be paused and we can\n>>> subsequently change (for example) the exposure and the gain won't\n>>> float again.\n>>>\n>>> Signed-off-by: David Plowman \n>>> ---\n>>> src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++\n>>> src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +++++\n>>> 2 files changed, 29 insertions(+)\n>>>\n>>> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n>>> index 30a1c1c1..9da18c31 100644\n>>> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n>>> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n>>> @@ -184,6 +184,26 @@ void Agc::Read(boost::property_tree::ptree const ¶ms)\n>>> exposure_mode_ = &config_.exposure_modes[exposure_mode_name_];\n>>> constraint_mode_name_ = config_.default_constraint_mode;\n>>> constraint_mode_ = &config_.constraint_modes[constraint_mode_name_];\n>>> + // Set up the \"last shutter/gain\" values, in case AGC starts \"disabled\".\n>>> + status_.shutter_time = config_.default_exposure_time;\n>>> + status_.analogue_gain = config_.default_analogue_gain;\n>>> +}\n>>> +\n>>> +bool Agc::IsPaused() const\n>>> +{\n>>> + return false;\n>>> +}\n>>> +\n>>> +void Agc::Pause()\n>>> +{\n>>> + fixed_shutter_ = status_.shutter_time;\n>>> + fixed_analogue_gain_ = status_.analogue_gain;\n>>> +}\n>>> +\n>>> +void Agc::Resume()\n>>> +{\n>>> + fixed_shutter_ = 0;\n>>> + fixed_analogue_gain_ = 0;\n\nthere won't be any side effects here if the requests are specifying\nfixed values will there?\n\nI.e. something calling 'Resume' isn't going to remove the users requests...\n\n\n>>> }\n>>>\n>>> void Agc::SetEv(double ev)\n>>> @@ -199,11 +219,15 @@ void Agc::SetFlickerPeriod(double flicker_period)\n>>> void Agc::SetFixedShutter(double fixed_shutter)\n>>> {\n>>> fixed_shutter_ = fixed_shutter;\n>>\n>> Maybe there's something I'm missing, but when a request is queued with\n>> an exposure time, Agc::SetFixedShutter is called regardless of whether\n>> AGC is enabled or disabled. fixed_shutter_ is thus set to a non-zero\n>> value, which will be copied to status.fixed_shutter in\n>> Agc::housekeepConfig(), called in Agc::Process(). As the core code\n>> doesn't see the AGC being paused anymore, it will call Process() for\n>> every frame. Won't the fixed shutter take precedence, even when AGC is\n>> enabled ?\n> \n> The AGC really has 4 modes of operation, depending on the various\n> combinations of fixed_shutter_ and fixed_analogue_gain_ being zero or\n> not. For example, fixed ISO (fixed gain, really) exposures are\n> accomplished by fixing the analogue gain, and the exposure time can\n> still float.\n> \n> In all cases except where both are non-zero, the AGC has to run. But\n> even in this last case we still leave AGC running so that things work\n> in the same way, but it actually does some useful things for us too:\n> \n> 1. If you ask for a fixed shutter and gain, but the gain is bigger\n> than the sensor can deliver, the AGC will make up the rest with\n> digital gain.\n> \n> 2. There's always this headache case if any of the colour gains go\n> less than 1. The AGC takes care of that too by forcing the digital\n> gain up to 1 / minimum_colour_gain.\n> \n> So in short, yes, Prepare and Process keep running every frame - and\n> those \"fixed\" values take precedence if they are non-zero.\n> \n> I hope that makes it clearer... (but I'm not sure!)\n> \n>>\n>>> + // Set this in case someone calls Pause() straight after.\n\nBaha, Ok that answers my question above.\n\nSo this is fine with me.\n\nWith the overrides added in the next version,\n\nReviewed-by: Kieran Bingham \n\nWe're in src/ipa/raspberrypi so it would be helpful to have some RB or\nTB tags from Naush on this series.\n\n\n\n>>> + status_.shutter_time = fixed_shutter;\n>>> }\n>>>\n>>> void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n>>> {\n>>> fixed_analogue_gain_ = fixed_analogue_gain;\n>>> + // Set this in case someone calls Pause() straight after.\n>>> + status_.analogue_gain = fixed_analogue_gain;\n>>> }\n>>>\n>>> void Agc::SetMeteringMode(std::string const &metering_mode_name)\n>>> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n>>> index 47ebb324..8a1a20e6 100644\n>>> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n>>> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n>>> @@ -70,6 +70,11 @@ public:\n>>> Agc(Controller *controller);\n>>> char const *Name() const override;\n>>> void Read(boost::property_tree::ptree const ¶ms) override;\n>>> + // AGC handles \"pausing\" for itself.\n>>> + bool IsPaused() const;\n>>> + void Pause();\n>>> + void Resume();\n>>\n>> Could you add \"override\" for those three functions ?\n> \n> Oops, of course. Sorry about that!\n> \n> Best regards\n> David\n> \n>>\n>>> + unsigned int GetFrameDrops() 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\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 93D90BE177\n\tfor ;\n\tMon, 30 Nov 2020 10:56:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E5B7634A1;\n\tMon, 30 Nov 2020 11:56:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6742563457\n\tfor ;\n\tMon, 30 Nov 2020 11:56:36 +0100 (CET)","from [192.168.0.217]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D9ABAB26;\n\tMon, 30 Nov 2020 11:56:35 +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=\"qxH7jTX2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606733796;\n\tbh=PQsMts8p/sCLya8M+wGUFML9cdLCdXhYgq4/J7BVM5g=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qxH7jTX23hvRDsl/i2cxpEwTRM96rDriyFJ2mL32LrjO8rKdwd8NMRAVBopQfjW7K\n\tILYfcQBJXtCIRiJNiFQ1QKWZuir7BZlZe4ovzIl2PxWSWJu0t4Hl2Tj1Ra1ah5rLl6\n\tvpWD8INjUILLGyj3Lm0Boz9UTonFNBCIpUoFVzas=","To":"David Plowman ,\n\tLaurent Pinchart ","References":"<20201126123203.19105-1-david.plowman@raspberrypi.com>\n\t<20201126123203.19105-3-david.plowman@raspberrypi.com>\n\t<20201126125618.GG3905@pendragon.ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 30 Nov 2020 10:56:33 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","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","Reply-To":"kieran.bingham@ideasonboard.com","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":13986,"web_url":"https://patchwork.libcamera.org/comment/13986/","msgid":"<20201130182540.GV4141@pendragon.ideasonboard.com>","date":"2020-11-30T18:25:40","subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Thu, Nov 26, 2020 at 02:10:25PM +0000, David Plowman wrote:\n> On Thu, 26 Nov 2020 at 12:56, Laurent Pinchart wrote:\n> > On Thu, Nov 26, 2020 at 12:32:01PM +0000, David Plowman wrote:\n> > > AGC, when paused, sets the last exposure/gain it wrote to be its\n> > > \"fixed\" values and will therefore continue to return them. When\n> > > resumed, we clear them so that both will float again.\n> > >\n> > > This approach is better because AGC can be paused and we can\n> > > subsequently change (for example) the exposure and the gain won't\n> > > float again.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++\n> > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +++++\n> > > 2 files changed, 29 insertions(+)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > index 30a1c1c1..9da18c31 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > @@ -184,6 +184,26 @@ void Agc::Read(boost::property_tree::ptree const ¶ms)\n> > > exposure_mode_ = &config_.exposure_modes[exposure_mode_name_];\n> > > constraint_mode_name_ = config_.default_constraint_mode;\n> > > constraint_mode_ = &config_.constraint_modes[constraint_mode_name_];\n> > > + // Set up the \"last shutter/gain\" values, in case AGC starts \"disabled\".\n> > > + status_.shutter_time = config_.default_exposure_time;\n> > > + status_.analogue_gain = config_.default_analogue_gain;\n> > > +}\n> > > +\n> > > +bool Agc::IsPaused() const\n> > > +{\n> > > + return false;\n> > > +}\n> > > +\n> > > +void Agc::Pause()\n> > > +{\n> > > + fixed_shutter_ = status_.shutter_time;\n> > > + fixed_analogue_gain_ = status_.analogue_gain;\n> > > +}\n> > > +\n> > > +void Agc::Resume()\n> > > +{\n> > > + fixed_shutter_ = 0;\n> > > + fixed_analogue_gain_ = 0;\n> > > }\n> > >\n> > > void Agc::SetEv(double ev)\n> > > @@ -199,11 +219,15 @@ void Agc::SetFlickerPeriod(double flicker_period)\n> > > void Agc::SetFixedShutter(double fixed_shutter)\n> > > {\n> > > fixed_shutter_ = fixed_shutter;\n> >\n> > Maybe there's something I'm missing, but when a request is queued with\n> > an exposure time, Agc::SetFixedShutter is called regardless of whether\n> > AGC is enabled or disabled. fixed_shutter_ is thus set to a non-zero\n> > value, which will be copied to status.fixed_shutter in\n> > Agc::housekeepConfig(), called in Agc::Process(). As the core code\n> > doesn't see the AGC being paused anymore, it will call Process() for\n> > every frame. Won't the fixed shutter take precedence, even when AGC is\n> > enabled ?\n> \n> The AGC really has 4 modes of operation, depending on the various\n> combinations of fixed_shutter_ and fixed_analogue_gain_ being zero or\n> not. For example, fixed ISO (fixed gain, really) exposures are\n> accomplished by fixing the analogue gain, and the exposure time can\n> still float.\n> \n> In all cases except where both are non-zero, the AGC has to run. But\n> even in this last case we still leave AGC running so that things work\n> in the same way, but it actually does some useful things for us too:\n> \n> 1. If you ask for a fixed shutter and gain, but the gain is bigger\n> than the sensor can deliver, the AGC will make up the rest with\n> digital gain.\n\nThat's interesting, I hadn't thought about that.\n\n> 2. There's always this headache case if any of the colour gains go\n> less than 1. The AGC takes care of that too by forcing the digital\n> gain up to 1 / minimum_colour_gain.\n> \n> So in short, yes, Prepare and Process keep running every frame - and\n> those \"fixed\" values take precedence if they are non-zero.\n> \n> I hope that makes it clearer... (but I'm not sure!)\n\nIt does, but doesn't address my concern fully :-)\n\nMy understanding of the code is that, when AGC is on, if an application\nsets a fixed shutter or fixed analog gain, those will take precedence.\nThere however seems to be no way to go back to dynamic shutter or analog\ngain. And not that I've written this, I wonder if this can be achieved\nby setting the shutter or gain to 0 explicitly. Is that the expected\nusage of this API ? If so it should at least be documented in\ncontrols_ids.yaml.\n\nI however wonder if we should create an AeMode control that would\nexplicitly select between the four modes (manual, auto,\nexposure-priority and gain-priority). And thinking about it, how about\ndevices that can also control the iris aperture ? Maybe the mode (or\nshould we call it AePriority ?) should be a bitmask with bits telling\nwhich parameters are manual and which are controlled by the AEGC\nalgorithm. But then, maybe setting manual parameters to zero would be a\ngood enough API to state they should be automatic, but in that case,\nwhat would the AeEnable control be used for if setting AeEnable to true\nwith non-zero value for ExposureTime and AnalogueGain effectively\ndisable auto-exposure ?\n\nWe don't have to solve this as part of this series, but I think there's\na bit of an inconsistency in the controls we have, which I'd like to\naddress. What would your preference be between the possible options\n(something better than me naive proposals above will certainly be\nwelcome too :-)) ?\n\n> > > + // Set this in case someone calls Pause() straight after.\n> > > + status_.shutter_time = fixed_shutter;\n> > > }\n> > >\n> > > void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> > > {\n> > > fixed_analogue_gain_ = fixed_analogue_gain;\n> > > + // Set this in case someone calls Pause() straight after.\n> > > + status_.analogue_gain = fixed_analogue_gain;\n> > > }\n> > >\n> > > void Agc::SetMeteringMode(std::string const &metering_mode_name)\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > index 47ebb324..8a1a20e6 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > @@ -70,6 +70,11 @@ public:\n> > > Agc(Controller *controller);\n> > > char const *Name() const override;\n> > > void Read(boost::property_tree::ptree const ¶ms) override;\n> > > + // AGC handles \"pausing\" for itself.\n> > > + bool IsPaused() const;\n> > > + void Pause();\n> > > + void Resume();\n> >\n> > Could you add \"override\" for those three functions ?\n> \n> Oops, of course. Sorry about that!\n>\n> > > + unsigned int GetFrameDrops() 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 4BBF9BE177\n\tfor ;\n\tMon, 30 Nov 2020 18:25:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2192634A1;\n\tMon, 30 Nov 2020 19:25:50 +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 BBA6B63320\n\tfor ;\n\tMon, 30 Nov 2020 19:25:49 +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 2EA09B26;\n\tMon, 30 Nov 2020 19:25:49 +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=\"qNTqMgOw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606760749;\n\tbh=K1tJZIo6Sq1w/XtoFAwc7DUVonEzBDi3i2XXi5ua1KE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qNTqMgOwhbtr3RTRMsyUxZjmUfy1qiaGccr47SzglaQRgZD3tX4jA14IcnW34JsWL\n\t8MXLQn5cDib8XXnilGESryiZxWlxPJ5SdMVTO3dQiS3Vg+5k50pFbsDrt4aAXj3sF1\n\t31AaT/vu6wjjb68t6ToA6Y8hS3F6HiWe2HyTHi4Y=","Date":"Mon, 30 Nov 2020 20:25:40 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201130182540.GV4141@pendragon.ideasonboard.com>","References":"<20201126123203.19105-1-david.plowman@raspberrypi.com>\n\t<20201126123203.19105-3-david.plowman@raspberrypi.com>\n\t<20201126125618.GG3905@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","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":13991,"web_url":"https://patchwork.libcamera.org/comment/13991/","msgid":"","date":"2020-11-30T20:21:05","subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nOn Mon, 30 Nov 2020 at 18:25, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> On Thu, Nov 26, 2020 at 02:10:25PM +0000, David Plowman wrote:\n> > On Thu, 26 Nov 2020 at 12:56, Laurent Pinchart wrote:\n> > > On Thu, Nov 26, 2020 at 12:32:01PM +0000, David Plowman wrote:\n> > > > AGC, when paused, sets the last exposure/gain it wrote to be its\n> > > > \"fixed\" values and will therefore continue to return them. When\n> > > > resumed, we clear them so that both will float again.\n> > > >\n> > > > This approach is better because AGC can be paused and we can\n> > > > subsequently change (for example) the exposure and the gain won't\n> > > > float again.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++\n> > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +++++\n> > > > 2 files changed, 29 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > index 30a1c1c1..9da18c31 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > > @@ -184,6 +184,26 @@ void Agc::Read(boost::property_tree::ptree const ¶ms)\n> > > > exposure_mode_ = &config_.exposure_modes[exposure_mode_name_];\n> > > > constraint_mode_name_ = config_.default_constraint_mode;\n> > > > constraint_mode_ = &config_.constraint_modes[constraint_mode_name_];\n> > > > + // Set up the \"last shutter/gain\" values, in case AGC starts \"disabled\".\n> > > > + status_.shutter_time = config_.default_exposure_time;\n> > > > + status_.analogue_gain = config_.default_analogue_gain;\n> > > > +}\n> > > > +\n> > > > +bool Agc::IsPaused() const\n> > > > +{\n> > > > + return false;\n> > > > +}\n> > > > +\n> > > > +void Agc::Pause()\n> > > > +{\n> > > > + fixed_shutter_ = status_.shutter_time;\n> > > > + fixed_analogue_gain_ = status_.analogue_gain;\n> > > > +}\n> > > > +\n> > > > +void Agc::Resume()\n> > > > +{\n> > > > + fixed_shutter_ = 0;\n> > > > + fixed_analogue_gain_ = 0;\n> > > > }\n> > > >\n> > > > void Agc::SetEv(double ev)\n> > > > @@ -199,11 +219,15 @@ void Agc::SetFlickerPeriod(double flicker_period)\n> > > > void Agc::SetFixedShutter(double fixed_shutter)\n> > > > {\n> > > > fixed_shutter_ = fixed_shutter;\n> > >\n> > > Maybe there's something I'm missing, but when a request is queued with\n> > > an exposure time, Agc::SetFixedShutter is called regardless of whether\n> > > AGC is enabled or disabled. fixed_shutter_ is thus set to a non-zero\n> > > value, which will be copied to status.fixed_shutter in\n> > > Agc::housekeepConfig(), called in Agc::Process(). As the core code\n> > > doesn't see the AGC being paused anymore, it will call Process() for\n> > > every frame. Won't the fixed shutter take precedence, even when AGC is\n> > > enabled ?\n> >\n> > The AGC really has 4 modes of operation, depending on the various\n> > combinations of fixed_shutter_ and fixed_analogue_gain_ being zero or\n> > not. For example, fixed ISO (fixed gain, really) exposures are\n> > accomplished by fixing the analogue gain, and the exposure time can\n> > still float.\n> >\n> > In all cases except where both are non-zero, the AGC has to run. But\n> > even in this last case we still leave AGC running so that things work\n> > in the same way, but it actually does some useful things for us too:\n> >\n> > 1. If you ask for a fixed shutter and gain, but the gain is bigger\n> > than the sensor can deliver, the AGC will make up the rest with\n> > digital gain.\n>\n> That's interesting, I hadn't thought about that.\n>\n> > 2. There's always this headache case if any of the colour gains go\n> > less than 1. The AGC takes care of that too by forcing the digital\n> > gain up to 1 / minimum_colour_gain.\n> >\n> > So in short, yes, Prepare and Process keep running every frame - and\n> > those \"fixed\" values take precedence if they are non-zero.\n> >\n> > I hope that makes it clearer... (but I'm not sure!)\n>\n> It does, but doesn't address my concern fully :-)\n>\n> My understanding of the code is that, when AGC is on, if an application\n> sets a fixed shutter or fixed analog gain, those will take precedence.\n> There however seems to be no way to go back to dynamic shutter or analog\n> gain. And not that I've written this, I wonder if this can be achieved\n> by setting the shutter or gain to 0 explicitly. Is that the expected\n> usage of this API ? If so it should at least be documented in\n> controls_ids.yaml.\n\nYes, setting them to zero makes them float again but we should\ndocument that and I'll do so.\n\n>\n> I however wonder if we should create an AeMode control that would\n> explicitly select between the four modes (manual, auto,\n> exposure-priority and gain-priority). And thinking about it, how about\n> devices that can also control the iris aperture ? Maybe the mode (or\n> should we call it AePriority ?) should be a bitmask with bits telling\n> which parameters are manual and which are controlled by the AEGC\n> algorithm. But then, maybe setting manual parameters to zero would be a\n> good enough API to state they should be automatic, but in that case,\n> what would the AeEnable control be used for if setting AeEnable to true\n> with non-zero value for ExposureTime and AnalogueGain effectively\n> disable auto-exposure ?\n>\n> We don't have to solve this as part of this series, but I think there's\n> a bit of an inconsistency in the controls we have, which I'd like to\n> address. What would your preference be between the possible options\n> (something better than me naive proposals above will certainly be\n> welcome too :-)) ?\n\nI think there's quite a lot to think about here. The camera world\ntends to talk about AE being on or off, fixed ISO, aperture or shutter\npriority, those kinds of terms. But I think computer people think\nabout 3 degrees of freedom (shutter, gain and aperture where it\nexists), and what combinations you can have in which they are fixed or\nfloating.\n\nBut actually it's worse than that because as soon as you have at least\ntwo of your variables that float, you can start to talk about the\norder and at which values they change.\n\nNow there is a solution to this, namely exposure profiles. In our\nimplementation we have only shutter and gain, but you could add\naperture and define exposure/aperture/gain profiles for all the\nsituations you could imagine.\n\nUnfortunately, I don't know if many vendors would do this, and you\ncould easily end up with vast numbers of seemingly arbitrary profiles.\nTherefore, I think I'm drawn to the conclusion that one is better off\ngoing with the \"camera\" view of the world. It's a bit of a lowest\ncommon denominator, but I think it would be broadly acceptable to\neveryone.\n\nI think this means you'd have, essentially, user-specified shutter or\ngain or aperture. Whether you can fix only one, or two would be up to\nthe implementation. Fixing all 3 would probably be known as \"AE off\".\nAnyway, that's my initial reaction, but there's definitely something\nto sleep on there!\n\nBest regards\nDavid\n\n>\n> > > > + // Set this in case someone calls Pause() straight after.\n> > > > + status_.shutter_time = fixed_shutter;\n> > > > }\n> > > >\n> > > > void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> > > > {\n> > > > fixed_analogue_gain_ = fixed_analogue_gain;\n> > > > + // Set this in case someone calls Pause() straight after.\n> > > > + status_.analogue_gain = fixed_analogue_gain;\n> > > > }\n> > > >\n> > > > void Agc::SetMeteringMode(std::string const &metering_mode_name)\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > index 47ebb324..8a1a20e6 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > > > @@ -70,6 +70,11 @@ public:\n> > > > Agc(Controller *controller);\n> > > > char const *Name() const override;\n> > > > void Read(boost::property_tree::ptree const ¶ms) override;\n> > > > + // AGC handles \"pausing\" for itself.\n> > > > + bool IsPaused() const;\n> > > > + void Pause();\n> > > > + void Resume();\n> > >\n> > > Could you add \"override\" for those three functions ?\n> >\n> > Oops, of course. Sorry about that!\n> >\n> > > > + unsigned int GetFrameDrops() 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 17A71BE176\n\tfor ;\n\tMon, 30 Nov 2020 20:21:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91E3E63320;\n\tMon, 30 Nov 2020 21:21:20 +0100 (CET)","from mail-oi1-x241.google.com (mail-oi1-x241.google.com\n\t[IPv6:2607:f8b0:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E26C963320\n\tfor ;\n\tMon, 30 Nov 2020 21:21:18 +0100 (CET)","by mail-oi1-x241.google.com with SMTP id w15so15565383oie.13\n\tfor ;\n\tMon, 30 Nov 2020 12:21:18 -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=\"DGTjOIuT\"; 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=zjQw7D2ZHihPddR5w7c/4kQMlAxRN/iieefoKlb9PqI=;\n\tb=DGTjOIuTgeAqfMW4hn7nqFX4c69NWJDyq3FrJl2oNB1k0TDqU6wrTw3+MwjyVJwT03\n\tZbeO9k+phdsuHJ8DfCdGaUQJnvbDGxgjpYGCj5jj+QYv6isEC3oqNTAUgvXcjUmcTCl0\n\twete5a1z8dWxOCuVqa9YfHKO7do5DgUJJl5sFHZl5ByJRUqCrx4fHJmj12C2IC5kcO/v\n\tvPTGosQYGjdnTZgXLbqMt6AyT3z2sYzjaMG2kUyowLoTSh19H8bKIQXDyXepEe8vIkuU\n\tjDeFrSTcMcMLVQX/iS7aU8gqZkBVhalJ4VEuMGKRmrthr6C99RP92a4EQs8RaIf/SgHQ\n\tOuIA==","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=zjQw7D2ZHihPddR5w7c/4kQMlAxRN/iieefoKlb9PqI=;\n\tb=nW6+DcJ9zSpiUL12mT/oLdrnv+4fKK5/GIgoGBThpYp4kWMYlchxfjoTBgCK1jDVyO\n\t4F4Mz8dUQpdzK2drSO8oDgm/kE9OLUGqXo2Cbuo2+jD0/Ta8oP8CqRVBIs/rYHvSjyMk\n\tDbjHf/+N0wdI7p81bipt9fBaA9hDZo0ocvqyOE7jq1evn+WOOA9/jiDslfKzNAkcbrE8\n\tvCBs8h9ltUoV3pBpZQx7r/8mgWLvoj/tjRYTB4i7/LR25Sgh9Z/JRfjf/usFHIp0C7Ax\n\tYfkKAy2pxW8VWKZJEQzVSi3OEeCV585Ie8XhWKxkypDM4OdUBjyXna+OSLwec433QaPs\n\tuQoQ==","X-Gm-Message-State":"AOAM533LJQP85x4eXxXwbFqOlQZD5r74TAfiJPjZGwJ7YfuFj4vABn7U\n\tuXdLr6hOMvvRf5boxMYdZBZFryp0RaiJYATbG3UUmw==","X-Google-Smtp-Source":"ABdhPJyQmtnAQ6D7gOhYKEy2DglQV876mDLe7T/MWdUmHylNFZGFvyYk9dENSymqElFIzZktd7D6kgBM8O0o6n7cLjU=","X-Received":"by 2002:aca:de89:: with SMTP id v131mr519114oig.55.1606767677413;\n\tMon, 30 Nov 2020 12:21:17 -0800 (PST)","MIME-Version":"1.0","References":"<20201126123203.19105-1-david.plowman@raspberrypi.com>\n\t<20201126123203.19105-3-david.plowman@raspberrypi.com>\n\t<20201126125618.GG3905@pendragon.ideasonboard.com>\n\t\n\t<20201130182540.GV4141@pendragon.ideasonboard.com>","In-Reply-To":"<20201130182540.GV4141@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Mon, 30 Nov 2020 20:21:05 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] src: ipa: raspberrypi: agc:\n\tMake AGC handle Pause/Resume for itself","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\" "}}]