[libcamera-devel,2/3] src: ipa: raspberrypi: Improve behaviour when AE disabled
diff mbox series

Message ID 20201125113640.20246-3-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Raspberry Pi AGC improvements
Related show

Commit Message

David Plowman Nov. 25, 2020, 11:36 a.m. UTC 
AE/AGC "disabled" is now implemented by leaving it running but fixing
the exposure/gain to the last values we requested. This behaves better
when, for example, a fixed shutter or gain is then specified - the
other value remains fixed and doesn't start floating again.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp | 48 ++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 18 deletions(-)

Comments

Naushir Patuck Nov. 26, 2020, 9:45 a.m. UTC | #1 
Hi David,

Thank you for the patch.

On Wed, 25 Nov 2020 at 11:36, David Plowman <david.plowman@raspberrypi.com>
wrote:

> AE/AGC "disabled" is now implemented by leaving it running but fixing
> the exposure/gain to the last values we requested. This behaves better
> when, for example, a fixed shutter or gain is then specified - the
> other value remains fixed and doesn't start floating again.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 48 ++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343..30516665 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -67,7 +67,8 @@ public:
>         IPARPi()
>                 : lastMode_({}), controller_(), controllerInit_(false),
>                   frameCount_(0), checkCount_(0), mistrustCount_(0),
> -                 lsTable_(nullptr)
> +                 lsTable_(nullptr),
> +                 lastShutter_(0), lastGain_(0)
>         {
>         }
>
> @@ -145,6 +146,10 @@ private:
>         /* LS table allocation passed in from the pipeline handler. */
>         FileDescriptor lsTableHandle_;
>         void *lsTable_;
> +
> +       /* For enabling/disabling the AEC/AGC algorithm. */
> +       uint32_t lastShutter_;
> +       float lastGain_;
>  };
>
>  int IPARPi::init(const IPASettings &settings)
> @@ -493,12 +498,22 @@ void IPARPi::queueRequest(const ControlList
> &controls)
>
>                 switch (ctrl.first) {
>                 case controls::AE_ENABLE: {
> -                       RPiController::Algorithm *agc =
> controller_.GetAlgorithm("agc");
> +                       RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> +                               controller_.GetAlgorithm("agc"));
>                         ASSERT(agc);
> -                       if (ctrl.second.get<bool>() == false)
> -                               agc->Pause();
> -                       else
> -                               agc->Resume();
> +
> +                       /*
> +                        * We always run the AGC even if it's "disabled".
> +                        * Both exposure and gain are "fixed" to the last
> +                        * values it produced.
> +                        */
> +                       if (ctrl.second.get<bool>() == false) {
> +                               agc->SetFixedShutter(lastShutter_);
> +                               agc->SetFixedAnalogueGain(lastGain_);
> +                       } else {
> +                               agc->SetFixedShutter(0);
> +                               agc->SetFixedAnalogueGain(0);
> +                       }
>

I wonder if we could do this within the AGC controller itself?  So we keep
the existing Pause()/Resume() logic in the IPA, and have the AGC do the
above block on the appropriate API call.  This way the IPA would not need
to track the last used exposure/gain values that are already stored within
the AGC block?

The rest of the changes would still be valid with this tweak.

Regards,
Naush



>
>                         libcameraMetadata_.set(controls::AeEnable,
> ctrl.second.get<bool>());
>                         break;
> @@ -510,13 +525,10 @@ void IPARPi::queueRequest(const ControlList
> &controls)
>                         ASSERT(agc);
>
>                         /* This expects units of micro-seconds. */
> -                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> +                       int32_t shutter = ctrl.second.get<int32_t>();
> +                       agc->SetFixedShutter(shutter);
>
> -                       /* For the manual values to take effect, AGC must
> be unpaused. */
> -                       if (agc->IsPaused())
> -                               agc->Resume();
> -
> -                       libcameraMetadata_.set(controls::ExposureTime,
> ctrl.second.get<int32_t>());
> +                       libcameraMetadata_.set(controls::ExposureTime,
> shutter);
>                         break;
>                 }
>
> @@ -524,14 +536,11 @@ void IPARPi::queueRequest(const ControlList
> &controls)
>                         RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
>                                 controller_.GetAlgorithm("agc"));
>                         ASSERT(agc);
> -
>  agc->SetFixedAnalogueGain(ctrl.second.get<float>());
>
> -                       /* For the manual values to take effect, AGC must
> be unpaused. */
> -                       if (agc->IsPaused())
> -                               agc->Resume();
> +                       float gain = ctrl.second.get<float>();
> +                       agc->SetFixedAnalogueGain(gain);
>
> -                       libcameraMetadata_.set(controls::AnalogueGain,
> -                                              ctrl.second.get<float>());
> +                       libcameraMetadata_.set(controls::AnalogueGain,
> gain);
>                         break;
>                 }
>
> @@ -861,6 +870,9 @@ void IPARPi::applyAWB(const struct AwbStatus
> *awbStatus, ControlList &ctrls)
>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls)
>  {
> +       lastShutter_ = agcStatus->shutter_time;
> +       lastGain_ = agcStatus->analogue_gain;
> +
>         int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
>         int32_t exposureLines =
> helper_->ExposureLines(agcStatus->shutter_time);
>
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
David Plowman Nov. 26, 2020, 9:53 a.m. UTC | #2 
Hi Naush

Thanks for the suggestion.

On Thu, 26 Nov 2020 at 09:45, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Wed, 25 Nov 2020 at 11:36, David Plowman <david.plowman@raspberrypi.com> wrote:
>>
>> AE/AGC "disabled" is now implemented by leaving it running but fixing
>> the exposure/gain to the last values we requested. This behaves better
>> when, for example, a fixed shutter or gain is then specified - the
>> other value remains fixed and doesn't start floating again.
>>
>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>> ---
>>  src/ipa/raspberrypi/raspberrypi.cpp | 48 ++++++++++++++++++-----------
>>  1 file changed, 30 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 9853a343..30516665 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -67,7 +67,8 @@ public:
>>         IPARPi()
>>                 : lastMode_({}), controller_(), controllerInit_(false),
>>                   frameCount_(0), checkCount_(0), mistrustCount_(0),
>> -                 lsTable_(nullptr)
>> +                 lsTable_(nullptr),
>> +                 lastShutter_(0), lastGain_(0)
>>         {
>>         }
>>
>> @@ -145,6 +146,10 @@ private:
>>         /* LS table allocation passed in from the pipeline handler. */
>>         FileDescriptor lsTableHandle_;
>>         void *lsTable_;
>> +
>> +       /* For enabling/disabling the AEC/AGC algorithm. */
>> +       uint32_t lastShutter_;
>> +       float lastGain_;
>>  };
>>
>>  int IPARPi::init(const IPASettings &settings)
>> @@ -493,12 +498,22 @@ void IPARPi::queueRequest(const ControlList &controls)
>>
>>                 switch (ctrl.first) {
>>                 case controls::AE_ENABLE: {
>> -                       RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
>> +                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>> +                               controller_.GetAlgorithm("agc"));
>>                         ASSERT(agc);
>> -                       if (ctrl.second.get<bool>() == false)
>> -                               agc->Pause();
>> -                       else
>> -                               agc->Resume();
>> +
>> +                       /*
>> +                        * We always run the AGC even if it's "disabled".
>> +                        * Both exposure and gain are "fixed" to the last
>> +                        * values it produced.
>> +                        */
>> +                       if (ctrl.second.get<bool>() == false) {
>> +                               agc->SetFixedShutter(lastShutter_);
>> +                               agc->SetFixedAnalogueGain(lastGain_);
>> +                       } else {
>> +                               agc->SetFixedShutter(0);
>> +                               agc->SetFixedAnalogueGain(0);
>> +                       }
>
>
> I wonder if we could do this within the AGC controller itself?  So we keep the existing Pause()/Resume() logic in the IPA, and have the AGC do the above block on the appropriate API call.  This way the IPA would not need to track the last used exposure/gain values that are already stored within the AGC block?
>
> The rest of the changes would still be valid with this tweak.
>
> Regards,
> Naush

Yes, good idea, that might look a bit tidier. Let me do a v2 set including this.

Best regards
David

>
>
>>
>>
>>                         libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
>>                         break;
>> @@ -510,13 +525,10 @@ void IPARPi::queueRequest(const ControlList &controls)
>>                         ASSERT(agc);
>>
>>                         /* This expects units of micro-seconds. */
>> -                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
>> +                       int32_t shutter = ctrl.second.get<int32_t>();
>> +                       agc->SetFixedShutter(shutter);
>>
>> -                       /* For the manual values to take effect, AGC must be unpaused. */
>> -                       if (agc->IsPaused())
>> -                               agc->Resume();
>> -
>> -                       libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
>> +                       libcameraMetadata_.set(controls::ExposureTime, shutter);
>>                         break;
>>                 }
>>
>> @@ -524,14 +536,11 @@ void IPARPi::queueRequest(const ControlList &controls)
>>                         RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>>                                 controller_.GetAlgorithm("agc"));
>>                         ASSERT(agc);
>> -                       agc->SetFixedAnalogueGain(ctrl.second.get<float>());
>>
>> -                       /* For the manual values to take effect, AGC must be unpaused. */
>> -                       if (agc->IsPaused())
>> -                               agc->Resume();
>> +                       float gain = ctrl.second.get<float>();
>> +                       agc->SetFixedAnalogueGain(gain);
>>
>> -                       libcameraMetadata_.set(controls::AnalogueGain,
>> -                                              ctrl.second.get<float>());
>> +                       libcameraMetadata_.set(controls::AnalogueGain, gain);
>>                         break;
>>                 }
>>
>> @@ -861,6 +870,9 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>>
>>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>>  {
>> +       lastShutter_ = agcStatus->shutter_time;
>> +       lastGain_ = agcStatus->analogue_gain;
>> +
>>         int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
>>         int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
>>
>> --
>> 2.20.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 9853a343..30516665 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -67,7 +67,8 @@  public:
 	IPARPi()
 		: lastMode_({}), controller_(), controllerInit_(false),
 		  frameCount_(0), checkCount_(0), mistrustCount_(0),
-		  lsTable_(nullptr)
+		  lsTable_(nullptr),
+		  lastShutter_(0), lastGain_(0)
 	{
 	}
 
@@ -145,6 +146,10 @@  private:
 	/* LS table allocation passed in from the pipeline handler. */
 	FileDescriptor lsTableHandle_;
 	void *lsTable_;
+
+	/* For enabling/disabling the AEC/AGC algorithm. */
+	uint32_t lastShutter_;
+	float lastGain_;
 };
 
 int IPARPi::init(const IPASettings &settings)
