[libcamera-devel] [RFC PATCH v1] libcamera: isp: Add ISP class
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue Sep 28 12:18:35 CEST 2021
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.
>
> 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.
>
> > >
> > > >> ---
> > > >> 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?
We need another one for statistics buffers.
> > > *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.
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