[libcamera-devel] [PATCH 3/5] libcamera: raspberrypi: Plumb user transform through to individual IPAs
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 29 17:26:02 CEST 2020
Hi David,
Thank you for the patch.
On Wed, Jul 29, 2020 at 10:31:52AM +0100, David Plowman wrote:
> This commit takes the user/application supplied Transform and plumbs
> it through to all our individual IPAs using the SwitchMode method of
> the Controller/Algorithms. IPARPi::configure has to be re-ordered just
> a little to ensure the userTransform_ is available before calling
> SwitchMode.
> ---
> src/ipa/raspberrypi/controller/algorithm.cpp | 4 ++-
> src/ipa/raspberrypi/controller/algorithm.hpp | 3 +-
> src/ipa/raspberrypi/controller/controller.cpp | 5 +--
> src/ipa/raspberrypi/controller/controller.hpp | 5 ++-
> src/ipa/raspberrypi/controller/rpi/agc.cpp | 3 +-
> src/ipa/raspberrypi/controller/rpi/agc.hpp | 3 +-
> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 3 +-
> src/ipa/raspberrypi/controller/rpi/alsc.hpp | 3 +-
> src/ipa/raspberrypi/controller/rpi/noise.cpp | 4 ++-
> src/ipa/raspberrypi/controller/rpi/noise.hpp | 3 +-
> .../raspberrypi/controller/rpi/sharpen.cpp | 3 +-
> .../raspberrypi/controller/rpi/sharpen.hpp | 3 +-
> src/ipa/raspberrypi/raspberrypi.cpp | 31 ++++++++++---------
> 13 files changed, 45 insertions(+), 28 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp
> index 55cb201..b95ebb8 100644
> --- a/src/ipa/raspberrypi/controller/algorithm.cpp
> +++ b/src/ipa/raspberrypi/controller/algorithm.cpp
> @@ -16,9 +16,11 @@ void Algorithm::Read(boost::property_tree::ptree const ¶ms)
>
> void Algorithm::Initialise() {}
>
> -void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> +void Algorithm::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata)
That's a strange indentation. Is your text editor configure with 4
spaces per tab by any chance ? :-)
Same comment for the rest of the file.
> {
> (void)camera_mode;
> + (void)transform;
> (void)metadata;
By the way, we will likely switch to C++17 in the not too distant
future, it will give us the [[maybe_unused]] attribute.
> }
>
> diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp
> index 187c50c..b38f167 100644
> --- a/src/ipa/raspberrypi/controller/algorithm.hpp
> +++ b/src/ipa/raspberrypi/controller/algorithm.hpp
> @@ -37,7 +37,8 @@ public:
> virtual void Resume() { paused_ = false; }
> virtual void Read(boost::property_tree::ptree const ¶ms);
> virtual void Initialise();
> - virtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);
> + virtual void SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata);
Instead of passing it explicitly everywhere, would it make sense to
store the transformation in CameraMode ?
> virtual void Prepare(Metadata *image_metadata);
> virtual void Process(StatisticsPtr &stats, Metadata *image_metadata);
> Metadata &GetGlobalMetadata() const
> diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp
> index 7c4b04f..7a4310f 100644
> --- a/src/ipa/raspberrypi/controller/controller.cpp
> +++ b/src/ipa/raspberrypi/controller/controller.cpp
> @@ -56,11 +56,12 @@ void Controller::Initialise()
> RPI_LOG("Controller finished");
> }
>
> -void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> +void Controller::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata)
> {
> RPI_LOG("Controller starting");
> for (auto &algo : algorithms_)
> - algo->SwitchMode(camera_mode, metadata);
> + algo->SwitchMode(camera_mode, transform, metadata);
> switch_mode_called_ = true;
> RPI_LOG("Controller finished");
> }
> diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp
> index 6ba9412..738c4b6 100644
> --- a/src/ipa/raspberrypi/controller/controller.hpp
> +++ b/src/ipa/raspberrypi/controller/controller.hpp
> @@ -15,6 +15,8 @@
>
> #include <linux/bcm2835-isp.h>
>
> +#include <libcamera/transform.h>
> +
> #include "camera_mode.h"
> #include "device_status.h"
> #include "metadata.hpp"
> @@ -39,7 +41,8 @@ public:
> Algorithm *CreateAlgorithm(char const *name);
> void Read(char const *filename);
> void Initialise();
> - void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);
> + void SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata);
> void Prepare(Metadata *image_metadata);
> void Process(StatisticsPtr stats, Metadata *image_metadata);
> Metadata &GetGlobalMetadata();
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index c02b5ec..ceac771 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -221,7 +221,8 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)
> constraint_mode_name_ = constraint_mode_name;
> }
>
> -void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> +void Agc::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + 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
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 9a7e89c..78f016e 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -75,7 +75,8 @@ 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 SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata) override;
> void Prepare(Metadata *image_metadata) override;
> void Process(StatisticsPtr &stats, Metadata *image_metadata) override;
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index 76e2f04..5fac9dd 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -12,6 +12,7 @@
> // Raspberry Pi ALSC (Auto Lens Shading Correction) algorithm.
>
> using namespace RPi;
> +using libcamera::Transform;
You use libcamera::Transform directly in the other files, shouldn't we
try to be consistent ?
>
> #define NAME "rpi.alsc"
>
> @@ -173,7 +174,7 @@ void Alsc::Initialise()
> lambda_r_[i] = lambda_b_[i] = 1.0;
> }
>
> -void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> +void Alsc::SwitchMode(CameraMode const &camera_mode, Transform transform, Metadata *metadata)
> {
> (void)metadata;
(void)metadata;
?
Same for Agc::SwitchMode() and other functions below.
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> index 3806257..9b54578 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp
> @@ -50,7 +50,8 @@ public:
> ~Alsc();
> char const *Name() const override;
> void Initialise() override;
> - void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;
> + void SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata) override;
> void Read(boost::property_tree::ptree const ¶ms) override;
> void Prepare(Metadata *image_metadata) override;
> void Process(StatisticsPtr &stats, Metadata *image_metadata) override;
> diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp
> index 2cafde3..0f4002b 100644
> --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp
> @@ -27,8 +27,10 @@ char const *Noise::Name() const
> return NAME;
> }
>
> -void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> +void Noise::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata)
> {
> + (void)transform;
> (void)metadata;
>
> // For example, we would expect a 2x2 binned mode to have a "noise
> diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp
> index 25bf188..37a22fd 100644
> --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp
> @@ -18,7 +18,8 @@ class Noise : public Algorithm
> public:
> Noise(Controller *controller);
> char const *Name() const override;
> - void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;
> + void SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata) override;
> void Read(boost::property_tree::ptree const ¶ms) override;
> void Prepare(Metadata *image_metadata) override;
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp
> index 2b701db..64d269d 100644
> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp
> @@ -26,7 +26,8 @@ char const *Sharpen::Name() const
> return NAME;
> }
>
> -void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> +void Sharpen::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata)
> {
> (void)metadata;
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp
> index 568521b..8a1c164 100644
> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp
> @@ -18,7 +18,8 @@ class Sharpen : public SharpenAlgorithm
> public:
> Sharpen(Controller *controller);
> char const *Name() const override;
> - void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;
> + void SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,
> + Metadata *metadata) override;
> void Read(boost::property_tree::ptree const ¶ms) override;
> void SetStrength(double strength) override;
> void Prepare(Metadata *image_metadata) override;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 2809521..a8676fb 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -270,21 +270,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> 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) {
> - ControlList ctrls(unicam_ctrls_);
> - applyAGC(&agcStatus, ctrls);
> - result->controls.push_back(ctrls);
> -
> - result->operation |= RPI_IPA_CONFIG_SENSOR;
> - }
> -
> - lastMode_ = mode_;
> -
> /* Store the lens shading table pointer and handle if available. */
> unsigned int next_element = 0;
> if (ipaConfig.operation & RPI_IPA_CONFIG_LS_TABLE) {
> @@ -312,6 +297,22 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> uint32_t transformType = ipaConfig.data[next_element++];
> userTransform_ = reinterpret_cast<Transform &>(transformType);
> }
> +
> + /* Finally we can tell the IPAs about the new camera mode. */
> + RPi::Metadata metadata;
> + controller_.SwitchMode(mode_, userTransform_, &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) {
> + ControlList ctrls(unicam_ctrls_);
> + applyAGC(&agcStatus, ctrls);
> + result->controls.push_back(ctrls);
> +
> + result->operation |= RPI_IPA_CONFIG_SENSOR;
> + }
> +
> + lastMode_ = mode_;
> }
>
> void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list