[libcamera-devel] [PATCH] ipa: rkisp1: ipa_context: Add camera data to IPAContext

[Laurent Pinchart]

Hi Daniel,

On Fri, May 12, 2023 at 02:46:56PM +0200, Daniel Semkowicz wrote:
> Hi Laurent, Hi Kieran, Hi Jacopo,
> 
> Thank you for this patch! This will simplify the work with AF and lens
> control.

You're welcome.

> On Sun, Apr 30, 2023 at 7:03 PM Jacopo Mondi via libcamera-devel wrote:
> > On Wed, Apr 26, 2023 at 04:10:37AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > The IPARkISP1 class caches data that is immutable over the life time of
> > > the camera in private class members at init() time. This covers the
> > > current needs to the IPA module, but doesn't provide a mechanism to
> > > access that data from IPA algorithms. This will be needed for instance
> > > by the upcoming auto-focus algorithm.
> > >
> > > To prepare for such use cases, create a new IPACameraData structure that
> > > stores immutable camera data, and instantiate it in the IPAContext
> > > structure. The IPACameraData structure is currently empty, new members
> > > will be added as needed.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > Patch looks good, should we already collect it or wait for Daniel to
> > take it into his AF series ?
> 
> If existence of the empty structures in the code base is not an issue, then
> it would be good to have this merged before the AF series. This makes
> life easier
> when pushing new versions of patch series.

It's not much of an issue. I'll send a v2 and merge it quickly.

> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > > ---
> > >
> > > Bikeshedding on the IPACameraData and IPAContext::data names is welcome
> > > :-)
> 
> I would like to take advantage of this opportunity and propose a small change :)
> I think "data" is too general. If it is intended for storing camera data, then
> We can just name it "camera" or "cameraData", so the typical usage will clearly
> state the data hierarchy:
> 
>     context.camera.lens.minFocusPosition

Good idea.

> The naming is secondary matter, and the logic part of the patch looks good,
> so I am fine with it even without the name change:
> 
> Reviewed-by: Daniel Semkowicz <dse at thaumatec.com>
> 
> > > ---
> > >  src/ipa/rkisp1/ipa_context.cpp | 13 +++++++++++++
> > >  src/ipa/rkisp1/ipa_context.h   |  4 ++++
> > >  src/ipa/rkisp1/rkisp1.cpp      |  2 +-
> > >  3 files changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > index 9bbf368432fa..986b955ea29d 100644
> > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > @@ -14,6 +14,15 @@
> > >
> > >  namespace libcamera::ipa::rkisp1 {
> > >
> > > +/**
> > > + * \struct IPACameraData
> > > + * \brief Camera data for the IPA module
> > > + *
> > > + * The camera data contains all information about the camera that remains
> > > + * constant for the lifetime of the IPA context. It is typically set during the
> > > + * init() operation of the IPA module.
> > > + */
> > > +
> > >  /**
> > >   * \struct IPASessionConfiguration
> > >   * \brief Session configuration for the IPA module
> > > @@ -337,6 +346,10 @@ namespace libcamera::ipa::rkisp1 {
> > >   * \struct IPAContext
> > >   * \brief Global IPA context data shared between all algorithms
> > >   *
> > > + * \var IPAContext::data
> > > + * \brief The IPA camera data, initialized at init time and immutable for the
> > > + * lifetime of the context
> > > + *
> > >   * \var IPAContext::configuration
> > >   * \brief The IPA session configuration, immutable during the session
> > >   *
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index b9b2065328d6..53b79110c932 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -20,6 +20,9 @@ namespace libcamera {
> > >
> > >  namespace ipa::rkisp1 {
> > >
> > > +struct IPACameraData {
> > > +};
> > > +
> > >  struct IPASessionConfiguration {
> > >       struct {
> > >               struct rkisp1_cif_isp_window measureWindow;
> > > @@ -143,6 +146,7 @@ struct IPAFrameContext : public FrameContext {
> > >  };
> > >
> > >  struct IPAContext {
> > > +     IPACameraData data;
> > >       IPASessionConfiguration configuration;
> > >       IPAActiveState activeState;
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 6544c925ba25..2fe66d34b6d4 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -111,7 +111,7 @@ const ControlInfoMap::Map rkisp1Controls{
> > >  } /* namespace */
> > >
> > >  IPARkISP1::IPARkISP1()
> > > -     : context_({ {}, {}, { kMaxFrameContexts } })
> > > +     : context_({ {}, {}, {}, { kMaxFrameContexts } })
> > >  {
> > >  }
> > >
> > >
> > > base-commit: 683c6da83f078d09fc020d2b48a4abe471853b2b

-- 
Regards,

Laurent Pinchart