[libcamera-devel,v2,6/6] src: ipa: raspberrypi: Move initial frame drop decision to AGC/AWB
diff mbox series

Message ID 20201207180121.6374-7-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi AGC: initial frame drop count
Related show

Commit Message

David Plowman Dec. 7, 2020, 6:01 p.m. UTC 
Previously the CamHelper was returning the number of frames to drop
(on account of AGC/AWB converging). This wasn't really appropriate,
it's better for the algorithms to do it as they know how many frames
they might need.

The CamHelper::HideFramesStartup method should now just be returning
the number of frames to hide because they're bad/invalid in some way,
not worrying about the AGC/AWB. For many sensors, the correct value
for this is zero. But the ov5647 needs updating as it must return 2.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.cpp        |  6 +++---
 src/ipa/raspberrypi/cam_helper_ov5647.cpp | 10 ++++++++++
 src/ipa/raspberrypi/raspberrypi.cpp       | 16 ++++++++++++++++
 3 files changed, 29 insertions(+), 3 deletions(-)

Comments

Naushir Patuck Dec. 8, 2020, 10:16 a.m. UTC | #1 
Hi David,

Thank you for your patch.

On Mon, 7 Dec 2020 at 18:02, David Plowman <david.plowman@raspberrypi.com>
wrote:

> Previously the CamHelper was returning the number of frames to drop
> (on account of AGC/AWB converging). This wasn't really appropriate,
> it's better for the algorithms to do it as they know how many frames
> they might need.
>
> The CamHelper::HideFramesStartup method should now just be returning
> the number of frames to hide because they're bad/invalid in some way,
> not worrying about the AGC/AWB. For many sensors, the correct value
> for this is zero. But the ov5647 needs updating as it must return 2.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp        |  6 +++---
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp | 10 ++++++++++
>  src/ipa/raspberrypi/raspberrypi.cpp       | 16 ++++++++++++++++
>  3 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> index c8ac3232..6efa0d7f 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const
>  unsigned int CamHelper::HideFramesStartup() const
>  {
>         /*
> -        * By default, hide 6 frames completely at start-up while AGC etc.
> sort
> -        * themselves out (converge).
> +        * The number of frames when a camera first starts that shouldn't
> be
> +        * displayed as they are invalid in some way.
>          */
> -       return 6;
> +       return 0;
>  }
>
>  unsigned int CamHelper::HideFramesModeSwitch() const
> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> index dc5d8275..0b841cd1 100644
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> @@ -19,6 +19,7 @@ public:
>         uint32_t GainCode(double gain) const override;
>         double Gain(uint32_t gain_code) const override;
>         void GetDelays(int &exposure_delay, int &gain_delay) const
> override;
> +       unsigned int HideFramesStartup() const override;
>         unsigned int HideFramesModeSwitch() const override;
>         unsigned int MistrustFramesStartup() const override;
>         unsigned int MistrustFramesModeSwitch() const override;
> @@ -54,6 +55,15 @@ void CamHelperOv5647::GetDelays(int &exposure_delay,
> int &gain_delay) const
>         gain_delay = 2;
>  }
>
> +unsigned int CamHelperOv5647::HideFramesStartup() const
> +{
> +       /*
> +        * On startup, we get a couple of under-exposed frames which
> +        * we don't want shown.
> +        */
> +       return 2;
> +}
> +
>  unsigned int CamHelperOv5647::HideFramesModeSwitch() const
>  {
>         /*
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0e89af00..da92e492 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -193,6 +193,22 @@ int IPARPi::start(const IPAOperationData &ipaConfig,
> IPAOperationData *result)
>         if (firstStart_) {
>                 dropFrame = helper_->HideFramesStartup();
>                 mistrustCount_ = helper_->MistrustFramesStartup();
> +
> +               /* The AGC and/or AWB algorithms may want us to drop more
> frames. */
> +               unsigned int convergence_frames = 0;
>

Minor nit, but could you use camel case for convergence_frames to match the
rest of the variables?


> +               RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> +                       controller_.GetAlgorithm("agc"));
> +               if (agc)
> +                       convergence_frames =
> agc->GetConvergenceFrames(mistrustCount_);
> +
> +               RPiController::AwbAlgorithm *awb =
> dynamic_cast<RPiController::AwbAlgorithm *>(
> +                       controller_.GetAlgorithm("awb"));
> +               if (awb)
> +                       convergence_frames = std::max(convergence_frames,
> +
>  awb->GetConvergenceFrames(mistrustCount_));
> +
> +               dropFrame = std::max(dropFrame, convergence_frames);
> +               LOG(IPARPI, Debug) << "Drop " << dropFrame << " frames on
> startup";
>

Apart from the above nit, and the earlier question of passing
convergence_frames into GetConvergenceFrames(),

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


