[libcamera-devel] [PATCH 2/3] src: ipa: raspberrypi: Improve behaviour when AE disabled

[David Plowman]

Hi Naush

Thanks for the suggestion.

On Thu, 26 Nov 2020 at 09:45, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Wed, 25 Nov 2020 at 11:36, David Plowman <david.plowman at 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 at 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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel