[libcamera-devel] [RFC PATCH v1] libcamera: isp: Add ISP class
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Wed Sep 29 10:02:54 CEST 2021
Hi Siyuan,
On Wed, Sep 29, 2021 at 03:14:53PM +0800, siyuan.fan wrote:
> Hi paul,
>
> On Tue, 28 Sep 2021 19:18:35 +0900
> paul.elder at ideasonboard.com wrote:
>
> > Hi Siyuan,
> >
> > Sorry for the delay.
> >
> > On Wed, Sep 15, 2021 at 08:56:43PM +0800, Siyuan Fan wrote:
> > > Hi paul,
> > >
> > > On Fri, 3 Sep 2021 18:56:43 +0900
> > > paul.elder at ideasonboard.com wrote:
> > >
> > > > Hi Siyuan,
> > > >
> > > > On Fri, Sep 03, 2021 at 10:57:01AM +0800, Fan Siyuan wrote:
> > > > > Hi paul,
> > > > >
> > > > > Thu, Sep 2, 2021 at 02:26 PM paul.elder
> > > > > <paul.elder at ideasonboard.com> wrote:
> > > > > >Hi Siyuan,
> > > > >
> > > > > >Thank you for the patch.
> > > > >
> > > > > >On Wed, Sep 01, 2021 at 02:59:08PM +0100, Siyuan Fan wrote:
> > > > > >> From: Fan Siyuan <siyuan.fan at foxmail.com>
> > > > > >>
> > > > > >> The ISP class is a abstract base class of software ISP. It
> > > > > >> includes image format configuration, ISP algorithm parameters
> > > > > >> parser, pixel processing image export and thread
> > > > > >> configuration.
> > > > > >>
> > > > > >> This new class will be used as the basic class for ISPCPU and
> > > > > >> ISPGPU.
> > > > > >>
> > > > > >> Signed-off-by: Fan Siyuan <siyuan.fan at foxmail.com>
> > > > > >> ---
> > > > > >>
> > > > > >> ISP Parameters Tuning is a important part, so I've designed a
> > > > > >> parameters parser in order that users can call any algorithm
> > > > > >> combination by passing any number of tuple. Each tuple
> > > > > >> format is including algorithm name string and algorithm
> > > > > >> param, such as tuple<string, int> for black level correct.
> > > > > >> Currently this function is only a demo. I'm thus sending it
> > > > > >> as an RFC.
> > > > >
> > > > > >I don't quite see how you imagine parse() to be used. Do you
> > > > > >"configure" the function that you want to run the ISP with
> > > > > >first, and then tell it to process it? What's wrong with
> > > > > >passing the list of command-parameter pairs into the
> > > > > >processing function directly?
> > > > > For parse(), we just call it before processing(). In
> > > > > processing(), we won't pass the param list. If we pass the list
> > > > > of parameter directly, the processing function should be
> > > > > template function. Maybe this makes function look more
> > > > > complicated.
> > > >
> > > > I think it would be better to pass all the parameters in
> > > > processing() directly. processing() doesn't necessarily have to
> > > > become a template function, it could take a template collection,
> > > > like std::vector<ISPControl>, or libcamera::ControlList.
> > > >
> > > > I wonder if it might be better to just leverage the ControlList
> > > > that we already have... afaik we already have properties and
> > > > controls that are in separate ControlId spaces; maybe we could
> > > > actually add an ISPControl ControlId space?
> > > >
> > > > >
> > > > > >I think that the processing function should also return
> > > > > >statistics. For a CPU ISP it's not unreasonable for the control
> > > > > >and processing to be together, but remember that this interface
> > > > > >must be usable for a GPU ISP as well, which would have separate
> > > > > >control and processing.
> > > > > sorry, I don't understand why processing() should return
> > > > > statistics. In my view,
> > > > > the current ISP design needs to separate statistics
> > > > > calibrated/computed and pixel processing. In processing(), we
> > > > > just process pixel using param/statistics. Maybe there is no
> > > > > need to return statistics.
> > > >
> > > > The job of the ISP is to calculate statistics, and to do pixel
> > > > processing. That is a different from the job to determine *what
> > > > kind* of pixel processing to do based on the statistics. This job
> > > > is done by the 3A algorithms (AE, AF, AWB), which in libcamera we
> > > > call image processing algorithms (IPA). So the ISP calculates
> > > > statistics and gives them to the IPA, and the IPA tells the ISP
> > > > what kind of pixel processing to do based on the statistics.
> > > >
> > > > In theory, the CPU ISP could indeed do both. But then when you
> > > > implement the GPU ISP (or any other software ISP for that matter),
> > > > then the GPU ISP will have to copy the same 3A algorithms that
> > > > are in the CPU ISP. libcamera already has infrastructure for the
> > > > IPAs, so let's reuse that instead of making a new IPA class
> > > > specifically for software ISPs.
> > > >
> > > >
> > > > Paul
> > > >
> > >
> > > This gives a simple example how to use the processing() function.
> > > (Modify the param and return value type)
> > >
> > > In declaration:
> > > virtual ControlList* processing(ControlList *ISPControl, ...)
> > >
> > > In pipeline handler:
> > > ControlList ISPControl;
> > > ISPControl.set(controls::SensorBlackLevels, {16, 16, 16, 16});
> > > ISPControl.set(controls::Sharpness, 0.5);
> > > isp_.invokeMethod(&ISPCPU::processing, ..., &ISPControl, ...);
> >
> > It seems we've been overcomplicating this. Instead of a ControlList,
> > we can just have one big struct to contain everything. It's just
> > passing pointers so I guess space wasn't an issue, plus we're reusing
> > them like buffers.
> >
> > Then for enabling/disabling specific ISP functions, you can just have
> > a flag for every parameter.
>
> Fine, I will use struct instead of ControlList.
>
> >
> > >
> > > In CPUISP::processing():
> > > int32_t colorTem = autoWhiteBalance(FrameBuffer ...);
> > > ISPControl->set(controls::ColourGains, colorTem);
> > > return ISPControl;
> >
> > Computing AWB and setting the color gains is the job of the IPA, not
> > the ISP.
>
> emmm, if compute color temperature and color gains needs to be done in
> IPA, then there is no other part about awb in isp. I'm confusion about
> it.
The ISP receives an image and calculates statistics on it, such as color
averages. The ISP returns these statistics, which the IPA consumes. The
IPA uses these statistics to compute the color temperature and set the
color gains.
I'm looking at src/ipa/ipu3/algorithms/awb.cpp, I think that's fairly
easy to follow. The IPAIPU3 receives statistics from the pipeline
handler (who got it from the ISP), and the IPA passes it to the AWB in
Awb::process(). You can then see in Awb::calculateWBGains(), in
Awb::generateAwbStats() that the statistics are extracted from the stats
struct that came from the ISP, and Awb::awbGreyWorld() does the
computation.
>
>
> >
> > >
> > > > >
> > > > > >> ---
> > > > > >> include/libcamera/internal/isp.h | 67
> > > > > >> ++++++++++++++++++++++++++++++++ 1 file changed, 67
> > > > > >> insertions(+) create mode 100644
> > > > > >> include/libcamera/internal/isp.h
> > > > > >>
> > > > > >> diff --git a/include/libcamera/internal/isp.h
> > > > > >> b/include/libcamera/internal/
> > > > > isp.h
> > > > > >> new file mode 100644
> > > > > >> index 00000000..caab7050
> > > > > >> --- /dev/null
> > > > > >> +++ b/include/libcamera/internal/isp.h
> > > > > >> @@ -0,0 +1,67 @@
> > > > > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > >> +/*
> > > > > >> + * Copyright (C) 2021, Siyuan Fan <siyuan.fan at foxmail.com>
> > > > > >> + *
> > > > > >> + * isp.h - The software ISP abstract base class
> > > > > >> + */
> > > > > >> +#ifndef __LIBCAMERA_INTERNAL_ISP_H__
> > > > > >> +#define __LIBCAMERA_INTERNAL_ISP_H__
> > > > > >> +
> > > > > >> +#include <vector>
> > > > > >> +#include <memory>
> > > > > >> +#include <tuple>
> > > > > >> +
> > > > > >> +#include <libcamera/formats.h>
> > > > > >> +#include <libcamera/framebuffer.h>
> > > > > >> +#include <libcamera/geometry.h>
> > > > > >> +#include <libcamera/pixel_format.h>
> > > > > >> +
> > > > > >> +#include "libcamera/base/object.h"
> > > > > >> +#include "libcamera/base/signal.h"
> > > > > >> +
> > > > > >> +namespace libcamera{
> > > > > >> +
> > > > > >> +class ISP : public Object
> > > > > >> +{
> > > > > >> +public:
> > > > > >> + ISP() {}
> > > > > >> +
> > > > > >> + virtual ~ISP() {}
> > > > > >> +
> > > > > >> + template<class F, class...Ts, std::size_t...Is>
> > > > > >> + void for_each_in_tuple(const std::tuple<Ts...> &
> > > > > >> tuple, F func,
> > > > > std::index_sequence<Is...>) {
> > > > > >> +
> > > > > >> + return (void(func(std::get<Is>(tuple))),
> > > > > >> ...);
> > > > > >> + }
> > > > > >> +
> > > > > >> + template<class F, class...Ts>
> > > > > >> + void for_each_in_tuple(const std::tuple<Ts...> &
> > > > > >> tuple, F func) {
> > > > > >> + for_each_in_tuple(tuple, func,
> > > > > >> std::make_index_sequence
> > > > > <sizeof...(Ts)>());
> > > > > >> + }
> > > > > >> +
> > > > > >> + template<typename T, typename... Args>
> > > > > >> + void parse(const T &head, const Args &... rest)
> > > > > >> + {
> > > > > >> + for_each_in_tuple(head, [&](const auto &x) {
> > > > > >> + // parse parameters for diff
> > > > > >> algorithm
> > > > > >> + });
> > > > > >> + }
> > > > > >> +
> > > > > >> + virtual int configure(PixelFormat inputFormat,
> > > > > >> PixelFormat
> > > > > outputFormat, Size inputSize, Size outputSize) = 0;
> > > > > >> +
> > > > > >> + virtual void processing(FrameBuffer *srcBuffer,
> >
> > s/processing/process/
> >
> > And then we can feed the parameters directly to process()
>
> >
> > > > > >> FrameBuffer
> > > > > *dstBuffer,
> > > > > >> + int width, int height) = 0;
> > > > > >>
> >
> > We don't need width and height.
> >
> > > > > >> +
> > > > > >> + virtual int
> > > > > >> exportBuffers(std::vector<std::unique_ptr<FrameBuffer>>
> >
> > This is the function allocate image buffers, right?
>
> yeah.
>
> >
> > We need another one for statistics buffers.
>
> OK, I will do this.
>
> >
> > > > > *buffers,
> > > > > >> + unsigned int count, int
> > > > > >> width, int
> > > > > height) = 0;
> > > > > >> +
> > > > > >> + virtual void start() = 0;
> > > > > >> +
> > > > > >> + virtual void stop() = 0;
> > > > > >> +
> > > > > >> + Signal<FrameBuffer *, FrameBuffer *> ispCompleted;
> >
> > The ISP does two things: calculating statistics, and doing image
> > processing. We need one signal for each. I think it would be good to
> > have separate functions for calling them as well.
> >
>
> Maybe it would be a good idea for me to try to draw a isp flowchart,
> So we can discuss it at tonight’s meeting.
Yeah, that's a good idea.
Paul
> >
> > > > > >> +};
> > > > > >> +
> > > > > >> +} /* namespace libcamera */
> > > > > > +> +
> > > > > > +#endif /* __LIBCAMERA_INTERNAL_ISP_H__ */> +#endif /*
> > > > > __LIBCAMERA_INTERNAL_ISP_H__ */
> > > > > > -- > --
> > > > > > 2.20.1> 2.20.1
> > > > > > >
> > > > >
> > > >
> > >
> >
>
>
More information about the libcamera-devel
mailing list