[libcamera-devel] apply methods in IPARPi

[David Plowman]

Hi Naush, Sebastian


On Thu, 6 May 2021 at 09:04, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi Sebastian,
>
> 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.

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. 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.

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. 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!

Thanks!
David

>
>>
>>
>> --
>>
>> 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,
> Naush
>
>
>>
>>
>>   Greetings,
>>   Sebastian