[07/12] ipa: rkisp1: agc: Don't update histogram parameters unnecessarily
diff mbox series

Message ID 20240616163910.5506-8-laurent.pinchart@ideasonboard.com
State Accepted
Commit 05d0f952a36c40392a9c8ccf86d3567a64ecba32
Headers show
Series
  • ipa: rkisp1: Miscellaneous AGC fixes
Related show

Commit Message

Laurent Pinchart June 16, 2024, 4:39 p.m. UTC 
The ISP histogram parameters depends on the AE metering mode, but not on
the other AE algorithm controls. The exposure mode, constraints mode and
frame duration limits influence the behaviour of the algorithm, but not
the histogram computation parameters. Update the histogram parameters
only when AE metering mode changes.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Kieran Bingham June 17, 2024, 9:20 a.m. UTC | #1 
Quoting Laurent Pinchart (2024-06-16 17:39:05)
> The ISP histogram parameters depends on the AE metering mode, but not on
> the other AE algorithm controls. The exposure mode, constraints mode and
> frame duration limits influence the behaviour of the algorithm, but not
> the histogram computation parameters. Update the histogram parameters
> only when AE metering mode changes.
> 

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 9dac60bdb24e..9f3b59b45f95 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -261,26 +261,21 @@ void Agc::queueRequest(IPAContext &context,
>         frameContext.agc.meteringMode = agc.meteringMode;
>  
>         const auto &exposureMode = controls.get(controls::AeExposureMode);
> -       if (exposureMode) {
> -               frameContext.agc.update = agc.exposureMode != *exposureMode;
> +       if (exposureMode)
>                 agc.exposureMode =
>                         static_cast<controls::AeExposureModeEnum>(*exposureMode);
> -       }
>         frameContext.agc.exposureMode = agc.exposureMode;
>  
>         const auto &constraintMode = controls.get(controls::AeConstraintMode);
> -       if (constraintMode) {
> -               frameContext.agc.update = agc.constraintMode != *constraintMode;
> +       if (constraintMode)
>                 agc.constraintMode =
>                         static_cast<controls::AeConstraintModeEnum>(*constraintMode);
> -       }
>         frameContext.agc.constraintMode = agc.constraintMode;
>  
>         const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
>         if (frameDurationLimits) {
>                 utils::Duration maxShutterSpeed =
>                         std::chrono::milliseconds((*frameDurationLimits).back());
> -               frameContext.agc.update = agc.maxShutterSpeed != maxShutterSpeed;
>                 agc.maxShutterSpeed = maxShutterSpeed;
>         }
>         frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;
> -- 
> Regards,
> 
> Laurent Pinchart
>
Paul Elder June 17, 2024, 11:47 a.m. UTC | #2 
On Sun, Jun 16, 2024 at 07:39:05PM +0300, Laurent Pinchart wrote:
> The ISP histogram parameters depends on the AE metering mode, but not on
> the other AE algorithm controls. The exposure mode, constraints mode and
> frame duration limits influence the behaviour of the algorithm, but not
> the histogram computation parameters. Update the histogram parameters
> only when AE metering mode changes.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 9dac60bdb24e..9f3b59b45f95 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -261,26 +261,21 @@ void Agc::queueRequest(IPAContext &context,
>  	frameContext.agc.meteringMode = agc.meteringMode;
>  
>  	const auto &exposureMode = controls.get(controls::AeExposureMode);
> -	if (exposureMode) {
> -		frameContext.agc.update = agc.exposureMode != *exposureMode;

I wonder if this should be renamed to updateMetering or something like
that? It's not a big deal at this point though I suppose.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> +	if (exposureMode)
>  		agc.exposureMode =
>  			static_cast<controls::AeExposureModeEnum>(*exposureMode);
> -	}
>  	frameContext.agc.exposureMode = agc.exposureMode;
>  
>  	const auto &constraintMode = controls.get(controls::AeConstraintMode);
> -	if (constraintMode) {
> -		frameContext.agc.update = agc.constraintMode != *constraintMode;
> +	if (constraintMode)
>  		agc.constraintMode =
>  			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
> -	}
>  	frameContext.agc.constraintMode = agc.constraintMode;
>  
>  	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
>  	if (frameDurationLimits) {
>  		utils::Duration maxShutterSpeed =
>  			std::chrono::milliseconds((*frameDurationLimits).back());
> -		frameContext.agc.update = agc.maxShutterSpeed != maxShutterSpeed;
>  		agc.maxShutterSpeed = maxShutterSpeed;
>  	}
>  	frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;
Laurent Pinchart June 17, 2024, 12:05 p.m. UTC | #3 
On Mon, Jun 17, 2024 at 08:47:44PM +0900, Paul Elder wrote:
> On Sun, Jun 16, 2024 at 07:39:05PM +0300, Laurent Pinchart wrote:
> > The ISP histogram parameters depends on the AE metering mode, but not on
> > the other AE algorithm controls. The exposure mode, constraints mode and
> > frame duration limits influence the behaviour of the algorithm, but not
> > the histogram computation parameters. Update the histogram parameters
> > only when AE metering mode changes.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 9dac60bdb24e..9f3b59b45f95 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -261,26 +261,21 @@ void Agc::queueRequest(IPAContext &context,
> >  	frameContext.agc.meteringMode = agc.meteringMode;
> >  
> >  	const auto &exposureMode = controls.get(controls::AeExposureMode);
> > -	if (exposureMode) {
> > -		frameContext.agc.update = agc.exposureMode != *exposureMode;
> 
> I wonder if this should be renamed to updateMetering or something like
> that? It's not a big deal at this point though I suppose.

Good idea. I'll send a patch on top.

> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> > +	if (exposureMode)
> >  		agc.exposureMode =
> >  			static_cast<controls::AeExposureModeEnum>(*exposureMode);
> > -	}
> >  	frameContext.agc.exposureMode = agc.exposureMode;
> >  
> >  	const auto &constraintMode = controls.get(controls::AeConstraintMode);
> > -	if (constraintMode) {
> > -		frameContext.agc.update = agc.constraintMode != *constraintMode;
> > +	if (constraintMode)
> >  		agc.constraintMode =
> >  			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
> > -	}
> >  	frameContext.agc.constraintMode = agc.constraintMode;
> >  
> >  	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> >  	if (frameDurationLimits) {
> >  		utils::Duration maxShutterSpeed =
> >  			std::chrono::milliseconds((*frameDurationLimits).back());
> > -		frameContext.agc.update = agc.maxShutterSpeed != maxShutterSpeed;
> >  		agc.maxShutterSpeed = maxShutterSpeed;
> >  	}
> >  	frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 9dac60bdb24e..9f3b59b45f95 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -261,26 +261,21 @@  void Agc::queueRequest(IPAContext &context,
 	frameContext.agc.meteringMode = agc.meteringMode;
 
 	const auto &exposureMode = controls.get(controls::AeExposureMode);
-	if (exposureMode) {
-		frameContext.agc.update = agc.exposureMode != *exposureMode;
+	if (exposureMode)
 		agc.exposureMode =
 			static_cast<controls::AeExposureModeEnum>(*exposureMode);
-	}
 	frameContext.agc.exposureMode = agc.exposureMode;
 
 	const auto &constraintMode = controls.get(controls::AeConstraintMode);
-	if (constraintMode) {
-		frameContext.agc.update = agc.constraintMode != *constraintMode;
+	if (constraintMode)
 		agc.constraintMode =
 			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
-	}
 	frameContext.agc.constraintMode = agc.constraintMode;
 
 	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
 	if (frameDurationLimits) {
 		utils::Duration maxShutterSpeed =
 			std::chrono::milliseconds((*frameDurationLimits).back());
-		frameContext.agc.update = agc.maxShutterSpeed != maxShutterSpeed;
 		agc.maxShutterSpeed = maxShutterSpeed;
 	}
 	frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;