[libcamera-devel,RFC,v2] libcamera: isp: Add ISP class

14083 diff mbox series

From: Fan Siyuan <siyuan.fan@foxmail.com>

This version adds the isp statistics struct and two signals, which should
be connected to corresponding seperate functions.

Signed-off-by: Fan Siyuan <siyuan.fan@foxmail.com>
---
 include/libcamera/internal/isp.h | 102 +++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)
 create mode 100644 include/libcamera/internal/isp.h

Hi Siyuan,

Quoting Siyuan Fan (2021-10-11 13:48:24)
> From: Fan Siyuan <siyuan.fan@foxmail.com>
> 
> This version adds the isp statistics struct and two signals, which should
> be connected to corresponding seperate functions.

s/seperate/separate/

My teachers always used to tell me "Watch out! Separate has a-rat in it" :-D


> Signed-off-by: Fan Siyuan <siyuan.fan@foxmail.com>
> ---
>  include/libcamera/internal/isp.h | 102 +++++++++++++++++++++++++++++++
>  1 file changed, 102 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..4b4c38e7
> --- /dev/null
> +++ b/include/libcamera/internal/isp.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Siyuan Fan <siyuan.fan@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 <libcamera/formats.h>
> +#include <libcamera/framebuffer.h>

I think if the FrameBuffer objects are all pointers, this could be a
forward declaration:

class FrameBuffer; (under namespace libcamera { below) instead of
pulling in the header.

> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include "libcamera/base/object.h"
> +#include "libcamera/base/signal.h"
> +
> +namespace libcamera{

s/libcamera{/libcamera {/

> +
> +class ISP : public Object
> +{
> +public:
> +        ISP() {}
> +
> +        virtual ~ISP() {}
> +
> +        virtual int configure(PixelFormat &inputFormat, PixelFormat &outputFormat, Size &inputSize, Size &outputSize) = 0;
> +
> +        virtual void processing(struct isp_stats *stats, FrameBuffer *srcBuffer, FrameBuffer *dstBuffer, int width, int height) = 0;
> +
> +        virtual void calculating(struct isp_stats* stats, FrameBuffer *srcBuffer) = 0;
> +
> +        virtual int exportBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers, unsigned int count, int width, int height) = 0;
> +
> +        virtual int exportStats(struct isp_stats *stats) = 0;
> +
> +        virtual void start() = 0;
> +
> +        virtual void stop() = 0;

Seeing this without much context, it's hard to comment much as it's hard
to know what usage all of these fucntions will have.

Is there any documentation already written to support these interfaces?

> +
> +        Signal<FrameBuffer *, FrameBuffer *> ispCompleted;

I know the variable names are optional, but I assume this is source and
destination? I think adding the variable names would provide a bit more
context.



> +        Signal<struct isp_stats *, FrameBuffer *> calcStats;
> +        Signal<struct isp_stats *, FrameBuffer *, FrameBuffer *, int, int> imgProcess;
> +
> +        struct isp_black_level {
> +            __u32 enabled;
> +            __u16 bl_r;
> +            __u16 bl_gr;
> +            __u16 bl_gb;
> +            __u16 bl_b;

Do these types need a specific header to define them? or are they built
in with the compilers?

Should we use something from the c++ std types? or are these used to be
compatible with something in particular?
https://en.cppreference.com/w/cpp/types/integer:
	
	Defined in header <cstdint>

	uint8_t, uint16_t, uint32_t, uint64_t
	unsigned integer type with width of exactly 8, 16, 32 and 64 bits respectively
	(provided if and only if the implementation directly supports the type)

--
Regards

Kieran


> +        };
> +
> +        struct isp_lens_shading {
> +            __u32 enabled;
> +            __u16 grid[16][16];
> +        };
> +
> +        struct isp_denoise_raw {
> +            __u32 enabled;
> +        };
> +
> +        struct isp_auto_white_balance {
> +            __u32 enabled;
> +            __u64 r_mean;
> +            __u64 g_mean;
> +            __u64 b_mean;
> +        };
> +
> +        struct isp_gamma_correct {
> +            __u32 enabled;
> +            __u16 gammaX[33];
> +            __u16 gammaY[33];
> +        };
> +
> +        struct isp_color_correct {
> +            __u32 enabled;
> +            __s32 ccm[3][3];
> +        };
> +
> +        struct isp_histogram {
> +            __u32 r_hist[1024];
> +            __u32 g_hist[1024];
> +            __u32 b_hist[1024];
> +        };
> +
> +        struct isp_stats {
> +            struct isp_black_level isp_bl;
> +            struct isp_lens_shading isp_ls;
> +            struct isp_denoise_raw isp_dr;
> +            struct isp_auto_white_balance isp_awb;
> +            struct isp_gamma_correct isp_gc;
> +            struct isp_color_correct isp_cc;
> +            struct isp_histogram isp_hist;
> +        };    
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_ISP_H__ */
> -- 
> 2.20.1
>