<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 6 May 2021 at 14:32, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
On Thu, May 06, 2021 at 09:45:38AM +0100, David Plowman wrote:<br>
> On Thu, 6 May 2021 at 09:04, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
> > On Wed, 5 May 2021 at 19:08, Sebastian Fricke <<a href="mailto:sebastian.fricke@posteo.net" target="_blank">sebastian.fricke@posteo.net</a>> wrote:<br>
> >><br>
> >> Hello Naushir and David,<br>
> >><br>
> >> I am currently performing a lot of investigation and preparatory work<br>
> >> for a few generalizations in form of an IPAFramework class, that reuses<br>
> >> some of the great concepts you introduced to libcamera. Along the way I<br>
> >> noticed, that you have implemented the apply methods (applyAGC, applyAWB,<br>
> >> etc.) as part of the IPARPi class.<br>
> >><br>
> >> I was wondering if it would be possible to move those to the controller<br>
> >> and specific algorithm implementation, as they mostly get the status from<br>
> >> the algorithm, perform a few calculations with it and insert the results<br>
> >> of those calculations into the ISP controls.<br>
> >><br>
> >> The upside of doing so, from my point of view, would be that we could<br>
> >> call `apply` similar to the way we call `initialize`, `process` and<br>
> >> `prepare`.<br>
> >> That way, I would be able to generalize the whole step of applying the<br>
> >> algorithm status to a set of controls, which can then be used for setting<br>
> >> the ISP controls.<br>
> >> Now, the reason why I ask you is that I want to be sure, that I didn't<br>
> >> miss a key concept.<br>
> ><br>
> ><br>
> > This was indeed something I did consider doing some time back, but<br>
> > never got to it.  There are a couple of considerations with this<br>
> > proposal that we need to address:<br>
> ><br>
> > 1) My original intention with keeping apply* out of the Controllers<br>
> > was to *somewhat* decouple the BCM2835 ISP parameters from the<br>
> > Controller algorithms.  I say somewhat because some algorithms do<br>
> > have to know the statistics structure defined in the bcm2835-isp.h<br>
> > header, so there is already a hardware dependency there.  Secondly,<br>
> > I wanted to try and not include any libcamera dependency in the<br>
> > Controller code.  We do now have libcamera logging in the controller<br>
> > source files, so again, this is also not strictly true.<br>
> ><br>
> > 2) applyAGC() is a special one in that it runs after<br>
> > Controller::Process() whereas all the others run after<br>
> > Controller::Prepare.  applyAGC() also runs on IPA::start() so this<br>
> > must be treated differently somehow.<br>
> ><br>
> > In a way, having a Controller::Apply() might be quite nice, if we<br>
> > are able to somehow add a HW abstraction layer (through templated<br>
> > functions perhaps?).  Or does it even matter? David, what are your<br>
> > thoughts on this?<br>
> <br>
> So I remain quite strongly in favour in keeping hardware specific code<br>
> away from the algorithms. As Naush says, they do get a bit mixed<br>
> together because of the statistics, but for the moment we have to live<br>
> with this. We could imagine a formal API which takes the statistics<br>
> from the hardware (whatever that is) and produces the numbers the<br>
> algorithms want, though sometimes you could imagine tweaking the<br>
> algorithm slightly might be better than going through lots of hoops<br>
> with statistics, so that's probably one to think about.<br>
<br>
Keeping the implementation of algorithms hardware-agnostic would be<br>
great, but it's a hard problem. We could imagine translating all<br>
statistics to a standard format before passing them to algorithms, but<br>
that will have a performance impact, and could also result in<br>
information loss as a format that would be a superset of all possible<br>
hardware features is likely impossible. We need to figure out some<br>
middleground, perhaps in the form of device-agnostic algorithm "pieces"<br>
that would be assembled by device-specific algorithm "implementations".<br>
There's room for research and clever designs.<br>
<br>
> Nonetheless, I really don't want more hardware knowledge getting into<br>
> the algorithms, nor would I want to see algorithms making direct calls<br>
> that will set hardware.<br>
<br>
Fully agreed on the second part, the IPA design explicitly forbids<br>
accessing the device from algorithms.<br></blockquote><div><br></div><div>I think the idea here is to allow Controller::Apply() to only populate the</div><div>ControlList items, and the IPA to send that ControlList to the PH to apply</div><div>the parameters as V4L2 controls, as is done today.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Similarly, I really don't want one algorithm<br>
> to make direct calls that affect another algorithm - this always<br>
> starts quite benignly but can ultimately tend towards spaghetti. The<br>
> rule that we have is that this is all communicated through metadata.<br>
<br>
While exception may sometimes be needed, I really want to follow that<br>
rule.<br>
<br>
> But I think that still leaves us with options. Controller::Apply()<br>
> sounds like a reasonable thing to have, so long as it doesn't touch<br>
> the algorithms and only uses the metadata that they produced. Perhaps<br>
> we need a "PreApply" and a "PostApply", to allow for them to operate<br>
> either before or after the ISP.<br>
> <br>
> One more point to consider is what we'd do with imaging pipelines that<br>
> have a different architecture. One obvious case would be the "straight<br>
> through" ISP where the image buffer goes straight from the CSI-2<br>
> receiver into the ISP and only reaches memory after that. Another<br>
> variant on that might be a hybrid architecture, where some processing<br>
> happens in this "straight through" manner, but then there's a "main"<br>
> ISP which runs memory-to-memory.<br>
<br>
The former is what the rkisp1 implements, the latter is also needed to<br>
support new platforms. We usually call the "straight through" part the<br>
"inline" ISP, and the memory-to-memory part the "offline" ISP. There<br>
will usually be a single inline ISP, but we could have multiple offline<br>
components (hardware ISPs, general-purpose DSPs, GPUs, ...) in the<br>
pipeline.<br>
<br>
> How would that look? It's probably<br>
> worth giving these some consideration now, rather than inventing<br>
> something for our current hardware platforms and finding ourselves in<br>
> a pickle later!<br>
> <br>
> >> --<br>
> >><br>
> >> Additionally, off-topic to the topic mentioned above. While<br>
> >> investigating the RPiStream abstraction, I noticed the following comment<br>
> >> within: `rpi_stream.cpp`:116<br>
> >> ```<br>
> >>   * A nullptr buffer implies an external stream, but no external<br>
> >>   * buffer has been supplied in the Request. So, pick one from the<br>
> >>   * availableBuffers_ queue.<br>
> >>   ```<br>
> >><br>
> >>   And within `rpi_steam.h`:139<br>
> >>   ```<br>
> >>   * A nullptr indicates any internal buffer can be used (from availableBuffers_),<br>
> >>   * whereas a valid pointer indicates an external buffer to be queued.<br>
> >>   ```<br>
> >><br>
> >>   Which confused me a little bit, could you enlighten me in this matter?<br>
> >>   Does a nullptr buffer imply an external stream and therefore an<br>
> >>   external buffer? Or does a nullptr buffer imply an internal buffer and<br>
> >>   therefore an internal stream?<br>
> ><br>
> ><br>
> > I can see why this caused some confusion :-)<br>
> ><br>
> > Firstly, my definitions of external and internal streams may need<br>
> > clarification.  The former is a stream that *may* be exported to the<br>
> > application, and the latter is a stream that can never be exported<br>
> > to the application.  For external streams, the buffers used may be<br>
> > provided by the application, and these buffers will be returned back<br>
> > to the application once they have been processed.  However, we can<br>
> > also use internally allocated buffers for external streams that will<br>
> > never go back to the application.  This is needed when:<br>
> ><br>
> > 1) The application has not provided an external buffer to use for<br>
> > that stream in a Request.  Example of this is when we have a RAW<br>
> > stream set to external, but that application will only request a RAW<br>
> > image once every N frames.<br>
> > 2) If we need to drop frames as instructed by the IPA, we cannot use<br>
> > external buffers as they will eventually be dropped and not returned<br>
> > to the application.  In this case, we queue an internal buffer that<br>
> > can be dropped safely.  The alternative is to requeue external<br>
> > buffers once dropped, but will add a ton of latency to the capture<br>
> > cycle.<br>
> ><br>
> > Hope that helps clarify things a bit.<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>