[{"id":18692,"web_url":"https://patchwork.libcamera.org/comment/18692/","msgid":"<20210811101516.GZ2167@pyrite.rasen.tech>","date":"2021-08-11T10:15:16","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/4] libcamera: swisp: The\n\tsoftware ISP class","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Siyuan,\n\nOn Wed, Aug 11, 2021 at 07:12:56AM +0100, Siyuan Fan wrote:\n> From: Fan Siyuan <siyuan.fan@foxmail.com>\n> \n>  Changes in V2:\n\nYou skipped all of my comments from v1, so I'll only comment on the\nISP interface.\n\n>  -Design the abstract class ISP, CPU_ISP and future GPU_ISP inherit from it.\n>  -Add the rgb buf alloc and thread control member function\n> \n>  Now the ISP must implement the below process:\n>  BLC-->demosaic-->AWB-->tone mapping-->gamma-->Noise reduction\n> \n>  Plan to add:\n>  lens shading correct-->Color correct-->color space transform\n> \n> Signed-off-by: Fan Siyuan <siyuan.fan@foxmail.com>\n> ---\n>  src/libcamera/swisp/isp.cpp | 643 ++++++++++++++++++++++++++++++++++++\n>  src/libcamera/swisp/isp.h   |  92 ++++++\n>  2 files changed, 735 insertions(+)\n>  create mode 100644 src/libcamera/swisp/isp.cpp\n>  create mode 100644 src/libcamera/swisp/isp.h\n> \n> diff --git a/src/libcamera/swisp/isp.cpp b/src/libcamera/swisp/isp.cpp\n> new file mode 100644\n> index 00000000..6d073040\n> --- /dev/null\n> +++ b/src/libcamera/swisp/isp.cpp\n> @@ -0,0 +1,643 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Siyuan Fan <siyuan.fan@foxmail.com>\n> + *\n> + * isp.cpp - The software ISP class\n> + */\n> +\n> +#include \"isp.h\"\n> +\n> +#include <math.h>\n> +#include <stdlib.h>\n> +#include <string.h>\n> +#include <sys/mman.h>\n> +#include <unistd.h>\n> +\n> +#include <libcamera/request.h>\n> +#include <libcamera/file_descriptor.h>\n> +\n> +#include \"libcamera/base/log.h\"\n> +\n> +namespace libcamera{\n> +\n> +LOG_DECLARE_CATEGORY(ISP)\n> +\n> +void CPU_ISP::autoContrast(uint16_t *data, float lowCut, float highCut, int width, int height)\n> +{\n> +        int blue, gr, gb, red;\n> +        int histBlue[1024] = {0}, histGb[1024] = {0}, histGr[1024] = {0}, histRed[1024] = {0};\n> +\n> +        int index;\n> +        for (int i = 0; i < height; i++) {\n> +                index = i * width;\n> +                for (int j = 0; j < width; j++) {\n> +                        if (i % 2 == 0 && j % 2 == 0) {\n> +                                blue = data[index];\n> +                                histBlue[blue]++;\n> +                        }\n> +                        else if ((i % 2 == 0 && j % 2 == 1)) {\n> +                                gb = data[index];\n> +                                histGb[gb]++;\n> +                        }\n> +                        else if ((i % 2 == 1 && j % 2 == 0)) {\n> +                                gr = data[index];\n> +                                histGr[gr]++;\n> +                        }\n> +                        else {\n> +                                red = data[index];\n> +                                histRed[red]++;\n> +                        }\n> +                        index++;\n> +                }    \n> +        }\n> +\n> +        int pixelAmount = width * height;\n> +        int sum = 0;\n> +        int minBlue;\n> +        for (int i = 0; i < 1024; i++){\n> +                sum = sum + histBlue[i];\n> +                if (sum >= pixelAmount * lowCut * 0.01) {\n> +                        minBlue = i;\n> +                        break;\n> +                }\n> +        }\n> +\n> +        sum = 0;\n> +        int maxBlue;\n> +        for (int i = 1023; i >= 0; i--){\n> +                sum = sum + histBlue[i];\n> +                if (sum >= pixelAmount * highCut * 0.01) {\n> +                        maxBlue = i;\n> +                        break;\n> +                }\n> +        }\n> +\n> +        sum = 0;\n> +        int minGb;\n> +        for (int i = 0; i < 1024; i++){\n> +                sum = sum + histGb[i];\n> +                if (sum >= pixelAmount * lowCut * 0.01) {\n> +                        minGb = i;\n> +                        break;\n> +                }\n> +        }\n> +\n> +        sum = 0;\n> +        int maxGb;\n> +        for (int i = 1023; i >= 0; i--){\n> +                sum = sum + histGb[i];\n> +                if (sum >= pixelAmount * highCut * 0.01) {\n> +                        maxGb = i;\n> +                        break;\n> +                }\n> +        }\n> +\n> +        sum = 0;\n> +        int minGr;\n> +        for (int i = 0; i < 1024; i++){\n> +                sum = sum + histGr[i];\n> +                if (sum >= pixelAmount * lowCut * 0.01) {\n> +                        minGr = i;\n> +                        break;\n> +                }\n> +        }\n> +\n> +        sum = 0;\n> +        int maxGr;\n> +        for (int i = 1023; i >= 0; i--){\n> +                sum = sum + histGr[i];\n> +                if (sum >= pixelAmount * highCut * 0.01) {\n> +                        maxGr = i;\n> +                        break;\n> +                }\n> +        }\n> +\n> +        sum = 0;\n> +        int minRed;\n> +        for (int i = 0; i < 1024; i++){\n> +                sum = sum + histRed[i];\n> +                if (sum >= pixelAmount * lowCut * 0.01) {\n> +                        minRed = i;\n> +                        break;\n> +                }\n> +        }\n> +\n> +        sum = 0;\n> +        int maxRed;\n> +        for (int i = 1023; i >= 0; i--){\n> +                sum = sum + histRed[i];\n> +                if (sum >= pixelAmount * highCut * 0.01) {\n> +                        maxRed = i;\n> +                        break;\n> +                }\n> +        }\n> +\n> +        int blueMap[1024];\n> +        float norb = 1.0 / (maxBlue - minBlue);\n> +        for (int i = 0; i < 1024; i++) {\n> +                if (i < minBlue) {\n> +                        blueMap[i] = 0;\n> +                }\n> +                else if (i > maxBlue) {\n> +                        blueMap[i] = 1023;\n> +                }\n> +                else {\n> +                        blueMap[i] = (i - minBlue) * norb * 1023;\n> +                }\n> +                if (blueMap[i] > 1023) blueMap[i] = 1023;\n> +        }\n> +\n> +        int gbMap[1024];\n> +        float norgb = 1.0 / (maxGb - minGb);\n> +        for (int i = 0; i < 1024; i++) {\n> +                if (i < minGb) {\n> +                        gbMap[i] = 0;\n> +                }\n> +                else if (i > maxGb) {\n> +                        gbMap[i] = 1023;\n> +                }\n> +                else {\n> +                        gbMap[i] = (i - minGb) * norgb * 1023;\n> +                }\n> +                if (gbMap[i] > 1023) gbMap[i] = 1023;\n> +        }\n> +\n> +        int grMap[1024];\n> +        float norgr = 1.0 / (maxGr - minGr);\n> +        for (int i = 0; i < 1024; i++) {\n> +                if (i < minGr) {\n> +                        grMap[i] = 0;\n> +                }\n> +                else if (i > maxGr) {\n> +                        grMap[i] = 1023;\n> +                }\n> +                else {\n> +                        grMap[i] = (i - minGr) * norgr * 1023;\n> +                }\n> +                if (grMap[i] > 1023) grMap[i] = 1023;\n> +        }\n> +\n> +        int redMap[1024];\n> +        float norr = 1.0 / (maxRed - minRed);\n> +        for (int i = 0; i < 1024; i++) {\n> +                if (i < minRed) {\n> +                        redMap[i] = 0;\n> +                }\n> +                else if (i > maxRed) {\n> +                        redMap[i] = 1023;\n> +                }\n> +                else{\n> +                        redMap[i] = (i - minRed) * norr * 1023;\n> +                }\n> +                if (redMap[i] > 1023) redMap[i] = 1023;\n> +        }\n> +\n> +        for (int i = 0;i < height; i++) {\n> +                for (int j = 0; j < width; j++){\n> +                        index = i * width;\n> +                        if (i % 2 == 0 && j % 2 == 0) {\n> +                                data[index] = blueMap[data[index]];\n> +                        }    \n> +                        else if (i % 2 == 0 && j % 2 == 1) {\n> +                                data[index] = gbMap[data[index]];\n> +                        }    \n> +                        else if (i % 2 == 1 && j % 2 == 0) {\n> +                                data[index] = grMap[data[index]];\n> +                        }    \n> +                        else {\n> +                                data[index] = redMap[data[index]];\n> +                        }\n> +                        index++;\n> +                }\n> +        }\n> +}\n> +\n> +void CPU_ISP::blackLevelCorrect(uint16_t *data, uint16_t offset, int width, int height)\n> +{\n> +        int len = width * height;\n> +        for(int i = 0; i < len; i++) {\n> +                if (data[i] < offset){\n> +                        data[i] = 0;\n> +                }\n> +                else {\n> +                        data[i] -= offset;\n> +                }\n> +        }\n> +}\n> +\n> +void CPU_ISP::readChannels(uint16_t *data, uint16_t *R, uint16_t *G, uint16_t *B,\n> +                       int width, int height)\n> +{\n> +        int index;\n> +        for (int i = 0; i < height; i++) {\n> +                index = i * width;\n> +                for (int j = 0; j < width; j++) {\n> +                        if (i % 2 == 0 && j % 2 == 0) {\n> +                                B[index] = data[index];\n> +                        }\n> +                        else if ((i % 2 == 0 && j % 2 == 1) || (i % 2 == 1 && j % 2 == 0)){\n> +                                G[index] = data[index];\n> +                        }\n> +                        else {\n> +                                R[index] = data[index];\n> +                        }\n> +                        index++;\n> +                }\n> +        }\n> +}\n> +\n> +void CPU_ISP::firstPixelInsert(uint16_t *src, uint16_t *dst, int width, int height)\n> +{\n> +        int index;\n> +        for (int i = 0; i < height; i++) {\n> +                index = i * width;\n> +                for (int j = 0; j < width; j++){\n> +                        if (i % 2 == 0 && j % 2 == 1) {\n> +                                if (j == (width - 1)) {\n> +                                        dst[index] = src[index - 1];\n> +                                }\n> +                                else {\n> +                                        dst[index] = (src[index - 1] +\n> +                                                src[index + 1]) >> 1;\n> +                                }\n> +                        }\n> +\n> +                        if (i % 2 == 1 && j % 2 == 0) {\n> +                                if(i == height - 1) {\n> +                                        dst[index] = src[index - width];\n> +                                }\n> +                                else {\n> +                                        dst[index] = (src[index - width]+\n> +                                                src[index + width]) >> 1;\n> +                                }\n> +                        }\n> +    \n> +                        if (i % 2 == 1 && j % 2 == 1) {\n> +                                if (j < width - 1 && i < height - 1) {\n> +                                        dst[index] = (src[index - width - 1] +\n> +                                                src[index - width + 1] +\n> +                                                src[index + width - 1] +\n> +                                                src[index + width + 1]) >> 2;\n> +                                }\n> +                                else if (i == height - 1 && j < width - 1) {\n> +                                        dst[index] = (src[index - width - 1] +\n> +                                                src[index - width + 1]) >> 1;\n> +                                }\n> +                                else if (i < height - 1 && j == width - 1) {\n> +                                        dst[index] = (src[index - width - 1] +\n> +                                                src[index + width - 1]) >> 1;\n> +                                }\n> +                                else {\n> +                                        dst[index] = src[index - width - 1];\n> +                                }          \n> +                        }\n> +                        index++;\n> +                }\n> +        }\n> +}\n> +\n> +void CPU_ISP::twoPixelInsert(uint16_t *src, uint16_t *dst, int width, int height)\n> +{\n> +        int index;\n> +        for (int i = 0; i < height; i++) {\n> +                index = i * width;\n> +                for (int j = 0; j < width; j++) {\n> +                        if (i == 0 && j == 0) {\n> +                                dst[index] = (src[index + width] +\n> +                                        src[index + 1]) >> 1;\n> +                        }\n> +                        else if (i == 0 && j > 0 && j % 2 == 0) {\n> +                                dst[index] = (src[index - 1] +\n> +                                        src[index + width] +\n> +                                        src[index + 1]) / 3;\n> +                        }\n> +                        else if (i > 0 && j == 0 && i % 2 == 0) {\n> +                                dst[index] = (src[index - width] +\n> +                                        src[index + 1] +\n> +                                        src[index + width]) / 3;\n> +                        }\n> +                        else if (i == (height - 1) && j < (width - 1) && j % 2 == 1) {\n> +                                dst[index] = (src[index - 1] +\n> +                                        src[index - width] +\n> +                                        src[index + 1]) / 3;\n> +                        }\n> +                        else if (i < (height - 1) && j == (width - 1) && i % 2 == 1) {\n> +                                dst[index] = (src[index - width] +\n> +                                        src[index - 1] +\n> +                                        src[index + width]) / 3;\n> +                        }\n> +                        else if (i == (height - 1) && j == (width - 1)) {\n> +                                dst[index] = (src[index - width] +\n> +                                        src[index - 1]) >> 1;\n> +                        }\n> +                        else if ((i % 2 == 0 && j % 2 == 0) || (i % 2 == 1 && j % 2 == 1)) {\n> +                                dst[index] = (src[index - 1] +\n> +                                        src[index + 1] +\n> +                                        src[index - width] +\n> +                                        src[index + width]) / 4;\n> +                        }\n> +                        index++;\n> +                }\n> +        }\n> +}\n> +\n> +void CPU_ISP::lastPixelInsert(uint16_t *src, uint16_t *dst, int width, int height)\n> +{\n> +        int index;\n> +        for (int i = 0; i < height; i++) {\n> +                index = i * width;\n> +                for (int j = 0; j < width; j++){\n> +                        if (i % 2 == 1 && j % 2 == 0) {\n> +                                if (j == 0) {\n> +                                        dst[index] = src[index + 1];\n> +                                }\n> +                                else {\n> +                                        dst[index] = (src[index - 1] +\n> +                                                src[index + 1]) >> 1;\n> +                                }\n> +                        }\n> +\n> +                        if (i % 2 == 0 && j % 2 == 1) {\n> +                                if(i == 0) {\n> +                                        dst[index] = src[index + width];\n> +                                }\n> +                                else {\n> +                                        dst[index] = (src[index - width]+\n> +                                                src[index + width]) >> 1;\n> +                                }\n> +                        }\n> +\n> +                        if (i % 2 == 0 && j % 2 == 0) {\n> +                                if (i > 0 && j > 0) {\n> +                                        dst[index] = (src[index - width - 1] +\n> +                                                src[index - width + 1] +\n> +                                                src[index + width - 1] +\n> +                                                src[index + width + 1]) >> 2;\n> +                                }\n> +                                else if (i == 0 && j > 0) {\n> +                                        dst[index] = (src[index + width - 1] +\n> +                                                src[index + width + 1]) >> 1;\n> +                                }\n> +                                else if (i > 0 && j == 0) {\n> +                                        dst[index] = (src[index - width + 1] +\n> +                                                src[index + width + 1]) >> 1;\n> +                                }\n> +                                else {\n> +                                        dst[index] = src[index + width + 1];\n> +                                }\n> +                        }\n> +                        index++;\n> +                }\n> +        }\n> +}\n> +\n> +void CPU_ISP::demosaic(uint16_t *data, uint16_t *R, uint16_t *G, uint16_t *B,\n> +                       int width, int height)\n> +{\n> +        firstPixelInsert(data, B, width, height);\n> +        twoPixelInsert(data, G, width, height);\n> +        lastPixelInsert(data, R, width, height);\n> +}\n> +\n> +void CPU_ISP::autoWhiteBalance(uint16_t *R, uint16_t *G, uint16_t *B, int width, int height)\n> +{\n> +        float aveB = 0, aveG = 0, aveR = 0;\n> +        float Kb, Kg, Kr;\n> +\n> +        for (int i = 0; i < width * height; i++) {\n> +                aveB += 1.0 * B[i];\n> +                aveG += 1.0 * G[i];\n> +                aveR += 1.0 * R[i];\n> +        }\n> +\n> +        aveB *= (1.0 / (width * height));\n> +        aveG *= (1.0 / (width * height));\n> +        aveR *= (1.0 / (width * height));\n> +\n> +        Kr = (aveB + aveG + aveR) / aveR * (1.0 / 3.0);\n> +        Kg = (aveB + aveG + aveR) / aveG * (1.0 / 3.0);\n> +        Kb = (aveB + aveG + aveR) / aveB * (1.0 / 3.0);\n> +\n> +        for (int i = 0; i < width * height; i++) {\n> +                B[i] = B[i] * Kb;\n> +                G[i] = G[i] * Kg;\n> +                R[i] = R[i] * Kr;\n> +\n> +                if (R[i] > 1023) R[i] = 1023;\n> +                if (G[i] > 1023) G[i] = 1023;\n> +                if (R[i] > 1023) B[i] = 1023;\n> +        }\n> +}\n> +\n> +void CPU_ISP::gammaCorrect(uint16_t *R, uint16_t *G, uint16_t *B, float val, int width, int height)\n> +{\n> +        float nor = 1.0 / 1023.0;\n> +        float gamma = 1.0 / val;\n> +        for (int i = 0; i < width * height; i++) {\n> +                R[i] = pow(R[i] * nor, gamma) * 1023;\n> +                G[i] = pow(G[i] * nor, gamma) * 1023;\n> +                B[i] = pow(B[i] * nor, gamma) * 1023;\n> +\n> +                if (R[i] > 1023) R[i] = 1023;\n> +                if (G[i] > 1023) G[i] = 1023;\n> +                if (B[i] > 1023) B[i] = 1023;\n> +        }\n> +}\n> +\n> +void CPU_ISP::compress_10bit_to_8bit (uint16_t *src, uint8_t *dst, int width, int height)\n> +{\n> +        for (int i = 0; i < width * height; i++) {\n> +                dst[i] = src[i] >> 2 & 0xff;\n> +        }\n> +}\n> +\n> +float CPU_ISP::distance(int x, int y, int i, int j)\n> +{\n> +    return float(sqrt(pow(x - i, 2) + pow(y - j, 2)));\n> +}\n> +\n> +double CPU_ISP::gaussian(float x, double sigma)\n> +{\n> +    return exp(-(pow(x, 2)) / (2 * pow(sigma, 2))) / (2 * 3.1415926 * pow(sigma, 2));\n> +}\n> +\n> +void CPU_ISP::bilateralFilter(uint16_t *R, uint16_t *G, uint16_t *B,\n> +                          int diameter, double sigmaI,\n> +                          double sigmaS, int width, int height)\n> +{\n> +        for (int i = 2; i < height - 2; i++) {\n> +                for (int j = 2; j < width - 2; j++) {\n> +                        double iFiltered = 0;\n> +                        double wp = 0;\n> +                        int neighbor_x = 0;\n> +                        int neighbor_y = 0;\n> +                        int half = diameter / 2;\n> +\n> +                        for (int k = 0; k < diameter; k++) {\n> +                                for (int l = 0; l < diameter; l++) {\n> +                                        neighbor_x = i - (half - k);\n> +                                        neighbor_y = j - (half - l);\n> +                                        double gi = gaussian(R[neighbor_x * width + neighbor_y] - R[i * width +j], sigmaI);\n> +                                        double gs = gaussian(distance(i, j, neighbor_x, neighbor_y), sigmaS);\n> +                                        double w = gi * gs;\n> +                                        iFiltered = iFiltered + R[neighbor_x * width + neighbor_y] * w;\n> +                                        wp = wp + w;\n> +                                }    \n> +                        }\n> +\n> +                        iFiltered = iFiltered / wp;\n> +                        R[i * width + j] = iFiltered;\n> +                }\n> +        }\n> +\n> +        for (int i = 2; i < height - 2; i++) {\n> +                for (int j = 2; j < width - 2; j++) {\n> +                        double iFiltered = 0;\n> +                        double wp = 0;\n> +                        int neighbor_x = 0;\n> +                        int neighbor_y = 0;\n> +                        int half = diameter / 2;\n> +\n> +                        for (int k = 0; k < diameter; k++) {\n> +                                for (int l = 0; l < diameter; l++) {\n> +                                        neighbor_x = i - (half - k);\n> +                                        neighbor_y = j - (half - l);\n> +                                        double gi = gaussian(G[neighbor_x * width + neighbor_y] - G[i * width +j], sigmaI);\n> +                                        double gs = gaussian(distance(i, j, neighbor_x, neighbor_y), sigmaS);\n> +                                        double w = gi * gs;\n> +                                        iFiltered = iFiltered + G[neighbor_x * width + neighbor_y] * w;\n> +                                        wp = wp + w;\n> +                                }    \n> +                        }\n> +\n> +                        iFiltered = iFiltered / wp;\n> +                        G[i * width + j] = iFiltered;\n> +                }\n> +        }\n> +\n> +        for (int i = 2; i < height - 2; i++) {\n> +                for (int j = 2; j < width - 2; j++) {\n> +                        double iFiltered = 0;\n> +                        double wp = 0;\n> +                        int neighbor_x = 0;\n> +                        int neighbor_y = 0;\n> +                        int half = diameter / 2;\n> +\n> +                        for (int k = 0; k < diameter; k++) {\n> +                                for (int l = 0; l < diameter; l++) {\n> +                                        neighbor_x = i - (half - k);\n> +                                        neighbor_y = j - (half - l);\n> +                                        double gi = gaussian(B[neighbor_x * width + neighbor_y] - B[i * width +j], sigmaI);\n> +                                        double gs = gaussian(distance(i, j, neighbor_x, neighbor_y), sigmaS);\n> +                                        double w = gi * gs;\n> +                                        iFiltered = iFiltered + B[neighbor_x * width + neighbor_y] * w;\n> +                                        wp = wp + w;\n> +                                }    \n> +                        }\n> +\n> +                        iFiltered = iFiltered / wp;\n> +                        B[i * width + j] = iFiltered;\n> +                }\n> +        }\n> +}\n> +\n> +void CPU_ISP::noiseReduction(uint16_t *R, uint16_t *G, uint16_t *B, int width, int height)\n> +{\n> +        bilateralFilter(R, G, B, 5, 24.0, 32.0, width, height);\n> +}\n> +\n> +void CPU_ISP::processing(FrameBuffer *srcBuffer, FrameBuffer *dstBuffer, int width, int height)\n> +{\n> +        uint8_t *rgb_buf;\n> +        uint16_t *rawData;\n> +        uint16_t *rgbData = new std::uint16_t[width * height * 3];\n> +\n> +        uint16_t *rData = rgbData;\n> +        uint16_t *gData = rData + width * height;\n> +        uint16_t *bData = gData + width * height;\n> +        memset(rgbData, 0x0, width * height * 3);\n> +\n> +        const FrameBuffer::Plane &plane =  srcBuffer->planes()[0]; \n> +        rawData = (uint16_t *)mmap(NULL, plane.length, PROT_READ|PROT_WRITE, MAP_SHARED, plane.fd.fd(), 0);\n> +        LOG(ISP, Debug) << plane.length << \" \" << plane.fd.fd();\n> +        if (rawData == MAP_FAILED) {\n> +            LOG(ISP, Error) << \"Read raw data failed\";\n> +            ispCompleted.emit(dstBuffer);\n> +        }\n> +\n> +        blackLevelCorrect(rawData, 16, width, height);\n> +        readChannels(rawData, rData, gData, bData, width, height);\n> +        demosaic(rawData, rData, gData, bData, width, height);\n> +        autoWhiteBalance(rData, gData, bData, width, height);\n> +        autoContrast(rData, 0.01, 0.01, width, height);\n> +        autoContrast(gData, 0.01, 0.01, width, height);\n> +        autoContrast(bData, 0.01, 0.01, width, height);\n> +        gammaCorrect(rData, gData, bData, 2.2, width, height);\n> +        //bilateralFilter(rData, gData, bData, 5, 24.0, 32.0, width, height);\n> +    \n> +        const FrameBuffer::Plane &rgbPlane = dstBuffer->planes()[0];\n> +        rgb_buf = (uint8_t *)mmap(NULL, rgbPlane.length, PROT_READ|PROT_WRITE, MAP_SHARED, rgbPlane.fd.fd(), 0);\n> +        if (rgb_buf == MAP_FAILED) {\n> +                LOG(ISP, Error) << \"Read rgb data failed\";\n> +                ispCompleted.emit(dstBuffer);\n> +        }\n> +\n> +        uint8_t *rData_8 = rgb_buf;\n> +        uint8_t *gData_8 = rData_8 + width * height;\n> +        uint8_t *bData_8 = gData_8 + width * height;\n> +\n> +        compress_10bit_to_8bit(rData, rData_8, width, height);\n> +        compress_10bit_to_8bit(gData, gData_8, width, height);\n> +        compress_10bit_to_8bit(bData, bData_8, width, height);\n> +\n> +        dstBuffer->metadata_.status = srcBuffer->metadata().status;\n> +        dstBuffer->metadata_.sequence = srcBuffer->metadata().sequence;\n> +        dstBuffer->metadata_.timestamp = srcBuffer->metadata().timestamp;\n> +\n> +        dstBuffer->metadata_.planes.clear();\n> +        dstBuffer->metadata_.planes.push_back({rgbPlane.length});\n> +\n> +        delete[] rgbData;\n> +\n> +        ispCompleted.emit(dstBuffer);\n> +}\n> +\n> +int CPU_ISP::exportBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers,\n> +                       unsigned int count, int width, int height)\n> +{\n> +        int bufferByte = width * height * 3;\n> +\n> +        for (unsigned int i = 0; i < count; i++) {\n> +                std::string name = \"frame-\" + std::to_string(i);\n> +\n> +                const int isp_fd = memfd_create(name.c_str(), 0);\n> +                int ret = ftruncate(isp_fd, bufferByte);\n> +                if (ret < 0) {\n> +                        LOG(ISP, Error) << \"Failed to resize memfd\" << strerror(-ret);\n> +                        return ret;\n> +                }\n> +\n> +                FrameBuffer::Plane rgbPlane;\n> +                rgbPlane.fd = FileDescriptor(std::move(isp_fd));\n> +                rgbPlane.length = bufferByte;\n> +\n> +                std::vector<FrameBuffer::Plane> planes{rgbPlane};\n> +                buffers->emplace_back(std::make_unique<FrameBuffer>(std::move(planes)));\n> +        }\n> +\n> +        return count;\n> +}\n> +\n> +void CPU_ISP::startThreadISP()\n> +{\n> +        moveToThread(&thread_);\n> +        thread_.start();\n> +}\n> +\n> +void CPU_ISP::stopThreadISP()\n> +{\n> +        thread_.exit();\n> +        thread_.wait();\n> +}\n> +\n> +} /* namespace libcamera */\n> \\ No newline at end of file\n> diff --git a/src/libcamera/swisp/isp.h b/src/libcamera/swisp/isp.h\n> new file mode 100644\n> index 00000000..4099e2c6\n> --- /dev/null\n> +++ b/src/libcamera/swisp/isp.h\n> @@ -0,0 +1,92 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Siyuan Fan <siyuan.fan@foxmail.com>\n> + *\n> + * isp.h - The software ISP class\n> + */\n> +#ifndef __LIBCAMERA_SWISP_ISP_H__\n> +#define __LIBCAMERA_SWISP_ISP_H__\n> +\n> +#include <libcamera/framebuffer.h>\n> +\n> +#include \"libcamera/base/object.h\"\n> +#include \"libcamera/base/signal.h\"\n> +#include \"libcamera/base/thread.h\"\n> +\n> +namespace libcamera{\n> +\n> +using std::uint16_t;\n> +using std::uint8_t;\n> +\n> +class ISP : public Object\n> +{\n> +public:\n> +        ISP() {}\n\nYou're missing the functions that form the interface to the ISP. At the\nmoment these are:\n- processing()\n- exportBuffers()\n- startThreadISP()\n- stopThreadISP()\n- Signal<FrameBuffer *> ispCompleted\n\nIn addition to these I think you need:\n- formats() (supportedFormats()? I'm not sure what a good name is for this)\n- configure()\n\nObviously for your implementation they'll be super simple, but they need\nto be defined in the common software ISP interface.\n\nMaybe some function for reporting supported sizes would be good too?\nYour ISP doesn't look like it has such restriction, but I'm wondering if\nGPU-based ones would. Or maybe GPU-based ISPs would only have a\nrestriction on size for specific performace ranges.\n\n> +\n> +        virtual void blackLevelCorrect(uint16_t *data, uint16_t offset, int width, int height) = 0;\n> +\n> +        virtual void demosaic(uint16_t *data, uint16_t *R, uint16_t *G, uint16_t *B,\n> +                              int width, int height) = 0;\n> +\n> +        virtual void autoWhiteBalance(uint16_t *R, uint16_t *G, uint16_t *B, int width, int height) = 0;\n> +\n> +        virtual void gammaCorrect(uint16_t *R, uint16_t *G, uint16_t *B, float val, int width, int height)  = 0;\n> +\n> +        virtual void noiseReduction(uint16_t *R, uint16_t *G, uint16_t *B, int width, int height) = 0;\n\nWill these be called directly by the pipeline handler? On one hand that\nallows flexibility on the side of the pipeline handler, but on the other\nhand it adds more inter-thread calls. Maybe it's better to just expose\none function (like you do now) and take a vector of operations that the\npipeline handler wants the ISP to do? Like\n\nisp_->invokeMethod(processing, { BLC, Demosaic, AWB, NR }, rawBuf, rgbBuf, ...)\n\nIf these functions won't be called directly by the pipeline handler, and\nsoftware ISPs aren't required to implement these, then they should not\ngo here.\n\n> +\n> +        virtual ~ISP() {}\n\nThis should go right below ISP() {}\n\n> +};\n> +\n> +class CPU_ISP : public ISP\n\nWhen you sort class names alphabetically, it's nice for related classes\nto be grouped together, which is why the common name goes first (like\nthe PipelineHandlers do), so I would call this ISPCPU.\n\n> +{\n> +public:\n> +        void autoContrast(uint16_t *data, float lowCut, float highCut, int width, int height);\n> +\n> +        void blackLevelCorrect(uint16_t *data, uint16_t offset, int width, int height) override;\n> +\n> +        void readChannels(uint16_t *data, uint16_t *R, uint16_t *G, uint16_t *B,\n> +                          int width, int height);\n> +\n> +        void firstPixelInsert(uint16_t *src, uint16_t *dst, int width, int height);\n> +\n> +        void twoPixelInsert(uint16_t *src, uint16_t *dst, int width, int height);\n> +\n> +        void lastPixelInsert(uint16_t *src, uint16_t *dst, int width, int height);\n> +\n> +        void demosaic(uint16_t *data, uint16_t *R, uint16_t *G, uint16_t *B,\n> +                      int width, int height) override;\n> +\n> +        void autoWhiteBalance(uint16_t *R, uint16_t *G, uint16_t *B, int width, int height) override;\n> +\n> +        void gammaCorrect(uint16_t *R, uint16_t *G, uint16_t *B, float val, int width, int height) override;\n> +\n> +        float distance(int x, int y, int i, int j);\n> +\n> +        double gaussian(float x, double sigma);\n> +\n> +        void bilateralFilter(uint16_t *R, uint16_t *G, uint16_t *B,\n> +                             int diameter, double sigmaI, double sigmaS,\n> +                             int width, int height);\n> +\n> +        void noiseReduction(uint16_t *R, uint16_t *G, uint16_t *B, int width, int height) override;\n> +\n> +        void compress_10bit_to_8bit(uint16_t *src, uint8_t *dst, int width, int height);    \n\nAll of these should be private.\n\n> +\n> +        void processing(FrameBuffer *srcBuffer, FrameBuffer *dstBuffer, int width, int height);\n> +\n> +        int exportBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers,\n> +                           unsigned int count, int width, int height);\n> +\n> +        void startThreadISP();\n> +\n> +        void stopThreadISP();\n> +\n> +        Signal<FrameBuffer *> ispCompleted;\n> +\n> +private:\n> +        Thread thread_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_SWISP_ISP_H__ */\n> \\ No newline at end of file\n\nYou should have a newline at end of file.\n\n\nPaul\n\n> -- \n> 2.20.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5FA72BD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 11 Aug 2021 10:15:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B90946884E;\n\tWed, 11 Aug 2021 12:15:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8692968826\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 11 Aug 2021 12:15:24 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 93206EE;\n\tWed, 11 Aug 2021 12:15:22 +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=\"bOJo+oOm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628676924;\n\tbh=hTn43h2g7bo7plCqBs2LeCVHquofs772NhTQB52eSBk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bOJo+oOmU7+m0rQvlnAo2Ox+eWSl9RxfILOpjR5neyoeFQud4P5hBTXb2GuAL4HwA\n\taD2iv9VQH6IO2KA2yKivx7OCCKUakkXFZ+Dy0zeP0j6lLhofyR6CFtP2lA4DOykryl\n\tOe2k4m+Fvi5lYn/sV6h/QTSp81iBEqM1K4jftK3w=","Date":"Wed, 11 Aug 2021 19:15:16 +0900","From":"paul.elder@ideasonboard.com","To":"Siyuan Fan <siyuan.fan@foxmail.com>","Message-ID":"<20210811101516.GZ2167@pyrite.rasen.tech>","References":"<20210811061258.7421-1-siyuan.fan@foxmail.com>\n\t<tencent_BF476F7B7C811ABED21EA0949CB140575509@qq.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<tencent_BF476F7B7C811ABED21EA0949CB140575509@qq.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/4] libcamera: swisp: The\n\tsoftware ISP class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]