[v5,3/6] ipa: simple: Report the ColourGains in metadata
diff mbox series

Message ID 20250326124856.75709-4-mzamazal@redhat.com
State Superseded
Headers show
Series
  • ipa: simple: Introduce metadata reporting
Related show

Commit Message

Milan Zamazal March 26, 2025, 12:48 p.m. UTC 
From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide the determined colour gains back into the metadata
to add to completed requests.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/awb.cpp | 21 ++++++++++++++++++++-
 src/ipa/simple/algorithms/awb.h   |  6 +++++-
 src/ipa/simple/ipa_context.h      |  4 ++++
 3 files changed, 29 insertions(+), 2 deletions(-)

Comments

Kieran Bingham March 26, 2025, 9:26 p.m. UTC | #1 
Quoting Milan Zamazal (2025-03-26 12:48:52)
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Provide the determined colour gains back into the metadata
> to add to completed requests.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/awb.cpp | 21 ++++++++++++++++++++-
>  src/ipa/simple/algorithms/awb.h   |  6 +++++-
>  src/ipa/simple/ipa_context.h      |  4 ++++
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> index ec77c6e5..55719059 100644
> --- a/src/ipa/simple/algorithms/awb.cpp
> +++ b/src/ipa/simple/algorithms/awb.cpp
> @@ -17,6 +17,8 @@
>  #include "libipa/colours.h"
>  #include "simple/ipa_context.h"
>  
> +#include "control_ids.h"
> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPASoftAwb)
> @@ -32,15 +34,32 @@ int Awb::configure(IPAContext &context,
>         return 0;
>  }
>  
> +void Awb::prepare(IPAContext &context,
> +                 [[maybe_unused]] const uint32_t frame,
> +                 IPAFrameContext &frameContext,
> +                 [[maybe_unused]] DebayerParams *params)
> +{
> +       auto &gains = context.activeState.awb.gains;
> +       frameContext.gains.red = gains.r();
> +       frameContext.gains.blue = gains.b();

Interesting that this highlights the white balance parameters are
probably being used from the wrong 'time' as they should be determined
here in prepare... (by taking the most recent active state and combining
any decision from the controls for any manual white balance).

But - as this series is just trying to get the reporting in - lets leave
that issue for later, and try to keep this moving forwards:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> +}
> +
>  void Awb::process(IPAContext &context,
>                   [[maybe_unused]] const uint32_t frame,
> -                 [[maybe_unused]] IPAFrameContext &frameContext,
> +                 IPAFrameContext &frameContext,
>                   const SwIspStats *stats,
>                   ControlList &metadata)
>  {
>         const SwIspStats::Histogram &histogram = stats->yHistogram;
>         const uint8_t blackLevel = context.activeState.blc.level;
>  
> +       const float maxGain = 1024.0;
> +       const float mdGains[] = {
> +               static_cast<float>(frameContext.gains.red / maxGain),
> +               static_cast<float>(frameContext.gains.blue / maxGain)
> +       };
> +       metadata.set(controls::ColourGains, mdGains);
> +
>         /*
>          * Black level must be subtracted to get the correct AWB ratios, they
>          * would be off if they were computed from the whole brightness range
> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
> index db1496cd..ad993f39 100644
> --- a/src/ipa/simple/algorithms/awb.h
> +++ b/src/ipa/simple/algorithms/awb.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  /*
> - * Copyright (C) 2024, Red Hat Inc.
> + * Copyright (C) 2024-2025 Red Hat Inc.
>   *
>   * Auto white balance
>   */
> @@ -20,6 +20,10 @@ public:
>         ~Awb() = default;
>  
>         int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> +       void prepare(IPAContext &context,
> +                    const uint32_t frame,
> +                    IPAFrameContext &frameContext,
> +                    DebayerParams *params) override;
>         void process(IPAContext &context,
>                      const uint32_t frame,
>                      IPAFrameContext &frameContext,
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 17bcd4ca..bfac835b 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -70,6 +70,10 @@ struct IPAFrameContext : public FrameContext {
>                 int32_t exposure;
>                 double gain;
>         } sensor;
> +       struct {
> +               double red;
> +               double blue;
> +       } gains;
>  };
>  
>  struct IPAContext {
> -- 
> 2.49.0
>
Laurent Pinchart March 26, 2025, 11:22 p.m. UTC | #2 
Hi Milan,

Thank you for the patch.

On Wed, Mar 26, 2025 at 01:48:52PM +0100, Milan Zamazal wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Provide the determined colour gains back into the metadata
> to add to completed requests.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/ipa/simple/algorithms/awb.cpp | 21 ++++++++++++++++++++-
>  src/ipa/simple/algorithms/awb.h   |  6 +++++-
>  src/ipa/simple/ipa_context.h      |  4 ++++
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> index ec77c6e5..55719059 100644
> --- a/src/ipa/simple/algorithms/awb.cpp
> +++ b/src/ipa/simple/algorithms/awb.cpp
> @@ -17,6 +17,8 @@
>  #include "libipa/colours.h"
>  #include "simple/ipa_context.h"
>  
> +#include "control_ids.h"
> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPASoftAwb)
> @@ -32,15 +34,32 @@ int Awb::configure(IPAContext &context,
>  	return 0;
>  }
>  
> +void Awb::prepare(IPAContext &context,
> +		  [[maybe_unused]] const uint32_t frame,
> +		  IPAFrameContext &frameContext,
> +		  [[maybe_unused]] DebayerParams *params)
> +{
> +	auto &gains = context.activeState.awb.gains;
> +	frameContext.gains.red = gains.r();
> +	frameContext.gains.blue = gains.b();
> +}
> +
>  void Awb::process(IPAContext &context,
>  		  [[maybe_unused]] const uint32_t frame,
> -		  [[maybe_unused]] IPAFrameContext &frameContext,
> +		  IPAFrameContext &frameContext,
>  		  const SwIspStats *stats,
>  		  ControlList &metadata)
>  {
>  	const SwIspStats::Histogram &histogram = stats->yHistogram;
>  	const uint8_t blackLevel = context.activeState.blc.level;
>  
> +	const float maxGain = 1024.0;
> +	const float mdGains[] = {
> +		static_cast<float>(frameContext.gains.red / maxGain),
> +		static_cast<float>(frameContext.gains.blue / maxGain)
> +	};
> +	metadata.set(controls::ColourGains, mdGains);
> +
>  	/*
>  	 * Black level must be subtracted to get the correct AWB ratios, they
>  	 * would be off if they were computed from the whole brightness range
> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
> index db1496cd..ad993f39 100644
> --- a/src/ipa/simple/algorithms/awb.h
> +++ b/src/ipa/simple/algorithms/awb.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  /*
> - * Copyright (C) 2024, Red Hat Inc.
> + * Copyright (C) 2024-2025 Red Hat Inc.
>   *
>   * Auto white balance
>   */
> @@ -20,6 +20,10 @@ public:
>  	~Awb() = default;
>  
>  	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> +	void prepare(IPAContext &context,
> +		     const uint32_t frame,
> +		     IPAFrameContext &frameContext,
> +		     DebayerParams *params) override;
>  	void process(IPAContext &context,
>  		     const uint32_t frame,
>  		     IPAFrameContext &frameContext,
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 17bcd4ca..bfac835b 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -70,6 +70,10 @@ struct IPAFrameContext : public FrameContext {
>  		int32_t exposure;
>  		double gain;
>  	} sensor;
> +	struct {
> +		double red;
> +		double blue;
> +	} gains;
>  };
>  
>  struct IPAContext {
Milan Zamazal March 27, 2025, 8:18 p.m. UTC | #3 
Hi Kieran,

thank you for review.

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2025-03-26 12:48:52)
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> 
>
>> Provide the determined colour gains back into the metadata
>> to add to completed requests.
>> 
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/ipa/simple/algorithms/awb.cpp | 21 ++++++++++++++++++++-
>>  src/ipa/simple/algorithms/awb.h   |  6 +++++-
>>  src/ipa/simple/ipa_context.h      |  4 ++++
>>  3 files changed, 29 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
>> index ec77c6e5..55719059 100644
>> --- a/src/ipa/simple/algorithms/awb.cpp
>> +++ b/src/ipa/simple/algorithms/awb.cpp
>> @@ -17,6 +17,8 @@
>>  #include "libipa/colours.h"
>>  #include "simple/ipa_context.h"
>>  
>> +#include "control_ids.h"
>> +
>>  namespace libcamera {
>>  
>>  LOG_DEFINE_CATEGORY(IPASoftAwb)
>> @@ -32,15 +34,32 @@ int Awb::configure(IPAContext &context,
>>         return 0;
>>  }
>>  
>> +void Awb::prepare(IPAContext &context,
>> +                 [[maybe_unused]] const uint32_t frame,
>> +                 IPAFrameContext &frameContext,
>> +                 [[maybe_unused]] DebayerParams *params)
>> +{
>> +       auto &gains = context.activeState.awb.gains;
>> +       frameContext.gains.red = gains.r();
>> +       frameContext.gains.blue = gains.b();
>
> Interesting that this highlights the white balance parameters are
> probably being used from the wrong 'time' as they should be determined
> here in prepare... (by taking the most recent active state and combining
> any decision from the controls for any manual white balance).

I'm not sure I understand what you mean.  They come from the most recent
active state, don't they?  We don't support manual AWB balance but if we
did the gains could be amended here accordingly.  And later used in Lut
algorithm.  The gains stored here and used to process the frame are then
reported in metadata for this frame.

What's wrong then?

> But - as this series is just trying to get the reporting in - lets leave
> that issue for later, and try to keep this moving forwards:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
>
>> +}
>> +
>>  void Awb::process(IPAContext &context,
>>                   [[maybe_unused]] const uint32_t frame,
>> -                 [[maybe_unused]] IPAFrameContext &frameContext,
>> +                 IPAFrameContext &frameContext,
>>                   const SwIspStats *stats,
>>                   ControlList &metadata)
>>  {
>>         const SwIspStats::Histogram &histogram = stats->yHistogram;
>>         const uint8_t blackLevel = context.activeState.blc.level;
>>  
>> +       const float maxGain = 1024.0;
>> +       const float mdGains[] = {
>> +               static_cast<float>(frameContext.gains.red / maxGain),
>> +               static_cast<float>(frameContext.gains.blue / maxGain)
>> +       };
>> +       metadata.set(controls::ColourGains, mdGains);
>> +
>>         /*
>>          * Black level must be subtracted to get the correct AWB ratios, they
>>          * would be off if they were computed from the whole brightness range
>> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
>> index db1496cd..ad993f39 100644
>> --- a/src/ipa/simple/algorithms/awb.h
>> +++ b/src/ipa/simple/algorithms/awb.h
>> @@ -1,6 +1,6 @@
>>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>>  /*
>> - * Copyright (C) 2024, Red Hat Inc.
>> + * Copyright (C) 2024-2025 Red Hat Inc.
>>   *
>>   * Auto white balance
>>   */
>> @@ -20,6 +20,10 @@ public:
>>         ~Awb() = default;
>>  
>>         int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>> +       void prepare(IPAContext &context,
>> +                    const uint32_t frame,
>> +                    IPAFrameContext &frameContext,
>> +                    DebayerParams *params) override;
>>         void process(IPAContext &context,
>>                      const uint32_t frame,
>>                      IPAFrameContext &frameContext,
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 17bcd4ca..bfac835b 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -70,6 +70,10 @@ struct IPAFrameContext : public FrameContext {
>>                 int32_t exposure;
>>                 double gain;
>>         } sensor;
>> +       struct {
>> +               double red;
>> +               double blue;
>> +       } gains;
>>  };
>>  
>>  struct IPAContext {
>> -- 
>> 2.49.0
>>
Kieran Bingham March 28, 2025, 6:32 a.m. UTC | #4 
Quoting Milan Zamazal (2025-03-27 20:18:35)
> Hi Kieran,
> 
> thank you for review.
> 
> Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
> 
> > Quoting Milan Zamazal (2025-03-26 12:48:52)
> >> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> 
> >
> >> Provide the determined colour gains back into the metadata
> >> to add to completed requests.
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> >> ---
> >>  src/ipa/simple/algorithms/awb.cpp | 21 ++++++++++++++++++++-
> >>  src/ipa/simple/algorithms/awb.h   |  6 +++++-
> >>  src/ipa/simple/ipa_context.h      |  4 ++++
> >>  3 files changed, 29 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> >> index ec77c6e5..55719059 100644
> >> --- a/src/ipa/simple/algorithms/awb.cpp
> >> +++ b/src/ipa/simple/algorithms/awb.cpp
> >> @@ -17,6 +17,8 @@
> >>  #include "libipa/colours.h"
> >>  #include "simple/ipa_context.h"
> >>  
> >> +#include "control_ids.h"
> >> +
> >>  namespace libcamera {
> >>  
> >>  LOG_DEFINE_CATEGORY(IPASoftAwb)
> >> @@ -32,15 +34,32 @@ int Awb::configure(IPAContext &context,
> >>         return 0;
> >>  }
> >>  
> >> +void Awb::prepare(IPAContext &context,
> >> +                 [[maybe_unused]] const uint32_t frame,
> >> +                 IPAFrameContext &frameContext,
> >> +                 [[maybe_unused]] DebayerParams *params)
> >> +{
> >> +       auto &gains = context.activeState.awb.gains;
> >> +       frameContext.gains.red = gains.r();
> >> +       frameContext.gains.blue = gains.b();
> >
> > Interesting that this highlights the white balance parameters are
> > probably being used from the wrong 'time' as they should be determined
> > here in prepare... (by taking the most recent active state and combining
> > any decision from the controls for any manual white balance).
> 
> I'm not sure I understand what you mean.  They come from the most recent
> active state, don't they?  We don't support manual AWB balance but if we
> did the gains could be amended here accordingly.  And later used in Lut
> algorithm.  The gains stored here and used to process the frame are then
> reported in metadata for this frame.
> 
> What's wrong then?

I'm sorry - I was probably thrown off by the fact that I couldn't see
any configuration being set into the output params at this point. If
it's handled in a separate algo then I guess that's the correct timing
... but means we should probably have a note in here to say that the
actual handling of AWB is combined with the LUT component.

But my original point was likely wrong above.

--
Kieran




> > But - as this series is just trying to get the reporting in - lets leave
> > that issue for later, and try to keep this moving forwards:
> >
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> >
> >
> >> +}
> >> +
> >>  void Awb::process(IPAContext &context,
> >>                   [[maybe_unused]] const uint32_t frame,
> >> -                 [[maybe_unused]] IPAFrameContext &frameContext,
> >> +                 IPAFrameContext &frameContext,
> >>                   const SwIspStats *stats,
> >>                   ControlList &metadata)
> >>  {
> >>         const SwIspStats::Histogram &histogram = stats->yHistogram;
> >>         const uint8_t blackLevel = context.activeState.blc.level;
> >>  
> >> +       const float maxGain = 1024.0;
> >> +       const float mdGains[] = {
> >> +               static_cast<float>(frameContext.gains.red / maxGain),
> >> +               static_cast<float>(frameContext.gains.blue / maxGain)
> >> +       };
> >> +       metadata.set(controls::ColourGains, mdGains);
> >> +
> >>         /*
> >>          * Black level must be subtracted to get the correct AWB ratios, they
> >>          * would be off if they were computed from the whole brightness range
> >> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
> >> index db1496cd..ad993f39 100644
> >> --- a/src/ipa/simple/algorithms/awb.h
> >> +++ b/src/ipa/simple/algorithms/awb.h
> >> @@ -1,6 +1,6 @@
> >>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>  /*
> >> - * Copyright (C) 2024, Red Hat Inc.
> >> + * Copyright (C) 2024-2025 Red Hat Inc.
> >>   *
> >>   * Auto white balance
> >>   */
> >> @@ -20,6 +20,10 @@ public:
> >>         ~Awb() = default;
> >>  
> >>         int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> >> +       void prepare(IPAContext &context,
> >> +                    const uint32_t frame,
> >> +                    IPAFrameContext &frameContext,
> >> +                    DebayerParams *params) override;
> >>         void process(IPAContext &context,
> >>                      const uint32_t frame,
> >>                      IPAFrameContext &frameContext,
> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> >> index 17bcd4ca..bfac835b 100644
> >> --- a/src/ipa/simple/ipa_context.h
> >> +++ b/src/ipa/simple/ipa_context.h
> >> @@ -70,6 +70,10 @@ struct IPAFrameContext : public FrameContext {
> >>                 int32_t exposure;
> >>                 double gain;
> >>         } sensor;
> >> +       struct {
> >> +               double red;
> >> +               double blue;
> >> +       } gains;
> >>  };
> >>  
> >>  struct IPAContext {
> >> -- 
> >> 2.49.0
> >>
>

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
index ec77c6e5..55719059 100644
--- a/src/ipa/simple/algorithms/awb.cpp
+++ b/src/ipa/simple/algorithms/awb.cpp
@@ -17,6 +17,8 @@ 
 #include "libipa/colours.h"
 #include "simple/ipa_context.h"
 
+#include "control_ids.h"
+
 namespace libcamera {
 
 LOG_DEFINE_CATEGORY(IPASoftAwb)
@@ -32,15 +34,32 @@  int Awb::configure(IPAContext &context,
 	return 0;
 }
 
+void Awb::prepare(IPAContext &context,
+		  [[maybe_unused]] const uint32_t frame,
+		  IPAFrameContext &frameContext,
+		  [[maybe_unused]] DebayerParams *params)
+{
+	auto &gains = context.activeState.awb.gains;
+	frameContext.gains.red = gains.r();
+	frameContext.gains.blue = gains.b();
+}
+
 void Awb::process(IPAContext &context,
 		  [[maybe_unused]] const uint32_t frame,
-		  [[maybe_unused]] IPAFrameContext &frameContext,
+		  IPAFrameContext &frameContext,
 		  const SwIspStats *stats,
 		  ControlList &metadata)
 {
 	const SwIspStats::Histogram &histogram = stats->yHistogram;
 	const uint8_t blackLevel = context.activeState.blc.level;
 
+	const float maxGain = 1024.0;
+	const float mdGains[] = {
+		static_cast<float>(frameContext.gains.red / maxGain),
+		static_cast<float>(frameContext.gains.blue / maxGain)
+	};
+	metadata.set(controls::ColourGains, mdGains);
+
 	/*
 	 * Black level must be subtracted to get the correct AWB ratios, they
 	 * would be off if they were computed from the whole brightness range
diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
index db1496cd..ad993f39 100644
--- a/src/ipa/simple/algorithms/awb.h
+++ b/src/ipa/simple/algorithms/awb.h
@@ -1,6 +1,6 @@ 
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 /*
- * Copyright (C) 2024, Red Hat Inc.
+ * Copyright (C) 2024-2025 Red Hat Inc.
  *
  * Auto white balance
  */
@@ -20,6 +20,10 @@  public:
 	~Awb() = default;
 
 	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
+	void prepare(IPAContext &context,
+		     const uint32_t frame,
+		     IPAFrameContext &frameContext,
+		     DebayerParams *params) override;
 	void process(IPAContext &context,
 		     const uint32_t frame,
 		     IPAFrameContext &frameContext,
diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
index 17bcd4ca..bfac835b 100644
--- a/src/ipa/simple/ipa_context.h
+++ b/src/ipa/simple/ipa_context.h
@@ -70,6 +70,10 @@  struct IPAFrameContext : public FrameContext {
 		int32_t exposure;
 		double gain;
 	} sensor;
+	struct {
+		double red;
+		double blue;
+	} gains;
 };
 
 struct IPAContext {