[{"id":18565,"web_url":"https://patchwork.libcamera.org/comment/18565/","msgid":"<20210805100855.GO2167@pyrite.rasen.tech>","date":"2021-08-05T10:08:55","subject":"Re: [libcamera-devel] [RFC PATCH v1 2/3] pipeline: isp: 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 04, 2021 at 08:33:46AM +0100, Siyuan Fan wrote:\n> From: Fan Siyuan <siyuan.fan@foxmail.com>\n> \n> Add the software ISP class only CPU-based. So far the basic alogrithm\n> includes black level correct, bilinear demosaic interpolation, auto \n> white balance(gray world), tone mapping(auto contrast), gamma correct.\n> bilateral filter, 10bit to 8bit compression.\n> \n> Signed-off-by: Fan Siyuan <siyuan.fan@foxmail.com>\n> \n> ---\n>  src/libcamera/pipeline/isp/isp_processing.cpp | 593 ++++++++++++++++++\n>  src/libcamera/pipeline/isp/isp_processing.h   |  62 ++\n>  2 files changed, 655 insertions(+)\n>  create mode 100644 src/libcamera/pipeline/isp/isp_processing.cpp\n>  create mode 100644 src/libcamera/pipeline/isp/isp_processing.h\n> \n> diff --git a/src/libcamera/pipeline/isp/isp_processing.cpp b/src/libcamera/pipeline/isp/isp_processing.cpp\n> new file mode 100644\n> index 00000000..94e8081d\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/isp/isp_processing.cpp\n> @@ -0,0 +1,593 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Siyuan Fan <siyuan.fan@foxmail.com>\n> + *\n> + * isp_processing.cpp - The software ISP class\n> + */\n> +\n> +#include \"isp_processing.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> +\n> +#include \"libcamera/base/log.h\"\n> +\n> +namespace libcamera{\n> +\n> +LOG_DECLARE_CATEGORY(ISP)\n> +\n> +void 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\nFrom what I see from the caller of this, data is already demosaiced, so\nit only includes one color. How are you going to get a histogram of the\nother colors?\n\n> +\n> +        int index;\n> +        for (int i = 0; i < height; i++) {\n> +                index = i * width;\n\nI see you using this idiom a lot. I think it's better to assign it under\nthe j loop...\n\n> +                for (int j = 0; j < width; j++) {\n\n... here, int index = i * width + j;\n\nSame everywhere else you use this pattern.\n\n> +                        if (i % 2 == 0 && j % 2 == 0) {\n\nIt would be nice if you could figure out some way to allow this to work\nwith any raw format, but I guess you don't have to worry about it now,\nbut make sure to add a configure() function to your ISP so that you can\nreject formats that you don't support.\n\n> +                                blue = data[index];\n> +                                histBlue[blue]++;\n> +                        }\n> +                        else if ((i % 2 == 0 && j % 2 == 1)) {\n\nelse and else if should both go on the same line as }\n\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\nI think it would be useful to move this calculation of the histogram to\nanother function, so if other algorithms in the future want to use the\nhistogram they can (plus you can expose it as a function to the pipeline\nhandlers for IPAs!).\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\nI see these two blocks are reused over and over again below. These\nshould be moved to other functions.\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\nSame for these; these should be moved to a function.\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 ISP::blackLevelCorrect(uint16_t *data, uint16_t offset, int width, int height)\n\nI see you hardcode offset to 16 in the caller. Where does that number\ncome from?\n\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 ISP::readChannels(uint16_t *data, uint16_t *R, uint16_t *G, uint16_t *B,\n> +                       int width, int height)\n\nI think you don't need this function, and you can just add another\nclause in your demosaic functions...\n\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 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\nI think some comments here would help, to describe each clause. After I\nbuilt the mental image it was easy to follow but before that it was\nhard.\n\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\n...like here:\n\nelse\n\tdst[index] = src[index];\n\n> +                        index++;\n> +                }\n> +        }\n> +}\n> +\n> +void 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 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 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\nI think these functions could use better names, but I can't think of\nanything better :S\n\n> +}\n> +\n> +void 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\nI think calculating these could be moved to a separate function too,\njust like the histogram.\n\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 ISP::gammaCorrect(uint16_t *R, uint16_t *G, uint16_t *B, float val,\n> +                       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 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 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 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 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 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\nInstead of using this as a scratch buffer, you could just do the\ndstBuffer mmap here already, and then use that directly.\n\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> +        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);\n\nWhy isn't this used?\n\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\nThis isn't a valid format, you have three separate planes of R, G, and\nB. iirc you were using RGB888? In which case you need to reorder your\ncolor bytes [1] (libcamera RGB888 is V4L2 V4L2_PIX_FMT_BGR24).\n\n[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-rgb.html\n\n\nPaul\n\n> +\n> +        delete[] rgbData;\n> +\n> +        ispCompleted.emit(dstBuffer);\n> +}\n> +\n> +} /* namespace libcamera */\n> +\n> diff --git a/src/libcamera/pipeline/isp/isp_processing.h b/src/libcamera/pipeline/isp/isp_processing.h\n> new file mode 100644\n> index 00000000..b80b4218\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/isp/isp_processing.h\n> @@ -0,0 +1,62 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Siyuan Fan <siyuan.fan@foxmail.com>\n> + *\n> + * isp_processing.h - The software ISP class\n> + */\n> +#ifndef __LIBCAMERA_PIPELINE_ISP_PROCESSING_H__\n> +#define __LIBCAMERA_PIPELINE_ISP_PROCESSING_H__\n> +\n> +#include <libcamera/framebuffer.h>\n> +\n> +#include \"libcamera/base/signal.h\"\n> +#include \"libcamera/base/object.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> +        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);\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);\n> +\n> +        void autoWhiteBalance(uint16_t *R, uint16_t *G, uint16_t *B, int width, int height);\n> +\n> +        void gammaCorrect(uint16_t *R, uint16_t *G, uint16_t *B, float val,\n> +                          int width, int height);\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 compress_10bit_to_8bit(uint16_t *src, uint8_t *dst, int width, int height);    \n> +\n> +        void processing(FrameBuffer *srcBuffer, FrameBuffer *dstBuffer, int width, int height);\n> +\n> +        Signal<FrameBuffer *> ispCompleted;\n> +\n> +};\n> +\n> +}\n> +\n> +#endif /* __LIBCAMERA_PIPELINE_ISP_PROCESSING_H__ */\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 CBD6AC3237\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  5 Aug 2021 10:09:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2AC0668811;\n\tThu,  5 Aug 2021 12:09:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 599CF6026D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  5 Aug 2021 12:09:03 +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 8A90651D;\n\tThu,  5 Aug 2021 12:09:01 +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=\"XQV4yMsY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628158143;\n\tbh=TZXiA15I5hcRVJEQeXpNPJo0MfHS2j/axDogK9dLJoI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XQV4yMsYS6xwWyOP1qOfuDimgaA+TEFmMuDIdikYMu+q80mvbeqfDgI0OpR/6vMRL\n\tQZi8d+QznYwUJt0iR9+OLzFIc0/8wZMjZBz3eSEGGf89hJr92eE3Do6JXjlNfD6UYl\n\tkR9Wkd1K+epAFnlzSwyunzhJruZ3TnWpZbxKMRlk=","Date":"Thu, 5 Aug 2021 19:08:55 +0900","From":"paul.elder@ideasonboard.com","To":"Siyuan Fan <siyuan.fan@foxmail.com>","Message-ID":"<20210805100855.GO2167@pyrite.rasen.tech>","References":"<20210804073347.1368-1-siyuan.fan@foxmail.com>\n\t<tencent_FF0DB70FBE0E0D89696050DFCEEEA04DAA08@qq.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<tencent_FF0DB70FBE0E0D89696050DFCEEEA04DAA08@qq.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v1 2/3] pipeline: isp: 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>"}}]