[{"id":14155,"web_url":"https://patchwork.libcamera.org/comment/14155/","msgid":"","date":"2020-12-08T19:04:46","subject":"Re: [libcamera-devel] [PATCH][RFC 2/2] qcam: viewfinder_gl: Add\n\tshader to render packed RAW12 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\nOn Tue, Nov 10, 2020 at 12:34:11AM +0300, Andrey Konovalov wrote:\n> The shader supports all 4 packed RAW12 variants.\n> Simple bi-linear filtering is implemented.\n> The 4 LS bits of the 12-bit colour values are dropped as the RGBA\n> format we convert into has only 8 bits per colour.\n> \n> Signed-off-by: Andrey Konovalov \n> ---\n> src/qcam/assets/shader/bayer.vert | 28 ++++++++\n> src/qcam/assets/shader/bayer_12_packed.frag | 68 +++++++++++++++++++\n> src/qcam/assets/shader/shaders.qrc | 2 +\n> src/qcam/viewfinder_gl.cpp | 74 ++++++++++++++++++++-\n> src/qcam/viewfinder_gl.h | 7 ++\n> 5 files changed, 177 insertions(+), 2 deletions(-)\n> create mode 100644 src/qcam/assets/shader/bayer.vert\n> create mode 100644 src/qcam/assets/shader/bayer_12_packed.frag\n> \n> diff --git a/src/qcam/assets/shader/bayer.vert b/src/qcam/assets/shader/bayer.vert\n> new file mode 100644\n> index 00000000..375e0b60\n> --- /dev/null\n> +++ b/src/qcam/assets/shader/bayer.vert\n> @@ -0,0 +1,28 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Linaro\n> + *\n> + * bayer.vert - Vertex shader for raw Bayer to RGB conversion\n> + */\n> +\n> +attribute vec4 vertexIn;\n> +attribute vec2 textureIn;\n> +\n> +uniform vec4 srcSize;\n> +uniform vec2 firstRed;\n> +\n> +varying vec4 center;\n> +varying vec2 xcoords;\n> +varying vec2 ycoords;\n> +\n> +void main(void)\n> +{\n> +\tcenter.xy = textureIn;\n> +\tcenter.zw = textureIn * srcSize.xy + firstRed;\n\ncenter.xy and center.zw store quite different data. Wouldn't the code be\nmore readable if we used two different variables ?\n\n> +\n> +\tvec2 invSize = srcSize.zw;\n> +\txcoords = center.x + vec2(-invSize.x, invSize.x);\n> +\tycoords = center.y + vec2(-invSize.y, invSize.y);\n> +\n> +\tgl_Position = vertexIn;\n> +}\n> diff --git a/src/qcam/assets/shader/bayer_12_packed.frag b/src/qcam/assets/shader/bayer_12_packed.frag\n> new file mode 100644\n> index 00000000..eb822b85\n> --- /dev/null\n> +++ b/src/qcam/assets/shader/bayer_12_packed.frag\n> @@ -0,0 +1,68 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Linaro\n> + *\n> + * bayer_12_packed.frag - Fragment shader code for raw Bayer 12-bit packed\n> + * format\n> + */\n> +\n> +#ifdef GL_ES\n> +precision mediump float;\n> +#endif\n> +\n> +varying vec4 center;\n> +varying vec2 xcoords;\n> +varying vec2 ycoords;\n\nIf I understand correctly, center.xy stores the texture coordinates of\nthe pixel, while xcoords (ycoords) stores the horizontal (vertical)\ncoordinates of the previous and next pixels. Is that correct ? If so, I\nwonder if we shouldn't instead declare xcoords and ycoords as vec3, to\nstore the coordinates of the previous, current and next pixels. The code\nbelow could become, for instance,\n\n\tvec4 vals_BCD = vec4(\n\t\ttexture2D(tex_raw, vec2(xcoords[1], ycoords[1]).rg,\n\t\ttexture2D(tex_raw, vec2(xcoords[0], ycoords[1])).g,\n\t\ttexture2D(tex_raw, vec2(xcoords[2], ycoords[1])).r);\n\n> +\n> +uniform sampler2D tex_raw;\n> +\n> +void main(void)\n> +{\n> +\tvec3 rgb;\n> +\n> +\tvec2 alternate = mod(center.zw, 2.0);\n> +\n> +\tbool even_col = alternate.x < 1.0;\n> +\tbool even_raw = alternate.y < 1.0;\n> +\n> +\t/* .xy = (0,-1).rg, zw = (0, 1).rg */\n> +\tvec4 vals_AD = vec4(\n> +\t\ttexture2D(tex_raw, vec2(center.x, ycoords[0])).rg,\n> +\t\ttexture2D(tex_raw, vec2(center.x, ycoords[1])).rg);\n> +\t/* .xy = (0,0).rg, .z = (-1,0).g, .w = (1,0).r */\n> +\tvec4 vals_BCD = vec4(\n> +\t\ttexture2D(tex_raw, center.xy).rg,\n> +\t\ttexture2D(tex_raw, vec2(xcoords[0], center.y)).g,\n> +\t\ttexture2D(tex_raw, vec2(xcoords[1], center.y)).r);\n> +\t/* .x = (-1,-1).g, .y = (-1,1).g, .z = (1,-1).r, .w = (1,1).r */\n> +\tvec4 vals_D = vec4(\n> +\t\ttexture2D(tex_raw, vec2(xcoords[0], ycoords[0])).g,\n> +\t\ttexture2D(tex_raw, vec2(xcoords[0], ycoords[1])).g,\n> +\t\ttexture2D(tex_raw, vec2(xcoords[1], ycoords[0])).r,\n> +\t\ttexture2D(tex_raw, vec2(xcoords[1], ycoords[1])).r);\n> +\n> +\tvec4 EFGH = vec4(\n> +\t\tvals_AD.x + vals_AD.z,\t\t/* 2*E = (0,-1).r + (0, 1).r */\n> +\t\tvals_AD.y + vals_AD.w, \t\t/* 2*F = (0,-1).g + (0, 1).g */\n> +\t\tvals_BCD.y + vals_BCD.z,\t/* 2*G = (0,0).g + (-1,0).g */\n> +\t\tvals_BCD.x + vals_BCD.w\t\t/* 2*H = (0,0).r + (1,0).r */\n> +\t\t) / 2.0;\n> +\tvec2 JK = vec2(\n> +\t\tvals_D.x + vals_D.y,\t\t/* 2*J = (-1,-1).g + (-1,1).g */\n> +\t\tvals_D.z + vals_D.w\t\t/* 2*K = (1,-1).r + (1,1).r */\n> +\t\t) / 2.0;\n> +\n> +\tif (even_col) {\n> +\t\trgb = (even_raw) ?\n> +\t\t\tvec3(vals_BCD.x, (EFGH.x + EFGH.z) / 2.0,\n> +\t\t\t (EFGH.y + JK.x) / 2.0) :\n> +\t\t\tvec3(EFGH.x, vals_BCD.x, EFGH.z);\n> +\t} else {\n> +\t\trgb = (even_raw) ?\n> +\t\t\tvec3(EFGH.w, vals_BCD.y, EFGH.y) :\n> +\t\t\tvec3((EFGH.x + JK.y) / 2.0,\n> +\t\t\t (EFGH.y + EFGH.w) / 2.0,\n> +\t\t\t vals_BCD.y);\n> +\t}\n> +\tgl_FragColor = vec4(rgb, 1.0);\n\nI'm having a very hard time reviewing this as ABCDEFGHJK are not very\ndescriptive names... Could this be improved, both with better variable\nnames, and a comment to explain the logic ?\n\n> +}\n> diff --git a/src/qcam/assets/shader/shaders.qrc b/src/qcam/assets/shader/shaders.qrc\n> index 8a8f9de1..7bb18033 100644\n> --- a/src/qcam/assets/shader/shaders.qrc\n> +++ b/src/qcam/assets/shader/shaders.qrc\n> @@ -5,6 +5,8 @@\n> \tYUV_2_planes.frag\n> \tYUV_3_planes.frag\n> \tYUV_packed.frag\n> +\tbayer_12_packed.frag\n> +\tbayer.vert\n> \tidentity.vert\n> \n> \n> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> index c74ce77b..e504a6c2 100644\n> --- a/src/qcam/viewfinder_gl.cpp\n> +++ b/src/qcam/viewfinder_gl.cpp\n> @@ -36,6 +36,11 @@ static const QList supportedFormats{\n> \tlibcamera::formats::RGBA8888,\n> \tlibcamera::formats::BGR888,\n> \tlibcamera::formats::RGB888,\n> +\t/* Raw Bayer 12-bit packed */\n> +\tlibcamera::formats::SBGGR12_CSI2P,\n> +\tlibcamera::formats::SGBRG12_CSI2P,\n> +\tlibcamera::formats::SGRBG12_CSI2P,\n> +\tlibcamera::formats::SRGGB12_CSI2P,\n> };\n> \n> ViewFinderGL::ViewFinderGL(QWidget *parent)\n> @@ -115,6 +120,7 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> \tbool ret = true;\n> \n> \tvertexShaderFile_ = \":identity.vert\";\n> +\ttextureMinMaxFilters_ = GL_LINEAR;\n> \n> \tfragmentShaderDefines_.clear();\n> \n> @@ -205,6 +211,34 @@ 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::SBGGR12_CSI2P:\n> +\t\tfirstRed_[0] = 1.0;\n> +\t\tfirstRed_[1] = 1.0;\n> +\t\tfragmentShaderFile_ = \":bayer_12_packed.frag\";\n> +\t\tvertexShaderFile_ = \":bayer.vert\";\n> +\t\ttextureMinMaxFilters_ = GL_NEAREST;\n> +\t\tbreak;\n> +\tcase libcamera::formats::SGBRG12_CSI2P:\n> +\t\tfirstRed_[0] = 0.0;\n> +\t\tfirstRed_[1] = 1.0;\n> +\t\tfragmentShaderFile_ = \":bayer_12_packed.frag\";\n> +\t\tvertexShaderFile_ = \":bayer.vert\";\n> +\t\ttextureMinMaxFilters_ = GL_NEAREST;\n> +\t\tbreak;\n> +\tcase libcamera::formats::SGRBG12_CSI2P:\n> +\t\tfirstRed_[0] = 1.0;\n> +\t\tfirstRed_[1] = 0.0;\n> +\t\tfragmentShaderFile_ = \":bayer_12_packed.frag\";\n> +\t\tvertexShaderFile_ = \":bayer.vert\";\n> +\t\ttextureMinMaxFilters_ = GL_NEAREST;\n> +\t\tbreak;\n> +\tcase libcamera::formats::SRGGB12_CSI2P:\n> +\t\tfirstRed_[0] = 0.0;\n> +\t\tfirstRed_[1] = 0.0;\n> +\t\tfragmentShaderFile_ = \":bayer_12_packed.frag\";\n> +\t\tvertexShaderFile_ = \":bayer.vert\";\n> +\t\ttextureMinMaxFilters_ = GL_NEAREST;\n\nAt some point it may make sense to store all this in an array of format\ninfo structures, but that's not a candidate for this patch.\n\n> +\t\tbreak;\n> \tdefault:\n> \t\tret = false;\n> \t\tqWarning() << \"[ViewFinderGL]:\"\n> @@ -292,6 +326,9 @@ bool ViewFinderGL::createFragmentShader()\n> \ttextureUniformU_ = shaderProgram_.uniformLocation(\"tex_u\");\n> \ttextureUniformV_ = shaderProgram_.uniformLocation(\"tex_v\");\n> \ttextureUniformStepX_ = shaderProgram_.uniformLocation(\"tex_stepx\");\n> +\ttextureUniformRaw_ = shaderProgram_.uniformLocation(\"tex_raw\");\n\nCould we reuse textureUniformY_ as we do for RGB formats ?\n\n> +\ttextureUniformSrcSize_ = shaderProgram_.uniformLocation(\"srcSize\");\n> +\ttextureUniformFirstRed_ = shaderProgram_.uniformLocation(\"firstRed\");\n\nHow about naming these \"tex_bayer_size\" and \"tex_bayer_first_red\" to\nmatch the naming scheme of other uniforms ?\n\n> \n> \t/* Create the textures. */\n> \tfor (std::unique_ptr &texture : textures_) {\n> @@ -308,8 +345,10 @@ bool ViewFinderGL::createFragmentShader()\n> void ViewFinderGL::configureTexture(QOpenGLTexture &texture)\n> {\n> \tglBindTexture(GL_TEXTURE_2D, texture.textureId());\n> -\tglTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);\n> -\tglTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);\n> +\tglTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,\n> +\t\t\ttextureMinMaxFilters_);\n\nShould this be called textureMinMagFilters_ (min stands for minifying\nand mag for magnification according to\nhttps://www.khronos.org/registry/OpenGL-Refpages/es2.0/xhtml/glTexParameter.xml)\n? Or just textureFilter_ as both are identical in this implementation ?\n\n> +\tglTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,\n> +\t\t\ttextureMinMaxFilters_);\n> \tglTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);\n> \tglTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);\n> }\n> @@ -548,6 +587,37 @@ void ViewFinderGL::doRender()\n> \t\tshaderProgram_.setUniformValue(textureUniformY_, 0);\n> \t\tbreak;\n> \n> +\tcase libcamera::formats::SBGGR12_CSI2P:\n> +\tcase libcamera::formats::SGBRG12_CSI2P:\n> +\tcase libcamera::formats::SGRBG12_CSI2P:\n> +\tcase libcamera::formats::SRGGB12_CSI2P:\n> +\t\t/*\n> +\t\t * Packed raw Bayer 12-bit foramts are stored in RGB texture\n\ns/foramts/formats/\n\n> +\t\t * to match the OpenGL texel size with the 3 bytes repeating\n> +\t\t * pattern in RAW12P.\n> +\t\t * The texture width is thus half of the image with.\n\nI dread to think how we'll handle packed RAW10, as we'll need 5 bytes\nfor 4 pixels. I suppose we'll need to use GL_R with an even more\ncomplicated fragment shader :-S\n\n> +\t\t */\n> +\t\tglActiveTexture(GL_TEXTURE0);\n> +\t\tconfigureTexture(*textures_[0]);\n> +\t\tglTexImage2D(GL_TEXTURE_2D,\n> +\t\t\t 0,\n> +\t\t\t GL_RGB,\n> +\t\t\t size_.width() / 2,\n> +\t\t\t size_.height(),\n> +\t\t\t 0,\n> +\t\t\t GL_RGB,\n> +\t\t\t GL_UNSIGNED_BYTE,\n> +\t\t\t data_);\n> +\t\tshaderProgram_.setUniformValue(textureUniformRaw_, 0);\n> +\t\tshaderProgram_.setUniformValue(textureUniformFirstRed_,\n> +\t\t\t\t\t firstRed_[0], firstRed_[1]);\n> +\t\tshaderProgram_.setUniformValue(textureUniformSrcSize_,\n> +\t\t\t\t\t size_.width(),\n> +\t\t\t\t\t size_.height(),\n> +\t\t\t\t\t 1.0f / (size_.width() / 2 - 1),\n> +\t\t\t\t\t 1.0f / (size_.height() - 1));\n\nInstead of bundling the two together, how about turning the float\ntex_stepx into a vec2 tex_step, and using it to convey the step in all\ncases ? The size could be stored in a separate uniform called tex_size.\nThis would make the different shaders a bit more consistent, which\nshould improve readability and maintainability.\n\n> +\t\tbreak;\n> +\n> \tdefault:\n> \t\tbreak;\n> \t};\n> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> index 6cf8f347..186492f3 100644\n> --- a/src/qcam/viewfinder_gl.h\n> +++ b/src/qcam/viewfinder_gl.h\n> @@ -82,6 +82,8 @@ private:\n> \t/* Textures */\n> \tstd::array, 3> textures_;\n> \n> +\t/* Common texture parameters */\n> +\tGLuint textureMinMaxFilters_;\n> \t/* YUV texture parameters */\n> \tGLuint textureUniformU_;\n> \tGLuint textureUniformV_;\n> @@ -89,6 +91,11 @@ private:\n> \tGLuint textureUniformStepX_;\n> \tunsigned int horzSubSample_;\n> \tunsigned int vertSubSample_;\n> +\t/* Raw Bayer texture parameters */\n> +\tGLuint textureUniformRaw_;\n> +\tGLuint textureUniformSrcSize_;\n> +\tGLuint textureUniformFirstRed_;\n> +\tGLfloat firstRed_[2];\n\nWould the code look a bit easier to read if we stored this as\n\n\tQPointF firstRed_;\n\nor\n\n? QOpenGLShaderProgram::setUniformValue() has an overload that takes a\nQPointF.\n\n> \tQMutex mutex_; /* Prevent concurrent access to image_ */\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 9AD87BDB1F\n\tfor ;\n\tTue, 8 Dec 2020 19:04:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E0C667F09;\n\tTue, 8 Dec 2020 20:04:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6B1FB67E4D\n\tfor ;\n\tTue, 8 Dec 2020 20:04:50 +0100 (CET)","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 CECA7543;\n\tTue, 8 Dec 2020 20:04:49 +0100 (CET)"],"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=\"C3QnGFy3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607454290;\n\tbh=NPyL1APj0/LaP+OIBl3AgMUXEdWjJm8lYr5LI+gXpgQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C3QnGFy3VTMHe3rGzvrvMVoSBQCo3lstFAAwhKdtQRPSdaBE9h3FiJDm263MFm1Fz\n\tid0hcGiUb169B4xYUfWo7s1FVSh7MCPYW9PshvUUr4iE+Ke4OGc9if6eOoHTfEpiH6\n\thTZGHfoH2OxnvglYUvOvQtJDZhn4qsAEAkX9WelY=","Date":"Tue, 8 Dec 2020 21:04:46 +0200","From":"Laurent Pinchart ","To":"Andrey Konovalov ","Message-ID":"","References":"<20201109213411.30987-1-andrey.konovalov@linaro.org>\n\t<20201109213411.30987-3-andrey.konovalov@linaro.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201109213411.30987-3-andrey.konovalov@linaro.org>","Subject":"Re: [libcamera-devel] [PATCH][RFC 2/2] qcam: viewfinder_gl: Add\n\tshader to render packed RAW12 formats","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]