[1/3] ipa: libipa: Add Lux helper
diff mbox series

Message ID 20240412091700.1817754-2-paul.elder@ideasonboard.com
State New
Headers show
Series
  • ipa: rkisp1: Add lux estimation
Related show

Commit Message

Paul Elder April 12, 2024, 9:16 a.m. UTC
Add a Lux helper to libipa that does the estimation of the lux level
given gain, exposure, aperture, and luminance histogram. The helper also
handles reading the reference values from the tuning file. These are
expected to be common operations of lux algorithm modules in IPAs, and
is modeled/copied from Raspberry Pi.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/libipa/lux.cpp     | 119 +++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/lux.h       |  45 ++++++++++++++
 src/ipa/libipa/meson.build |   2 +
 3 files changed, 166 insertions(+)
 create mode 100644 src/ipa/libipa/lux.cpp
 create mode 100644 src/ipa/libipa/lux.h

Comments

Kieran Bingham April 12, 2024, 10:53 a.m. UTC | #1
Hi Paul,

Quoting Paul Elder (2024-04-12 10:16:58)
> Add a Lux helper to libipa that does the estimation of the lux level
> given gain, exposure, aperture, and luminance histogram. The helper also
> handles reading the reference values from the tuning file. These are
> expected to be common operations of lux algorithm modules in IPAs, and
> is modeled/copied from Raspberry Pi.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/ipa/libipa/lux.cpp     | 119 +++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/lux.h       |  45 ++++++++++++++
>  src/ipa/libipa/meson.build |   2 +
>  3 files changed, 166 insertions(+)
>  create mode 100644 src/ipa/libipa/lux.cpp
>  create mode 100644 src/ipa/libipa/lux.h
> 
> diff --git a/src/ipa/libipa/lux.cpp b/src/ipa/libipa/lux.cpp
> new file mode 100644
> index 00000000..756cd3c4
> --- /dev/null
> +++ b/src/ipa/libipa/lux.cpp
> @@ -0,0 +1,119 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * lux.h - Helper class that implements lux estimation
> + */
> +#include "lux.h"
> +
> +#include <algorithm>
> +#include <chrono>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "histogram.h"
> +
> +/**
> + * \file lux.h
> + * \brief Helper class that implements lux estimation
> + *
> + * As estimating the lux level of an image is expected to be a common
> + * operation, it is implemented in a helper in libipa.

I think the brief should explain what it is and how to use it (or in
this case what it uses?) - not where it lives.

> + */
> +
> +namespace libcamera {
> +
> +using namespace std::literals::chrono_literals;
> +
> +LOG_DEFINE_CATEGORY(Lux)
> +
> +namespace ipa {
> +
> +/**
> + * \class Lux
> + * \brief Class that implements lux estimation
> + */
> +
> +/**
> + * \var Lux::referenceExposureTime_
> + * \brief The exposure time of the reference image, in microseconds.
> + */
> +
> +/**
> + * \var Lux::referenceGain_
> + * \brief The analogue gain of the reference image.
> + */
> +
> +/**
> + * \var Lux::referenceAperture_
> + * \brief The aperture of the reference image in units of 1/f.
> + */
> +
> +/**
> + * \var Lux::referenceY_
> + * \brief The measured luminance of the reference image, out of 65536.

Why 65536? Would it be better/more explicit to say that it's a 16 bit
value? Except of course that is BIT(16) so it's a 16-bit-value plus one?
Is 65536 a permitted value?

> + */
> +
> +/**
> + * \var Lux::referenceLux_
> + * \brief The estimated lux level of the reference image.
> + */
> +
> +int Lux::readYaml(const YamlObject &tuningData)
> +{
> +       auto value = tuningData["reference_exposure_time"].get<double>();
> +       if (!value) {
> +               LOG(Lux, Error) << "Missing tuning parameter: 'reference_exposure_time'";
> +               return -EINVAL;
> +       }
> +       referenceExposureTime_ = *value * 1.0us;
> +
> +       value = tuningData["reference_gain"].get<double>();
> +       if (!value) {
> +               LOG(Lux, Error) << "Missing tuning parameter: 'reference_gain'";
> +               return -EINVAL;
> +       }
> +       referenceGain_ = *value;
> +
> +       referenceAperture_ = tuningData["reference_aperture"].get<double>(1.0);
> +
> +       value = tuningData["reference_Y"].get<double>();
> +       if (!value) {
> +               LOG(Lux, Error) << "Missing tuning parameter: 'reference_Y'";
> +               return -EINVAL;
> +       }
> +       referenceY_ = *value;
> +
> +       value = tuningData["reference_lux"].get<double>();
> +       if (!value) {
> +               LOG(Lux, Error) << "Missing tuning parameter: 'reference_lux'";
> +               return -EINVAL;
> +       }
> +       referenceLux_ = *value;
> +
> +       return 0;
> +}
> +
> +double Lux::process(double gain, utils::Duration exposureTime, double aperture,
> +                   const Histogram &yHist) const

As this isn't an algorithm itself, it's a helper I wonder if this
function should be named to match what it /does/. I.e.
  double Lux::estimate() ?

> +{
> +       double currentY = yHist.interQuantileMean(0, 1);
> +       double gainRatio = referenceGain_ / gain;
> +       double exposureTimeRatio = referenceExposureTime_ / exposureTime;
> +       double apertureRatio = referenceAperture_ / aperture;
> +       double yRatio = currentY * (65536 / yHist.bins()) / referenceY_;
> +
> +       double estimatedLux = exposureTimeRatio * gainRatio *
> +                             apertureRatio * apertureRatio *
> +                             yRatio * referenceLux_;

Does the tuning process only generate a single reference point? I guess
it's expected to be closely linear here? Or maybe it's close enough as
an estimate anyway.



> +
> +       LOG(Lux, Debug) << "Estimated lux " << estimatedLux;
> +       return estimatedLux;
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/lux.h b/src/ipa/libipa/lux.h
> new file mode 100644
> index 00000000..6bc9cf9f
> --- /dev/null
> +++ b/src/ipa/libipa/lux.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * lux.h - Helper class that implements lux estimation
> + */
> +
> +#pragma once
> +
> +#include <algorithm>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/utils.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "histogram.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class Lux
> +{
> +public:
> +       Lux() = default;
> +       ~Lux() = default;
> +
> +       int readYaml(const YamlObject &tuningData);
> +       double process(double gain, utils::Duration exposureTime,
> +                      double aperture, const Histogram &yHist) const;
> +
> +private:
> +       utils::Duration referenceExposureTime_;
> +       double referenceGain_;
> +       double referenceAperture_;
> +       double referenceY_;
> +       double referenceLux_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 0796982e..b6d58900 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -8,6 +8,7 @@ libipa_headers = files([
>      'exposure_mode_helper.h',
>      'fc_queue.h',
>      'histogram.h',
> +    'lux.h',
>      'matrix.h',
>      'module.h',
>      'pwl.h',
> @@ -21,6 +22,7 @@ libipa_sources = files([
>      'exposure_mode_helper.cpp',
>      'fc_queue.cpp',
>      'histogram.cpp',
> +    'lux.cpp',
>      'matrix.cpp',
>      'module.cpp',
>      'pwl.cpp'
> -- 
> 2.39.2
>
Dan Scally May 21, 2024, 10:11 a.m. UTC | #2
Hi Paul - thanks for the patches

On 12/04/2024 10:16, Paul Elder wrote:
> Add a Lux helper to libipa that does the estimation of the lux level
> given gain, exposure, aperture, and luminance histogram. The helper also
> handles reading the reference values from the tuning file. These are
> expected to be common operations of lux algorithm modules in IPAs, and
> is modeled/copied from Raspberry Pi.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>   src/ipa/libipa/lux.cpp     | 119 +++++++++++++++++++++++++++++++++++++
>   src/ipa/libipa/lux.h       |  45 ++++++++++++++
>   src/ipa/libipa/meson.build |   2 +
>   3 files changed, 166 insertions(+)
>   create mode 100644 src/ipa/libipa/lux.cpp
>   create mode 100644 src/ipa/libipa/lux.h
>
> diff --git a/src/ipa/libipa/lux.cpp b/src/ipa/libipa/lux.cpp
> new file mode 100644
> index 00000000..756cd3c4
> --- /dev/null
> +++ b/src/ipa/libipa/lux.cpp
> @@ -0,0 +1,119 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * lux.h - Helper class that implements lux estimation
> + */
No filename please
> +#include "lux.h"
> +
> +#include <algorithm>
> +#include <chrono>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "histogram.h"
> +
> +/**
> + * \file lux.h
> + * \brief Helper class that implements lux estimation
> + *
> + * As estimating the lux level of an image is expected to be a common
> + * operation, it is implemented in a helper in libipa.
> + */
> +
> +namespace libcamera {
> +
> +using namespace std::literals::chrono_literals;
> +
> +LOG_DEFINE_CATEGORY(Lux)
> +
> +namespace ipa {
> +
> +/**
> + * \class Lux
> + * \brief Class that implements lux estimation
> + */
> +
> +/**
> + * \var Lux::referenceExposureTime_
> + * \brief The exposure time of the reference image, in microseconds.
> + */
> +
> +/**
> + * \var Lux::referenceGain_
> + * \brief The analogue gain of the reference image.
> + */
Analogue gain only? What if there was digital gain applied when the reference was collected?
> +
> +/**
> + * \var Lux::referenceAperture_
> + * \brief The aperture of the reference image in units of 1/f.
> + */
> +
> +/**
> + * \var Lux::referenceY_
> + * \brief The measured luminance of the reference image, out of 65536.


65536? Or 65535?

> + */
> +
> +/**
> + * \var Lux::referenceLux_
> + * \brief The estimated lux level of the reference image.
> + */
> +
> +int Lux::readYaml(const YamlObject &tuningData)


This is a public function, so I think it should have a doxygen comment?

> +{
> +	auto value = tuningData["reference_exposure_time"].get<double>();
> +	if (!value) {
> +		LOG(Lux, Error) << "Missing tuning parameter: 'reference_exposure_time'";
> +		return -EINVAL;
> +	}
> +	referenceExposureTime_ = *value * 1.0us;


Laurent suggested switching to integration time everywhere, as exposure time tends to get shortened 
to "exposure".

> +
> +	value = tuningData["reference_gain"].get<double>();
> +	if (!value) {
> +		LOG(Lux, Error) << "Missing tuning parameter: 'reference_gain'";
> +		return -EINVAL;
> +	}
> +	referenceGain_ = *value;
> +
> +	referenceAperture_ = tuningData["reference_aperture"].get<double>(1.0);
> +
> +	value = tuningData["reference_Y"].get<double>();
> +	if (!value) {
> +		LOG(Lux, Error) << "Missing tuning parameter: 'reference_Y'";
> +		return -EINVAL;
> +	}
> +	referenceY_ = *value;
> +
> +	value = tuningData["reference_lux"].get<double>();
> +	if (!value) {
> +		LOG(Lux, Error) << "Missing tuning parameter: 'reference_lux'";
> +		return -EINVAL;
> +	}
> +	referenceLux_ = *value;
> +
> +	return 0;
> +}
> +
> +double Lux::process(double gain, utils::Duration exposureTime, double aperture,
> +		    const Histogram &yHist) const
We've no constraints on the name here, so I think "estimateLux" would be better. Also needs a 
doxygen comment I think?
> +{
> +	double currentY = yHist.interQuantileMean(0, 1);
> +	double gainRatio = referenceGain_ / gain;
> +	double exposureTimeRatio = referenceExposureTime_ / exposureTime;
> +	double apertureRatio = referenceAperture_ / aperture;
> +	double yRatio = currentY * (65536 / yHist.bins()) / referenceY_;
> +
> +	double estimatedLux = exposureTimeRatio * gainRatio *
> +			      apertureRatio * apertureRatio *
> +			      yRatio * referenceLux_;


apertureRatio is in here twice; is that accidental? Do you know the background to the formula? It's 
a bit strange, in my view...our assumption from the mean luminance AGC is of a linear relationship 
between "total exposure" (comprising integration time multiplied by gain) and the average of the 
yHist, but that's not really reflected here.

> +
> +	LOG(Lux, Debug) << "Estimated lux " << estimatedLux;
> +	return estimatedLux;
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/lux.h b/src/ipa/libipa/lux.h
> new file mode 100644
> index 00000000..6bc9cf9f
> --- /dev/null
> +++ b/src/ipa/libipa/lux.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
> + *
> + * lux.h - Helper class that implements lux estimation
> + */
No file name please
> +
> +#pragma once
> +
> +#include <algorithm>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/utils.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "histogram.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class Lux
> +{
> +public:
> +	Lux() = default;
> +	~Lux() = default;
> +
> +	int readYaml(const YamlObject &tuningData);
> +	double process(double gain, utils::Duration exposureTime,
> +		       double aperture, const Histogram &yHist) const;
> +
> +private:
> +	utils::Duration referenceExposureTime_;
> +	double referenceGain_;
> +	double referenceAperture_;
> +	double referenceY_;
> +	double referenceLux_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 0796982e..b6d58900 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -8,6 +8,7 @@ libipa_headers = files([
>       'exposure_mode_helper.h',
>       'fc_queue.h',
>       'histogram.h',
> +    'lux.h',
>       'matrix.h',
>       'module.h',
>       'pwl.h',
> @@ -21,6 +22,7 @@ libipa_sources = files([
>       'exposure_mode_helper.cpp',
>       'fc_queue.cpp',
>       'histogram.cpp',
> +    'lux.cpp',
>       'matrix.cpp',
>       'module.cpp',
>       'pwl.cpp'

Patch
diff mbox series

diff --git a/src/ipa/libipa/lux.cpp b/src/ipa/libipa/lux.cpp
new file mode 100644
index 00000000..756cd3c4
--- /dev/null
+++ b/src/ipa/libipa/lux.cpp
@@ -0,0 +1,119 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2019, Raspberry Pi Ltd
+ * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
+ *
+ * lux.h - Helper class that implements lux estimation
+ */
+#include "lux.h"
+
+#include <algorithm>
+#include <chrono>
+
+#include <libcamera/base/log.h>
+
+#include "libcamera/internal/yaml_parser.h"
+
+#include "histogram.h"
+
+/**
+ * \file lux.h
+ * \brief Helper class that implements lux estimation
+ *
+ * As estimating the lux level of an image is expected to be a common
+ * operation, it is implemented in a helper in libipa.
+ */
+
+namespace libcamera {
+
+using namespace std::literals::chrono_literals;
+
+LOG_DEFINE_CATEGORY(Lux)
+
+namespace ipa {
+
+/**
+ * \class Lux
+ * \brief Class that implements lux estimation
+ */
+
+/**
+ * \var Lux::referenceExposureTime_
+ * \brief The exposure time of the reference image, in microseconds.
+ */
+
+/**
+ * \var Lux::referenceGain_
+ * \brief The analogue gain of the reference image.
+ */
+
+/**
+ * \var Lux::referenceAperture_
+ * \brief The aperture of the reference image in units of 1/f.
+ */
+
+/**
+ * \var Lux::referenceY_
+ * \brief The measured luminance of the reference image, out of 65536.
+ */
+
+/**
+ * \var Lux::referenceLux_
+ * \brief The estimated lux level of the reference image.
+ */
+
+int Lux::readYaml(const YamlObject &tuningData)
+{
+	auto value = tuningData["reference_exposure_time"].get<double>();
+	if (!value) {
+		LOG(Lux, Error) << "Missing tuning parameter: 'reference_exposure_time'";
+		return -EINVAL;
+	}
+	referenceExposureTime_ = *value * 1.0us;
+
+	value = tuningData["reference_gain"].get<double>();
+	if (!value) {
+		LOG(Lux, Error) << "Missing tuning parameter: 'reference_gain'";
+		return -EINVAL;
+	}
+	referenceGain_ = *value;
+
+	referenceAperture_ = tuningData["reference_aperture"].get<double>(1.0);
+
+	value = tuningData["reference_Y"].get<double>();
+	if (!value) {
+		LOG(Lux, Error) << "Missing tuning parameter: 'reference_Y'";
+		return -EINVAL;
+	}
+	referenceY_ = *value;
+
+	value = tuningData["reference_lux"].get<double>();
+	if (!value) {
+		LOG(Lux, Error) << "Missing tuning parameter: 'reference_lux'";
+		return -EINVAL;
+	}
+	referenceLux_ = *value;
+
+	return 0;
+}
+
+double Lux::process(double gain, utils::Duration exposureTime, double aperture,
+		    const Histogram &yHist) const
+{
+	double currentY = yHist.interQuantileMean(0, 1);
+	double gainRatio = referenceGain_ / gain;
+	double exposureTimeRatio = referenceExposureTime_ / exposureTime;
+	double apertureRatio = referenceAperture_ / aperture;
+	double yRatio = currentY * (65536 / yHist.bins()) / referenceY_;
+
+	double estimatedLux = exposureTimeRatio * gainRatio *
+			      apertureRatio * apertureRatio *
+			      yRatio * referenceLux_;
+
+	LOG(Lux, Debug) << "Estimated lux " << estimatedLux;
+	return estimatedLux;
+}
+
+} /* namespace ipa */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/lux.h b/src/ipa/libipa/lux.h
new file mode 100644
index 00000000..6bc9cf9f
--- /dev/null
+++ b/src/ipa/libipa/lux.h
@@ -0,0 +1,45 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2019, Raspberry Pi Ltd
+ * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com>
+ *
+ * lux.h - Helper class that implements lux estimation
+ */
+
+#pragma once
+
+#include <algorithm>
+#include <tuple>
+#include <vector>
+
+#include <libcamera/base/utils.h>
+
+#include "libcamera/internal/yaml_parser.h"
+
+#include "histogram.h"
+
+namespace libcamera {
+
+namespace ipa {
+
+class Lux
+{
+public:
+	Lux() = default;
+	~Lux() = default;
+
+	int readYaml(const YamlObject &tuningData);
+	double process(double gain, utils::Duration exposureTime,
+		       double aperture, const Histogram &yHist) const;
+
+private:
+	utils::Duration referenceExposureTime_;
+	double referenceGain_;
+	double referenceAperture_;
+	double referenceY_;
+	double referenceLux_;
+};
+
+} /* namespace ipa */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
index 0796982e..b6d58900 100644
--- a/src/ipa/libipa/meson.build
+++ b/src/ipa/libipa/meson.build
@@ -8,6 +8,7 @@  libipa_headers = files([
     'exposure_mode_helper.h',
     'fc_queue.h',
     'histogram.h',
+    'lux.h',
     'matrix.h',
     'module.h',
     'pwl.h',
@@ -21,6 +22,7 @@  libipa_sources = files([
     'exposure_mode_helper.cpp',
     'fc_queue.cpp',
     'histogram.cpp',
+    'lux.cpp',
     'matrix.cpp',
     'module.cpp',
     'pwl.cpp'