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

17333 diff mbox series

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@ideasonboard.com>
---
 src/ipa/rkisp1/ipa_context.cpp | 55 ++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 9 deletions(-)

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@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.
>   *
> - * \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.
> + *
> + *  - 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.
> + *
> + * 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.
> + *
> + *   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@ideasonboard.com>

>   */
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
>

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@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@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@ideasonboard.com>
>
> >   */
> >
> >  /**
> > --
> > Regards,
> >
> > Laurent Pinchart
> >

Hi Kieran,

On Wed, Sep 21, 2022 at 12:14:11AM +0100, Kieran Bingham 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@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.
> >   *
> > - * \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.
> > + *
> > + *  - 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.
> > + *
> > + * 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.
> > + *
> > + *   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

I agree. I'll probably rewrite this section to mention metadata instead
of colour temperature then.

> 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@ideasonboard.com>
> 
> >   */
> >  
> >  /**

Hi Jacopo,

On Wed, Sep 21, 2022 at 10:56:25PM +0200, Jacopo Mondi wrote:
> 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@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.

I think the active state should then only be accessed from the main IPA
thread, with all data that needs to be accessed from multiple threads
being stored inside individual algorithms.

> > >   *
> > > - * \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 ?

Note that I intentionally didn't mention control list here.

We store the value of all controls in the active state. Looking at the
filter algorithm for instance, the active state stores the sharpness
value, which comes directly from the request controls (after clamping to
the valid range and converting to an integer).

> > > + *
> > > + *  - 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 ?

The AWB algorithm stores both the manual and auto values in the active
state.

> > > + *
> > > + * 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@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@ideasonboard.com>
> >
> > >   */
> > >
> > >  /**