[2/5] ipa: rpi: lux: Handle camera mode sensitivity correctly
diff mbox series

Message ID 20251017102704.3887-3-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Fix and improve full image Y statistics
Related show

Commit Message

David Plowman Oct. 17, 2025, 10:05 a.m. UTC 
The camera mode sensitivity needs to be taken into account for the lux
calculation. For example, the IMX708 binned mode (with a sensitivity
of 2.0) would otherwise show double the correct lux value.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/rpi/controller/rpi/lux.cpp | 13 +++++++++++--
 src/ipa/rpi/controller/rpi/lux.h   |  3 +++
 2 files changed, 14 insertions(+), 2 deletions(-)

Comments

Stefan Klug Oct. 20, 2025, 9:54 a.m. UTC | #1 
Hi David,

Quoting David Plowman (2025-10-17 12:05:38)
> The camera mode sensitivity needs to be taken into account for the lux
> calculation. For example, the IMX708 binned mode (with a sensitivity
> of 2.0) would otherwise show double the correct lux value.

That makes a lot of sense and we need to port that over to other
pipeline handlers as well. On the imx708 I see a heuristic to decide on
the sensitivity based on the image width. That doesn't go too well with
arbitrary cropping in the upcoming raw sensor models. Do you envision a
generic way to carry that information?

Maybe something that could tie into my thoughts here?
https://lore.kernel.org/linux-media/176009169065.211618.2319574445990293219@localhost/

I would love to hear your opinion on that.

> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

The patch itself looks good to me.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

Best regards,
Stefan

> ---
>  src/ipa/rpi/controller/rpi/lux.cpp | 13 +++++++++++--
>  src/ipa/rpi/controller/rpi/lux.h   |  3 +++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/rpi/lux.cpp b/src/ipa/rpi/controller/rpi/lux.cpp
> index 27b89a8f..acaa6e57 100644
> --- a/src/ipa/rpi/controller/rpi/lux.cpp
> +++ b/src/ipa/rpi/controller/rpi/lux.cpp
> @@ -20,7 +20,7 @@ LOG_DEFINE_CATEGORY(RPiLux)
>  #define NAME "rpi.lux"
>  
>  Lux::Lux(Controller *controller)
> -       : Algorithm(controller)
> +       : Algorithm(controller), sensitivity_(1.0)
>  {
>         /*
>          * Put in some defaults as there will be no meaningful values until
> @@ -68,6 +68,13 @@ void Lux::setCurrentAperture(double aperture)
>         currentAperture_ = aperture;
>  }
>  
> +void Lux::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *metadata)
> +{
> +       /* We will need to compensate for the camera sensitivity. */
> +       ASSERT(cameraMode.sensitivity);
> +       sensitivity_ = cameraMode.sensitivity;
> +}
> +
>  void Lux::prepare(Metadata *imageMetadata)
>  {
>         std::unique_lock<std::mutex> lock(mutex_);
> @@ -88,10 +95,12 @@ void Lux::process(StatisticsPtr &stats, Metadata *imageMetadata)
>                 double yRatio = currentY * (65536 / stats->yHist.bins()) / referenceY_;
>                 double estimatedLux = exposureTimeRatio * gainRatio *
>                                       apertureRatio * apertureRatio *
> -                                     yRatio * referenceLux_;
> +                                     yRatio * referenceLux_ / sensitivity_;
> +
>                 LuxStatus status;
>                 status.lux = estimatedLux;
>                 status.aperture = currentAperture;
> +
>                 LOG(RPiLux, Debug) << ": estimated lux " << estimatedLux;
>                 {
>                         std::unique_lock<std::mutex> lock(mutex_);
> diff --git a/src/ipa/rpi/controller/rpi/lux.h b/src/ipa/rpi/controller/rpi/lux.h
> index da007fe9..db2227e4 100644
> --- a/src/ipa/rpi/controller/rpi/lux.h
> +++ b/src/ipa/rpi/controller/rpi/lux.h
> @@ -10,6 +10,7 @@
>  
>  #include <libcamera/base/utils.h>
>  
> +#include "../camera_mode.h"
>  #include "../lux_status.h"
>  #include "../algorithm.h"
>  
> @@ -23,6 +24,7 @@ public:
>         Lux(Controller *controller);
>         char const *name() const override;
>         int read(const libcamera::YamlObject &params) override;
> +       void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
>         void prepare(Metadata *imageMetadata) override;
>         void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
>         void setCurrentAperture(double aperture);
> @@ -40,6 +42,7 @@ private:
>         double currentAperture_;
>         LuxStatus status_;
>         std::mutex mutex_;
> +       double sensitivity_;
>  };
>  
>  } /* namespace RPiController */
> -- 
> 2.47.3
>
Naushir Patuck Oct. 23, 2025, 7:34 a.m. UTC | #2 
Hi David,


On Fri, 17 Oct 2025 at 11:27, David Plowman <david.plowman@raspberrypi.com>
wrote:

> The camera mode sensitivity needs to be taken into account for the lux
> calculation. For example, the IMX708 binned mode (with a sensitivity
> of 2.0) would otherwise show double the correct lux value.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


