[libcamera-devel] [RFC PATCH 2/4] libcamera: pipeline: raspberrypi: Support the new AE controls
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue Sep 28 11:51:21 CEST 2021
Hi David,
On Tue, Sep 28, 2021 at 10:04:20AM +0100, David Plowman wrote:
> Hi Paul
>
> Thanks for this patch. I think it looks good mostly, there might just
> be a couple of small tweaks that I would suggest.
Thank you for the review!
>
> On Tue, 28 Sept 2021 at 08:50, Paul Elder <paul.elder at ideasonboard.com> wrote:
> >
> > Add support for the new AE controls in the raspberrypi pipeline handler,
> > and in the IPA.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > This is very hacky. I wasn't sure what the best way was to plumb it into
> > the raspberrypi IPA as it was a bit hairy...
> > ---
> > include/libcamera/ipa/raspberrypi.h | 3 +-
> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 18 ++++++++-
> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +++
> > src/ipa/raspberrypi/raspberrypi.cpp | 42 ++++++++++++++++----
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 ++++----
> > 5 files changed, 67 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index 521eaecd..363ea038 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -28,8 +28,9 @@ namespace RPi {
> > * unsupported control is encountered.
> > */
> > static const ControlInfoMap Controls({
> > - { &controls::AeEnable, ControlInfo(false, true) },
>
> I guess no global control just yet... :)
That's a \todo :D
>
> > + { &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },
> > { &controls::ExposureTime, ControlInfo(0, 999999) },
> > + { &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },
> > { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 289c1fce..b45ea454 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -203,14 +203,30 @@ bool Agc::IsPaused() const
> > }
> >
> > void Agc::Pause()
> > +{
> > +}
> > +
> > +void Agc::Resume()
> > +{
> > +}
> > +
> > +void Agc::PauseExposure()
> > {
> > fixed_shutter_ = status_.shutter_time;
> > +}
> > +
> > +void Agc::PauseGain()
> > +{
> > fixed_analogue_gain_ = status_.analogue_gain;
> > }
> >
> > -void Agc::Resume()
> > +void Agc::ResumeExposure()
> > {
> > fixed_shutter_ = 0s;
> > +}
> > +
> > +void Agc::ResumeGain()
> > +{
> > fixed_analogue_gain_ = 0;
> > }
>
> Clearly we need these new Pause/Resume Exposure/Gain methods, but of
Phew, glad that was the right direction.
> course we're still left with the old ones (just Pause/Resume) as our
> Algorithm interface requires them. Maybe they should still do what
> they used to do? (perhaps where each calls the two new functions) This
Ah, good idea.
> might support a global enable/disable at some point, should we ever
> have one!
>
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index 82063636..7ca3ca2f 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -92,6 +92,11 @@ public:
> > void Prepare(Metadata *image_metadata) override;
> > void Process(StatisticsPtr &stats, Metadata *image_metadata) override;
> >
> > + void PauseExposure();
> > + void PauseGain();
> > + void ResumeExposure();
> > + void ResumeGain();
> > +
>
> I think I'd like to see these go as pure virtual functions into
> agc_algorithm.hpp, and then we override them here. The idea is that
> our controller will work with any AGC algorithm that implements the
> interface in agc_algorithm.hpp (so Foo Corporation could replace ours
> with their own) and I think this would break that.
I see; I'll move them there then.
>
> > private:
> > void updateLockStatus(DeviceStatus const &device_status);
> > AgcConfig config_;
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 047123ab..99935515 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -53,6 +53,8 @@
> > #include "sharpen_algorithm.hpp"
> > #include "sharpen_status.h"
> >
> > +#include "controller/rpi/agc.hpp"
> > +
> > namespace libcamera {
> >
> > using namespace std::literals::chrono_literals;
> > @@ -478,7 +480,10 @@ void IPARPi::reportMetadata()
> >
> > AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");
> > if (agcStatus) {
> > - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);
>
> I really want to replace "locked" by "converged" here, but that's a
> patch for another day!
>
> > + libcameraMetadata_.set(controls::AeState,
> > + agcStatus->locked ?
> > + controls::AeStateConverged :
> > + controls::AeStateSearching);
> > libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);
> > }
> >
> > @@ -623,20 +628,22 @@ void IPARPi::queueRequest(const ControlList &controls)
> > << " = " << ctrl.second.toString();
> >
> > switch (ctrl.first) {
> > - case controls::AE_ENABLE: {
> > - RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
> > + case controls::EXPOSURE_TIME_MODE: {
> > + RPiController::Algorithm *algo = controller_.GetAlgorithm("agc");
> > + RPiController::Agc *agc = reinterpret_cast<RPiController::Agc *>(algo);
>
> Ah yes, here it is. We should probably cast to an
> RPiController::AgcAlgorithm here. Once those new Pause/Resume
> functions have been moved to that class then a cast to AgcAlgorithm
> should be fine.
Aah okay.
>
> > if (!agc) {
> > LOG(IPARPI, Warning)
> > - << "Could not set AE_ENABLE - no AGC algorithm";
> > + << "Could not set EXPOSURE_TIME_MODE - no AGC algorithm";
> > break;
> > }
> >
> > - if (ctrl.second.get<bool>() == false)
> > - agc->Pause();
> > + if (ctrl.second.get<int32_t>() == controls::ExposureTimeModeDisabled)
> > + agc->PauseExposure();
> > else
> > - agc->Resume();
> > + agc->ResumeExposure();
> >
> > - libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
> > + libcameraMetadata_.set(controls::ExposureTimeMode,
> > + ctrl.second.get<int32_t>());
> > break;
> > }
> >
> > @@ -656,6 +663,25 @@ void IPARPi::queueRequest(const ControlList &controls)
> > break;
> > }
> >
> > + case controls::ANALOGUE_GAIN_MODE: {
> > + RPiController::Algorithm *algo = controller_.GetAlgorithm("agc");
> > + RPiController::Agc *agc = reinterpret_cast<RPiController::Agc *>(algo);
>
> Same thing here.
>
> > + if (!agc) {
> > + LOG(IPARPI, Warning)
> > + << "Could not set ANALOGUE_GAIN_MODE - no AGC algorithm";
> > + break;
> > + }
> > +
> > + if (ctrl.second.get<int32_t>() == controls::AnalogueGainModeDisabled)
> > + agc->PauseGain();
> > + else
> > + agc->ResumeGain();
> > +
> > + libcameraMetadata_.set(controls::AnalogueGainMode,
> > + ctrl.second.get<int32_t>());
> > + break;
> > + }
> > +
> > case controls::ANALOGUE_GAIN: {
> > RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > controller_.GetAlgorithm("agc"));
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index c227d885..3a9c3b8d 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -309,25 +309,25 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> > /* \todo Make this nicer. */
> > int32_t ivalue;
> > if (id == controls::ExposureTimeMode && exposureSet) {
> > - int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);
> > - ivalue = value.get<int32_t>() == ExposureTimeModeAuto
> > + int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
> > + ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
> > ? (exposureMode == V4L2_EXPOSURE_SHUTTER_PRIORITY
> > ? V4L2_EXPOSURE_AUTO
> > : V4L2_EXPOSURE_APERTURE_PRIORITY)
> > : V4L2_EXPOSURE_MANUAL;
> > } else if (id == controls::ExposureTimeMode && !exposureSet) {
> > - ivalue = value.get<int32_t>() == ExposureTimeModeAuto
> > + ivalue = value.get<int32_t>() == controls::ExposureTimeModeAuto
> > ? V4L2_EXPOSURE_APERTURE_PRIORITY
> > : V4L2_EXPOSURE_MANUAL;
> > } else if (id == controls::AnalogueGainMode && exposureSet) {
> > - int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);
> > - ivalue = value.get<int32_t>() == AnalogueGainModeAuto
> > + int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get<int32_t>();
> > + ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
> > ? (exposureMode == V4L2_EXPOSURE_APERTURE_PRIORITY
> > ? V4L2_EXPOSURE_AUTO
> > : V4L2_EXPOSURE_SHUTTER_PRIORITY)
> > : V4L2_EXPOSURE_MANUAL;
> > } else if (id == controls::AnalogueGainMode && !exposureSet) {
> > - ivalue = value.get<int32_t>() == AnalogueGainModeAuto
> > + ivalue = value.get<int32_t>() == controls::AnalogueGainModeAuto
> > ? V4L2_EXPOSURE_SHUTTER_PRIORITY
> > : V4L2_EXPOSURE_MANUAL;
> > }
> > @@ -636,8 +636,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > break;
> >
> > case V4L2_CID_EXPOSURE_AUTO:
> > - info = ControlInfo{ { ExposureTimeModeAuto, ExposureTimeModeDisabled },
> > - ExposureTimeModeDisabled };
> > + info = ControlInfo{ { controls::ExposureTimeModeAuto,
> > + controls::ExposureTimeModeDisabled },
> > + controls::ExposureTimeModeDisabled };
> > break;
> >
> > case V4L2_CID_EXPOSURE_ABSOLUTE:
> > --
> > 2.27.0
> >
>
> (Was this UVC stuff meant to be in the same patch?)
Oops, must be a rebase mistake :S
>
> Apart from those small things this is looking pretty close to me!
Glad to hear that!
(Just wondering, have you seen the patch that adds these new controls
yet?)
Thanks,
Paul
More information about the libcamera-devel
mailing list