[libcamera-devel] [RFC PATCH v1] libcamera: isp: Add ISP class
siyuan.fan
siyuan.fan at foxmail.com
Wed Sep 29 11:24:17 CEST 2021
Hi paul,
On Wed, 29 Sep 2021 17:02:54 +0900
paul.elder at ideasonboard.com wrote:
> 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.
Well, thanks for your explanation! My previous understanding of
statistics is not accurate, e.x. I will regard color temperature as a
statistic. So is there a standard explanation/doc about statistics for
each part of the isp pipeline?
Siyuan
>
> >
> >
> > >
> > > >
> > > > > >
> > > > > > >> ---
> > > > > > >> 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