@@ -493,12 +498,22 @@  void IPARPi::queueRequest(const ControlList &controls)
 
 		switch (ctrl.first) {
 		case controls::AE_ENABLE: {
-			RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
+			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
+				controller_.GetAlgorithm("agc"));
 			ASSERT(agc);
-			if (ctrl.second.get<bool>() == false)
-				agc->Pause();
-			else
-				agc->Resume();
+
+			/*
+			 * We always run the AGC even if it's "disabled".
+			 * Both exposure and gain are "fixed" to the last
+			 * values it produced.
+			 */
+			if (ctrl.second.get<bool>() == false) {
+				agc->SetFixedShutter(lastShutter_);
+				agc->SetFixedAnalogueGain(lastGain_);
+			} else {
+				agc->SetFixedShutter(0);
+				agc->SetFixedAnalogueGain(0);
+			}
 
 			libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
 			break;
@@ -510,13 +525,10 @@  void IPARPi::queueRequest(const ControlList &controls)
 			ASSERT(agc);
 
 			/* This expects units of micro-seconds. */
-			agc->SetFixedShutter(ctrl.second.get<int32_t>());
+			int32_t shutter = ctrl.second.get<int32_t>();
+			agc->SetFixedShutter(shutter);
 
-			/* For the manual values to take effect, AGC must be unpaused. */
-			if (agc->IsPaused())
-				agc->Resume();
-
-			libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
+			libcameraMetadata_.set(controls::ExposureTime, shutter);
 			break;
 		}
 
@@ -524,14 +536,11 @@  void IPARPi::queueRequest(const ControlList &controls)
 			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
 				controller_.GetAlgorithm("agc"));
 			ASSERT(agc);
-			agc->SetFixedAnalogueGain(ctrl.second.get<float>());
 
-			/* For the manual values to take effect, AGC must be unpaused. */
-			if (agc->IsPaused())
-				agc->Resume();
+			float gain = ctrl.second.get<float>();
+			agc->SetFixedAnalogueGain(gain);
 
-			libcameraMetadata_.set(controls::AnalogueGain,
-					       ctrl.second.get<float>());
+			libcameraMetadata_.set(controls::AnalogueGain, gain);
 			break;
 		}
 
@@ -861,6 +870,9 @@  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
 
 void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
 {
+	lastShutter_ = agcStatus->shutter_time;
+	lastGain_ = agcStatus->analogue_gain;
+
 	int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
 	int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);