> ---
>  src/ipa/rpi/controller/rpi/lux.cpp | 13 +++++++++++--
>  src/ipa/rpi/controller/rpi/lux.h   |  3 +++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/lux.cpp
> b/src/ipa/rpi/controller/rpi/lux.cpp
> index 27b89a8f..acaa6e57 100644
> --- a/src/ipa/rpi/controller/rpi/lux.cpp
> +++ b/src/ipa/rpi/controller/rpi/lux.cpp
> @@ -20,7 +20,7 @@ LOG_DEFINE_CATEGORY(RPiLux)
>  #define NAME "rpi.lux"
>
>  Lux::Lux(Controller *controller)
> -       : Algorithm(controller)
> +       : Algorithm(controller), sensitivity_(1.0)
>  {
>         /*
>          * Put in some defaults as there will be no meaningful values until
> @@ -68,6 +68,13 @@ void Lux::setCurrentAperture(double aperture)
>         currentAperture_ = aperture;
>  }
>
> +void Lux::switchMode(CameraMode const &cameraMode, [[maybe_unused]]
> Metadata *metadata)
> +{
> +       /* We will need to compensate for the camera sensitivity. */
> +       ASSERT(cameraMode.sensitivity);
> +       sensitivity_ = cameraMode.sensitivity;
> +}
> +
>  void Lux::prepare(Metadata *imageMetadata)
>  {
>         std::unique_lock<std::mutex> lock(mutex_);
> @@ -88,10 +95,12 @@ void Lux::process(StatisticsPtr &stats, Metadata
> *imageMetadata)
>                 double yRatio = currentY * (65536 / stats->yHist.bins()) /
> referenceY_;
>                 double estimatedLux = exposureTimeRatio * gainRatio *
>                                       apertureRatio * apertureRatio *
> -                                     yRatio * referenceLux_;
> +                                     yRatio * referenceLux_ /
> sensitivity_;
> +
>                 LuxStatus status;
>                 status.lux = estimatedLux;
>                 status.aperture = currentAperture;
> +
>                 LOG(RPiLux, Debug) << ": estimated lux " << estimatedLux;
>                 {
>                         std::unique_lock<std::mutex> lock(mutex_);
> diff --git a/src/ipa/rpi/controller/rpi/lux.h
> b/src/ipa/rpi/controller/rpi/lux.h
> index da007fe9..db2227e4 100644
> --- a/src/ipa/rpi/controller/rpi/lux.h
> +++ b/src/ipa/rpi/controller/rpi/lux.h
> @@ -10,6 +10,7 @@
>
>  #include <libcamera/base/utils.h>
>
> +#include "../camera_mode.h"
>  #include "../lux_status.h"
>  #include "../algorithm.h"
>
> @@ -23,6 +24,7 @@ public:
>         Lux(Controller *controller);
>         char const *name() const override;
>         int read(const libcamera::YamlObject &params) override;
> +       void switchMode(CameraMode const &cameraMode, Metadata *metadata)
> override;
>         void prepare(Metadata *imageMetadata) override;
>         void process(StatisticsPtr &stats, Metadata *imageMetadata)
> override;
>         void setCurrentAperture(double aperture);
> @@ -40,6 +42,7 @@ private:
>         double currentAperture_;
>         LuxStatus status_;
>         std::mutex mutex_;
> +       double sensitivity_;
>  };
>
>  } /* namespace RPiController */
> --
> 2.47.3
>
>

Patch
diff mbox series 

diff --git a/src/ipa/rpi/controller/rpi/lux.cpp b/src/ipa/rpi/controller/rpi/lux.cpp
index 27b89a8f..acaa6e57 100644
--- a/src/ipa/rpi/controller/rpi/lux.cpp
+++ b/src/ipa/rpi/controller/rpi/lux.cpp
@@ -20,7 +20,7 @@  LOG_DEFINE_CATEGORY(RPiLux)
 #define NAME "rpi.lux"
 
 Lux::Lux(Controller *controller)
-	: Algorithm(controller)
+	: Algorithm(controller), sensitivity_(1.0)
 {
 	/*
 	 * Put in some defaults as there will be no meaningful values until
@@ -68,6 +68,13 @@  void Lux::setCurrentAperture(double aperture)
 	currentAperture_ = aperture;
 }
 
+void Lux::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *metadata)
+{
+	/* We will need to compensate for the camera sensitivity. */
+	ASSERT(cameraMode.sensitivity);
+	sensitivity_ = cameraMode.sensitivity;
+}
+
 void Lux::prepare(Metadata *imageMetadata)
 {
 	std::unique_lock<std::mutex> lock(mutex_);
@@ -88,10 +95,12 @@  void Lux::process(StatisticsPtr &stats, Metadata *imageMetadata)
 		double yRatio = currentY * (65536 / stats->yHist.bins()) / referenceY_;
 		double estimatedLux = exposureTimeRatio * gainRatio *
 				      apertureRatio * apertureRatio *
-				      yRatio * referenceLux_;
+				      yRatio * referenceLux_ / sensitivity_;
+
 		LuxStatus status;
 		status.lux = estimatedLux;
 		status.aperture = currentAperture;
+
 		LOG(RPiLux, Debug) << ": estimated lux " << estimatedLux;
 		{
 			std::unique_lock<std::mutex> lock(mutex_);
diff --git a/src/ipa/rpi/controller/rpi/lux.h b/src/ipa/rpi/controller/rpi/lux.h
index da007fe9..db2227e4 100644
--- a/src/ipa/rpi/controller/rpi/lux.h
+++ b/src/ipa/rpi/controller/rpi/lux.h
@@ -10,6 +10,7 @@ 
 
 #include <libcamera/base/utils.h>
 
+#include "../camera_mode.h"
 #include "../lux_status.h"
 #include "../algorithm.h"
 
@@ -23,6 +24,7 @@  public:
 	Lux(Controller *controller);
 	char const *name() const override;
 	int read(const libcamera::YamlObject &params) override;
+	void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
 	void prepare(Metadata *imageMetadata) override;
 	void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
 	void setCurrentAperture(double aperture);
@@ -40,6 +42,7 @@  private:
 	double currentAperture_;
 	LuxStatus status_;
 	std::mutex mutex_;
+	double sensitivity_;
 };
 
 } /* namespace RPiController */