[libcamera-devel] [PATCH 1/4] ipa: libipa: algorithm: Add queueRequest() to the Algorithm class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 15 11:55:38 CEST 2022
Hi Kieran,
On Fri, Jul 15, 2022 at 09:58:39AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-07-13 02:09:44)
> > 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 ?
No we don't, as you've noticed I've commented on that below.
> > >
> > > } /* 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...
I'm fine with that. The reason I didn't take an extra step (or a few
actually) is that we're still hamering down the details of per-frame
control, so whatever we write here will be reworded anyway :-)
> > *
> > * 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.
Good point, let's do that already.
> > > + [[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
More information about the libcamera-devel
mailing list