[libcamera-devel,1/3] ipa: rpi: agc: Fetch AWB status in process method, not prepare
diff mbox series

Message ID 20230728133700.3713-2-david.plowman@raspberrypi.com
State Accepted
Commit 2ea57d0b7702efb6e290f7ec2ef35b5646f91a6c
Headers show
Series
  • Raspberry Pi AGC tidying
Related show

Commit Message

David Plowman July 28, 2023, 1:36 p.m. UTC
prepare() doesn't use the AWB status, so fetching it in process() is
probably better. This change is preparatory to other changes, where we
may find ourselves calling process() without having called prepare()
previously.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/rpi/controller/rpi/agc.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Naushir Patuck Aug. 21, 2023, 9:11 a.m. UTC | #1
Hi David,

Thank you for your patch.

On Fri, 28 Jul 2023 at 14:37, David Plowman via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> prepare() doesn't use the AWB status, so fetching it in process() is
> probably better. This change is preparatory to other changes, where we
> may find ourselves calling process() without having called prepare()
> previously.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  src/ipa/rpi/controller/rpi/agc.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index ae9ff219..e8526355 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -424,7 +424,6 @@ void Agc::prepare(Metadata *imageMetadata)
>                 totalExposureValue = delayedStatus.totalExposureValue;
>
>         status_.digitalGain = 1.0;
> -       fetchAwbStatus(imageMetadata); /* always fetch it so that Process knows it's been done */
>
>         if (status_.totalExposureValue) {
>                 /* Process has run, so we have meaningful values. */
> @@ -461,6 +460,8 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)
>          * configuration, that kind of thing.
>          */
>         housekeepConfig();
> +       /* Fetch the AWB status immediately, so that we can assume it's there. */
> +       fetchAwbStatus(imageMetadata);
>         /* Get the current exposure values for the frame that's just arrived. */
>         fetchCurrentExposure(imageMetadata);
>         /* Compute the total gain we require relative to the current exposure. */
> --
> 2.30.2
>
Jacopo Mondi Aug. 29, 2023, 1:30 p.m. UTC | #2
Hi David

On Fri, Jul 28, 2023 at 02:36:58PM +0100, David Plowman via libcamera-devel wrote:
> prepare() doesn't use the AWB status, so fetching it in process() is
> probably better. This change is preparatory to other changes, where we
> may find ourselves calling process() without having called prepare()
> previously.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Seems to make sense to me
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  src/ipa/rpi/controller/rpi/agc.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index ae9ff219..e8526355 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -424,7 +424,6 @@ void Agc::prepare(Metadata *imageMetadata)
>  		totalExposureValue = delayedStatus.totalExposureValue;
>
>  	status_.digitalGain = 1.0;
> -	fetchAwbStatus(imageMetadata); /* always fetch it so that Process knows it's been done */
>
>  	if (status_.totalExposureValue) {
>  		/* Process has run, so we have meaningful values. */
> @@ -461,6 +460,8 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)
>  	 * configuration, that kind of thing.
>  	 */
>  	housekeepConfig();
> +	/* Fetch the AWB status immediately, so that we can assume it's there. */
> +	fetchAwbStatus(imageMetadata);
>  	/* Get the current exposure values for the frame that's just arrived. */
>  	fetchCurrentExposure(imageMetadata);
>  	/* Compute the total gain we require relative to the current exposure. */
> --
> 2.30.2
>

Patch
diff mbox series

diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
index ae9ff219..e8526355 100644
--- a/src/ipa/rpi/controller/rpi/agc.cpp
+++ b/src/ipa/rpi/controller/rpi/agc.cpp
@@ -424,7 +424,6 @@  void Agc::prepare(Metadata *imageMetadata)
 		totalExposureValue = delayedStatus.totalExposureValue;
 
 	status_.digitalGain = 1.0;
-	fetchAwbStatus(imageMetadata); /* always fetch it so that Process knows it's been done */
 
 	if (status_.totalExposureValue) {
 		/* Process has run, so we have meaningful values. */
@@ -461,6 +460,8 @@  void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)
 	 * configuration, that kind of thing.
 	 */
 	housekeepConfig();
+	/* Fetch the AWB status immediately, so that we can assume it's there. */
+	fetchAwbStatus(imageMetadata);
 	/* Get the current exposure values for the frame that's just arrived. */
 	fetchCurrentExposure(imageMetadata);
 	/* Compute the total gain we require relative to the current exposure. */