[libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add queueRequest() to the Algorithm class

[Kieran Bingham]

Quoting Laurent Pinchart via libcamera-devel (2022-07-13 02:09:44)
> Hi Florian,
> 
> Thank you for the patch.
> 
> On Mon, Jul 04, 2022 at 05:23:15PM +0200, Florian Sylvestre via libcamera-devel wrote:
> > Add queueRequest() function to the Algorithm class. The queueRequest() function
> > provides controls values coming from the application to each algorithm.
> > Each algorithm is responsible for retrieving the controls associated to them.

Fantastic - just what I had in mind, and was envisaging!

> > Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> > ---
> >  src/ipa/ipu3/module.h        |  2 +-
> >  src/ipa/libipa/algorithm.cpp | 13 +++++++++++++
> >  src/ipa/libipa/algorithm.h   |  6 ++++++
> >  src/ipa/libipa/module.cpp    |  5 +++++
> >  src/ipa/libipa/module.h      |  3 ++-
> >  src/ipa/rkisp1/module.h      |  2 +-
> >  6 files changed, 28 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/module.h b/src/ipa/ipu3/module.h
> > index d94fc459..5c2179e0 100644
> > --- a/src/ipa/ipu3/module.h
> > +++ b/src/ipa/ipu3/module.h
> > @@ -20,7 +20,7 @@ namespace libcamera {
> >  namespace ipa::ipu3 {
> >  
> >  using Module = ipa::Module<IPAContext, IPAFrameContext, IPAConfigInfo,
> > -                        ipu3_uapi_params, ipu3_uapi_stats_3a>;
> > +                        ipu3_uapi_params, ipu3_uapi_stats_3a, ControlList>;

I'm not sure that we need to define ControlList here ?

> >  
> >  } /* namespace ipa::ipu3 */
> >  
> > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp
> > index 8549fe3f..1fb811ef 100644
> > --- a/src/ipa/libipa/algorithm.cpp
> > +++ b/src/ipa/libipa/algorithm.cpp
> > @@ -81,6 +81,19 @@ namespace ipa {
> >   * includes setting fields and flags that enable those processing blocks.
> >   */
> >  
> > +/**
> > + * \fn Algorithm::queueRequest()
> > + * \brief Provide control values to the algorithm
> > + * \param[in] context The shared IPA context
> > + * \param[in] frame The frame number to aplly the control values
> 
> s/aplly/apply/
> 
> > + * \param[in] controls The list of user controls
> > + *
> > + * This function provides controls values coming from the application to the
> > + * algorithm. A frame number is provided to indicate the concerned frame.
> > + * Each algorithm is responsible for retrieving the controls associated to
> > + * them.
> 
> Let's also mention when this is called:
> 
>  * This function is called for each request queued to the camera. It provides
>  * the controls stored in the request to the algorithm. The \a frame number
>  * identifies the corresponding frame.

Technically - it identifies the request sequence that the control list
was queued in.

We are working to infer that the request sequence represents the desired
corresponding frame number - but that is not fully settled throughout
yet, so I would probably prefer to see:

* This function is called for each request queued to the camera. It
* provides the controls stored in the request to the algorithm. The \a
* frame number is the Request sequence number and identifies the desired
* corresponding frame to target for the controls to take effect.


But perhaps that gets 'too' wordy...

>  *
>  * Algorithms shall read the applicable controls and store their value for later
>  * use during frame processing.
> 
> > + */
> > +
> >  /**
> >   * \fn Algorithm::process()
> >   * \brief Process ISP statistics, and run algorithm operations
> > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h
> > index 2a8871d8..aa846625 100644
> > --- a/src/ipa/libipa/algorithm.h
> > +++ b/src/ipa/libipa/algorithm.h
> > @@ -40,6 +40,12 @@ public:
> >       {
> >       }
> >  
> > +     virtual void queueRequest([[maybe_unused]] typename Module::Context &context,
> > +                               [[maybe_unused]] const uint32_t frame,

To match the 'current' implementation of process, this should pass the
frameContext for the corresponding frame number.

                               [[maybe_unused]] typename Module::FrameContext *frameContext,

Any algorithm should store actionable state in that frameContext for
when it actually is processing that frame.


> > +                               [[maybe_unused]] const typename Module::ControlList &controls)
> > +     {
> > +     }
> > +
> >       virtual void process([[maybe_unused]] typename Module::Context &context,
> >                            [[maybe_unused]] typename Module::FrameContext *frameContext,
> >                            [[maybe_unused]] const typename Module::Stats *stats)
> > diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp
> > index 77352104..e87a52fc 100644
> > --- a/src/ipa/libipa/module.cpp
> > +++ b/src/ipa/libipa/module.cpp
> > @@ -77,6 +77,11 @@ namespace ipa {
> >   * \brief The type of the IPA statistics and ISP results
> >   */
> >  
> > +/**
> > + * \typedef Module::ControlList
> > + * \brief The type of the ISP runtime controls list
> > + */
> > +
> >  /**
> >   * \fn Module::algorithms()
> >   * \brief Retrieve the list of instantiated algorithms
> > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h
> > index 4149a353..81d3bf7f 100644
> > --- a/src/ipa/libipa/module.h
> > +++ b/src/ipa/libipa/module.h
> > @@ -26,7 +26,7 @@ LOG_DECLARE_CATEGORY(IPAModuleAlgo)
> >  namespace ipa {
> >  
> >  template<typename _Context, typename _FrameContext, typename _Config,
> > -      typename _Params, typename _Stats>
> > +      typename _Params, typename _Stats, typename _ControlList>
> 
> I don't expect usage of a different container than
> libcamera::ControlList for controls, so this can be hardcoded. No need
> to change the template parameters to the Module class, just use
> ControlList directly in queueRequest() instead of Module::ControlList.

Ah yes - agreed here. It should just be a ControlList.

Really keen to see this move forwards, with the above topics considered:

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



> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> >  class Module : public Loggable
> >  {
> >  public:
> > @@ -35,6 +35,7 @@ public:
> >       using Config = _Config;
> >       using Params = _Params;
> >       using Stats = _Stats;
> > +     using ControlList = _ControlList;
> >  
> >       virtual ~Module() {}
> >  
> > diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h
> > index 89f83208..08381a08 100644
> > --- a/src/ipa/rkisp1/module.h
> > +++ b/src/ipa/rkisp1/module.h
> > @@ -20,7 +20,7 @@ namespace libcamera {
> >  namespace ipa::rkisp1 {
> >  
> >  using Module = ipa::Module<IPAContext, IPAFrameContext, IPACameraSensorInfo,
> > -                        rkisp1_params_cfg, rkisp1_stat_buffer>;
> > +                        rkisp1_params_cfg, rkisp1_stat_buffer, ControlList>;
> >  
> >  } /* namespace ipa::rkisp1 */
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart