<meta http-equiv="Content-Type" content="text/html; charset=GB18030"><div>Hi paul,</div><div><br></div>Thu, Sep 2, 2021 at 02:26 PM  paul.elder <paul.elder@ideasonboard.com> wrote:<br><br>>Hi Siyuan,<br><br>>Thank you for the patch.<br><br>>On Wed, Sep 01, 2021 at 02:59:08PM +0100, Siyuan Fan wrote:<br>>> From: Fan Siyuan <siyuan.fan@foxmail.com><br>>> <br>>> The ISP class is a abstract base class of software ISP. It includes image<br>>> format configuration, ISP algorithm parameters parser, pixel processing<br>>> image export and thread configuration.<br>>> <br>>> This new class will be used as the basic class for ISPCPU and ISPGPU.<br>>> <br>>> Signed-off-by: Fan Siyuan <siyuan.fan@foxmail.com><br>>> ---<br>>> <br>>> ISP Parameters Tuning is a important part, so I've designed a parameters<br>>> parser in order that users can call any algorithm combination by passing<br>>> any number of tuple. Each tuple format is including algorithm name string<br>>> and algorithm param, such as tuple<string, int> for black level correct. <br>>> Currently this function is only a demo. I'm thus sending it as an RFC.<br><br>>I don't quite see how you imagine parse() to be used. Do you "configure"<br>>the function that you want to run the ISP with first, and then tell it<br>>to process it? What's wrong with passing the list of command-parameter<br>>pairs into the processing function directly?<br>For parse(), we just call it before processing(). In processing(), we won't<div>pass the param list. If we pass the list of parameter directly, the processing</div><div>function should be template function. Maybe this makes  function look</div><div>more complicated.</div><div><div><br>>I think that the processing function should also return statistics. For<br>>a CPU ISP it's not unreasonable for the control and processing to be<br>>together, but remember that this interface must be usable for a GPU ISP<br>>as well, which would have separate control and processing.<br>sorry, I don't understand why processing() should return statistics. In my view,</div><div>the current ISP design needs to separate statistics calibrated/computed</div><div>and pixel processing. In processing(), we just process pixel using</div><div>param/statistics. Maybe there is no need to return statistics.<br><br>>Paul<br><br>>> ---<br>>>  include/libcamera/internal/isp.h | 67 ++++++++++++++++++++++++++++++++<br>>>  1 file changed, 67 insertions(+)<br>>>  create mode 100644 include/libcamera/internal/isp.h<br>>> <br>>> diff --git a/include/libcamera/internal/isp.h b/include/libcamera/internal/isp.h<br>>> new file mode 100644<br>>> index 00000000..caab7050<br>>> --- /dev/null<br>>> +++ b/include/libcamera/internal/isp.h<br>>> @@ -0,0 +1,67 @@<br>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>>> +/*<br>>> + * Copyright (C) 2021, Siyuan Fan <siyuan.fan@foxmail.com><br>>> + *<br>>> + * isp.h - The software ISP abstract base class<br>>> + */<br>>> +#ifndef __LIBCAMERA_INTERNAL_ISP_H__<br>>> +#define __LIBCAMERA_INTERNAL_ISP_H__<br>>> +<br>>> +#include <vector><br>>> +#include <memory><br>>> +#include <tuple><br>>> +<br>>> +#include <libcamera/formats.h><br>>> +#include <libcamera/framebuffer.h><br>>> +#include <libcamera/geometry.h><br>>> +#include <libcamera/pixel_format.h><br>>> +<br>>> +#include "libcamera/base/object.h"<br>>> +#include "libcamera/base/signal.h"<br>>> +<br>>> +namespace libcamera{<br>>> +<br>>> +class ISP : public Object<br>>> +{<br>>> +public:<br>>> +        ISP() {}<br>>> +<br>>> +        virtual ~ISP() {}<br>>> +<br>>> +        template<class F, class...Ts, std::size_t...Is><br>>> +        void for_each_in_tuple(const std::tuple<Ts...> & tuple, F func, std::index_sequence<Is...>) {<br>>> +   <br>>> +                return (void(func(std::get<Is>(tuple))), ...);<br>>> +        }<br>>> +<br>>> +        template<class F, class...Ts><br>>> +        void for_each_in_tuple(const std::tuple<Ts...> & tuple, F func) {<br>>> +                for_each_in_tuple(tuple, func, std::make_index_sequence<sizeof...(Ts)>());<br>>> +        }<br>>> +<br>>> +        template<typename T, typename... Args><br>>> +        void parse(const T &head, const Args &... rest)<br>>> +        {<br>>> +                for_each_in_tuple(head, [&](const auto &x) {<br>>> +                        // parse parameters for diff algorithm<br>>> +                });<br>>> +        }<br>>> +<br>>> +        virtual int configure(PixelFormat inputFormat, PixelFormat outputFormat, Size inputSize, Size outputSize) = 0;<br>>> +<br>>> +        virtual void processing(FrameBuffer *srcBuffer, FrameBuffer *dstBuffer,<br>>> +                                int width, int height) = 0;<br>>> +<br>>> +        virtual int exportBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers,<br>>> +                                  unsigned int count, int width, int height) = 0;<br>>> +<br>>> +        virtual void start() = 0;<br>>> +<br>>> +        virtual void stop() = 0;<br>>> +<br>>> +        Signal<FrameBuffer *, FrameBuffer *> ispCompleted;<br>>> +};<br>>> +<br>>> +} /* namespace libcamera */<br>> +> +<br>> +#endif /* __LIBCAMERA_INTERNAL_ISP_H__ */> +#endif /* __LIBCAMERA_INTERNAL_ISP_H__ */<br>> -- > -- <br>> 2.20.1> 2.20.1<br>> > <br><br></div></div>