[libcamera-devel] [RFC PATCH 2/4] libcamera: pipeline: raspberrypi: Support the new AE controls
David Plowman
david.plowman at raspberrypi.com
Tue Sep 28 11:04:20 CEST 2021
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.
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... :)
> + { &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
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
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.
> 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.
> 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?)
Apart from those small things this is looking pretty close to me!
Thanks
David
More information about the libcamera-devel
mailing list