[{"id":18176,"web_url":"https://patchwork.libcamera.org/comment/18176/","msgid":"<624ac637-8252-8fae-f9ae-bef46eb710b3@ideasonboard.com>","date":"2021-07-14T14:02:05","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: AGC: handle\n\tmodes with different sensitivities","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 11/06/2021 11:07, David Plowman wrote:\n> When the sensor is switched to a mode with a different sensitivity,\n> the target exposure values need to be adjusted proportionately to\n> maintain the same image brightness.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 25 +++++++++++++++++-----\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 +\n> 2 files changed, 21 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 55e80ac7..431029d0 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -163,7 +163,7 @@ Agc::Agc(Controller *controller)\n> \t: AgcAlgorithm(controller), metering_mode_(nullptr),\n> \t exposure_mode_(nullptr), constraint_mode_(nullptr),\n> \t frame_count_(0), lock_count_(0),\n> -\t last_target_exposure_(0s),\n> +\t last_target_exposure_(0s), last_sensitivity_(0.0),\n> \t ev_(1.0), flicker_period_(0s),\n> \t max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)\n> {\n> @@ -269,7 +269,7 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n> \tconstraint_mode_name_ = constraint_mode_name;\n> }\n> \n> -void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> +void Agc::SwitchMode(CameraMode const &camera_mode,\n> \t\t Metadata *metadata)\n> {\n> \thousekeepConfig();\n> @@ -293,9 +293,20 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\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> -\t\t// or a fixed exposure/gain might be set so we divide up the exposure/\n> -\t\t// gain again, but we don't change any target values.\n> +\t\t// On a mode switch, various things could happen:\n> +\t\t// - the exposure profile might change\n> +\t\t// - a fixed exposure or gain might be set\n> +\t\t// - the new mode's sensitivity might be different\n> +\t\t// We cope with the last of these by scaling the target values. After\n> +\t\t// that we just need to re-divide the exposure/gain according to the\n> +\t\t// current exposure profile, which takes care of everything else.\n> +\n> +\t\tdouble ratio = last_sensitivity_ / camera_mode.sensitivity;\n\nIs it right that we ASSERT(camera_mode.sensitivity != 0) below here,\nwhich is presumably after this statement, and therefore we've already\nbroken here?\n\nPerhaps I'm missing some calling context that isn't visible in the patch.\n\n\n\n> +\t\ttarget_.total_exposure_no_dg *= ratio;\n> +\t\ttarget_.total_exposure *= ratio;\n> +\t\tfiltered_.total_exposure_no_dg *= ratio;\n> +\t\tfiltered_.total_exposure *= ratio;\n> +\n> \t\tdivideUpExposure();\n> \t} else {\n> \t\t// We come through here on startup, when at least one of the shutter\n> @@ -309,6 +320,10 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> \t}\n> \n> \twriteAndFinish(metadata, false);\n> +\n> +\t// We must remember the sensitivity of this mode for the next SwitchMode.\n> +\tASSERT(camera_mode.sensitivity != 0.0);\n\nIs this check to ensure that a cam helper doesn't go setting it to 0.0?\n\n\n> +\tlast_sensitivity_ = camera_mode.sensitivity;\n> }\n> \n> void Agc::Prepare(Metadata *image_metadata)\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 75078948..0d08dcd7 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -126,6 +126,7 @@ private:\n> \tint lock_count_;\n> \tDeviceStatus last_device_status_;\n> \tlibcamera::utils::Duration last_target_exposure_;\n> +\tdouble last_sensitivity_; // sensitivity of the previous camera mode\n> \t// Below here the \"settings\" that applications can change.\n> \tstd::string metering_mode_name_;\n> \tstd::string exposure_mode_name_;\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 AAF6DC3225\n\tfor ;\n\tWed, 14 Jul 2021 14:02:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E34E368525;\n\tWed, 14 Jul 2021 16:02:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A23C68503\n\tfor ;\n\tWed, 14 Jul 2021 16:02:09 +0200 (CEST)","from [192.168.0.20]\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 96B5CCC;\n\tWed, 14 Jul 2021 16:02:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"h6YYlUjm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626271328;\n\tbh=FFq2cgkFZWfmEG8Qsngb+iNW7UGGI+uzECV5Qm2YIhY=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=h6YYlUjmYssPd0S6JaehFIxvSo87H/8gnM+c70M0NSl8Q6JWqCRGBGPylJA/nICaR\n\ttnn+xu0i0RXnDoSydtZS0VYAYBulMqIp6sNbBwQx+fuDIiGINA2Ac1DcdlVOt3CtRg\n\tQbJdaYniemP2cB9HLmOzbB0UZvo6msf+DFiE2orQ=","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210611100703.2285-1-david.plowman@raspberrypi.com>\n\t<20210611100703.2285-3-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<624ac637-8252-8fae-f9ae-bef46eb710b3@ideasonboard.com>","Date":"Wed, 14 Jul 2021 15:02:05 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210611100703.2285-3-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: AGC: handle\n\tmodes with different sensitivities","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18214,"web_url":"https://patchwork.libcamera.org/comment/18214/","msgid":"","date":"2021-07-18T11:20:38","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: AGC: handle\n\tmodes with different sensitivities","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran\n\nThanks for the comments.\n\nOn Wed, 14 Jul 2021 at 15:02, Kieran Bingham\n wrote:\n>\n> Hi David,\n>\n> On 11/06/2021 11:07, David Plowman wrote:\n> > When the sensor is switched to a mode with a different sensitivity,\n> > the target exposure values need to be adjusted proportionately to\n> > maintain the same image brightness.\n> >\n> > Signed-off-by: David Plowman \n> > Reviewed-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 25 +++++++++++++++++-----\n> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 +\n> > 2 files changed, 21 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 55e80ac7..431029d0 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -163,7 +163,7 @@ Agc::Agc(Controller *controller)\n> > : AgcAlgorithm(controller), metering_mode_(nullptr),\n> > exposure_mode_(nullptr), constraint_mode_(nullptr),\n> > frame_count_(0), lock_count_(0),\n> > - last_target_exposure_(0s),\n> > + last_target_exposure_(0s), last_sensitivity_(0.0),\n> > ev_(1.0), flicker_period_(0s),\n> > max_shutter_(0s), fixed_shutter_(0s), fixed_analogue_gain_(0.0)\n> > {\n> > @@ -269,7 +269,7 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n> > constraint_mode_name_ = constraint_mode_name;\n> > }\n> >\n> > -void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> > +void Agc::SwitchMode(CameraMode const &camera_mode,\n> > Metadata *metadata)\n> > {\n> > housekeepConfig();\n> > @@ -293,9 +293,20 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\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> > - // or a fixed exposure/gain might be set so we divide up the exposure/\n> > - // gain again, but we don't change any target values.\n> > + // On a mode switch, various things could happen:\n> > + // - the exposure profile might change\n> > + // - a fixed exposure or gain might be set\n> > + // - the new mode's sensitivity might be different\n> > + // We cope with the last of these by scaling the target values. After\n> > + // that we just need to re-divide the exposure/gain according to the\n> > + // current exposure profile, which takes care of everything else.\n> > +\n> > + double ratio = last_sensitivity_ / camera_mode.sensitivity;\n>\n> Is it right that we ASSERT(camera_mode.sensitivity != 0) below here,\n> which is presumably after this statement, and therefore we've already\n> broken here?\n>\n> Perhaps I'm missing some calling context that isn't visible in the patch.\n\nNo, I think that's a fair cop actually. Probably I decided to be\nsuper-cautious and added the ASSERT later without double-checking.\nSigh. Never mind, let me submit a version 4 (or whatever) and then\nmaybe we'll be good to go. But anyway, thanks for spotting that.\n\nDavid\n\n>\n>\n>\n> > + target_.total_exposure_no_dg *= ratio;\n> > + target_.total_exposure *= ratio;\n> > + filtered_.total_exposure_no_dg *= ratio;\n> > + filtered_.total_exposure *= ratio;\n> > +\n> > divideUpExposure();\n> > } else {\n> > // We come through here on startup, when at least one of the shutter\n> > @@ -309,6 +320,10 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,\n> > }\n> >\n> > writeAndFinish(metadata, false);\n> > +\n> > + // We must remember the sensitivity of this mode for the next SwitchMode.\n> > + ASSERT(camera_mode.sensitivity != 0.0);\n>\n> Is this check to ensure that a cam helper doesn't go setting it to 0.0?\n>\n>\n> > + last_sensitivity_ = camera_mode.sensitivity;\n> > }\n> >\n> > void Agc::Prepare(Metadata *image_metadata)\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > index 75078948..0d08dcd7 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > @@ -126,6 +126,7 @@ private:\n> > int lock_count_;\n> > DeviceStatus last_device_status_;\n> > libcamera::utils::Duration last_target_exposure_;\n> > + double last_sensitivity_; // sensitivity of the previous camera mode\n> > // Below here the \"settings\" that applications can change.\n> > std::string metering_mode_name_;\n> > std::string exposure_mode_name_;\n> >\n>\n> --\n> Regards\n> --\n> Kieran","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 E3415C3228\n\tfor ;\n\tSun, 18 Jul 2021 11:20:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E4886853A;\n\tSun, 18 Jul 2021 13:20:55 +0200 (CEST)","from mail-io1-xd2c.google.com (mail-io1-xd2c.google.com\n\t[IPv6:2607:f8b0:4864:20::d2c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3039D60280\n\tfor ;\n\tSun, 18 Jul 2021 13:20:53 +0200 (CEST)","by mail-io1-xd2c.google.com with SMTP id x10so16371440ion.9\n\tfor ;\n\tSun, 18 Jul 2021 04:20:53 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"F9h+FDmc\"; 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=JrNrfzxxHhknujzB+8gP13qeV535IW+EESqsZYb8ohw=;\n\tb=F9h+FDmcZXQ1eml6AkLTkK2Op2dWZAMsyy5msjoPppmt2wK5A8BRnDbLdQS0iodG3u\n\t9PsKVaUlV8ohTrARm8N29heBqnPv+0mVguz59ZjDlzjKmJ6vp2ihfTUhuA4VO3/0jPMX\n\tHioFLP7bjWioIuvA3rUMDZgO9iBNYhHEJKO40asxuawF55s0ANpl46xwRStMebWMmxRK\n\tIMCGgjaGTzMEao6nrCHKW8HRCYjzo/n6hz+tWmZAcs38A9sATIVfrUXcbwLWkkcBL1De\n\tz0HBwHBUQNpBCSfJsYeGC6kG0rFZB7uaazlCbUXtkhgC1wqS/IZ38k/KmcJnw/CbtGkn\n\ttH+w==","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=JrNrfzxxHhknujzB+8gP13qeV535IW+EESqsZYb8ohw=;\n\tb=LoyoiORVJCDD4X1k01B5aj5jsNs+Xuuz/62NVuYx6zyoOTZ1yJBPpoOl4yRzXofaI0\n\t3uCnXnd4GNvQ6+KWZzhDao8OFslr5GJPUc3dUtQX4Wt9Y2W9Pi2zKbjer/uvBBYEl47q\n\tYTJAfnMX+0JNUrEwdUwXe3KDErN73iIpQfSmRRD50J0Zc78m7qse8/CnCAeyIN0ldh5C\n\t287+EUNfC/pukT64JfV06r12hyp6JXC0KMVSBkhaRSdsExVAtTO3InneHcAaE+AmtN7a\n\t5CX2VCebJaTnr+jUpzRiETDb3kbQPY4XUIPBH/KL8Lbe9hEx1cjXt1TO13lJBrjyciQb\n\twZ7w==","X-Gm-Message-State":"AOAM533UW+fIKO1Jit4MpGa4MMvr3vAFs8agGUh6VnZRVHnDnzBL+77+\n\tYgCqP1bHUIY957L5aImH25B3iHlKzsJ7iOuD9qRxl37IHs0=","X-Google-Smtp-Source":"ABdhPJz4kV+ySHcIR+g1hjniwJQrEgFd5ucg/dI1e4PGgG8YGZRJZ7L74bOpOyas7Wg+nIwqL8QQyLD2TsRDuTo7i0g=","X-Received":"by 2002:a02:818c:: with SMTP id n12mr16776670jag.2.1626607251668;\n\tSun, 18 Jul 2021 04:20:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20210611100703.2285-1-david.plowman@raspberrypi.com>\n\t<20210611100703.2285-3-david.plowman@raspberrypi.com>\n\t<624ac637-8252-8fae-f9ae-bef46eb710b3@ideasonboard.com>","In-Reply-To":"<624ac637-8252-8fae-f9ae-bef46eb710b3@ideasonboard.com>","From":"David Plowman ","Date":"Sun, 18 Jul 2021 12:20:38 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: AGC: handle\n\tmodes with different sensitivities","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]