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

[Laurent Pinchart]

Hi Siyuan,

Nice to see your first contribution to libcamera, welcome to the
project.

As this is an RFC series, I'll start by sharing comments on the
architecture, without focussing on implementation details.

As I understand it, the goal of this software ISP implementation is to
be used as a reference on platforms that don't have a hardware ISP,
paving the way for a GPU-based ISP implementation. As such, the simple
pipeline handler is the most interesting target. You've used the
Raspberry Pi as a test platform due to hardware availability constraints
if I'm not mistaken. This is fine for test purposes, and we can
integrate the software ISP with the simple pipeline handler in a
subsequen step.

This being said, I think that patch 1/3 should not get integrated as-is.
It's fine for test purpose, but it will compete with the Rasperry Pi
pipeline handler, and if both are compiled in, may prevent the Raspberry
Pi pipeline handler from being loaded.

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.

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.

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.

On Wed, Aug 04, 2021 at 08:33:44AM +0100, Siyuan Fan wrote:
> From: Fan Siyuan <siyuan.fan at foxmail.com>
> 
> This patch series adds support for CPU-based software ISP.
> 
> Patch 1/3 add software ISP-based pipeline handler. Patch 2/3 add 
> software ISP class. Patch 3/3 add all meson configure files.
> 
> This is a RFC patch because so far the buffer processed by pipeline
> cannot be output due to lack of metadata information which needs to
> apply the ISP friend class in class FrameBuffer just like V4L2VideoDevice. 
> 
> 
> Fan Siyuan (3):
>   pipeline: isp: The software ISP-based pipeline handler
>   pipeline: isp: The software ISP class
>   pipeline: isp: All meson configure files
> 
>  meson_options.txt                             |   2 +-
>  src/libcamera/pipeline/isp/isp.cpp            | 323 ++++++++++
>  src/libcamera/pipeline/isp/isp_processing.cpp | 593 ++++++++++++++++++
>  src/libcamera/pipeline/isp/isp_processing.h   |  62 ++
>  src/libcamera/pipeline/isp/meson.build        |   6 +
>  5 files changed, 985 insertions(+), 1 deletion(-)
>  create mode 100644 src/libcamera/pipeline/isp/isp.cpp
>  create mode 100644 src/libcamera/pipeline/isp/isp_processing.cpp
>  create mode 100644 src/libcamera/pipeline/isp/isp_processing.h
>  create mode 100644 src/libcamera/pipeline/isp/meson.build

-- 
Regards,

Laurent Pinchart