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

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

Commit Message

Milan Zamazal Jan. 13, 2025, 1:34 p.m. UTC 
From: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

Metadata must be set before computing the new gain values in order to
report the metadata used to process the image rather than the metadata
determined from the image (and to be used for processing the next
image).

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/simple/algorithms/awb.cpp | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Jan. 27, 2025, 7:04 a.m. UTC | #1 
Hi Milan,

Thank you for the patch.

On Mon, Jan 13, 2025 at 02:34:02PM +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.
> 
> Metadata must be set before computing the new gain values in order to
> report the metadata used to process the image rather than the metadata
> determined from the image (and to be used for processing the next
> image).

That's not right. Setting the metadata before computing the gains won't
magically solve the synchronization problem. The code can be kept as-is
(ideally with a comment to indicate synchronization needs to be fixed),
but the commit message shouldn't be misleading.

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/simple/algorithms/awb.cpp | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> index 195de41d..95ff4434 100644
> --- a/src/ipa/simple/algorithms/awb.cpp
> +++ b/src/ipa/simple/algorithms/awb.cpp
> @@ -14,6 +14,8 @@
>  
>  #include "simple/ipa_context.h"
>  
> +#include "control_ids.h"
> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPASoftAwb)
> @@ -33,10 +35,16 @@ void Awb::process(IPAContext &context,
>  		  [[maybe_unused]] const uint32_t frame,
>  		  [[maybe_unused]] IPAFrameContext &frameContext,
>  		  const SwIspStats *stats,
> -		  [[maybe_unused]] ControlList &metadata)
> +		  ControlList &metadata)
>  {
>  	const SwIspStats::Histogram &histogram = stats->yHistogram;
>  	const uint8_t blackLevel = context.activeState.blc.level;
> +	auto &gains = context.activeState.gains;
> +
> +	const float maxGain = 1024.0;
> +	const float mdGains[] = { static_cast<float>(gains.red / maxGain),
> +				  static_cast<float>(gains.blue / maxGain) };
> +	metadata.set(controls::ColourGains, mdGains);
>  
>  	/*
>  	 * Black level must be subtracted to get the correct AWB ratios, they
> @@ -54,7 +62,6 @@ void Awb::process(IPAContext &context,
>  	 * Calculate red and blue gains for AWB.
>  	 * Clamp max gain at 4.0, this also avoids 0 division.
>  	 */
> -	auto &gains = context.activeState.gains;
>  	gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR;
>  	gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB;
>  	/* Green gain is fixed to 1.0 */
Kieran Bingham Jan. 27, 2025, 6:33 p.m. UTC | #2 
Quoting Laurent Pinchart (2025-01-27 07:04:27)
> Hi Milan,
> 
> Thank you for the patch.
> 
> On Mon, Jan 13, 2025 at 02:34:02PM +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.
> > 
> > Metadata must be set before computing the new gain values in order to
> > report the metadata used to process the image rather than the metadata
> > determined from the image (and to be used for processing the next
> > image).
> 
> That's not right. Setting the metadata before computing the gains won't
> magically solve the synchronization problem. The code can be kept as-is
> (ideally with a comment to indicate synchronization needs to be fixed),
> but the commit message shouldn't be misleading.

I think this patch was written before we had a frame context queue.

Now this patch should be discarded, (or re-written) to store during
prepare() the gains used for that frame in the frame context, and set
the metadata from the values that were used when preparing the frame.


--
Kieran
 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/simple/algorithms/awb.cpp | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> > index 195de41d..95ff4434 100644
> > --- a/src/ipa/simple/algorithms/awb.cpp
> > +++ b/src/ipa/simple/algorithms/awb.cpp
> > @@ -14,6 +14,8 @@
> >  
> >  #include "simple/ipa_context.h"
> >  
> > +#include "control_ids.h"
> > +
> >  namespace libcamera {
> >  
> >  LOG_DEFINE_CATEGORY(IPASoftAwb)
> > @@ -33,10 +35,16 @@ void Awb::process(IPAContext &context,
> >                 [[maybe_unused]] const uint32_t frame,
> >                 [[maybe_unused]] IPAFrameContext &frameContext,
> >                 const SwIspStats *stats,
> > -               [[maybe_unused]] ControlList &metadata)
> > +               ControlList &metadata)
> >  {
> >       const SwIspStats::Histogram &histogram = stats->yHistogram;
> >       const uint8_t blackLevel = context.activeState.blc.level;
> > +     auto &gains = context.activeState.gains;
> > +
> > +     const float maxGain = 1024.0;
> > +     const float mdGains[] = { static_cast<float>(gains.red / maxGain),
> > +                               static_cast<float>(gains.blue / maxGain) };
> > +     metadata.set(controls::ColourGains, mdGains);
> >  
> >       /*
> >        * Black level must be subtracted to get the correct AWB ratios, they
> > @@ -54,7 +62,6 @@ void Awb::process(IPAContext &context,
> >        * Calculate red and blue gains for AWB.
> >        * Clamp max gain at 4.0, this also avoids 0 division.
> >        */
> > -     auto &gains = context.activeState.gains;
> >       gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR;
> >       gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB;
> >       /* Green gain is fixed to 1.0 */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Milan Zamazal Jan. 30, 2025, 10:44 a.m. UTC | #3 
Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Laurent Pinchart (2025-01-27 07:04:27)
>> Hi Milan,
>> 
>
>> Thank you for the patch.
>> 
>> On Mon, Jan 13, 2025 at 02:34:02PM +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.
>> > 
>> > Metadata must be set before computing the new gain values in order to
>> > report the metadata used to process the image rather than the metadata
>> > determined from the image (and to be used for processing the next
>> > image).
>> 
>> That's not right. Setting the metadata before computing the gains won't
>> magically solve the synchronization problem. The code can be kept as-is
>> (ideally with a comment to indicate synchronization needs to be fixed),
>> but the commit message shouldn't be misleading.
>
> I think this patch was written before we had a frame context queue.
>
> Now this patch should be discarded, (or re-written) to store during
> prepare() the gains used for that frame in the frame context, and set
> the metadata from the values that were used when preparing the frame.

I'll do this in v4.

> --
> Kieran
>  
>> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> > ---
>> >  src/ipa/simple/algorithms/awb.cpp | 11 +++++++++--
>> >  1 file changed, 9 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
>> > index 195de41d..95ff4434 100644
>> > --- a/src/ipa/simple/algorithms/awb.cpp
>> > +++ b/src/ipa/simple/algorithms/awb.cpp
>> > @@ -14,6 +14,8 @@
>> >  
>> >  #include "simple/ipa_context.h"
>> >  
>> > +#include "control_ids.h"
>> > +
>> >  namespace libcamera {
>> >  
>> >  LOG_DEFINE_CATEGORY(IPASoftAwb)
>> > @@ -33,10 +35,16 @@ void Awb::process(IPAContext &context,
>> >                 [[maybe_unused]] const uint32_t frame,
>> >                 [[maybe_unused]] IPAFrameContext &frameContext,
>> >                 const SwIspStats *stats,
>> > -               [[maybe_unused]] ControlList &metadata)
>> > +               ControlList &metadata)
>> >  {
>> >       const SwIspStats::Histogram &histogram = stats->yHistogram;
>> >       const uint8_t blackLevel = context.activeState.blc.level;
>> > +     auto &gains = context.activeState.gains;
>> > +
>> > +     const float maxGain = 1024.0;
>> > +     const float mdGains[] = { static_cast<float>(gains.red / maxGain),
>> > +                               static_cast<float>(gains.blue / maxGain) };
>> > +     metadata.set(controls::ColourGains, mdGains);
>> >  
>> >       /*
>> >        * Black level must be subtracted to get the correct AWB ratios, they
>> > @@ -54,7 +62,6 @@ void Awb::process(IPAContext &context,
>> >        * Calculate red and blue gains for AWB.
>> >        * Clamp max gain at 4.0, this also avoids 0 division.
>> >        */
>> > -     auto &gains = context.activeState.gains;
>> >       gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR;
>> >       gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB;
>> >       /* Green gain is fixed to 1.0 */
>> 
>> -- 
>> Regards,
>> 
>> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
index 195de41d..95ff4434 100644
--- a/src/ipa/simple/algorithms/awb.cpp
+++ b/src/ipa/simple/algorithms/awb.cpp
@@ -14,6 +14,8 @@ 
 
 #include "simple/ipa_context.h"
 
+#include "control_ids.h"
+
 namespace libcamera {
 
 LOG_DEFINE_CATEGORY(IPASoftAwb)
@@ -33,10 +35,16 @@  void Awb::process(IPAContext &context,
 		  [[maybe_unused]] const uint32_t frame,
 		  [[maybe_unused]] IPAFrameContext &frameContext,
 		  const SwIspStats *stats,
-		  [[maybe_unused]] ControlList &metadata)
+		  ControlList &metadata)
 {
 	const SwIspStats::Histogram &histogram = stats->yHistogram;
 	const uint8_t blackLevel = context.activeState.blc.level;
+	auto &gains = context.activeState.gains;
+
+	const float maxGain = 1024.0;
+	const float mdGains[] = { static_cast<float>(gains.red / maxGain),
+				  static_cast<float>(gains.blue / maxGain) };
+	metadata.set(controls::ColourGains, mdGains);
 
 	/*
 	 * Black level must be subtracted to get the correct AWB ratios, they
@@ -54,7 +62,6 @@  void Awb::process(IPAContext &context,
 	 * Calculate red and blue gains for AWB.
 	 * Clamp max gain at 4.0, this also avoids 0 division.
 	 */
-	auto &gains = context.activeState.gains;
 	gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR;
 	gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB;
 	/* Green gain is fixed to 1.0 */