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

[David Plowman]

Hi Laurent

Thanks for the review.

On Thu, 26 Nov 2020 at 12:58, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Thu, Nov 26, 2020 at 12:32:02PM +0000, David Plowman wrote:
> > AE/AGC "disabled" is now handled better by the algorithm for itself,
> > so it no longer needs to be "resumed" before setting fixed shutter or
> > gain values.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 9853a343..e437c626 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -495,6 +495,7 @@ void IPARPi::queueRequest(const ControlList &controls)
> >               case controls::AE_ENABLE: {
> >                       RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> >                       ASSERT(agc);
> > +
>
> It's customary, when including unrelated minor changes such as this, to
> mention it in the commit message, to let reviewers know the change
> didn't get included by mistake.

It was a mistake, of course! :)

I'll remove it and address those other points (about the override).

Thanks
David

>
> >                       if (ctrl.second.get<bool>() == false)
> >                               agc->Pause();
> >                       else
> > @@ -512,10 +513,6 @@ void IPARPi::queueRequest(const ControlList &controls)
> >                       /* This expects units of micro-seconds. */
> >                       agc->SetFixedShutter(ctrl.second.get<int32_t>());
> >
> > -                     /* For the manual values to take effect, AGC must be unpaused. */
> > -                     if (agc->IsPaused())
> > -                             agc->Resume();
> > -
>
> Only AWB now uses the pause mechanism. I wonder if it will survive, or
> be dropped there too for the same reason as for the AGC :-)
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> >                       libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
> >                       break;
> >               }
> > @@ -526,10 +523,6 @@ void IPARPi::queueRequest(const ControlList &controls)
> >                       ASSERT(agc);
> >                       agc->SetFixedAnalogueGain(ctrl.second.get<float>());
> >
> > -                     /* For the manual values to take effect, AGC must be unpaused. */
> > -                     if (agc->IsPaused())
> > -                             agc->Resume();
> > -
> >                       libcameraMetadata_.set(controls::AnalogueGain,
> >                                              ctrl.second.get<float>());
> >                       break;
>
> --
> Regards,
>
> Laurent Pinchart