[{"id":17502,"web_url":"https://patchwork.libcamera.org/comment/17502/","msgid":"<YME96Dhgra7u25qL@pendragon.ideasonboard.com>","date":"2021-06-09T22:17:12","subject":"Re: [libcamera-devel] [PATCH v2 5/5] qcam: viewfinder_gl: Add\n\tsupport for RAW8 Bayer formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Andrey,\n\nThank you for the patch.\n\nI would also like to thank Morgan for his code.\n\nOn Thu, May 27, 2021 at 01:55:11PM +0300, Andrey Konovalov wrote:\n> All the four Bayer orders are supported.\n> \n> The texture coordinates passed to the fragment shader are ajusted\n> to point to the nearest pixel in the image. This prevents artifacts\n> when the image is scaled from the frame resolution to the window size.\n> \n> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>\n> ---\n>  src/qcam/assets/shader/bayer_8.frag | 136 ++++++++++++++++++++++++++++\n>  src/qcam/assets/shader/bayer_8.vert |  72 +++++++++++++++\n>  src/qcam/assets/shader/shaders.qrc  |   1 +\n>  src/qcam/viewfinder_gl.cpp          |  37 +++++++-\n>  4 files changed, 244 insertions(+), 2 deletions(-)\n>  create mode 100644 src/qcam/assets/shader/bayer_8.frag\n>  create mode 100644 src/qcam/assets/shader/bayer_8.vert\n> \n> diff --git a/src/qcam/assets/shader/bayer_8.frag b/src/qcam/assets/shader/bayer_8.frag\n> new file mode 100644\n> index 00000000..d93ef1da\n> --- /dev/null\n> +++ b/src/qcam/assets/shader/bayer_8.frag\n> @@ -0,0 +1,136 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> +From http://jgt.akpeters.com/papers/McGuire08/\n> +\n> +Efficient, High-Quality Bayer Demosaic Filtering on GPUs\n> +\n> +Morgan McGuire\n> +\n> +This paper appears in issue Volume 13, Number 4.\n> +---------------------------------------------------------\n> +Copyright (c) 2008, Morgan McGuire. All rights reserved.\n> +\n> +\n> +Modified by Linaro Ltd to integrate it into libcamera, and to\n> +fix the artifacts due to pixel coordinates interpolation.\n> +Copyright (C) 2021, Linaro\n\nWould it make sense to import the original version as-is in a patch, and\nthen add the changes on top ? It would be easier to review them.\n\n> +*/\n> +\n> +//Pixel Shader\n\nWe try to standardize on C-style comments.\n\n> +\n> +varying vec2 textureOut;\n> +\n> +/* The texture size: tex_size.xy is in bytes, tex_size.zw is in pixels */\n> +uniform vec4 tex_size;\n> +uniform vec2 tex_step;\n> +\n> +/** Pixel position of the first red pixel in the */\n> +/**  Bayer pattern.  [{0,1}, {0, 1}]*/\n> +uniform vec2            tex_bayer_first_red;\n> +\n> +/** Monochrome RGBA or GL_LUMINANCE Bayer encoded texture.*/\n> +uniform sampler2D       tex_raw;\n> +\n> +void main(void) {\n> +    #define fetch(x, y) texture2D(tex_raw, vec2(x, y)).r\n> +\n> +    /** .xy = Pixel being sampled in the fragment shader on the range [0, 1]\n> +        .zw = ...on the range [0, sourceSize], offset by firstRed */\n\nFor multiblock comments, our coding style is\n\n    /*\n     * .xy = Pixel being sampled in the fragment shader on the range [0, 1]\n     * .zw = ...on the range [0, sourceSize], offset by firstRed\n     */\n\nThis being said, if we want to minimize changes with the original, I\nwould be OK.\n\n> +    vec4            center;\n> +\n> +    /** center.x + (-2/w, -1/w, 1/w, 2/w); These are the x-positions */\n> +    /** of the adjacent pixels.*/\n> +    vec4            xCoord;\n> +\n> +    /** center.y + (-2/h, -1/h, 1/h, 2/h); These are the y-positions */\n> +    /** of the adjacent pixels.*/\n> +    vec4            yCoord;\n\nDoesn't computing the center, xCoord and yCoord values here defeat the\npoint of computing them in the vertex shader, where the interpolator\nhardware can be used for the computation ?\n\n> +\n> +    /* Align the center coordinates to the nearest pixel */\n> +    center.zw = floor(textureOut * tex_size.zw);\n> +    center.xy = center.zw * tex_step;\n> +    center.zw += tex_bayer_first_red;\n> +\n> +    xCoord = center.x + vec4(-2.0 * tex_step.x,\n> +                             -tex_step.x, tex_step.x, 2.0 * tex_step.x);\n> +    yCoord = center.y + vec4(-2.0 * tex_step.y,\n> +                              -tex_step.y, tex_step.y, 2.0 * tex_step.y);\n> +\n> +    float C = texture2D(tex_raw, center.xy).r; // ( 0, 0)\n> +    const vec4 kC = vec4( 4.0,  6.0,  5.0,  5.0) / 8.0;\n> +\n> +    // Determine which of four types of pixels we are on.\n> +    vec2 alternate = mod(floor(center.zw), 2.0);\n> +\n> +    vec4 Dvec = vec4(\n> +        fetch(xCoord[1], yCoord[1]),  // (-1,-1)\n> +        fetch(xCoord[1], yCoord[2]),  // (-1, 1)\n> +        fetch(xCoord[2], yCoord[1]),  // ( 1,-1)\n> +        fetch(xCoord[2], yCoord[2])); // ( 1, 1)\n> +\n> +    vec4 PATTERN = (kC.xyz * C).xyzz;\n> +\n> +    // Can also be a dot product with (1,1,1,1) on hardware where that is\n> +    // specially optimized.\n> +    // Equivalent to: D = Dvec[0] + Dvec[1] + Dvec[2] + Dvec[3];\n> +    Dvec.xy += Dvec.zw;\n> +    Dvec.x  += Dvec.y;\n> +\n> +    vec4 value = vec4(\n> +        fetch(center.x, yCoord[0]),   // ( 0,-2)\n> +        fetch(center.x, yCoord[1]),   // ( 0,-1)\n> +        fetch(xCoord[0], center.y),   // (-2, 0)\n> +        fetch(xCoord[1], center.y));  // (-1, 0)\n> +\n> +    vec4 temp = vec4(\n> +        fetch(center.x, yCoord[3]),   // ( 0, 2)\n> +        fetch(center.x, yCoord[2]),   // ( 0, 1)\n> +        fetch(xCoord[3], center.y),   // ( 2, 0)\n> +        fetch(xCoord[2], center.y));  // ( 1, 0)\n> +\n> +    // Even the simplest compilers should be able to constant-fold these to\n> +    // avoid the division.\n> +    // Note that on scalar processors these constants force computation of some\n> +    // identical products twice.\n> +    const vec4 kA = vec4(-1.0, -1.5,  0.5, -1.0) / 8.0;\n> +    const vec4 kB = vec4( 2.0,  0.0,  0.0,  4.0) / 8.0;\n> +    const vec4 kD = vec4( 0.0,  2.0, -1.0, -1.0) / 8.0;\n> +\n> +    // Conserve constant registers and take advantage of free swizzle on load\n> +    #define kE (kA.xywz)\n> +    #define kF (kB.xywz)\n> +\n> +    value += temp;\n> +\n> +    // There are five filter patterns (identity, cross, checker,\n> +    // theta, phi).  Precompute the terms from all of them and then\n> +    // use swizzles to assign to color channels.\n> +    //\n> +    // Channel   Matches\n> +    //   x       cross   (e.g., EE G)\n> +    //   y       checker (e.g., EE B)\n> +    //   z       theta   (e.g., EO R)\n> +    //   w       phi     (e.g., EO R)\n> +    #define A (value[0])\n> +    #define B (value[1])\n> +    #define D (Dvec.x)\n> +    #define E (value[2])\n> +    #define F (value[3])\n> +\n> +    // Avoid zero elements. On a scalar processor this saves two MADDs\n> +    // and it has no effect on a vector processor.\n> +    PATTERN.yzw += (kD.yz * D).xyy;\n> +\n> +    PATTERN += (kA.xyz * A).xyzx + (kE.xyw * E).xyxz;\n> +    PATTERN.xw  += kB.xw * B;\n> +    PATTERN.xz  += kF.xz * F;\n> +\n> +    vec3 rgb = (alternate.y == 0.0) ?\n> +        ((alternate.x == 0.0) ?\n> +            vec3(C, PATTERN.xy) :\n> +            vec3(PATTERN.z, C, PATTERN.w)) :\n> +        ((alternate.x == 0.0) ?\n> +            vec3(PATTERN.w, C, PATTERN.z) :\n> +            vec3(PATTERN.yx, C));\n> +    gl_FragColor = vec4(rgb, 1.0);\n> +}\n> diff --git a/src/qcam/assets/shader/bayer_8.vert b/src/qcam/assets/shader/bayer_8.vert\n> new file mode 100644\n> index 00000000..26525db7\n> --- /dev/null\n> +++ b/src/qcam/assets/shader/bayer_8.vert\n> @@ -0,0 +1,72 @@\n\nThis needs and SPDX license identifier:\n\n/* SPDX-License-Identifier: BSD-2-Clause */\n\n> +/*\n> +From http://jgt.akpeters.com/papers/McGuire08/\n> +\n> +Efficient, High-Quality Bayer Demosaic Filtering on GPUs\n> +\n> +Morgan McGuire\n> +\n> +This paper appears in issue Volume 13, Number 4.\n> +---------------------------------------------------------\n> +Copyright (c) 2008, Morgan McGuire. All rights reserved.\n> +\n> +Redistribution and use in source and binary forms, with or without\n> +modification, are permitted provided that the following conditions are\n> +met:\n> +\n> +    * Redistributions of source code must retain the above copyright\n> +      notice, this list of conditions and the following disclaimer.\n> +\n> +    * Redistributions in binary form must reproduce the above\n> +      copyright notice, this list of conditions and the following\n> +      disclaimer in the documentation and/or other materials provided\n> +      with the distribution.\n> +\n> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS\n> +\"AS IS\" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT\n> +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR\n> +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT\n> +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,\n> +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT\n> +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,\n> +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY\n> +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT\n> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE\n> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.\n\nThe whole text from \"Redistribution ...\" to here is conveyed by the SPDX\ntag, so you can drop it.\n\n> +*/\n> +\n> +//Vertex Shader\n> +\n> +attribute vec4 vertexIn;\n> +attribute vec2 textureIn;\n> +\n> +/* The texture size: tex_size.xy is in bytes, tex_size.zw is in pixels */\n> +uniform vec4 tex_size;\n> +uniform vec2 tex_step;\n> +\n> +/** Pixel position of the first red pixel in the */\n> +/**  Bayer pattern.  [{0,1}, {0, 1}]*/\n> +uniform vec2            tex_bayer_first_red;\n> +\n> +/** .xy = Pixel being sampled in the fragment shader on the range [0, 1]\n> +    .zw = ...on the range [0, sourceSize], offset by firstRed */\n> +varying vec4            center;\n> +\n> +/** center.x + (-2/w, -1/w, 1/w, 2/w); These are the x-positions */\n> +/** of the adjacent pixels.*/\n> +varying vec4            xCoord;\n> +\n> +/** center.y + (-2/h, -1/h, 1/h, 2/h); These are the y-positions */\n> +/** of the adjacent pixels.*/\n> +varying vec4            yCoord;\n> +\n> +void main(void) {\n> +    center.xy = textureIn;\n> +    center.zw = textureIn * tex_size.zw + tex_bayer_first_red;\n> +\n> +    xCoord = center.x + vec4(-2.0 * tex_step.x,\n> +                             -tex_step.x, tex_step.x, 2.0 * tex_step.x);\n> +    yCoord = center.y + vec4(-2.0 * tex_step.y,\n> +                              -tex_step.y, tex_step.y, 2.0 * tex_step.y);\n> +\n> +    gl_Position = vertexIn;\n> +}\n> diff --git a/src/qcam/assets/shader/shaders.qrc b/src/qcam/assets/shader/shaders.qrc\n> index d76d65c5..79f44a30 100644\n> --- a/src/qcam/assets/shader/shaders.qrc\n> +++ b/src/qcam/assets/shader/shaders.qrc\n> @@ -5,6 +5,7 @@\n>  \t<file>YUV_2_planes.frag</file>\n>  \t<file>YUV_3_planes.frag</file>\n>  \t<file>YUV_packed.frag</file>\n> +\t<file>bayer_8.frag</file>\n\nCould you please move this after bayer_1x_packed.frag to keep them\nalphabetically sorted ?\n\nYou're missing bayer_8.vert.\n\n>  \t<file>bayer_1x_packed.frag</file>\n>  \t<file>identity.vert</file>\n>  </qresource>\n> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> index 13ab31aa..ef85e93d 100644\n> --- a/src/qcam/viewfinder_gl.cpp\n> +++ b/src/qcam/viewfinder_gl.cpp\n> @@ -36,6 +36,11 @@ static const QList<libcamera::PixelFormat> supportedFormats{\n>  \tlibcamera::formats::RGBA8888,\n>  \tlibcamera::formats::BGR888,\n>  \tlibcamera::formats::RGB888,\n> +\t/* Raw Bayer 8-bit */\n> +\tlibcamera::formats::SBGGR8,\n> +\tlibcamera::formats::SGBRG8,\n> +\tlibcamera::formats::SGRBG8,\n> +\tlibcamera::formats::SRGGB8,\n>  \t/* Raw Bayer 10-bit packed */\n>  \tlibcamera::formats::SBGGR10_CSI2P,\n>  \tlibcamera::formats::SGBRG10_CSI2P,\n> @@ -220,6 +225,30 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n>  \t\tfragmentShaderDefines_.append(\"#define RGB_PATTERN bgr\");\n>  \t\tfragmentShaderFile_ = \":RGB.frag\";\n>  \t\tbreak;\n> +\tcase libcamera::formats::SBGGR8:\n> +\t\tfirstRed_.setX(1.0);\n> +\t\tfirstRed_.setY(1.0);\n> +\t\tfragmentShaderFile_ = \":bayer_8.frag\";\n> +\t\ttextureMinMagFilters_ = GL_NEAREST;\n> +\t\tbreak;\n> +\tcase libcamera::formats::SGBRG8:\n> +\t\tfirstRed_.setX(0.0);\n> +\t\tfirstRed_.setY(1.0);\n> +\t\tfragmentShaderFile_ = \":bayer_8.frag\";\n> +\t\ttextureMinMagFilters_ = GL_NEAREST;\n> +\t\tbreak;\n> +\tcase libcamera::formats::SGRBG8:\n> +\t\tfirstRed_.setX(1.0);\n> +\t\tfirstRed_.setY(0.0);\n> +\t\tfragmentShaderFile_ = \":bayer_8.frag\";\n> +\t\ttextureMinMagFilters_ = GL_NEAREST;\n> +\t\tbreak;\n> +\tcase libcamera::formats::SRGGB8:\n> +\t\tfirstRed_.setX(0.0);\n> +\t\tfirstRed_.setY(0.0);\n> +\t\tfragmentShaderFile_ = \":bayer_8.frag\";\n> +\t\ttextureMinMagFilters_ = GL_NEAREST;\n> +\t\tbreak;\n>  \tcase libcamera::formats::SBGGR10_CSI2P:\n>  \t\tfirstRed_.setX(1.0);\n>  \t\tfirstRed_.setY(1.0);\n> @@ -624,6 +653,10 @@ void ViewFinderGL::doRender()\n>  \t\tshaderProgram_.setUniformValue(textureUniformY_, 0);\n>  \t\tbreak;\n>  \n> +\tcase libcamera::formats::SBGGR8:\n> +\tcase libcamera::formats::SGBRG8:\n> +\tcase libcamera::formats::SGRBG8:\n> +\tcase libcamera::formats::SRGGB8:\n>  \tcase libcamera::formats::SBGGR10_CSI2P:\n>  \tcase libcamera::formats::SGBRG10_CSI2P:\n>  \tcase libcamera::formats::SGRBG10_CSI2P:\n> @@ -633,8 +666,8 @@ void ViewFinderGL::doRender()\n>  \tcase libcamera::formats::SGRBG12_CSI2P:\n>  \tcase libcamera::formats::SRGGB12_CSI2P:\n>  \t\t/*\n> -\t\t * Packed raw Bayer 10-bit and 12-bit formats are stored in\n> -\t\t * GL_RED texture.\n> +\t\t * Raw Bayer 8-bit, and packed raw Bayer 10-bit/12-bit formats\n> +\t\t * are stored in GL_RED texture.\n>  \t\t */\n>  \t\tglActiveTexture(GL_TEXTURE0);\n>  \t\tconfigureTexture(*textures_[0]);","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 C5A1BBD78E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  9 Jun 2021 22:17:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4162A602C5;\n\tThu, 10 Jun 2021 00:17:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FB05602A7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 10 Jun 2021 00:17:31 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B4FDB436;\n\tThu, 10 Jun 2021 00:17:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LRfrQwuz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623277050;\n\tbh=1mUxUM5Sjg9OoD6lAvyMMOISTt8UVDqhEWChIhOcJh4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LRfrQwuzUMjptF+ycZY36tMwoKH0py6+6Veyo8OjvmsUuzgIEfs/T4s465eRG9TbB\n\tVBJxxVLGARMhtmUXNJPh35V9OSqwEoJZcSUbBpQaZIjzI+MheEtEIOY9IZzBGhdycN\n\tjPb85Aavm4JShuP6dOvLYrsk9xA2plnKpFxq49Qw=","Date":"Thu, 10 Jun 2021 01:17:12 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Andrey Konovalov <andrey.konovalov@linaro.org>","Message-ID":"<YME96Dhgra7u25qL@pendragon.ideasonboard.com>","References":"<20210527105511.447089-1-andrey.konovalov@linaro.org>\n\t<20210527105511.447089-6-andrey.konovalov@linaro.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210527105511.447089-6-andrey.konovalov@linaro.org>","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] qcam: viewfinder_gl: Add\n\tsupport for RAW8 Bayer formats","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":"morgan@casual-effects.com, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]