[libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: recalculate camera exposure/gain when camera mode changes

[David Plowman]

Hi Laurent

On Mon, 22 Jun 2020 at 05:04, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> Nitpicking a little bit, the commit message subject prefix should be
> "libcamera: ipa: raspberrypi:". Same for 1/2.

Noted for next time!  :)

>
> On Thu, Jun 18, 2020 at 12:12:36PM +0100, David Plowman wrote:
> > This commit causes the AGC to recalculate its camera exposure/gain
> > values when the camera mode changes. For example it's possible
> > that the exposure profile could be changed by the application so
> > the division between exposure time and analogue gain may be
> > different.
> >
> > The other underlying reason (and which this commit accomplishes too)
> > is that the sensor's line timing may change in a new mode, and because
> > V4L2 drivers store a number of exposure _lines_, the resulting _time_
> > will "change under our feet". So we have to go through the process of
> > recalculating the correct number of lines and writing this back to the
> > sensor with every mode switch, regardless of anything else.
>
> I wonder if in the future we shouldn't try to abstract this in the
> CameraSensor class, to avoid having to deal with this sensor-specific
> behaviour in IPAs. Not something for this patch series.

Agree. I think the current behaviour is probably unhelpful for many
pipeline handlers.

Best regards
David

>
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Both patches applied to my tree with the subjects updated. I'll push to
> master after running the compilation tests.
>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 +++++++++++
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp        | 25 +++++++++++-----------
> >  3 files changed, 26 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index a474287..c02b5ec 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)
> >       constraint_mode_name_ = constraint_mode_name;
> >  }
> >
> > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> > +{
> > +     // On a mode switch, it's possible the exposure profile could change,
> > +     // so we run through the dividing up of exposure/gain again and
> > +     // write the results into the metadata we've been given.
> > +     if (status_.total_exposure_value) {
> > +             housekeepConfig();
> > +             divvyupExposure();
> > +             writeAndFinish(metadata, false);
> > +     }
> > +}
> > +
> >  void Agc::Prepare(Metadata *image_metadata)
> >  {
> >       AgcStatus status;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index dbcefba..9a7e89c 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -75,6 +75,7 @@ public:
> >       void SetMeteringMode(std::string const &metering_mode_name) override;
> >       void SetExposureMode(std::string const &exposure_mode_name) override;
> >       void SetConstraintMode(std::string const &contraint_mode_name) override;
> > +     void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;
> >       void Prepare(Metadata *image_metadata) override;
> >       void Process(StatisticsPtr &stats, Metadata *image_metadata) override;
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index d6fd3df..42c84b1 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -247,29 +247,30 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >               mistrust_count_ = helper_->MistrustFramesStartup();
> >       }
> >
> > +     struct AgcStatus agcStatus;
> > +     /* These zero values mean not program anything (unless overwritten). */
> > +     agcStatus.shutter_time = 0.0;
> > +     agcStatus.analogue_gain = 0.0;
> > +
> >       if (!controllerInit_) {
> >               /* Load the tuning file for this sensor. */
> >               controller_.Read(tuningFile_.c_str());
> >               controller_.Initialise();
> >               controllerInit_ = true;
> >
> > -             /* Calculate initial values for gain and exposure. */
> > -             int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);
> > -             int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);
> > -
> > -             ControlList ctrls(unicam_ctrls_);
> > -             ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> > -             ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> > -
> > -             IPAOperationData op;
> > -             op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> > -             op.controls.push_back(ctrls);
> > -             queueFrameAction.emit(0, op);
> > +             /* Supply initial values for gain and exposure. */
> > +             agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
> > +             agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
> >       }
> >
> >       RPi::Metadata metadata;
> >       controller_.SwitchMode(mode_, &metadata);
> >
> > +     /* SwitchMode may supply updated exposure/gain values to use. */
> > +     metadata.Get("agc.status", agcStatus);
> > +     if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)
> > +             applyAGC(&agcStatus);
> > +
> >       lastMode_ = mode_;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart