[libcamera-devel] apply methods in IPARPi

[Laurent Pinchart]

Hello,

On Thu, May 06, 2021 at 09:45:38AM +0100, David Plowman wrote:
> On Thu, 6 May 2021 at 09:04, Naushir Patuck <naush at raspberrypi.com> wrote:
> > On Wed, 5 May 2021 at 19:08, Sebastian Fricke <sebastian.fricke at posteo.net> wrote:
> >>
> >> Hello Naushir and David,
> >>
> >> I am currently performing a lot of investigation and preparatory work
> >> for a few generalizations in form of an IPAFramework class, that reuses
> >> some of the great concepts you introduced to libcamera. Along the way I
> >> noticed, that you have implemented the apply methods (applyAGC, applyAWB,
> >> etc.) as part of the IPARPi class.
> >>
> >> I was wondering if it would be possible to move those to the controller
> >> and specific algorithm implementation, as they mostly get the status from
> >> the algorithm, perform a few calculations with it and insert the results
> >> of those calculations into the ISP controls.
> >>
> >> The upside of doing so, from my point of view, would be that we could
> >> call `apply` similar to the way we call `initialize`, `process` and
> >> `prepare`.
> >> That way, I would be able to generalize the whole step of applying the
> >> algorithm status to a set of controls, which can then be used for setting
> >> the ISP controls.
> >> Now, the reason why I ask you is that I want to be sure, that I didn't
> >> miss a key concept.
> >
> >
> > This was indeed something I did consider doing some time back, but
> > never got to it.  There are a couple of considerations with this
> > proposal that we need to address:
> >
> > 1) My original intention with keeping apply* out of the Controllers
> > was to *somewhat* decouple the BCM2835 ISP parameters from the
> > Controller algorithms.  I say somewhat because some algorithms do
> > have to know the statistics structure defined in the bcm2835-isp.h
> > header, so there is already a hardware dependency there.  Secondly,
> > I wanted to try and not include any libcamera dependency in the
> > Controller code.  We do now have libcamera logging in the controller
> > source files, so again, this is also not strictly true.
> >
> > 2) applyAGC() is a special one in that it runs after
> > Controller::Process() whereas all the others run after
> > Controller::Prepare.  applyAGC() also runs on IPA::start() so this
> > must be treated differently somehow.
> >
> > In a way, having a Controller::Apply() might be quite nice, if we
> > are able to somehow add a HW abstraction layer (through templated
> > functions perhaps?).  Or does it even matter? David, what are your
> > thoughts on this?
> 
> So I remain quite strongly in favour in keeping hardware specific code
> away from the algorithms. As Naush says, they do get a bit mixed
> together because of the statistics, but for the moment we have to live
> with this. We could imagine a formal API which takes the statistics
> from the hardware (whatever that is) and produces the numbers the
> algorithms want, though sometimes you could imagine tweaking the
> algorithm slightly might be better than going through lots of hoops
> with statistics, so that's probably one to think about.

Keeping the implementation of algorithms hardware-agnostic would be
great, but it's a hard problem. We could imagine translating all
statistics to a standard format before passing them to algorithms, but
that will have a performance impact, and could also result in
information loss as a format that would be a superset of all possible
hardware features is likely impossible. We need to figure out some
middleground, perhaps in the form of device-agnostic algorithm "pieces"
that would be assembled by device-specific algorithm "implementations".
There's room for research and clever designs.

> Nonetheless, I really don't want more hardware knowledge getting into
> the algorithms, nor would I want to see algorithms making direct calls
> that will set hardware.

Fully agreed on the second part, the IPA design explicitly forbids
accessing the device from algorithms.

> Similarly, I really don't want one algorithm
> to make direct calls that affect another algorithm - this always
> starts quite benignly but can ultimately tend towards spaghetti. The
> rule that we have is that this is all communicated through metadata.

While exception may sometimes be needed, I really want to follow that
rule.

> But I think that still leaves us with options. Controller::Apply()
> sounds like a reasonable thing to have, so long as it doesn't touch
> the algorithms and only uses the metadata that they produced. Perhaps
> we need a "PreApply" and a "PostApply", to allow for them to operate
> either before or after the ISP.
> 
> One more point to consider is what we'd do with imaging pipelines that
> have a different architecture. One obvious case would be the "straight
> through" ISP where the image buffer goes straight from the CSI-2
> receiver into the ISP and only reaches memory after that. Another
> variant on that might be a hybrid architecture, where some processing
> happens in this "straight through" manner, but then there's a "main"
> ISP which runs memory-to-memory.

The former is what the rkisp1 implements, the latter is also needed to
support new platforms. We usually call the "straight through" part the
"inline" ISP, and the memory-to-memory part the "offline" ISP. There
will usually be a single inline ISP, but we could have multiple offline
components (hardware ISPs, general-purpose DSPs, GPUs, ...) in the
pipeline.

> How would that look? It's probably
> worth giving these some consideration now, rather than inventing
> something for our current hardware platforms and finding ourselves in
> a pickle later!
> 
> >> --
> >>
> >> Additionally, off-topic to the topic mentioned above. While
> >> investigating the RPiStream abstraction, I noticed the following comment
> >> within: `rpi_stream.cpp`:116
> >> ```
> >>   * A nullptr buffer implies an external stream, but no external
> >>   * buffer has been supplied in the Request. So, pick one from the
> >>   * availableBuffers_ queue.
> >>   ```
> >>
> >>   And within `rpi_steam.h`:139
> >>   ```
> >>   * A nullptr indicates any internal buffer can be used (from availableBuffers_),
> >>   * whereas a valid pointer indicates an external buffer to be queued.
> >>   ```
> >>
> >>   Which confused me a little bit, could you enlighten me in this matter?
> >>   Does a nullptr buffer imply an external stream and therefore an
> >>   external buffer? Or does a nullptr buffer imply an internal buffer and
> >>   therefore an internal stream?
> >
> >
> > I can see why this caused some confusion :-)
> >
> > Firstly, my definitions of external and internal streams may need
> > clarification.  The former is a stream that *may* be exported to the
> > application, and the latter is a stream that can never be exported
> > to the application.  For external streams, the buffers used may be
> > provided by the application, and these buffers will be returned back
> > to the application once they have been processed.  However, we can
> > also use internally allocated buffers for external streams that will
> > never go back to the application.  This is needed when:
> >
> > 1) The application has not provided an external buffer to use for
> > that stream in a Request.  Example of this is when we have a RAW
> > stream set to external, but that application will only request a RAW
> > image once every N frames.
> > 2) If we need to drop frames as instructed by the IPA, we cannot use
> > external buffers as they will eventually be dropped and not returned
> > to the application.  In this case, we queue an internal buffer that
> > can be dropped safely.  The alternative is to requeue external
> > buffers once dropped, but will add a ton of latency to the capture
> > cycle.
> >
> > Hope that helps clarify things a bit.

-- 
Regards,

Laurent Pinchart