[libcamera-devel] [PATCH v4 09/32] ipa: ipu3: Use base FrameContext class

[Kieran Bingham]

Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:37)
> Inherit from the base FrameContext class in the IPU3 IPAFrameContext.
> This allows dropping the frame member, which is now stored in the base
> class.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipa_context.cpp | 24 ++++--------------------
>  src/ipa/ipu3/ipa_context.h   |  7 ++++---
>  src/ipa/ipu3/ipu3.cpp        |  5 +----
>  3 files changed, 9 insertions(+), 27 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 13cdb835ef7f..9cfca0db3a0d 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 {
>   * most recently computed by the IPA algorithms.
>   */
>  
> -/**
> - * \struct IPAFrameContext
> - * \brief Context for a frame
> - *
> - * The frame context stores data specific to a single frame processed by the
> - * IPA. Each frame processed by the IPA has a context associated with it,
> - * accessible through the IPAContext structure.
> - *
> - * Fields in the frame context should reflect values and controls
> - * associated with the specific frame as requested by the application, and
> - * as configured by the hardware. Fields can be read by algorithms to
> - * determine if they should update any specific action for this frame, and
> - * finally to update the metadata control lists when the frame is fully
> - * completed.
> - */
> -
>  /**
>   * \struct IPAContext
>   * \brief Global IPA context data shared between all algorithms
> @@ -188,15 +172,15 @@ IPAFrameContext::IPAFrameContext() = default;
>  /**
>   * \brief Construct a IPAFrameContext instance
>   */
> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> -       : frame(id), frameControls(reqControls)
> +IPAFrameContext::IPAFrameContext(const ControlList &reqControls)
> +       : frameControls(reqControls)
>  {
>         sensor = {};
>  }
>  
>  /**
> - * \var IPAFrameContext::frame
> - * \brief The frame number
> + * \struct IPAFrameContext
> + * \brief IPU3-specific FrameContext
>   *
>   * \var IPAFrameContext::frameControls
>   * \brief Controls sent in by the application while queuing the request
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 42e11141d3a1..e8fc42769075 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -17,6 +17,8 @@
>  #include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
>  
> +#include <libipa/fc_queue.h>
> +
>  namespace libcamera {
>  
>  namespace ipa::ipu3 {
> @@ -76,16 +78,15 @@ struct IPAActiveState {
>         } toneMapping;
>  };
>  
> -struct IPAFrameContext {
> +struct IPAFrameContext : public FrameContext {
>         IPAFrameContext();
> -       IPAFrameContext(uint32_t id, const ControlList &reqControls);
> +       IPAFrameContext(const ControlList &reqControls);
>  
>         struct {
>                 uint32_t exposure;
>                 double gain;
>         } sensor;
>  
> -       uint32_t frame;
>         ControlList frameControls;
>  };
>  
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index e5a763fd2b08..b1b23fd8f927 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -607,9 +607,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>  
>         IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
>  
> -       if (frameContext.frame != frame)
> -               LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> -

(almost) unrelated change. Is this intentional?

It could be (but the frameContext.frame will still find the derived
frame member right?), so an updated commit message, or split to a
separate patch and :

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

>         frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>         frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>  
> @@ -654,7 +651,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>  void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>  {
>         /* \todo Start processing for 'frame' based on 'controls'. */
> -       context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> +       context_.frameContexts[frame % kMaxFrameContexts] = { controls };
>  }
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
>