[libcamera-devel] [RFC PATCH v1] libcamera: isp: Add ISP class
siyuan.fan
siyuan.fan at foxmail.com
Wed Sep 29 09:14:53 CEST 2021
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.
>
> >
> > > >
> > > > >> ---
> > > > >> 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.
Regards
Siyuan
>
> > > > >> +};
> > > > >> +
> > > > >> +} /* 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