[libcamera-devel] Re： [RFC PATCH v1 0/3] pipeline: isp: The software ISP Module

[Fan Siyuan]

Hi Laurent,


Thanks for your comments.


>This is fine for test purposes, and we can integrate the software ISP
>with the simple pipeline handler in a subsequen step.


Yeah, these patches will not be merged, and just implemented for test
purposes, which can make me familiar with the workflow of the 
pipeline handler. After finishing this, I will think about intergrating the
software ISP with simple pipeline handler. 

>I see that the implementation is multi-threaded, which is good, as the
>software processing is heavy. However, the thread should be moved to the
>ISP class. Generally speaking, the ISP class should expose an interface
>that resembles a hardware ISP, with functions called from the pipeline
>handler to queue work to the ISP, and signals being emitted later when
>the work completes. The work itself needs to be performed in a separate
>thread.

>Speaking of interfaces, I think we should also create a base abstract
>class that defines the software ISP API. This CPU-based implementation
>would then inherit from the base class, and a future GPU-based
>implementation would also do the same, exposing the same API. This will
>allow switching between the two implementations seamlessly, without any
>change in the pipeline handler.


Thanks for your advice. I will move the thread to ISP and move ISP out of 
pipeline to make ISP interface in the next version. The abstract class ISP will
be considered too.

>I've also looked at the ISP implementation itself, and I wonder if it
>shouldn't be restructure to match what a hardware ISP would do. ISPs
>typically compute statistics from the image, and process pixels in
>hardware. A CPU-based algorithm (implemented in libcamera as an IPA
>module) consumes those statistics and produces parameters for the pixel
>processing. Practically speaking, this means that, for instance, the
>autoWhiteBalance() function would need to be split in three parts:
>statistics calculation, computation of the colour gains from the
>statistics, and multiplication of each pixel by the colour gains. One
>reason why I think statistics computation should probably be split out
>is that the same statistics could probably be reused in different
>processing steps. It's also tempting to move the computation of the
>colour gains (and other processing parameters) to an IPA module, but
>that may be overkill, maybe having a clear split but keeping all the
>code in the ISP class would be better. In any case, different options
>should be considered, and we should pick the best one and explain the
>reasons.


I think spliting out the statistics parameters is a good idea. However, the
most important thing right now is to fix the pipeline handler. With this working,
I will consider the details how to split out isp algorithm later.

>Finally, a few miscellaneous comments about the implementation of the
>ISP class:

>- Internal functions should be private, to clearly differentiate between
> the public API and the internal implementation.

>- Memory management needs to be considered. You currently allocate a
> large scratch buffer (rgbData) in ISP::processing(), and I wonder if
> it would make more sense to perform processing line by line to only
> require line buffers. This would have an effect on cache usage, which
> needs to be considered too. It's an internal implementation decision
> though, so it won't affect the API.


I will consider these in the design process of isp class.

Regards,

Fan Siyuan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210804/8c22d866/attachment-0001.htm>