[libcamera-devel] [RFC PATCH v1] libcamera: isp: Add ISP class
Siyuan Fan
siyuan.fan at foxmail.com
Tue Sep 14 09:38:56 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
Thanks for your advice, the namespace for libcamera controls is
enough. if not, maybe add other ISPControl in it is better.
Considering the interface for both CPU and GPU, the function
processing() should only calculate the statistics and ipa is
exec in another function. processing() just needs to receive
the calibrated statistics, such as black level, lens shading
table and so on. In processing(), other statistics calculated
by current frame, for example color gains. Finally, processing()
return all statistics.
So I will declare the processing like this:
virtual ControlList& processing(ControlList 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