[libcamera-devel] [PATCH v4 24/32] ipa: rkisp1: Document the active state and frame context

[Jacopo Mondi]

Hi Laurent

On Wed, Sep 21, 2022 at 12:14:11AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:52)
> > Now that data used by algorithms has been partitioned between the active
> > state and frame context, we have a better view of the role of each of
> > those structures. Document them appropriately.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/ipa_context.cpp | 55 ++++++++++++++++++++++++++++------
> >  1 file changed, 46 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index b2628ef73d49..335cb32c538d 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -88,16 +88,28 @@ namespace libcamera::ipa::rkisp1 {
> >   * \struct IPAActiveState
> >   * \brief Active state for algorithms
> >   *
> > - * The active state stores algorithm-specific data that needs to be shared
> > - * between multiple algorithms and the IPA module. It is accessible through the
> > - * IPAContext structure.
> > + * The active state contains all algorithm-specific data that need to be
>
> /need/needs/
>
> > + * maintained by algorithms across frames. Unlike the session configuration,
> > + * the active state is mutable and constantly updated by algorithms. The active
> > + * state is accessible through the IPAContext structure.

Once IPA will be threaded, if ever, it will be sweet to implement and
debug concurrent accesses to the active state.

> >   *
> > - * \todo Split the data contained in this structure between the active state
> > - * and the frame contexts.
> > + * The active state stores two distinct categories of information:
> >   *
> > - * Each of the fields in the active state belongs to either a specific
> > - * algorithm, or to the top-level IPA module. A field may be read by any
> > - * algorithm, but should only be written by its owner.
> > + *  - The consolidated value of all algorithm controls. Requests passed to
> > + *    the queueRequest() function store values for controls that the
> > + *    application wants to modify for that particular frame, and the
> > + *    queueRequest() function updates the active state with those values.
> > + *    The active state thus contains a consolidated view of the value of all
> > + *    controls handled by the algorithm.

I think what we actually store are only controls that change the
algorithm state (enable/disable). What are the examples of
"consolidate control lists" in the active state ?

> > + *
> > + *  - The value of parameters computed by the algorithm when running in auto
> > + *    mode. Algorithms running in auto mode compute new parameters every
> > + *    time statistics buffers are received (either synchronously, or
> > + *    possibly in a background thread). The latest computed value of those
> > + *    parameters is stored in the active state in the process() function.

AWB stores manual controls, and I understand it might be an outliner,
but doesn't the active state actually store the last values -applied-
by an algorithm, being it from the manual or the auto mode ?

> > + *
> > + * Each of the members in the active state belongs to a specific algorithm. A
> > + * member may be read by any algorithm, but shall only be written by its owner.
> >   */
> >
> >  /**
> > @@ -185,7 +197,32 @@ namespace libcamera::ipa::rkisp1 {
> >   * \struct RkISP1FrameContext
> >   * \brief Per-frame context for algorithms
> >   *
> > - * \todo Populate the frame context for all algorithms
> > + * The frame context stores two distinct categories of information:
> > + *
> > + * - The value of the controls to be applied to the frame. These values are
> > + *   typically set in the queueRequest() function, from the consolidated
> > + *   control values stored in the active state. The frame context thus stores
> > + *   values for all controls related to the algorithm, not limited to the
> > + *   controls specified in the corresponding request, but consolidated from all
> > + *   requests that have been queued so far.
> > + *
> > + *   For controls that can be set manually or computed by an algorithm
> > + *   (depending on the algorithm operation mode), such as for instance the
> > + *   colour gains for the AWB algorithm, the control value will be stored in
> > + *   the frame context in the queueRequest() function only when operating in
> > + *   manual mode. When operating in auto mode, the values are computed by the
> > + *   algorithm in process(), stored in the active state, and copied to the
> > + *   frame context in prepare(), just before being stored in the ISP parameters
> > + *   buffer.

I presume AEGC will behave the same, so this doesn't make AWB an
outliner, but the above will become a pattern.

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>



> > + *
> > + *   The queueRequest() function can also store ancillary data in the frame
> > + *   context, such as flags to indicate if (and what) control values have
> > + *   changed compared to the previous request.
> > + *
> > + * - Status information computed by the algorithm for a frame. For instance,
> > + *   the colour temperature estimated by the AWB algorithm from ISP statistics
> > + *   calculated on a frame is stored in the frame context for that frame in
> > + *   the process() function.
>
> That all sounds fine. I wonder if some of the examples might need
> tweaking later, as I think for instance the colour temperature for a
> frame might go directlty into the metadata, as mentioned earlier. The
> FrameContext is really only storing information about a frame that is
> required at multiple processing steps/calls. And the ColourTemperature
> is likely not needed (for a specific frame) after it's calculated,
> except to put it into the metadata for that frame.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >   */
> >
> >  /**
> > --
> > Regards,
> >
> > Laurent Pinchart
> >