[PATCH v3 3/6] ipa: simple: Report the ColourGains in metadata

[Milan Zamazal]

Kieran Bingham <kieran.bingham at 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 at 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 at 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