[{"id":27673,"web_url":"https://patchwork.libcamera.org/comment/27673/","msgid":"","date":"2023-08-21T09:28:13","subject":"Re: [libcamera-devel] [PATCH 2/3] ipa: rpi: agc: Filter exposures\n\tbefore dealing with digital gain","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nNice simplification!\n\nOn Fri, 28 Jul 2023 at 14:37, David Plowman via libcamera-devel\n wrote:\n>\n> We now time-filter the exposure before sorting out how much digital\n> gain is required. This is actually a little more natural and\n> simplifies the code. It also prepares us for some future work where\n> this arrangement will be helpful.\n>\n> Signed-off-by: David Plowman \n\nReviewed-by: Naushir Patuck \n\n> ---\n> src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++-------------------------\n> src/ipa/rpi/controller/rpi/agc.h | 2 +-\n> 2 files changed, 6 insertions(+), 26 deletions(-)\n>\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index e8526355..6087fc60 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> @@ -469,14 +469,14 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> computeGain(stats, imageMetadata, gain, targetY);\n> /* Now compute the target (final) exposure which we think we want. */\n> computeTargetExposure(gain);\n> + /* The results have to be filtered so as not to change too rapidly. */\n> + filterExposure();\n> /*\n> * Some of the exposure has to be applied as digital gain, so work out\n> * what that is. This function also tells us whether it's decided to\n> * \"desaturate\" the image more quickly.\n> */\n> bool desaturate = applyDigitalGain(gain, targetY);\n> - /* The results have to be filtered so as not to change too rapidly. */\n> - filterExposure(desaturate);\n> /*\n> * The last thing is to divide up the exposure value into a shutter time\n> * and analogue gain, according to the current exposure mode.\n> @@ -757,12 +757,12 @@ bool Agc::applyDigitalGain(double gain, double targetY)\n> if (desaturate)\n> dg /= config_.fastReduceThreshold;\n> LOG(RPiAgc, Debug) << \"Digital gain \" << dg << \" desaturate? \" << desaturate;\n> - target_.totalExposureNoDG = target_.totalExposure / dg;\n> - LOG(RPiAgc, Debug) << \"Target totalExposureNoDG \" << target_.totalExposureNoDG;\n> + filtered_.totalExposureNoDG = filtered_.totalExposure / dg;\n> + LOG(RPiAgc, Debug) << \"Target totalExposureNoDG \" << filtered_.totalExposureNoDG;\n> return desaturate;\n> }\n>\n> -void Agc::filterExposure(bool desaturate)\n> +void Agc::filterExposure()\n> {\n> double speed = config_.speed;\n> /*\n> @@ -774,7 +774,6 @@ void Agc::filterExposure(bool desaturate)\n> speed = 1.0;\n> if (!filtered_.totalExposure) {\n> filtered_.totalExposure = target_.totalExposure;\n> - filtered_.totalExposureNoDG = target_.totalExposureNoDG;\n> } else {\n> /*\n> * If close to the result go faster, to save making so many\n> @@ -785,26 +784,7 @@ void Agc::filterExposure(bool desaturate)\n> speed = sqrt(speed);\n> filtered_.totalExposure = speed * target_.totalExposure +\n> filtered_.totalExposure * (1.0 - speed);\n> - /*\n> - * When desaturing, take a big jump down in totalExposureNoDG,\n> - * which we'll hide with digital gain.\n> - */\n> - if (desaturate)\n> - filtered_.totalExposureNoDG =\n> - target_.totalExposureNoDG;\n> - else\n> - filtered_.totalExposureNoDG =\n> - speed * target_.totalExposureNoDG +\n> - filtered_.totalExposureNoDG * (1.0 - speed);\n> }\n> - /*\n> - * We can't let the totalExposureNoDG exposure deviate too far below the\n> - * total exposure, as there might not be enough digital gain available\n> - * in the ISP to hide it (which will cause nasty oscillation).\n> - */\n> - if (filtered_.totalExposureNoDG <\n> - filtered_.totalExposure * config_.fastReduceThreshold)\n> - filtered_.totalExposureNoDG = filtered_.totalExposure * config_.fastReduceThreshold;\n> LOG(RPiAgc, Debug) << \"After filtering, totalExposure \" << filtered_.totalExposure\n> << \" no dg \" << filtered_.totalExposureNoDG;\n> }\n> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> index 939f9729..b7122de3 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -93,8 +93,8 @@ private:\n> void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> double &gain, double &targetY);\n> void computeTargetExposure(double gain);\n> + void filterExposure();\n> bool applyDigitalGain(double gain, double targetY);\n> - void filterExposure(bool desaturate);\n> void divideUpExposure();\n> void writeAndFinish(Metadata *imageMetadata, bool desaturate);\n> libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);\n> --\n> 2.30.2\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 D2647BDE17\n\tfor ;\n\tMon, 21 Aug 2023 09:28:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED22F627E0;\n\tMon, 21 Aug 2023 11:28:04 +0200 (CEST)","from mail-yw1-x1132.google.com (mail-yw1-x1132.google.com\n\t[IPv6:2607:f8b0:4864:20::1132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E832960379\n\tfor ;\n\tMon, 21 Aug 2023 11:28:02 +0200 (CEST)","by mail-yw1-x1132.google.com with SMTP id\n\t00721157ae682-5921a962adfso9066357b3.1\n\tfor ;\n\tMon, 21 Aug 2023 02:28:02 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692610085;\n\tbh=YFbx+WWyYzuEzfJXXjAsJSJbE+9Cw5/Ebp4Fx60ITXc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=R/GxGbYyHEW4/mjqH8xfermLiVjSrQTGXMPIFPrS2yBSyplPsSvwvQD0GZFN99Frb\n\t9O9ele6FuWpq6SNq2SbbEhc8OVyNMwc9dG2EVPQkcY9XRCc84wY98PDwHR6cQTZB8N\n\t4+dTquSAYHsJA9Z4kmdxH/8XVrYSB/Mvw5gJsziTN73lisjkSWuOD+EH2YgoHrnml+\n\t6eygeQDt6X/tVq09AVkUiTuNk71NLi7LcCTaZwRK9odpVPflpRfRlngo+AdEGDL5d/\n\tA0lXYIvr8Hde0IVgLp2Gwvp2RLD2+N9N0b+0biiokEzNKVVYjIvn4n2XBg0B6m8cvl\n\t4YtzJCNtAQCjA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1692610081; x=1693214881;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=jh0BN88G4VVhmatSNOypOEw6gutgCrNhBXF+S8sYwHg=;\n\tb=YGKA5y0C1UqBAzYHQu5UBh2AWUUGYp+hRrgL3wxGG1haDZEDcjePPNaTY/svmFBNMq\n\tYFc+G+9BmjDiVAhaQwsUrVyNMBEf5i3gZpcccYH5sKBlxykcKE5V14CfSqYez9/X/YLn\n\tsGBYdQQak9GqNZ88TygHrnwRao7+boI5FzCPcD0/L4rJ6+R9uTXJRt1oSckPnbUO9UAK\n\tr8/6ThuCxs6iOu/SnmagHx7iypOIq7CqLNxXDz3fZ6YeZCXTq9iFQpGi6M6YsHEgTTEl\n\tGZm1ilYSVs0s/isCNVDXi+bEPqCneYwd6rGcXMj7HkyZpXIlfBuI+XxuNCkh84k38DgW\n\tV5qg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"YGKA5y0C\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1692610081; x=1693214881;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=jh0BN88G4VVhmatSNOypOEw6gutgCrNhBXF+S8sYwHg=;\n\tb=JeHlH6mEMxGd5U4V7U/sMX9thKgj0C7JXp3R364ACOeU7osi57ckZMqAJ8Uz7IzIA2\n\tqpI1KywvvygszvZs9jg5ybnJObXI/ryBFNUnmcfgJRbUeTL9nqj+3TTXYeeJvwcs1hWN\n\tEg+l+u4Ex1AlqMdATULzGDKfDXmjm6mCLFsSBjQyCYYUPi/uRX7kfWuPklKgRqzGM4jK\n\tbCPWqwhCfxnLtrzLRM9m/+3s6T+bnREbOlSK4E882zX0WRKWzVTnBYh/Acscd0UN1opx\n\tfV3DDT31xkR71+6gi/wXbQ45ik5RVqj5dtAycGw15FnW6Fc8p6ikgjY0xxmRmRZKTV8p\n\t1xsg==","X-Gm-Message-State":"AOJu0YzKC/NdPtOOFk3XD7yqAljv8CzbHtBu26MfVipzxDWQBLj/nbTl\n\t6RbGR2sxzBMpTczrB+T8L+ZnQsR8BVGruStDXMOR8hP3bqeL7mFcfvE=","X-Google-Smtp-Source":"AGHT+IF5a1NB1+F0L4r/trPXl412QkNSphBC37bdUUKZsUtMr6Bi4uHcdXIzyn7le9lIKl95pN7LVr5zmudmVdkjW9w=","X-Received":"by 2002:a81:528d:0:b0:56d:5060:f91a with SMTP id\n\tg135-20020a81528d000000b0056d5060f91amr7238949ywb.11.1692610081722;\n\tMon, 21 Aug 2023 02:28:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20230728133700.3713-1-david.plowman@raspberrypi.com>\n\t<20230728133700.3713-3-david.plowman@raspberrypi.com>","In-Reply-To":"<20230728133700.3713-3-david.plowman@raspberrypi.com>","Date":"Mon, 21 Aug 2023 10:28:13 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] ipa: rpi: agc: Filter exposures\n\tbefore dealing with digital gain","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27706,"web_url":"https://patchwork.libcamera.org/comment/27706/","msgid":"<5rgjbsgapfs7jd2pt337qlu6fbs5fgkxj2yqibtkwf4dpf7f6f@7bsigvbidbn2>","date":"2023-08-29T13:57:02","subject":"Re: [libcamera-devel] [PATCH 2/3] ipa: rpi: agc: Filter exposures\n\tbefore dealing with digital gain","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Fri, Jul 28, 2023 at 02:36:59PM +0100, David Plowman via libcamera-devel wrote:\n> We now time-filter the exposure before sorting out how much digital\n> gain is required. This is actually a little more natural and\n> simplifies the code. It also prepares us for some future work where\n> this arrangement will be helpful.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++-------------------------\n> src/ipa/rpi/controller/rpi/agc.h | 2 +-\n> 2 files changed, 6 insertions(+), 26 deletions(-)\n>\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index e8526355..6087fc60 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> @@ -469,14 +469,14 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> \tcomputeGain(stats, imageMetadata, gain, targetY);\n> \t/* Now compute the target (final) exposure which we think we want. */\n> \tcomputeTargetExposure(gain);\n> +\t/* The results have to be filtered so as not to change too rapidly. */\n> +\tfilterExposure();\n> \t/*\n> \t * Some of the exposure has to be applied as digital gain, so work out\n> \t * what that is. This function also tells us whether it's decided to\n> \t * \"desaturate\" the image more quickly.\n> \t */\n> \tbool desaturate = applyDigitalGain(gain, targetY);\n> -\t/* The results have to be filtered so as not to change too rapidly. */\n> -\tfilterExposure(desaturate);\n> \t/*\n> \t * The last thing is to divide up the exposure value into a shutter time\n> \t * and analogue gain, according to the current exposure mode.\n> @@ -757,12 +757,12 @@ bool Agc::applyDigitalGain(double gain, double targetY)\n> \tif (desaturate)\n> \t\tdg /= config_.fastReduceThreshold;\n> \tLOG(RPiAgc, Debug) << \"Digital gain \" << dg << \" desaturate? \" << desaturate;\n> -\ttarget_.totalExposureNoDG = target_.totalExposure / dg;\n> -\tLOG(RPiAgc, Debug) << \"Target totalExposureNoDG \" << target_.totalExposureNoDG;\n> +\tfiltered_.totalExposureNoDG = filtered_.totalExposure / dg;\n> +\tLOG(RPiAgc, Debug) << \"Target totalExposureNoDG \" << filtered_.totalExposureNoDG;\n> \treturn desaturate;\n> }\n>\n> -void Agc::filterExposure(bool desaturate)\n> +void Agc::filterExposure()\n> {\n> \tdouble speed = config_.speed;\n> \t/*\n> @@ -774,7 +774,6 @@ void Agc::filterExposure(bool desaturate)\n> \t\tspeed = 1.0;\n> \tif (!filtered_.totalExposure) {\n> \t\tfiltered_.totalExposure = target_.totalExposure;\n> -\t\tfiltered_.totalExposureNoDG = target_.totalExposureNoDG;\n> \t} else {\n> \t\t/*\n> \t\t * If close to the result go faster, to save making so many\n> @@ -785,26 +784,7 @@ void Agc::filterExposure(bool desaturate)\n> \t\t\tspeed = sqrt(speed);\n> \t\tfiltered_.totalExposure = speed * target_.totalExposure +\n> \t\t\t\t\t filtered_.totalExposure * (1.0 - speed);\n> -\t\t/*\n> -\t\t * When desaturing, take a big jump down in totalExposureNoDG,\n> -\t\t * which we'll hide with digital gain.\n> -\t\t */\n> -\t\tif (desaturate)\n> -\t\t\tfiltered_.totalExposureNoDG =\n> -\t\t\t\ttarget_.totalExposureNoDG;\n> -\t\telse\n> -\t\t\tfiltered_.totalExposureNoDG =\n> -\t\t\t\tspeed * target_.totalExposureNoDG +\n> -\t\t\t\tfiltered_.totalExposureNoDG * (1.0 - speed);\n> \t}\n> -\t/*\n> -\t * We can't let the totalExposureNoDG exposure deviate too far below the\n> -\t * total exposure, as there might not be enough digital gain available\n> -\t * in the ISP to hide it (which will cause nasty oscillation).\n> -\t */\n> -\tif (filtered_.totalExposureNoDG <\n> -\t filtered_.totalExposure * config_.fastReduceThreshold)\n> -\t\tfiltered_.totalExposureNoDG = filtered_.totalExposure * config_.fastReduceThreshold;\n\nI won't pretend I've fully understood why this check can now be\ndropped, but I the flow seems indeed more natural\n\nReviewed-by: Jacopo Mondi \n\n> \tLOG(RPiAgc, Debug) << \"After filtering, totalExposure \" << filtered_.totalExposure\n> \t\t\t << \" no dg \" << filtered_.totalExposureNoDG;\n> }\n> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> index 939f9729..b7122de3 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -93,8 +93,8 @@ private:\n> \tvoid computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> \t\t\t double &gain, double &targetY);\n> \tvoid computeTargetExposure(double gain);\n> +\tvoid filterExposure();\n> \tbool applyDigitalGain(double gain, double targetY);\n> -\tvoid filterExposure(bool desaturate);\n> \tvoid divideUpExposure();\n> \tvoid writeAndFinish(Metadata *imageMetadata, bool desaturate);\n> \tlibcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);\n> --\n> 2.30.2\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 37D11BE080\n\tfor ;\n\tTue, 29 Aug 2023 13:57:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F15B627E0;\n\tTue, 29 Aug 2023 15:57:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 43CB761E02\n\tfor ;\n\tTue, 29 Aug 2023 15:57:07 +0200 (CEST)","from ideasonboard.com (mob-5-90-48-16.net.vodafone.it [5.90.48.16])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40B8F8CC;\n\tTue, 29 Aug 2023 15:55:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693317428;\n\tbh=kmR3SAKM+AOX9bBV2HW9yHj7JKXtW19heWdae98+ONI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=xns9D8L6CVCR3VVdTYfBNrnclffG3+QHkBUbh+oFIrnSs54SoYQnsMIFSHA7vGU4v\n\tfvisxdpj818lLgLSVlpJCyUgXmCzw1uMY28/qOC4pmCtfmbDDfcNsaJ2bflH5dUaGu\n\tD8Lp5CMlUURoQ6jhTVcmUj+t+nA4r14JeNmvsTV8wF4kyYW/u+z9VWYzrt/LHRFVo4\n\tIcHxtY7d40OTxypz1oCOkYRf4j3t3XI1N5ziRWARYkCad0VHH2Hius6/ETvJxV/qCi\n\tSuPfE1sb7mjIxW3AFPUsxFxylLtn1r3SKsq7wNlgpHzf8MFHS9JLxMz6FiwymhmbXw\n\tGvJdS77TSwVHw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693317345;\n\tbh=kmR3SAKM+AOX9bBV2HW9yHj7JKXtW19heWdae98+ONI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aq4QbB37hswP983lSNPx9qtNN77DbsEH/11SNLwTjqZnJ/PqHIc85h/mxnijLnlzy\n\tIU99DIDVa7xPDGgeMgb66mufiap4/hqb8ZPXbeHtY7CCMZdNQfWD6oVCjsqzfPP25f\n\tFCctIGa0dSpI+StAOkilKdPq0RVEF3MuY9wTkHBI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"aq4QbB37\"; dkim-atps=neutral","Date":"Tue, 29 Aug 2023 15:57:02 +0200","To":"David Plowman ","Message-ID":"<5rgjbsgapfs7jd2pt337qlu6fbs5fgkxj2yqibtkwf4dpf7f6f@7bsigvbidbn2>","References":"<20230728133700.3713-1-david.plowman@raspberrypi.com>\n\t<20230728133700.3713-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230728133700.3713-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] ipa: rpi: agc: Filter exposures\n\tbefore dealing with digital gain","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]