[{"id":20451,"web_url":"https://patchwork.libcamera.org/comment/20451/","msgid":"<163519152489.1095920.4983489950243604360@Monstersaurus>","date":"2021-10-25T19:52:04","subject":"Re: [libcamera-devel] [RFC PATCH v2] libcamera: isp: Add ISP class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Siyuan,\n\nQuoting Siyuan Fan (2021-10-11 13:48:24)\n> From: Fan Siyuan \n> \n> This version adds the isp statistics struct and two signals, which should\n> be connected to corresponding seperate functions.\n\ns/seperate/separate/\n\nMy teachers always used to tell me \"Watch out! Separate has a-rat in it\" :-D\n\n\n> Signed-off-by: Fan Siyuan \n> ---\n> include/libcamera/internal/isp.h | 102 +++++++++++++++++++++++++++++++\n> 1 file changed, 102 insertions(+)\n> create mode 100644 include/libcamera/internal/isp.h\n> \n> diff --git a/include/libcamera/internal/isp.h b/include/libcamera/internal/isp.h\n> new file mode 100644\n> index 00000000..4b4c38e7\n> --- /dev/null\n> +++ b/include/libcamera/internal/isp.h\n> @@ -0,0 +1,102 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Siyuan Fan \n> + *\n> + * isp.h - The software ISP abstract base class\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_ISP_H__\n> +#define __LIBCAMERA_INTERNAL_ISP_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n\nI think if the FrameBuffer objects are all pointers, this could be a\nforward declaration:\n\nclass FrameBuffer; (under namespace libcamera { below) instead of\npulling in the header.\n\n> +#include \n> +#include \n> +\n> +#include \"libcamera/base/object.h\"\n> +#include \"libcamera/base/signal.h\"\n> +\n> +namespace libcamera{\n\ns/libcamera{/libcamera {/\n\n> +\n> +class ISP : public Object\n> +{\n> +public:\n> + ISP() {}\n> +\n> + virtual ~ISP() {}\n> +\n> + virtual int configure(PixelFormat &inputFormat, PixelFormat &outputFormat, Size &inputSize, Size &outputSize) = 0;\n> +\n> + virtual void processing(struct isp_stats *stats, FrameBuffer *srcBuffer, FrameBuffer *dstBuffer, int width, int height) = 0;\n> +\n> + virtual void calculating(struct isp_stats* stats, FrameBuffer *srcBuffer) = 0;\n> +\n> + virtual int exportBuffers(std::vector> *buffers, unsigned int count, int width, int height) = 0;\n> +\n> + virtual int exportStats(struct isp_stats *stats) = 0;\n> +\n> + virtual void start() = 0;\n> +\n> + virtual void stop() = 0;\n\nSeeing this without much context, it's hard to comment much as it's hard\nto know what usage all of these fucntions will have.\n\nIs there any documentation already written to support these interfaces?\n\n> +\n> + Signal ispCompleted;\n\nI know the variable names are optional, but I assume this is source and\ndestination? I think adding the variable names would provide a bit more\ncontext.\n\n\n\n> + Signal calcStats;\n> + Signal imgProcess;\n> +\n> + struct isp_black_level {\n> + __u32 enabled;\n> + __u16 bl_r;\n> + __u16 bl_gr;\n> + __u16 bl_gb;\n> + __u16 bl_b;\n\nDo these types need a specific header to define them? or are they built\nin with the compilers?\n\nShould we use something from the c++ std types? or are these used to be\ncompatible with something in particular?\nhttps://en.cppreference.com/w/cpp/types/integer:\n\t\n\tDefined in header \n\n\tuint8_t, uint16_t, uint32_t, uint64_t\n\tunsigned integer type with width of exactly 8, 16, 32 and 64 bits respectively\n\t(provided if and only if the implementation directly supports the type)\n\n--\nRegards\n\nKieran\n\n\n> + };\n> +\n> + struct isp_lens_shading {\n> + __u32 enabled;\n> + __u16 grid[16][16];\n> + };\n> +\n> + struct isp_denoise_raw {\n> + __u32 enabled;\n> + };\n> +\n> + struct isp_auto_white_balance {\n> + __u32 enabled;\n> + __u64 r_mean;\n> + __u64 g_mean;\n> + __u64 b_mean;\n> + };\n> +\n> + struct isp_gamma_correct {\n> + __u32 enabled;\n> + __u16 gammaX[33];\n> + __u16 gammaY[33];\n> + };\n> +\n> + struct isp_color_correct {\n> + __u32 enabled;\n> + __s32 ccm[3][3];\n> + };\n> +\n> + struct isp_histogram {\n> + __u32 r_hist[1024];\n> + __u32 g_hist[1024];\n> + __u32 b_hist[1024];\n> + };\n> +\n> + struct isp_stats {\n> + struct isp_black_level isp_bl;\n> + struct isp_lens_shading isp_ls;\n> + struct isp_denoise_raw isp_dr;\n> + struct isp_auto_white_balance isp_awb;\n> + struct isp_gamma_correct isp_gc;\n> + struct isp_color_correct isp_cc;\n> + struct isp_histogram isp_hist;\n> + }; \n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_ISP_H__ */\n> -- \n> 2.20.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4DEB2BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 19:52:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C9396487A;\n\tMon, 25 Oct 2021 21:52:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80FE360125\n\tfor ;\n\tMon, 25 Oct 2021 21:52:07 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F190E0A;\n\tMon, 25 Oct 2021 21:52:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ksM1oA++\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635191527;\n\tbh=GoeIkwbqmHKtG6FZX0Nf9J0WTaCSt3bTlVgvawCLC5k=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=ksM1oA++z3nRp3L4CEZnAvmRtft1vnxycd/rZShvi7CvjzOSnzRZNzT9PXKHirjtw\n\tpjeLgChxKgaBk2hQ3h1oUiKgaXE0NcRGIcAi8w97cFYQTbZYwtAuT/rCA6WeZl8QpH\n\te2l3rxozOsUcbXRkj958BrbhVQaNFtTAX2a5PLUw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"","From":"Kieran Bingham ","To":"Siyuan Fan , libcamera-devel@lists.libcamera.org","Date":"Mon, 25 Oct 2021 20:52:04 +0100","Message-ID":"<163519152489.1095920.4983489950243604360@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [RFC PATCH v2] libcamera: isp: Add ISP class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]