| Message ID | 20220704152318.221213-2-fsylvestre@baylibre.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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. > > Signed-off-by: Florian Sylvestre <fsylvestre@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>; > > } /* 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. * * 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, > + [[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. Reviewed-by: Laurent Pinchart <laurent.pinchart@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 */ >
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@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@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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@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@ideasonboard.com> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 */ > > >
Hello Kieran, On Fri, 15 Jul 2022 at 11:56, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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@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. As it's not currently available (at least on rkisp) I am using the current frameContext. I think that when the development will be done to manage several frameContext, each one associated with the correct frame number, then we can update this interface if needed. For the moment I have no clue what it will look like... > > > > [[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@ideasonboard.com> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 I will send a v2 of this patch series with all comments addressed (I hope) but with keeping the frameContext associated with the current frame (and not the target one). Let's discuss the new version if needed. Regards,
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>; } /* 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 + * \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. + */ + /** * \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, + [[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> 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 */
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. Signed-off-by: Florian Sylvestre <fsylvestre@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(-)