>         } else {
>                 dropFrame = helper_->HideFramesModeSwitch();
>                 mistrustCount_ = helper_->MistrustFramesModeSwitch();
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart Dec. 8, 2020, 12:15 p.m. UTC | #2 
Hi David,

Thank you for the patch.

On Mon, Dec 07, 2020 at 06:01:21PM +0000, David Plowman wrote:
> Previously the CamHelper was returning the number of frames to drop
> (on account of AGC/AWB converging). This wasn't really appropriate,
> it's better for the algorithms to do it as they know how many frames
> they might need.
> 
> The CamHelper::HideFramesStartup method should now just be returning
> the number of frames to hide because they're bad/invalid in some way,
> not worrying about the AGC/AWB. For many sensors, the correct value
> for this is zero. But the ov5647 needs updating as it must return 2.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp        |  6 +++---
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp | 10 ++++++++++
>  src/ipa/raspberrypi/raspberrypi.cpp       | 16 ++++++++++++++++
>  3 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index c8ac3232..6efa0d7f 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const
>  unsigned int CamHelper::HideFramesStartup() const
>  {
>  	/*
> -	 * By default, hide 6 frames completely at start-up while AGC etc. sort
> -	 * themselves out (converge).
> +	 * The number of frames when a camera first starts that shouldn't be
> +	 * displayed as they are invalid in some way.
>  	 */
> -	return 6;
> +	return 0;
>  }
>  
>  unsigned int CamHelper::HideFramesModeSwitch() const
> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> index dc5d8275..0b841cd1 100644
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> @@ -19,6 +19,7 @@ public:
>  	uint32_t GainCode(double gain) const override;
>  	double Gain(uint32_t gain_code) const override;
>  	void GetDelays(int &exposure_delay, int &gain_delay) const override;
> +	unsigned int HideFramesStartup() const override;
>  	unsigned int HideFramesModeSwitch() const override;
>  	unsigned int MistrustFramesStartup() const override;
>  	unsigned int MistrustFramesModeSwitch() const override;
> @@ -54,6 +55,15 @@ void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const
>  	gain_delay = 2;
>  }
>  
> +unsigned int CamHelperOv5647::HideFramesStartup() const
> +{
> +	/*
> +	 * On startup, we get a couple of under-exposed frames which
> +	 * we don't want shown.
> +	 */
> +	return 2;
> +}
> +

Now both HideFramesStartup() and HideFramesModeSwitch() always return
the same value. Should they be merged into a single function ? This can
be done in a separate patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  unsigned int CamHelperOv5647::HideFramesModeSwitch() const
>  {
>  	/*
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0e89af00..da92e492 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -193,6 +193,22 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)
>  	if (firstStart_) {
>  		dropFrame = helper_->HideFramesStartup();
>  		mistrustCount_ = helper_->MistrustFramesStartup();
> +
> +		/* The AGC and/or AWB algorithms may want us to drop more frames. */
> +		unsigned int convergence_frames = 0;
> +		RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +			controller_.GetAlgorithm("agc"));
> +		if (agc)
> +			convergence_frames = agc->GetConvergenceFrames(mistrustCount_);
> +
> +		RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> +			controller_.GetAlgorithm("awb"));
> +		if (awb)
> +			convergence_frames = std::max(convergence_frames,
> +						      awb->GetConvergenceFrames(mistrustCount_));
> +
> +		dropFrame = std::max(dropFrame, convergence_frames);
> +		LOG(IPARPI, Debug) << "Drop " << dropFrame << " frames on startup";
>  	} else {
>  		dropFrame = helper_->HideFramesModeSwitch();
>  		mistrustCount_ = helper_->MistrustFramesModeSwitch();

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
index c8ac3232..6efa0d7f 100644
--- a/src/ipa/raspberrypi/cam_helper.cpp
+++ b/src/ipa/raspberrypi/cam_helper.cpp
@@ -82,10 +82,10 @@  bool CamHelper::SensorEmbeddedDataPresent() const
 unsigned int CamHelper::HideFramesStartup() const
 {
 	/*
-	 * By default, hide 6 frames completely at start-up while AGC etc. sort
-	 * themselves out (converge).
+	 * The number of frames when a camera first starts that shouldn't be
+	 * displayed as they are invalid in some way.
 	 */
-	return 6;
+	return 0;
 }
 
 unsigned int CamHelper::HideFramesModeSwitch() const
diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
index dc5d8275..0b841cd1 100644
--- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
+++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
@@ -19,6 +19,7 @@  public:
 	uint32_t GainCode(double gain) const override;
 	double Gain(uint32_t gain_code) const override;
 	void GetDelays(int &exposure_delay, int &gain_delay) const override;
+	unsigned int HideFramesStartup() const override;
 	unsigned int HideFramesModeSwitch() const override;
 	unsigned int MistrustFramesStartup() const override;
 	unsigned int MistrustFramesModeSwitch() const override;
@@ -54,6 +55,15 @@  void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const
 	gain_delay = 2;
 }
 
+unsigned int CamHelperOv5647::HideFramesStartup() const
+{
+	/*
+	 * On startup, we get a couple of under-exposed frames which
+	 * we don't want shown.
+	 */
+	return 2;
+}
+
 unsigned int CamHelperOv5647::HideFramesModeSwitch() const
 {
 	/*
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 0e89af00..da92e492 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -193,6 +193,22 @@  int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)
 	if (firstStart_) {
 		dropFrame = helper_->HideFramesStartup();
 		mistrustCount_ = helper_->MistrustFramesStartup();
+
+		/* The AGC and/or AWB algorithms may want us to drop more frames. */
+		unsigned int convergence_frames = 0;
+		RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
+			controller_.GetAlgorithm("agc"));
+		if (agc)
+			convergence_frames = agc->GetConvergenceFrames(mistrustCount_);
+
+		RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
+			controller_.GetAlgorithm("awb"));
+		if (awb)
+			convergence_frames = std::max(convergence_frames,
+						      awb->GetConvergenceFrames(mistrustCount_));
+
+		dropFrame = std::max(dropFrame, convergence_frames);
+		LOG(IPARPI, Debug) << "Drop " << dropFrame << " frames on startup";
 	} else {
 		dropFrame = helper_->HideFramesModeSwitch();
 		mistrustCount_ = helper_->MistrustFramesModeSwitch();