[libcamera-devel] [RFC PATCH v1] libcamera: isp: Add ISP class
Siyuan Fan
siyuan.fan at foxmail.com
Wed Sep 15 14:56:43 CEST 2021
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, ...);
In CPUISP::processing():
int32_t colorTem = autoWhiteBalance(FrameBuffer ...);
ISPControl->set(controls::ColourGains, colorTem);
return ISPControl;
Regards
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,
> > >> FrameBuffer
> > *dstBuffer,
> > >> + int width, int height) = 0;
> > >> +
> > >> + virtual int
> > >> exportBuffers(std::vector<std::unique_ptr<FrameBuffer>>
> > *buffers,
> > >> + unsigned int count, int
> > >> width, int
> > height) = 0;
> > >> +
> > >> + virtual void start() = 0;
> > >> +
> > >> + virtual void stop() = 0;
> > >> +
> > >> + Signal<FrameBuffer *, FrameBuffer *> ispCompleted;
> > >> +};
> > >> +
> > >> +} /* 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