[PATCH v1 5/5] ipa: rkisp1: ccm: Ensure metadata contains valid ccm

[Kieran Bingham]

Quoting Stefan Klug (2024-07-12 15:32:06)
> When the colour temperature does not change between frames, the ccm
> inside the frame context is not updated and the metadata contains
> invalid data. Fix that by caching the ccm inside the active state.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/ccm.cpp | 7 +++++--
>  src/ipa/rkisp1/ipa_context.h      | 4 ++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index 4c4e3f5029a4..e9b1e78b7ec1 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -101,7 +101,7 @@ void Ccm::setParameters(rkisp1_params_cfg *params,
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void Ccm::prepare(IPAContext &context, const uint32_t frame,
> +void Ccm::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,

Is this maybe_unused required? You've added usage, not removed it ... it
certainly looks used to me ?

>                   IPAFrameContext &frameContext,
>                   rkisp1_params_cfg *params)
>  {
> @@ -111,13 +111,16 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>          * \todo The colour temperature will likely be noisy, add filtering to
>          * avoid updating the CCM matrix all the time.
>          */
> -       if (frame > 0 && ct == ct_)
> +       if (frame > 0 && ct == ct_) {
> +               frameContext.ccm.ccm = context.activeState.ccm.ccm;
>                 return;
> +       }
>  
>         ct_ = ct;
>         Matrix<float, 3, 3> ccm = ccm_.get(ct);
>         Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
>  
> +       context.activeState.ccm.ccm = ccm;

Should we save a copy - and fill the context.activeState.ccm.ccm
directly from the ccm_.get ? (Though that might then look unbalanced
against the offsets).

Or otherwise, as this is only accessed internally here - what about
moving Matrix<float, 3, 3> ccm; and Matrix<int16_t, 3, 1> offsets; to
the private class instance ? Then there's no extra copy ...

But if you still prefer this route - then

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

>         frameContext.ccm.ccm = ccm;
>  
>         setParameters(params, ccm, offsets);
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 27a9bf62fc16..061efc0c578e 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -97,6 +97,10 @@ struct IPAActiveState {
>                 bool autoEnabled;
>         } awb;
>  
> +       struct {
> +               Matrix<float, 3, 3> ccm;
> +       } ccm;
> +
>         struct {
>                 int8_t brightness;
>                 uint8_t contrast;
> -- 
> 2.43.0
>