[libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce IpaBase class

[Laurent Pinchart]

Hi Naush,

On Tue, May 02, 2023 at 03:54:16PM +0100, Naushir Patuck wrote:
> Hi Laurent and Jacopo,
> 
> On Fri, 28 Apr 2023 at 14:57, Naushir Patuck wrote:
> >
> > Hi Jacopo,
> 
>  <snip>
> 
> > > When it comes to the call order, it's the base class driving the
> > > procesing flow, while dispatching calls to the derived classes at the
> > > right time. Moving to your suggestion would allow derived classes to
> > > call the base class method, which means most of the control is moved
> > > to the derived classes with possible code duplication.
> > >
> > > I think to actually decide where to go, it is useful to analyze what
> > > class implements the operations flow control
> > >
> > > In one case
> > >
> > >         Base::method()                  Derived::platformMethod()
> > >
> > >                 doSomeWork();
> > >                 ....
> > >                 platformMethod() -->    doPlaformWork();
> > >                 ...
> > >                 finalizeWork();
> > >
> > > While in the other
> > >
> > >         Base::method()                  Derived::method()
> > >
> > >                                         doSomeWork();
> > >                                         ....
> > >                 method()        <--     Base::method()
> > >                                         ...
> > >                                         finalizeWork();
> > >
> > > Is this an over-simplification ?
> > >
> > > Naush, do you expect the derived classes to control the operation
> > > flow, or will this be common to both and could be implemented in the
> > > base classes ?
> >
> > No, I see no explicit need for operation flow to be dictated by the derived
> > class.  Hence changing things around to your suggestion and making things more
> > consistent.
> 
> Having looked further into this, I'm a bit more conflicted.  I initially thought
> that I could simply revert to the original mechanism of having the control flow
> start from the derived class and call into the base class, as the 2nd case
> drawn above.  This does work neatly for the ipa::init() and ipa::configure().
> However platformProcessStats() and platformPrepareIsp() require the control flow
> from base class to derived class, as in the 1st case drawn above.
> 
> I don't think it's sensible to choose flow 1 for some functions and flow 2 for
> others.  So that suggests to me that flow 1 (i.e. the existing flow) is probably
> the right approach for the IPA changes.  Do folks think that makes sense?
> Again, I am happy to change things if it seems more appropriate.

As mentioned in a different conversation, I'm fine keeping the existing
code flow, even if I think it would be best to reverse it. We can
improve this incrementally on top.

-- 
Regards,

Laurent Pinchart