[{"id":12335,"web_url":"https://patchwork.libcamera.org/comment/12335/","msgid":"<20200906161807.GA6047@pendragon.ideasonboard.com>","date":"2020-09-06T16:18:07","subject":"Re: [libcamera-devel] [PATCH v5 3/4] qcam: add viewfinderGL class\n\tto accelerate the format convert","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Show,\n\nThank you for the patch.\n\nOn Fri, Sep 04, 2020 at 04:43:15PM +0800, Show Liu wrote:\n> the viewfinderGL accelerates the format conversion by\n> using OpenGL ES shader\n> \n> Signed-off-by: Show Liu \n> ---\n> src/qcam/meson.build | 2 +\n> src/qcam/viewfinder_gl.cpp | 441 +++++++++++++++++++++++++++++++++++++\n> src/qcam/viewfinder_gl.h | 97 ++++++++\n> 3 files changed, 540 insertions(+)\n> create mode 100644 src/qcam/viewfinder_gl.cpp\n> create mode 100644 src/qcam/viewfinder_gl.h\n> \n> diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> index a4bad0a..32c0fc3 100644\n> --- a/src/qcam/meson.build\n> +++ b/src/qcam/meson.build\n> @@ -7,11 +7,13 @@ qcam_sources = files([\n> 'main.cpp',\n> 'main_window.cpp',\n> 'viewfinder_qt.cpp',\n> + 'viewfinder_gl.cpp',\n\nLet's keep files alphabetically sorted.\n\n> ])\n> \n> qcam_moc_headers = files([\n> 'main_window.h',\n> 'viewfinder_qt.h',\n> + 'viewfinder_gl.h',\n\nHere too.\n\n> ])\n> \n> qcam_resources = files([\n\nYou need to set the minimum Qt version to 5.4, as QOpenGLWidget wasn't\navailable before that.\n\ndiff --git a/src/qcam/meson.build b/src/qcam/meson.build\nindex 32c0fc3e0f6b..9d3f189a896b 100644\n--- a/src/qcam/meson.build\n+++ b/src/qcam/meson.build\n@@ -25,7 +25,8 @@ qt5 = import('qt5')\n qt5_dep = dependency('qt5',\n method : 'pkg-config',\n modules : ['Core', 'Gui', 'Widgets'],\n- required : get_option('qcam'))\n+ required : get_option('qcam'),\n+ version : '>=5.4')\n\n if qt5_dep.found()\n qcam_deps = [\n\nFurthermore, Qt can be compiled without OpenGL support, in which case\nthis patch will fail to compile. The following change should address it.\n\ndiff --git a/meson.build b/meson.build\nindex b6c99ba8e0eb..5f7d619a79d0 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -26,6 +26,7 @@ libcamera_version = libcamera_git_version.split('+')[0]\n \n # Configure the build environment.\n cc = meson.get_compiler('c')\n+cxx = meson.get_compiler('cpp')\n config_h = configuration_data()\n \n if cc.has_header_symbol('execinfo.h', 'backtrace')\ndiff --git a/src/qcam/meson.build b/src/qcam/meson.build\nindex 32c0fc3e0f6b..9bb48c0d06c5 100644\n--- a/src/qcam/meson.build\n+++ b/src/qcam/meson.build\n@@ -7,18 +7,15 @@ qcam_sources = files([\n 'main.cpp',\n 'main_window.cpp',\n 'viewfinder_qt.cpp',\n- 'viewfinder_gl.cpp',\n ])\n \n qcam_moc_headers = files([\n 'main_window.h',\n 'viewfinder_qt.h',\n- 'viewfinder_gl.h',\n ])\n \n qcam_resources = files([\n 'assets/feathericons/feathericons.qrc',\n- 'assets/shader/shaders.qrc'\n ])\n \n qt5 = import('qt5')\n@@ -44,6 +42,19 @@ if qt5_dep.found()\n ])\n endif\n \n+ if cxx.has_header_symbol('QOpenGLWidget', 'QOpenGLWidget',\n+ dependencies : qt5_dep, args : '-fPIC')\n+ qcam_sources += files([\n+ 'viewfinder_gl.cpp',\n+ ])\n+ qcam_moc_headers += files([\n+ 'viewfinder_gl.h',\n+ ])\n+ qcam_resources += files([\n+ 'assets/shader/shaders.qrc'\n+ ])\n+ endif\n+\n # gcc 9 introduced a deprecated-copy warning that is triggered by Qt until\n # Qt 5.13. clang 10 introduced the same warning, but detects more issues\n # that are not fixed in Qt yet. Disable the warning manually in both cases.\n\nPatch 4/4 will need to be updated to with conditional compilation on\nQT_NO_OPENGL.\n\n> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> new file mode 100644\n> index 0000000..5591916\n> --- /dev/null\n> +++ b/src/qcam/viewfinder_gl.cpp\n> @@ -0,0 +1,441 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Linaro\n> + *\n> + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader\n> + */\n> +\n> +#include \"viewfinder_gl.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +#define ATTRIB_VERTEX 0\n> +#define ATTRIB_TEXTURE 1\n\nIs there a guarantee that attribute locations match the declaration\norder in the shader program ? Wouldn't it be better to use\nshaderProgram.attributeLocation() to retrieve the attribute locations by\nname (before linking), or shaderProgram.bindAttributeLocation() to set\nthem explicitly (after linking) ?\n\n> +\n> +static const QList supportFormats{\n\ns/supportFormats/supportedFormats/\n\n> +\tlibcamera::formats::NV12,\n> +\tlibcamera::formats::NV21,\n> +\tlibcamera::formats::NV16,\n> +\tlibcamera::formats::NV61,\n> +\tlibcamera::formats::NV24,\n> +\tlibcamera::formats::NV42,\n> +\tlibcamera::formats::YUV420,\n> +\tlibcamera::formats::YVU420\n> +};\n> +\n> +ViewFinderGL::ViewFinderGL(QWidget *parent)\n> +\t: QOpenGLWidget(parent),\n> +\t buffer_(nullptr),\n> +\t pFShader_(nullptr),\n> +\t pVShader_(nullptr),\n> +\t vbuf_(QOpenGLBuffer::VertexBuffer),\n> +\t yuvDataPtr_(nullptr),\n> +\t textureU_(QOpenGLTexture::Target2D),\n> +\t textureV_(QOpenGLTexture::Target2D),\n> +\t textureY_(QOpenGLTexture::Target2D)\n\nFeel free to have multiple members per line if desired.\n\n> +{\n> +}\n> +\n> +ViewFinderGL::~ViewFinderGL()\n> +{\n> +\tremoveShader();\n> +\n> +\tif (textureY_.isCreated())\n> +\t\ttextureY_.destroy();\n> +\n> +\tif (textureU_.isCreated())\n> +\t\ttextureU_.destroy();\n> +\n> +\tif (textureV_.isCreated())\n> +\t\ttextureV_.destroy();\n\nAre these needed, or does the QOpenGLTexture destructor destroy the\ntextures ?\n\n> +\n> +\tvbuf_.destroy();\n\nSame question for vbuf_.\n\n> +}\n> +\n> +const QList &ViewFinderGL::nativeFormats() const\n> +{\n> +\treturn (::supportFormats);\n\nNo need for parentheses or an explicit namespace.\n\n\treturn supportedFormats;\n\n> +}\n> +\n> +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> +\t\t\t const QSize &size)\n> +{\n> +\tint ret = 0;\n> +\n> +\tif (isFormatSupport(format)) {\n> +\t\tformat_ = format;\n> +\t\tsize_ = size;\n> +\t} else {\n> +\t\tret = -1;\n> +\t}\n> +\tupdateGeometry();\n\nWhen the format change, don't you need to recreate the shaders ?\n\n> +\treturn ret;\n> +}\n> +\n> +void ViewFinderGL::stop()\n> +{\n> +\tif (buffer_) {\n> +\t\trenderComplete(buffer_);\n> +\t\tbuffer_ = nullptr;\n> +\t}\n> +}\n> +\n> +QImage ViewFinderGL::getCurrentImage()\n> +{\n> +\tQMutexLocker locker(&mutex_);\n> +\n> +\treturn (grabFramebuffer());\n\n\treturn grabFrameBuffer();\n\n> +}\n> +\n> +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> +{\n> +\tif (buffer->planes().size() != 1) {\n> +\t\tqWarning() << \"Multi-planar buffers are not supported\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (buffer_)\n> +\t\trenderComplete(buffer_);\n> +\n> +\tunsigned char *memory = static_cast(map->memory);\n> +\tif (memory) {\n\nCan memory be null ?\n\n> +\t\tyuvDataPtr_ = memory;\n> +\t\tupdate();\n> +\t\tbuffer_ = buffer;\n> +\t}\n> +}\n> +\n> +bool ViewFinderGL::isFormatSupport(const libcamera::PixelFormat &format)\n> +{\n\nAs this function sets internal members based on the format, I would call\nit selectFormat(), it does more than just checking if the format is\nsupported.\n\n> +\tbool ret = true;\n> +\tswitch (format) {\n> +\tcase libcamera::formats::NV12:\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 2;\n> +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> +\t\tfsrc_ = \":NV_2_planes_UV_f.glsl\";\n> +\t\tbreak;\n> +\tcase libcamera::formats::NV21:\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 2;\n> +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> +\t\tfsrc_ = \":NV_2_planes_VU_f.glsl\";\n> +\t\tbreak;\n> +\tcase libcamera::formats::NV16:\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 1;\n> +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> +\t\tfsrc_ = \":NV_2_planes_UV_f.glsl\";\n> +\t\tbreak;\n> +\tcase libcamera::formats::NV61:\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 1;\n> +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> +\t\tfsrc_ = \":NV_2_planes_VU_f.glsl\";\n> +\t\tbreak;\n> +\tcase libcamera::formats::NV24:\n> +\t\thorzSubSample_ = 1;\n> +\t\tvertSubSample_ = 1;\n> +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> +\t\tfsrc_ = \":NV_2_planes_UV_f.glsl\";\n> +\t\tbreak;\n> +\tcase libcamera::formats::NV42:\n> +\t\thorzSubSample_ = 1;\n> +\t\tvertSubSample_ = 1;\n> +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> +\t\tfsrc_ = \":NV_2_planes_VU_f.glsl\";\n> +\t\tbreak;\n> +\tcase libcamera::formats::YUV420:\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 2;\n> +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> +\t\tfsrc_ = \":NV_3_planes_UV_f.glsl\";\n> +\t\tbreak;\n> +\tcase libcamera::formats::YVU420:\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 2;\n> +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> +\t\tfsrc_ = \":NV_3_planes_VU_f.glsl\";\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tret = false;\n> +\t\tqWarning() << \"[ViewFinderGL]:\"\n> +\t\t\t << \"format not support yet.\";\n\ns/support yet./supported/\n\n> +\t\tbreak;\n> +\t};\n> +\n> +\treturn ret;\n> +}\n> +\n> +void ViewFinderGL::createVertexShader()\n> +{\n> +\tbool bCompile;\n\nNo need to prefix variables with the type name.\n\n> +\t/* Create Vertex Shader */\n> +\tpVShader_ = new QOpenGLShader(QOpenGLShader::Vertex, this);\n> +\n> +\tbCompile = pVShader_->compileSourceFile(vsrc_);\n> +\tif (!bCompile) {\n> +\t\tqWarning() << \"[ViewFinderGL]:\" << pVShader_->log();\n> +\t}\n\nThis can simply be written\n\n\tif (!pVShader_->compileSourceFile(vsrc_))\n\t\tqWarning() << \"[ViewFinderGL]:\" << pVShader_->log();\n\n> +\n> +\tshaderProgram_.addShader(pVShader_);\n\nWon't this crash if shader compilation failed ? I think\ncreateVertexShader() should return a status as a bool.\n\n> +}\n> +\n> +bool ViewFinderGL::createFragmentShader()\n> +{\n> +\tbool bCompile;\n> +\n> +\t/* Create Fragment Shader */\n> +\tpFShader_ = new QOpenGLShader(QOpenGLShader::Fragment, this);\n> +\n> +\tbCompile = pFShader_->compileSourceFile(fsrc_);\n> +\tif (!bCompile) {\n\n\tif (!pFShader_->compileSourceFile(fsrc_)) {\n\n> +\t\tqWarning() << \"[ViewFinderGL]:\" << pFShader_->log();\n> +\t\treturn bCompile;\n\n\t\treturn false;\n\n> +\t}\n> +\n> +\tshaderProgram_.addShader(pFShader_);\n> +\n> +\t/* Link shader pipeline */\n> +\tif (!shaderProgram_.link()) {\n> +\t\tqWarning() << \"[ViewFinderGL]:\" << shaderProgram_.log();\n> +\t\tclose();\n> +\t}\n> +\n> +\t/* Bind shader pipeline for use */\n> +\tif (!shaderProgram_.bind()) {\n> +\t\tqWarning() << \"[ViewFinderGL]:\" << shaderProgram_.log();\n> +\t\tclose();\n> +\t}\n> +\n> +\tshaderProgram_.enableAttributeArray(ATTRIB_VERTEX);\n> +\tshaderProgram_.enableAttributeArray(ATTRIB_TEXTURE);\n> +\n> +\tshaderProgram_.setAttributeBuffer(ATTRIB_VERTEX,\n> +\t\t\t\t\t GL_FLOAT,\n> +\t\t\t\t\t 0,\n> +\t\t\t\t\t 2,\n> +\t\t\t\t\t 2 * sizeof(GLfloat));\n> +\tshaderProgram_.setAttributeBuffer(ATTRIB_TEXTURE,\n> +\t\t\t\t\t GL_FLOAT,\n> +\t\t\t\t\t 8 * sizeof(GLfloat),\n> +\t\t\t\t\t 2,\n> +\t\t\t\t\t 2 * sizeof(GLfloat));\n> +\n> +\ttextureUniformY_ = shaderProgram_.uniformLocation(\"tex_y\");\n> +\ttextureUniformU_ = shaderProgram_.uniformLocation(\"tex_u\");\n> +\ttextureUniformV_ = shaderProgram_.uniformLocation(\"tex_v\");\n> +\n> +\tif (!textureY_.isCreated())\n> +\t\ttextureY_.create();\n> +\n> +\tif (!textureU_.isCreated())\n> +\t\ttextureU_.create();\n> +\n> +\tif (!textureV_.isCreated())\n> +\t\ttextureV_.create();\n> +\n> +\tid_y_ = textureY_.textureId();\n> +\tid_u_ = textureU_.textureId();\n> +\tid_v_ = textureV_.textureId();\n> +\treturn true;\n> +}\n> +\n> +void ViewFinderGL::configureTexture(unsigned int id)\n> +{\n> +\tglBindTexture(GL_TEXTURE_2D, id);\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_WRAP_S, GL_CLAMP_TO_EDGE);\n> +\tglTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);\n> +}\n> +\n> +void ViewFinderGL::removeShader()\n> +{\n> +\tif (shaderProgram_.isLinked()) {\n> +\t\tshaderProgram_.release();\n> +\t\tshaderProgram_.removeAllShaders();\n> +\t}\n> +\n> +\tif (pFShader_)\n> +\t\tdelete pFShader_;\n> +\n> +\tif (pVShader_)\n> +\t\tdelete pVShader_;\n> +}\n> +\n> +void ViewFinderGL::initializeGL()\n> +{\n> +\tinitializeOpenGLFunctions();\n> +\tglEnable(GL_TEXTURE_2D);\n> +\tglDisable(GL_DEPTH_TEST);\n> +\n> +\tstatic const GLfloat vertices[]{\n> +\t\t-1.0f, -1.0f, -1.0f, +1.0f,\n> +\t\t+1.0f, +1.0f, +1.0f, -1.0f,\n> +\t\t0.0f, 1.0f, 0.0f, 0.0f,\n> +\t\t1.0f, 0.0f, 1.0f, 1.0f\n> +\t};\n\nThis is vertex and texture coordinates, not just vertices. How about\nwriting it as follows ?\n\n\tstatic const GLfloat coordinates[2][4][2] {\n\t\t{\n\t\t\t/* Vertex coordinates */\n\t\t\t{ -1.0f, -1.0f },\n\t\t\t{ -1.0f, +1.0f },\n\t\t\t{ +1.0f, +1.0f },\n\t\t\t{ +1.0f, -1.0f },\n\t\t}, {\n\t\t\t/* Texture coordinates */\n\t\t\t{ 0.0f, 1.0f },\n\t\t\t{ 0.0f, 0.0f },\n\t\t\t{ 1.0f, 0.0f },\n\t\t\t{ 1.0f, 1.0f },\n\t\t},\n\t};\n\nThere's something I don't get though, maybe you can help me understand\nit. The vertex coordinates are copied directly to gl_Position in the\nvertex shader, so they're essentially expressed in clip space, which I\nunderstand has X pointing towards the right and Y pointing towards the\ntop. The texture coordinates, if my understand is correct again, have\ntheir origin at the bottom-left corner too. The first vertex, (-1.0,\n-1.0), which is at the bottom-left, then maps to texture coordinate\n(0.0, 1.0), which is the top-left pixel of the texture. The image should\nthus be flipped vertically. Why isn't it ? I'm sure I'm missing\nsomethign simple.\n\n> +\n> +\tvbuf_.create();\n> +\tvbuf_.bind();\n> +\tvbuf_.allocate(vertices, sizeof(vertices));\n> +\n> +\t/* Create Vertex Shader */\n> +\tcreateVertexShader();\n> +\n> +\tglClearColor(1.0f, 1.0f, 1.0f, 0.0f);\n> +}\n> +\n> +void ViewFinderGL::doRender()\n> +{\n> +\tswitch (format_) {\n> +\tcase libcamera::formats::NV12:\n> +\tcase libcamera::formats::NV21:\n> +\tcase libcamera::formats::NV16:\n> +\tcase libcamera::formats::NV61:\n> +\tcase libcamera::formats::NV24:\n> +\tcase libcamera::formats::NV42:\n> +\t\t/* activate texture 0 */\n> +\t\tglActiveTexture(GL_TEXTURE0);\n> +\t\tconfigureTexture(id_y_);\n> +\t\tglTexImage2D(GL_TEXTURE_2D,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t size_.width(),\n> +\t\t\t size_.height(),\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t GL_UNSIGNED_BYTE,\n> +\t\t\t yuvDataPtr_);\n> +\t\tglUniform1i(textureUniformY_, 0);\n\nWould it make sense to use\n\n\t\tshaderProgram_.setUniformValue(textureUniformY_, 0);\n\n(and similarly below) ?\n\n> +\n> +\t\t/* activate texture 1 */\n> +\t\tglActiveTexture(GL_TEXTURE1);\n> +\t\tconfigureTexture(id_u_);\n> +\t\tglTexImage2D(GL_TEXTURE_2D,\n> +\t\t\t 0,\n> +\t\t\t GL_RG,\n> +\t\t\t size_.width() / horzSubSample_,\n> +\t\t\t size_.height() / vertSubSample_,\n> +\t\t\t 0,\n> +\t\t\t GL_RG,\n> +\t\t\t GL_UNSIGNED_BYTE,\n> +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height());\n> +\t\tglUniform1i(textureUniformU_, 1);\n> +\t\tbreak;\n\nA blank line here would increase readability. Same below.\n\n> +\tcase libcamera::formats::YUV420:\n> +\t\t/* activate texture 0 */\n> +\t\tglActiveTexture(GL_TEXTURE0);\n> +\t\tconfigureTexture(id_y_);\n> +\t\tglTexImage2D(GL_TEXTURE_2D,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t size_.width(),\n> +\t\t\t size_.height(),\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t GL_UNSIGNED_BYTE,\n> +\t\t\t yuvDataPtr_);\n> +\t\tglUniform1i(textureUniformY_, 0);\n> +\n> +\t\t/* activate texture 1 */\n> +\t\tglActiveTexture(GL_TEXTURE1);\n> +\t\tconfigureTexture(id_u_);\n> +\t\tglTexImage2D(GL_TEXTURE_2D,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t size_.width() / horzSubSample_,\n> +\t\t\t size_.height() / vertSubSample_,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t GL_UNSIGNED_BYTE,\n> +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height());\n> +\t\tglUniform1i(textureUniformU_, 1);\n> +\n> +\t\t/* activate texture 2 */\n> +\t\tglActiveTexture(GL_TEXTURE2);\n> +\t\tconfigureTexture(id_v_);\n> +\t\tglTexImage2D(GL_TEXTURE_2D,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t size_.width() / horzSubSample_,\n> +\t\t\t size_.height() / vertSubSample_,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t GL_UNSIGNED_BYTE,\n> +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height() * 5 / 4);\n> +\t\tglUniform1i(textureUniformV_, 2);\n> +\t\tbreak;\n> +\tcase libcamera::formats::YVU420:\n> +\t\t/* activate texture 0 */\n> +\t\tglActiveTexture(GL_TEXTURE0);\n> +\t\tconfigureTexture(id_y_);\n> +\t\tglTexImage2D(GL_TEXTURE_2D,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t size_.width(),\n> +\t\t\t size_.height(),\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t GL_UNSIGNED_BYTE,\n> +\t\t\t yuvDataPtr_);\n> +\t\tglUniform1i(textureUniformY_, 0);\n> +\n> +\t\t/* activate texture 1 */\n> +\t\tglActiveTexture(GL_TEXTURE2);\n> +\t\tconfigureTexture(id_v_);\n> +\t\tglTexImage2D(GL_TEXTURE_2D,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t size_.width() / horzSubSample_,\n> +\t\t\t size_.height() / vertSubSample_,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t GL_UNSIGNED_BYTE,\n> +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height());\n> +\t\tglUniform1i(textureUniformV_, 1);\n\nOK, now I understand why the NV_3_planes_UV_f.glsl and\nNV_3_planes_VU_f.glsl shaders are identical, you switch the U and V\nplanes here. I think we should then merge the two files into\nNV_3_planes_f_glsl. The above line should become\n\n\t\tglUniform1i(textureUniformU_, 2);\n\nas you deal with texture 2 here (and a similar change is needed below),\nand the two blocks should be swapped as the comments are incorrect (the\ncomment above refers to texture 1 while the code deals with texture 2).\n\n> +\n> +\t\t/* activate texture 2 */\n> +\t\tglActiveTexture(GL_TEXTURE1);\n> +\t\tconfigureTexture(id_u_);\n> +\t\tglTexImage2D(GL_TEXTURE_2D,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t size_.width() / horzSubSample_,\n> +\t\t\t size_.height() / vertSubSample_,\n> +\t\t\t 0,\n> +\t\t\t GL_RED,\n> +\t\t\t GL_UNSIGNED_BYTE,\n> +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height() * 5 / 4);\n> +\t\tglUniform1i(textureUniformU_, 2);\n\nPlease add a break here, let's not rely on implicit fall-through.\n\n> +\tdefault:\n> +\t\tbreak;\n> +\t};\n> +}\n> +\n> +void ViewFinderGL::paintGL()\n> +{\n> +\tif (pFShader_ == nullptr)\n\n\tif (!pfShader_)\n\n> +\t\tcreateFragmentShader();\n> +\n> +\tif (yuvDataPtr_) {\n> +\t\tglClearColor(0.0, 0.0, 0.0, 1.0);\n> +\t\tglClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);\n> +\n> +\t\tdoRender();\n> +\t\tglDrawArrays(GL_TRIANGLE_FAN, 0, 4);\n> +\t}\n> +}\n> +\n> +void ViewFinderGL::resizeGL(int w, int h)\n> +{\n> +\tglViewport(0, 0, w, h);\n> +}\n> +\n> +QSize ViewFinderGL::sizeHint() const\n> +{\n> +\treturn size_.isValid() ? size_ : QSize(640, 480);\n> +}\n> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> new file mode 100644\n> index 0000000..e708c32\n> --- /dev/null\n> +++ b/src/qcam/viewfinder_gl.h\n> @@ -0,0 +1,97 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Linaro\n> + *\n> + * viewfinder_GL.h - OpenGL Viewfinder for rendering by OpenGL shader\n> + *\n> + */\n> +#ifndef __VIEWFINDER_GL_H__\n> +#define __VIEWFINDER_GL_H__\n> +\n> +#include \n> +#include \n> +#include \n\nThis header shouldn't be needed.\n\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n\nMissing blank line.\n\n> +#include \"viewfinder.h\"\n> +\n> +class ViewFinderGL : public QOpenGLWidget,\n> +\t\t public ViewFinder,\n> +\t\t protected QOpenGLFunctions\n> +{\n> +\tQ_OBJECT\n> +\n> +public:\n> +\tViewFinderGL(QWidget *parent = 0);\n\n = nullptr\n\n> +\t~ViewFinderGL();\n> +\n> +\tconst QList &nativeFormats() const override;\n> +\n> +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> +\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> +\tvoid stop() override;\n> +\n> +\tQImage getCurrentImage() override;\n> +\n> +Q_SIGNALS:\n> +\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> +\n> +protected:\n> +\tvoid initializeGL() override;\n> +\tvoid paintGL() override;\n> +\tvoid resizeGL(int w, int h) override;\n> +\tQSize sizeHint() const override;\n> +\n> +private:\n> +\tbool isFormatSupport(const libcamera::PixelFormat &format);\n\ns/isFormatSupport/isFormatSupported/\n\n> +\n> +\tvoid configureTexture(unsigned int id);\n> +\tbool createFragmentShader();\n> +\tvoid createVertexShader();\n> +\tvoid removeShader();\n> +\tvoid doRender();\n> +\n> +\t/* Captured image size, format and buffer */\n> +\tlibcamera::FrameBuffer *buffer_;\n> +\tlibcamera::PixelFormat format_;\n> +\tQSize size_;\n> +\n> +\t/* OpenGL components for render */\n\ns/render/rendering/\n\n> +\tQOpenGLShader *pFShader_;\n\nNo need to prefix pointers with 'p'. I'd name this fragmentShader_.\n\n> +\tQOpenGLShader *pVShader_;\n\nSame here, vertexShader_.\n\n> +\tQOpenGLShaderProgram shaderProgram_;\n\nIs there a specific reason why pFShader_ and pVShader_ are pointers,\nwhile shaderProgram_ is embedded directly in ViewFinderGL ?\n\n> +\n> +\t/* Vertex buffer */\n> +\tQOpenGLBuffer vbuf_;\n> +\n> +\t/* Fragment and Vertex shader file name */\n> +\tQString fsrc_;\n\nfragmentShaderSrc_ ?\n\n> +\tQString vsrc_;\n\nAnd vertexShaderSrc_.\n\n> +\n> +\tunsigned char *yuvDataPtr_;\n\nAnd no need for a Ptr suffix either :-)\n\n> +\n> +\t/* YUV texture planars and parameters */\n> +\tGLuint id_u_;\n> +\tGLuint id_v_;\n> +\tGLuint id_y_;\n> +\tGLuint textureUniformU_;\n> +\tGLuint textureUniformV_;\n> +\tGLuint textureUniformY_;\n> +\tQOpenGLTexture textureU_;\n> +\tQOpenGLTexture textureV_;\n> +\tQOpenGLTexture textureY_;\n> +\tunsigned int horzSubSample_;\n> +\tunsigned int vertSubSample_;\n> +\n> +\tQImage image_;\n\nThis is never used.\n\n> +\tQMutex mutex_; /* Prevent concurrent access to image_ */\n> +};\n> +#endif /* __VIEWFINDER_GL_H__ */","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 61F5FBDB1D\n\tfor ;\n\tSun, 6 Sep 2020 16:18:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE4D162B5D;\n\tSun, 6 Sep 2020 18:18:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A36960370\n\tfor ;\n\tSun, 6 Sep 2020 18:18: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 D1FDC276;\n\tSun, 6 Sep 2020 18:18: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=\"p4hiiyqY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599409111;\n\tbh=Iul+5JA/QttnBD4BYfYx2ao4geAYMWY6rchbHmuoQZ0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p4hiiyqYdWVzb6WnN937OQI7GopneUkTD/DiyYS/WQ4+hFxf5wKe2WLH/fIgRT3/l\n\tXXxXO00NGbjxB4YfM3H/su/EDmOuZ/mhmwz9/UpvYOYOu9IduEePR3hxwOe+wwz08i\n\tUkx1lrzJ3X2XqBLucK4Lnq+h1CxoaQ8Uo4ifm38w=","Date":"Sun, 6 Sep 2020 19:18:07 +0300","From":"Laurent Pinchart ","To":"Show Liu ","Message-ID":"<20200906161807.GA6047@pendragon.ideasonboard.com>","References":"<20200904084316.7319-1-show.liu@linaro.org>\n\t<20200904084316.7319-5-show.liu@linaro.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200904084316.7319-5-show.liu@linaro.org>","Subject":"Re: [libcamera-devel] [PATCH v5 3/4] qcam: add viewfinderGL class\n\tto accelerate the format convert","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\" "}},{"id":12339,"web_url":"https://patchwork.libcamera.org/comment/12339/","msgid":"<20200906231001.GA15818@pendragon.ideasonboard.com>","date":"2020-09-06T23:10:01","subject":"Re: [libcamera-devel] [PATCH v5 3/4] qcam: add viewfinderGL class\n\tto accelerate the format convert","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Sep 06, 2020 at 07:18:07PM +0300, Laurent Pinchart wrote:\n> Hi Show,\n> \n> Thank you for the patch.\n> \n> On Fri, Sep 04, 2020 at 04:43:15PM +0800, Show Liu wrote:\n> > the viewfinderGL accelerates the format conversion by\n> > using OpenGL ES shader\n> > \n> > Signed-off-by: Show Liu \n> > ---\n> > src/qcam/meson.build | 2 +\n> > src/qcam/viewfinder_gl.cpp | 441 +++++++++++++++++++++++++++++++++++++\n> > src/qcam/viewfinder_gl.h | 97 ++++++++\n> > 3 files changed, 540 insertions(+)\n> > create mode 100644 src/qcam/viewfinder_gl.cpp\n> > create mode 100644 src/qcam/viewfinder_gl.h\n> > \n> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > index a4bad0a..32c0fc3 100644\n> > --- a/src/qcam/meson.build\n> > +++ b/src/qcam/meson.build\n> > @@ -7,11 +7,13 @@ qcam_sources = files([\n> > 'main.cpp',\n> > 'main_window.cpp',\n> > 'viewfinder_qt.cpp',\n> > + 'viewfinder_gl.cpp',\n> \n> Let's keep files alphabetically sorted.\n> \n> > ])\n> > \n> > qcam_moc_headers = files([\n> > 'main_window.h',\n> > 'viewfinder_qt.h',\n> > + 'viewfinder_gl.h',\n> \n> Here too.\n> \n> > ])\n> > \n> > qcam_resources = files([\n> \n> You need to set the minimum Qt version to 5.4, as QOpenGLWidget wasn't\n> available before that.\n> \n> diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> index 32c0fc3e0f6b..9d3f189a896b 100644\n> --- a/src/qcam/meson.build\n> +++ b/src/qcam/meson.build\n> @@ -25,7 +25,8 @@ qt5 = import('qt5')\n> qt5_dep = dependency('qt5',\n> method : 'pkg-config',\n> modules : ['Core', 'Gui', 'Widgets'],\n> - required : get_option('qcam'))\n> + required : get_option('qcam'),\n> + version : '>=5.4')\n> \n> if qt5_dep.found()\n> qcam_deps = [\n> \n> Furthermore, Qt can be compiled without OpenGL support, in which case\n> this patch will fail to compile. The following change should address it.\n> \n> diff --git a/meson.build b/meson.build\n> index b6c99ba8e0eb..5f7d619a79d0 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -26,6 +26,7 @@ libcamera_version = libcamera_git_version.split('+')[0]\n> \n> # Configure the build environment.\n> cc = meson.get_compiler('c')\n> +cxx = meson.get_compiler('cpp')\n> config_h = configuration_data()\n> \n> if cc.has_header_symbol('execinfo.h', 'backtrace')\n> diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> index 32c0fc3e0f6b..9bb48c0d06c5 100644\n> --- a/src/qcam/meson.build\n> +++ b/src/qcam/meson.build\n> @@ -7,18 +7,15 @@ qcam_sources = files([\n> 'main.cpp',\n> 'main_window.cpp',\n> 'viewfinder_qt.cpp',\n> - 'viewfinder_gl.cpp',\n> ])\n> \n> qcam_moc_headers = files([\n> 'main_window.h',\n> 'viewfinder_qt.h',\n> - 'viewfinder_gl.h',\n> ])\n> \n> qcam_resources = files([\n> 'assets/feathericons/feathericons.qrc',\n> - 'assets/shader/shaders.qrc'\n> ])\n> \n> qt5 = import('qt5')\n> @@ -44,6 +42,19 @@ if qt5_dep.found()\n> ])\n> endif\n> \n> + if cxx.has_header_symbol('QOpenGLWidget', 'QOpenGLWidget',\n> + dependencies : qt5_dep, args : '-fPIC')\n> + qcam_sources += files([\n> + 'viewfinder_gl.cpp',\n> + ])\n> + qcam_moc_headers += files([\n> + 'viewfinder_gl.h',\n> + ])\n> + qcam_resources += files([\n> + 'assets/shader/shaders.qrc'\n> + ])\n> + endif\n> +\n> # gcc 9 introduced a deprecated-copy warning that is triggered by Qt until\n> # Qt 5.13. clang 10 introduced the same warning, but detects more issues\n> # that are not fixed in Qt yet. Disable the warning manually in both cases.\n> \n> Patch 4/4 will need to be updated to with conditional compilation on\n> QT_NO_OPENGL.\n> \n> > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > new file mode 100644\n> > index 0000000..5591916\n> > --- /dev/null\n> > +++ b/src/qcam/viewfinder_gl.cpp\n> > @@ -0,0 +1,441 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Linaro\n> > + *\n> > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader\n> > + */\n> > +\n> > +#include \"viewfinder_gl.h\"\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#define ATTRIB_VERTEX 0\n> > +#define ATTRIB_TEXTURE 1\n> \n> Is there a guarantee that attribute locations match the declaration\n> order in the shader program ? Wouldn't it be better to use\n> shaderProgram.attributeLocation() to retrieve the attribute locations by\n> name (before linking), or shaderProgram.bindAttributeLocation() to set\n> them explicitly (after linking) ?\n> \n> > +\n> > +static const QList supportFormats{\n> \n> s/supportFormats/supportedFormats/\n> \n> > +\tlibcamera::formats::NV12,\n> > +\tlibcamera::formats::NV21,\n> > +\tlibcamera::formats::NV16,\n> > +\tlibcamera::formats::NV61,\n> > +\tlibcamera::formats::NV24,\n> > +\tlibcamera::formats::NV42,\n> > +\tlibcamera::formats::YUV420,\n> > +\tlibcamera::formats::YVU420\n> > +};\n> > +\n> > +ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > +\t: QOpenGLWidget(parent),\n> > +\t buffer_(nullptr),\n> > +\t pFShader_(nullptr),\n> > +\t pVShader_(nullptr),\n> > +\t vbuf_(QOpenGLBuffer::VertexBuffer),\n> > +\t yuvDataPtr_(nullptr),\n> > +\t textureU_(QOpenGLTexture::Target2D),\n> > +\t textureV_(QOpenGLTexture::Target2D),\n> > +\t textureY_(QOpenGLTexture::Target2D)\n> \n> Feel free to have multiple members per line if desired.\n> \n> > +{\n> > +}\n> > +\n> > +ViewFinderGL::~ViewFinderGL()\n> > +{\n> > +\tremoveShader();\n> > +\n> > +\tif (textureY_.isCreated())\n> > +\t\ttextureY_.destroy();\n> > +\n> > +\tif (textureU_.isCreated())\n> > +\t\ttextureU_.destroy();\n> > +\n> > +\tif (textureV_.isCreated())\n> > +\t\ttextureV_.destroy();\n> \n> Are these needed, or does the QOpenGLTexture destructor destroy the\n> textures ?\n> \n> > +\n> > +\tvbuf_.destroy();\n> \n> Same question for vbuf_.\n> \n> > +}\n> > +\n> > +const QList &ViewFinderGL::nativeFormats() const\n> > +{\n> > +\treturn (::supportFormats);\n> \n> No need for parentheses or an explicit namespace.\n> \n> \treturn supportedFormats;\n> \n> > +}\n> > +\n> > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> > +\t\t\t const QSize &size)\n> > +{\n> > +\tint ret = 0;\n> > +\n> > +\tif (isFormatSupport(format)) {\n> > +\t\tformat_ = format;\n> > +\t\tsize_ = size;\n> > +\t} else {\n> > +\t\tret = -1;\n> > +\t}\n> > +\tupdateGeometry();\n> \n> When the format change, don't you need to recreate the shaders ?\n> \n> > +\treturn ret;\n> > +}\n> > +\n> > +void ViewFinderGL::stop()\n> > +{\n> > +\tif (buffer_) {\n> > +\t\trenderComplete(buffer_);\n> > +\t\tbuffer_ = nullptr;\n> > +\t}\n> > +}\n> > +\n> > +QImage ViewFinderGL::getCurrentImage()\n> > +{\n> > +\tQMutexLocker locker(&mutex_);\n> > +\n> > +\treturn (grabFramebuffer());\n> \n> \treturn grabFrameBuffer();\n> \n> > +}\n> > +\n> > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > +{\n> > +\tif (buffer->planes().size() != 1) {\n> > +\t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tif (buffer_)\n> > +\t\trenderComplete(buffer_);\n> > +\n> > +\tunsigned char *memory = static_cast(map->memory);\n> > +\tif (memory) {\n> \n> Can memory be null ?\n> \n> > +\t\tyuvDataPtr_ = memory;\n> > +\t\tupdate();\n> > +\t\tbuffer_ = buffer;\n> > +\t}\n> > +}\n> > +\n> > +bool ViewFinderGL::isFormatSupport(const libcamera::PixelFormat &format)\n> > +{\n> \n> As this function sets internal members based on the format, I would call\n> it selectFormat(), it does more than just checking if the format is\n> supported.\n> \n> > +\tbool ret = true;\n> > +\tswitch (format) {\n> > +\tcase libcamera::formats::NV12:\n> > +\t\thorzSubSample_ = 2;\n> > +\t\tvertSubSample_ = 2;\n> > +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> > +\t\tfsrc_ = \":NV_2_planes_UV_f.glsl\";\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::NV21:\n> > +\t\thorzSubSample_ = 2;\n> > +\t\tvertSubSample_ = 2;\n> > +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> > +\t\tfsrc_ = \":NV_2_planes_VU_f.glsl\";\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::NV16:\n> > +\t\thorzSubSample_ = 2;\n> > +\t\tvertSubSample_ = 1;\n> > +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> > +\t\tfsrc_ = \":NV_2_planes_UV_f.glsl\";\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::NV61:\n> > +\t\thorzSubSample_ = 2;\n> > +\t\tvertSubSample_ = 1;\n> > +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> > +\t\tfsrc_ = \":NV_2_planes_VU_f.glsl\";\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::NV24:\n> > +\t\thorzSubSample_ = 1;\n> > +\t\tvertSubSample_ = 1;\n> > +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> > +\t\tfsrc_ = \":NV_2_planes_UV_f.glsl\";\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::NV42:\n> > +\t\thorzSubSample_ = 1;\n> > +\t\tvertSubSample_ = 1;\n> > +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> > +\t\tfsrc_ = \":NV_2_planes_VU_f.glsl\";\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::YUV420:\n> > +\t\thorzSubSample_ = 2;\n> > +\t\tvertSubSample_ = 2;\n> > +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> > +\t\tfsrc_ = \":NV_3_planes_UV_f.glsl\";\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::YVU420:\n> > +\t\thorzSubSample_ = 2;\n> > +\t\tvertSubSample_ = 2;\n> > +\t\tvsrc_ = \":NV_vertex_shader.glsl\";\n> > +\t\tfsrc_ = \":NV_3_planes_VU_f.glsl\";\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tret = false;\n> > +\t\tqWarning() << \"[ViewFinderGL]:\"\n> > +\t\t\t << \"format not support yet.\";\n> \n> s/support yet./supported/\n> \n> > +\t\tbreak;\n> > +\t};\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +void ViewFinderGL::createVertexShader()\n> > +{\n> > +\tbool bCompile;\n> \n> No need to prefix variables with the type name.\n> \n> > +\t/* Create Vertex Shader */\n> > +\tpVShader_ = new QOpenGLShader(QOpenGLShader::Vertex, this);\n> > +\n> > +\tbCompile = pVShader_->compileSourceFile(vsrc_);\n> > +\tif (!bCompile) {\n> > +\t\tqWarning() << \"[ViewFinderGL]:\" << pVShader_->log();\n> > +\t}\n> \n> This can simply be written\n> \n> \tif (!pVShader_->compileSourceFile(vsrc_))\n> \t\tqWarning() << \"[ViewFinderGL]:\" << pVShader_->log();\n> \n> > +\n> > +\tshaderProgram_.addShader(pVShader_);\n> \n> Won't this crash if shader compilation failed ? I think\n> createVertexShader() should return a status as a bool.\n> \n> > +}\n> > +\n> > +bool ViewFinderGL::createFragmentShader()\n> > +{\n> > +\tbool bCompile;\n> > +\n> > +\t/* Create Fragment Shader */\n> > +\tpFShader_ = new QOpenGLShader(QOpenGLShader::Fragment, this);\n> > +\n> > +\tbCompile = pFShader_->compileSourceFile(fsrc_);\n> > +\tif (!bCompile) {\n> \n> \tif (!pFShader_->compileSourceFile(fsrc_)) {\n> \n> > +\t\tqWarning() << \"[ViewFinderGL]:\" << pFShader_->log();\n> > +\t\treturn bCompile;\n> \n> \t\treturn false;\n> \n> > +\t}\n> > +\n> > +\tshaderProgram_.addShader(pFShader_);\n> > +\n> > +\t/* Link shader pipeline */\n> > +\tif (!shaderProgram_.link()) {\n> > +\t\tqWarning() << \"[ViewFinderGL]:\" << shaderProgram_.log();\n> > +\t\tclose();\n> > +\t}\n> > +\n> > +\t/* Bind shader pipeline for use */\n> > +\tif (!shaderProgram_.bind()) {\n> > +\t\tqWarning() << \"[ViewFinderGL]:\" << shaderProgram_.log();\n> > +\t\tclose();\n> > +\t}\n> > +\n> > +\tshaderProgram_.enableAttributeArray(ATTRIB_VERTEX);\n> > +\tshaderProgram_.enableAttributeArray(ATTRIB_TEXTURE);\n> > +\n> > +\tshaderProgram_.setAttributeBuffer(ATTRIB_VERTEX,\n> > +\t\t\t\t\t GL_FLOAT,\n> > +\t\t\t\t\t 0,\n> > +\t\t\t\t\t 2,\n> > +\t\t\t\t\t 2 * sizeof(GLfloat));\n> > +\tshaderProgram_.setAttributeBuffer(ATTRIB_TEXTURE,\n> > +\t\t\t\t\t GL_FLOAT,\n> > +\t\t\t\t\t 8 * sizeof(GLfloat),\n> > +\t\t\t\t\t 2,\n> > +\t\t\t\t\t 2 * sizeof(GLfloat));\n> > +\n> > +\ttextureUniformY_ = shaderProgram_.uniformLocation(\"tex_y\");\n> > +\ttextureUniformU_ = shaderProgram_.uniformLocation(\"tex_u\");\n> > +\ttextureUniformV_ = shaderProgram_.uniformLocation(\"tex_v\");\n> > +\n> > +\tif (!textureY_.isCreated())\n> > +\t\ttextureY_.create();\n> > +\n> > +\tif (!textureU_.isCreated())\n> > +\t\ttextureU_.create();\n> > +\n> > +\tif (!textureV_.isCreated())\n> > +\t\ttextureV_.create();\n> > +\n> > +\tid_y_ = textureY_.textureId();\n> > +\tid_u_ = textureU_.textureId();\n> > +\tid_v_ = textureV_.textureId();\n> > +\treturn true;\n> > +}\n> > +\n> > +void ViewFinderGL::configureTexture(unsigned int id)\n> > +{\n> > +\tglBindTexture(GL_TEXTURE_2D, id);\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_WRAP_S, GL_CLAMP_TO_EDGE);\n> > +\tglTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);\n> > +}\n> > +\n> > +void ViewFinderGL::removeShader()\n> > +{\n> > +\tif (shaderProgram_.isLinked()) {\n> > +\t\tshaderProgram_.release();\n> > +\t\tshaderProgram_.removeAllShaders();\n> > +\t}\n> > +\n> > +\tif (pFShader_)\n> > +\t\tdelete pFShader_;\n> > +\n> > +\tif (pVShader_)\n> > +\t\tdelete pVShader_;\n> > +}\n> > +\n> > +void ViewFinderGL::initializeGL()\n> > +{\n> > +\tinitializeOpenGLFunctions();\n> > +\tglEnable(GL_TEXTURE_2D);\n> > +\tglDisable(GL_DEPTH_TEST);\n> > +\n> > +\tstatic const GLfloat vertices[]{\n> > +\t\t-1.0f, -1.0f, -1.0f, +1.0f,\n> > +\t\t+1.0f, +1.0f, +1.0f, -1.0f,\n> > +\t\t0.0f, 1.0f, 0.0f, 0.0f,\n> > +\t\t1.0f, 0.0f, 1.0f, 1.0f\n> > +\t};\n> \n> This is vertex and texture coordinates, not just vertices. How about\n> writing it as follows ?\n> \n> \tstatic const GLfloat coordinates[2][4][2] {\n> \t\t{\n> \t\t\t/* Vertex coordinates */\n> \t\t\t{ -1.0f, -1.0f },\n> \t\t\t{ -1.0f, +1.0f },\n> \t\t\t{ +1.0f, +1.0f },\n> \t\t\t{ +1.0f, -1.0f },\n> \t\t}, {\n> \t\t\t/* Texture coordinates */\n> \t\t\t{ 0.0f, 1.0f },\n> \t\t\t{ 0.0f, 0.0f },\n> \t\t\t{ 1.0f, 0.0f },\n> \t\t\t{ 1.0f, 1.0f },\n> \t\t},\n> \t};\n> \n> There's something I don't get though, maybe you can help me understand\n> it. The vertex coordinates are copied directly to gl_Position in the\n> vertex shader, so they're essentially expressed in clip space, which I\n> understand has X pointing towards the right and Y pointing towards the\n> top. The texture coordinates, if my understand is correct again, have\n> their origin at the bottom-left corner too. The first vertex, (-1.0,\n> -1.0), which is at the bottom-left, then maps to texture coordinate\n> (0.0, 1.0), which is the top-left pixel of the texture. The image should\n> thus be flipped vertically. Why isn't it ? I'm sure I'm missing\n> somethign simple.\n\nI figured it out. The texture created with glTexImage2D() has (0,0) at\nbyte 0. As the camera captures the image with the top line at the\nbeginning of the buffer, the texture is stored with the top line on row\n0. texture coordinate (0.0, 1.0) is thus the bottom-left corner of the\ntexture, not the top-left corner.\n\n> > +\n> > +\tvbuf_.create();\n> > +\tvbuf_.bind();\n> > +\tvbuf_.allocate(vertices, sizeof(vertices));\n> > +\n> > +\t/* Create Vertex Shader */\n> > +\tcreateVertexShader();\n> > +\n> > +\tglClearColor(1.0f, 1.0f, 1.0f, 0.0f);\n> > +}\n> > +\n> > +void ViewFinderGL::doRender()\n> > +{\n> > +\tswitch (format_) {\n> > +\tcase libcamera::formats::NV12:\n> > +\tcase libcamera::formats::NV21:\n> > +\tcase libcamera::formats::NV16:\n> > +\tcase libcamera::formats::NV61:\n> > +\tcase libcamera::formats::NV24:\n> > +\tcase libcamera::formats::NV42:\n> > +\t\t/* activate texture 0 */\n> > +\t\tglActiveTexture(GL_TEXTURE0);\n> > +\t\tconfigureTexture(id_y_);\n> > +\t\tglTexImage2D(GL_TEXTURE_2D,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t size_.width(),\n> > +\t\t\t size_.height(),\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t GL_UNSIGNED_BYTE,\n> > +\t\t\t yuvDataPtr_);\n> > +\t\tglUniform1i(textureUniformY_, 0);\n> \n> Would it make sense to use\n> \n> \t\tshaderProgram_.setUniformValue(textureUniformY_, 0);\n> \n> (and similarly below) ?\n> \n> > +\n> > +\t\t/* activate texture 1 */\n> > +\t\tglActiveTexture(GL_TEXTURE1);\n> > +\t\tconfigureTexture(id_u_);\n> > +\t\tglTexImage2D(GL_TEXTURE_2D,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RG,\n> > +\t\t\t size_.width() / horzSubSample_,\n> > +\t\t\t size_.height() / vertSubSample_,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RG,\n> > +\t\t\t GL_UNSIGNED_BYTE,\n> > +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height());\n> > +\t\tglUniform1i(textureUniformU_, 1);\n> > +\t\tbreak;\n> \n> A blank line here would increase readability. Same below.\n> \n> > +\tcase libcamera::formats::YUV420:\n> > +\t\t/* activate texture 0 */\n> > +\t\tglActiveTexture(GL_TEXTURE0);\n> > +\t\tconfigureTexture(id_y_);\n> > +\t\tglTexImage2D(GL_TEXTURE_2D,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t size_.width(),\n> > +\t\t\t size_.height(),\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t GL_UNSIGNED_BYTE,\n> > +\t\t\t yuvDataPtr_);\n> > +\t\tglUniform1i(textureUniformY_, 0);\n> > +\n> > +\t\t/* activate texture 1 */\n> > +\t\tglActiveTexture(GL_TEXTURE1);\n> > +\t\tconfigureTexture(id_u_);\n> > +\t\tglTexImage2D(GL_TEXTURE_2D,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t size_.width() / horzSubSample_,\n> > +\t\t\t size_.height() / vertSubSample_,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t GL_UNSIGNED_BYTE,\n> > +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height());\n> > +\t\tglUniform1i(textureUniformU_, 1);\n> > +\n> > +\t\t/* activate texture 2 */\n> > +\t\tglActiveTexture(GL_TEXTURE2);\n> > +\t\tconfigureTexture(id_v_);\n> > +\t\tglTexImage2D(GL_TEXTURE_2D,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t size_.width() / horzSubSample_,\n> > +\t\t\t size_.height() / vertSubSample_,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t GL_UNSIGNED_BYTE,\n> > +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height() * 5 / 4);\n> > +\t\tglUniform1i(textureUniformV_, 2);\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::YVU420:\n> > +\t\t/* activate texture 0 */\n> > +\t\tglActiveTexture(GL_TEXTURE0);\n> > +\t\tconfigureTexture(id_y_);\n> > +\t\tglTexImage2D(GL_TEXTURE_2D,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t size_.width(),\n> > +\t\t\t size_.height(),\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t GL_UNSIGNED_BYTE,\n> > +\t\t\t yuvDataPtr_);\n> > +\t\tglUniform1i(textureUniformY_, 0);\n> > +\n> > +\t\t/* activate texture 1 */\n> > +\t\tglActiveTexture(GL_TEXTURE2);\n> > +\t\tconfigureTexture(id_v_);\n> > +\t\tglTexImage2D(GL_TEXTURE_2D,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t size_.width() / horzSubSample_,\n> > +\t\t\t size_.height() / vertSubSample_,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t GL_UNSIGNED_BYTE,\n> > +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height());\n> > +\t\tglUniform1i(textureUniformV_, 1);\n> \n> OK, now I understand why the NV_3_planes_UV_f.glsl and\n> NV_3_planes_VU_f.glsl shaders are identical, you switch the U and V\n> planes here. I think we should then merge the two files into\n> NV_3_planes_f_glsl. The above line should become\n> \n> \t\tglUniform1i(textureUniformU_, 2);\n> \n> as you deal with texture 2 here (and a similar change is needed below),\n> and the two blocks should be swapped as the comments are incorrect (the\n> comment above refers to texture 1 while the code deals with texture 2).\n> \n> > +\n> > +\t\t/* activate texture 2 */\n> > +\t\tglActiveTexture(GL_TEXTURE1);\n> > +\t\tconfigureTexture(id_u_);\n> > +\t\tglTexImage2D(GL_TEXTURE_2D,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t size_.width() / horzSubSample_,\n> > +\t\t\t size_.height() / vertSubSample_,\n> > +\t\t\t 0,\n> > +\t\t\t GL_RED,\n> > +\t\t\t GL_UNSIGNED_BYTE,\n> > +\t\t\t (char *)yuvDataPtr_ + size_.width() * size_.height() * 5 / 4);\n> > +\t\tglUniform1i(textureUniformU_, 2);\n> \n> Please add a break here, let's not rely on implicit fall-through.\n> \n> > +\tdefault:\n> > +\t\tbreak;\n> > +\t};\n> > +}\n> > +\n> > +void ViewFinderGL::paintGL()\n> > +{\n> > +\tif (pFShader_ == nullptr)\n> \n> \tif (!pfShader_)\n> \n> > +\t\tcreateFragmentShader();\n> > +\n> > +\tif (yuvDataPtr_) {\n> > +\t\tglClearColor(0.0, 0.0, 0.0, 1.0);\n> > +\t\tglClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);\n> > +\n> > +\t\tdoRender();\n> > +\t\tglDrawArrays(GL_TRIANGLE_FAN, 0, 4);\n> > +\t}\n> > +}\n> > +\n> > +void ViewFinderGL::resizeGL(int w, int h)\n> > +{\n> > +\tglViewport(0, 0, w, h);\n> > +}\n> > +\n> > +QSize ViewFinderGL::sizeHint() const\n> > +{\n> > +\treturn size_.isValid() ? size_ : QSize(640, 480);\n> > +}\n> > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> > new file mode 100644\n> > index 0000000..e708c32\n> > --- /dev/null\n> > +++ b/src/qcam/viewfinder_gl.h\n> > @@ -0,0 +1,97 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Linaro\n> > + *\n> > + * viewfinder_GL.h - OpenGL Viewfinder for rendering by OpenGL shader\n> > + *\n> > + */\n> > +#ifndef __VIEWFINDER_GL_H__\n> > +#define __VIEWFINDER_GL_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> \n> This header shouldn't be needed.\n> \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> \n> Missing blank line.\n> \n> > +#include \"viewfinder.h\"\n> > +\n> > +class ViewFinderGL : public QOpenGLWidget,\n> > +\t\t public ViewFinder,\n> > +\t\t protected QOpenGLFunctions\n> > +{\n> > +\tQ_OBJECT\n> > +\n> > +public:\n> > +\tViewFinderGL(QWidget *parent = 0);\n> \n> = nullptr\n> \n> > +\t~ViewFinderGL();\n> > +\n> > +\tconst QList &nativeFormats() const override;\n> > +\n> > +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > +\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > +\tvoid stop() override;\n> > +\n> > +\tQImage getCurrentImage() override;\n> > +\n> > +Q_SIGNALS:\n> > +\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > +\n> > +protected:\n> > +\tvoid initializeGL() override;\n> > +\tvoid paintGL() override;\n> > +\tvoid resizeGL(int w, int h) override;\n> > +\tQSize sizeHint() const override;\n> > +\n> > +private:\n> > +\tbool isFormatSupport(const libcamera::PixelFormat &format);\n> \n> s/isFormatSupport/isFormatSupported/\n> \n> > +\n> > +\tvoid configureTexture(unsigned int id);\n> > +\tbool createFragmentShader();\n> > +\tvoid createVertexShader();\n> > +\tvoid removeShader();\n> > +\tvoid doRender();\n> > +\n> > +\t/* Captured image size, format and buffer */\n> > +\tlibcamera::FrameBuffer *buffer_;\n> > +\tlibcamera::PixelFormat format_;\n> > +\tQSize size_;\n> > +\n> > +\t/* OpenGL components for render */\n> \n> s/render/rendering/\n> \n> > +\tQOpenGLShader *pFShader_;\n> \n> No need to prefix pointers with 'p'. I'd name this fragmentShader_.\n> \n> > +\tQOpenGLShader *pVShader_;\n> \n> Same here, vertexShader_.\n> \n> > +\tQOpenGLShaderProgram shaderProgram_;\n> \n> Is there a specific reason why pFShader_ and pVShader_ are pointers,\n> while shaderProgram_ is embedded directly in ViewFinderGL ?\n> \n> > +\n> > +\t/* Vertex buffer */\n> > +\tQOpenGLBuffer vbuf_;\n> > +\n> > +\t/* Fragment and Vertex shader file name */\n> > +\tQString fsrc_;\n> \n> fragmentShaderSrc_ ?\n> \n> > +\tQString vsrc_;\n> \n> And vertexShaderSrc_.\n> \n> > +\n> > +\tunsigned char *yuvDataPtr_;\n> \n> And no need for a Ptr suffix either :-)\n> \n> > +\n> > +\t/* YUV texture planars and parameters */\n> > +\tGLuint id_u_;\n> > +\tGLuint id_v_;\n> > +\tGLuint id_y_;\n> > +\tGLuint textureUniformU_;\n> > +\tGLuint textureUniformV_;\n> > +\tGLuint textureUniformY_;\n> > +\tQOpenGLTexture textureU_;\n> > +\tQOpenGLTexture textureV_;\n> > +\tQOpenGLTexture textureY_;\n> > +\tunsigned int horzSubSample_;\n> > +\tunsigned int vertSubSample_;\n> > +\n> > +\tQImage image_;\n> \n> This is never used.\n> \n> > +\tQMutex mutex_; /* Prevent concurrent access to image_ */\n> > +};\n> > +#endif /* __VIEWFINDER_GL_H__ */","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 E252EBDB1D\n\tfor ;\n\tSun, 6 Sep 2020 23:10:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D22F62B5D;\n\tMon, 7 Sep 2020 01:10:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F4AB62901\n\tfor ;\n\tMon, 7 Sep 2020 01:10:26 +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 B3D0647;\n\tMon, 7 Sep 2020 01:10:25 +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=\"IOVhZaI5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599433825;\n\tbh=21nx6AlVrQP0+p+CcE2eWRPcmSmtvghtAZ5QZ3T01Yg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IOVhZaI5eSJMInieIwItCyMDtxX7uMHRJSt2aFH7M92OzMDk8CEN9SUEv6B9geuFV\n\tFYMn4MHwcyL74qSFL7KhZDnza7/bTapyoCJZZUgCAeYOiOYs3aJbg9q6tnc8kh6FrP\n\tLhNEYTxf6H8BLi1F7INTP1puKR/Idr3medL4yMps=","Date":"Mon, 7 Sep 2020 02:10:01 +0300","From":"Laurent Pinchart ","To":"Show Liu ","Message-ID":"<20200906231001.GA15818@pendragon.ideasonboard.com>","References":"<20200904084316.7319-1-show.liu@linaro.org>\n\t<20200904084316.7319-5-show.liu@linaro.org>\n\t<20200906161807.GA6047@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200906161807.GA6047@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 3/4] qcam: add viewfinderGL class\n\tto accelerate the format convert","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\" "}},{"id":12399,"web_url":"https://patchwork.libcamera.org/comment/12399/","msgid":"","date":"2020-09-10T09:38:31","subject":"Re: [libcamera-devel] [PATCH v5 3/4] qcam: add viewfinderGL class\n\tto accelerate the format convert","submitter":{"id":24,"url":"https://patchwork.libcamera.org/api/people/24/","name":"Show Liu","email":"show.liu@linaro.org"},"content":"Hi Laurent,\n\nOn Mon, Sep 7, 2020 at 7:10 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Sun, Sep 06, 2020 at 07:18:07PM +0300, Laurent Pinchart wrote:\n> > Hi Show,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Sep 04, 2020 at 04:43:15PM +0800, Show Liu wrote:\n> > > the viewfinderGL accelerates the format conversion by\n> > > using OpenGL ES shader\n> > >\n> > > Signed-off-by: Show Liu \n> > > ---\n> > > src/qcam/meson.build | 2 +\n> > > src/qcam/viewfinder_gl.cpp | 441 +++++++++++++++++++++++++++++++++++++\n> > > src/qcam/viewfinder_gl.h | 97 ++++++++\n> > > 3 files changed, 540 insertions(+)\n> > > create mode 100644 src/qcam/viewfinder_gl.cpp\n> > > create mode 100644 src/qcam/viewfinder_gl.h\n> > >\n> > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > > index a4bad0a..32c0fc3 100644\n> > > --- a/src/qcam/meson.build\n> > > +++ b/src/qcam/meson.build\n> > > @@ -7,11 +7,13 @@ qcam_sources = files([\n> > > 'main.cpp',\n> > > 'main_window.cpp',\n> > > 'viewfinder_qt.cpp',\n> > > + 'viewfinder_gl.cpp',\n> >\n> > Let's keep files alphabetically sorted.\n> >\n> > > ])\n> > >\n> > > qcam_moc_headers = files([\n> > > 'main_window.h',\n> > > 'viewfinder_qt.h',\n> > > + 'viewfinder_gl.h',\n> >\n> > Here too.\n> >\n> > > ])\n> > >\n> > > qcam_resources = files([\n> >\n> > You need to set the minimum Qt version to 5.4, as QOpenGLWidget wasn't\n> > available before that.\n> >\n> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > index 32c0fc3e0f6b..9d3f189a896b 100644\n> > --- a/src/qcam/meson.build\n> > +++ b/src/qcam/meson.build\n> > @@ -25,7 +25,8 @@ qt5 = import('qt5')\n> > qt5_dep = dependency('qt5',\n> > method : 'pkg-config',\n> > modules : ['Core', 'Gui', 'Widgets'],\n> > - required : get_option('qcam'))\n> > + required : get_option('qcam'),\n> > + version : '>=5.4')\n> >\n> > if qt5_dep.found()\n> > qcam_deps = [\n> >\n> > Furthermore, Qt can be compiled without OpenGL support, in which case\n> > this patch will fail to compile. The following change should address it.\n> >\n> > diff --git a/meson.build b/meson.build\n> > index b6c99ba8e0eb..5f7d619a79d0 100644\n> > --- a/meson.build\n> > +++ b/meson.build\n> > @@ -26,6 +26,7 @@ libcamera_version = libcamera_git_version.split('+')[0]\n> >\n> > # Configure the build environment.\n> > cc = meson.get_compiler('c')\n> > +cxx = meson.get_compiler('cpp')\n> > config_h = configuration_data()\n> >\n> > if cc.has_header_symbol('execinfo.h', 'backtrace')\n> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > index 32c0fc3e0f6b..9bb48c0d06c5 100644\n> > --- a/src/qcam/meson.build\n> > +++ b/src/qcam/meson.build\n> > @@ -7,18 +7,15 @@ qcam_sources = files([\n> > 'main.cpp',\n> > 'main_window.cpp',\n> > 'viewfinder_qt.cpp',\n> > - 'viewfinder_gl.cpp',\n> > ])\n> >\n> > qcam_moc_headers = files([\n> > 'main_window.h',\n> > 'viewfinder_qt.h',\n> > - 'viewfinder_gl.h',\n> > ])\n> >\n> > qcam_resources = files([\n> > 'assets/feathericons/feathericons.qrc',\n> > - 'assets/shader/shaders.qrc'\n> > ])\n> >\n> > qt5 = import('qt5')\n> > @@ -44,6 +42,19 @@ if qt5_dep.found()\n> > ])\n> > endif\n> >\n> > + if cxx.has_header_symbol('QOpenGLWidget', 'QOpenGLWidget',\n> > + dependencies : qt5_dep, args : '-fPIC')\n> > + qcam_sources += files([\n> > + 'viewfinder_gl.cpp',\n> > + ])\n> > + qcam_moc_headers += files([\n> > + 'viewfinder_gl.h',\n> > + ])\n> > + qcam_resources += files([\n> > + 'assets/shader/shaders.qrc'\n> > + ])\n> > + endif\n> > +\n> > # gcc 9 introduced a deprecated-copy warning that is triggered by\n> Qt until\n> > # Qt 5.13. clang 10 introduced the same warning, but detects more\n> issues\n> > # that are not fixed in Qt yet. Disable the warning manually in\n> both cases.\n> >\n> > Patch 4/4 will need to be updated to with conditional compilation on\n> > QT_NO_OPENGL.\n> >\n> > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > > new file mode 100644\n> > > index 0000000..5591916\n> > > --- /dev/null\n> > > +++ b/src/qcam/viewfinder_gl.cpp\n> > > @@ -0,0 +1,441 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Linaro\n> > > + *\n> > > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader\n> > > + */\n> > > +\n> > > +#include \"viewfinder_gl.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#define ATTRIB_VERTEX 0\n> > > +#define ATTRIB_TEXTURE 1\n> >\n> > Is there a guarantee that attribute locations match the declaration\n> > order in the shader program ? Wouldn't it be better to use\n> > shaderProgram.attributeLocation() to retrieve the attribute locations by\n> > name (before linking), or shaderProgram.bindAttributeLocation() to set\n> > them explicitly (after linking) ?\n> >\n> > > +\n> > > +static const QList supportFormats{\n> >\n> > s/supportFormats/supportedFormats/\n> >\n> > > + libcamera::formats::NV12,\n> > > + libcamera::formats::NV21,\n> > > + libcamera::formats::NV16,\n> > > + libcamera::formats::NV61,\n> > > + libcamera::formats::NV24,\n> > > + libcamera::formats::NV42,\n> > > + libcamera::formats::YUV420,\n> > > + libcamera::formats::YVU420\n> > > +};\n> > > +\n> > > +ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > > + : QOpenGLWidget(parent),\n> > > + buffer_(nullptr),\n> > > + pFShader_(nullptr),\n> > > + pVShader_(nullptr),\n> > > + vbuf_(QOpenGLBuffer::VertexBuffer),\n> > > + yuvDataPtr_(nullptr),\n> > > + textureU_(QOpenGLTexture::Target2D),\n> > > + textureV_(QOpenGLTexture::Target2D),\n> > > + textureY_(QOpenGLTexture::Target2D)\n> >\n> > Feel free to have multiple members per line if desired.\n> >\n> > > +{\n> > > +}\n> > > +\n> > > +ViewFinderGL::~ViewFinderGL()\n> > > +{\n> > > + removeShader();\n> > > +\n> > > + if (textureY_.isCreated())\n> > > + textureY_.destroy();\n> > > +\n> > > + if (textureU_.isCreated())\n> > > + textureU_.destroy();\n> > > +\n> > > + if (textureV_.isCreated())\n> > > + textureV_.destroy();\n> >\n> > Are these needed, or does the QOpenGLTexture destructor destroy the\n> > textures ?\n> >\n> > > +\n> > > + vbuf_.destroy();\n> >\n> > Same question for vbuf_.\n> >\n> > > +}\n> > > +\n> > > +const QList &ViewFinderGL::nativeFormats()\n> const\n> > > +{\n> > > + return (::supportFormats);\n> >\n> > No need for parentheses or an explicit namespace.\n> >\n> > return supportedFormats;\n> >\n> > > +}\n> > > +\n> > > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> > > + const QSize &size)\n> > > +{\n> > > + int ret = 0;\n> > > +\n> > > + if (isFormatSupport(format)) {\n> > > + format_ = format;\n> > > + size_ = size;\n> > > + } else {\n> > > + ret = -1;\n> > > + }\n> > > + updateGeometry();\n> >\n> > When the format change, don't you need to recreate the shaders ?\n> >\n> > > + return ret;\n> > > +}\n> > > +\n> > > +void ViewFinderGL::stop()\n> > > +{\n> > > + if (buffer_) {\n> > > + renderComplete(buffer_);\n> > > + buffer_ = nullptr;\n> > > + }\n> > > +}\n> > > +\n> > > +QImage ViewFinderGL::getCurrentImage()\n> > > +{\n> > > + QMutexLocker locker(&mutex_);\n> > > +\n> > > + return (grabFramebuffer());\n> >\n> > return grabFrameBuffer();\n> >\n> > > +}\n> > > +\n> > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer,\n> MappedBuffer *map)\n> > > +{\n> > > + if (buffer->planes().size() != 1) {\n> > > + qWarning() << \"Multi-planar buffers are not supported\";\n> > > + return;\n> > > + }\n> > > +\n> > > + if (buffer_)\n> > > + renderComplete(buffer_);\n> > > +\n> > > + unsigned char *memory = static_cast(map->memory);\n> > > + if (memory) {\n> >\n> > Can memory be null ?\n> >\n> > > + yuvDataPtr_ = memory;\n> > > + update();\n> > > + buffer_ = buffer;\n> > > + }\n> > > +}\n> > > +\n> > > +bool ViewFinderGL::isFormatSupport(const libcamera::PixelFormat\n> &format)\n> > > +{\n> >\n> > As this function sets internal members based on the format, I would call\n> > it selectFormat(), it does more than just checking if the format is\n> > supported.\n> >\n> > > + bool ret = true;\n> > > + switch (format) {\n> > > + case libcamera::formats::NV12:\n> > > + horzSubSample_ = 2;\n> > > + vertSubSample_ = 2;\n> > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > + fsrc_ = \":NV_2_planes_UV_f.glsl\";\n> > > + break;\n> > > + case libcamera::formats::NV21:\n> > > + horzSubSample_ = 2;\n> > > + vertSubSample_ = 2;\n> > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > + fsrc_ = \":NV_2_planes_VU_f.glsl\";\n> > > + break;\n> > > + case libcamera::formats::NV16:\n> > > + horzSubSample_ = 2;\n> > > + vertSubSample_ = 1;\n> > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > + fsrc_ = \":NV_2_planes_UV_f.glsl\";\n> > > + break;\n> > > + case libcamera::formats::NV61:\n> > > + horzSubSample_ = 2;\n> > > + vertSubSample_ = 1;\n> > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > + fsrc_ = \":NV_2_planes_VU_f.glsl\";\n> > > + break;\n> > > + case libcamera::formats::NV24:\n> > > + horzSubSample_ = 1;\n> > > + vertSubSample_ = 1;\n> > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > + fsrc_ = \":NV_2_planes_UV_f.glsl\";\n> > > + break;\n> > > + case libcamera::formats::NV42:\n> > > + horzSubSample_ = 1;\n> > > + vertSubSample_ = 1;\n> > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > + fsrc_ = \":NV_2_planes_VU_f.glsl\";\n> > > + break;\n> > > + case libcamera::formats::YUV420:\n> > > + horzSubSample_ = 2;\n> > > + vertSubSample_ = 2;\n> > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > + fsrc_ = \":NV_3_planes_UV_f.glsl\";\n> > > + break;\n> > > + case libcamera::formats::YVU420:\n> > > + horzSubSample_ = 2;\n> > > + vertSubSample_ = 2;\n> > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > + fsrc_ = \":NV_3_planes_VU_f.glsl\";\n> > > + break;\n> > > + default:\n> > > + ret = false;\n> > > + qWarning() << \"[ViewFinderGL]:\"\n> > > + << \"format not support yet.\";\n> >\n> > s/support yet./supported/\n> >\n> > > + break;\n> > > + };\n> > > +\n> > > + return ret;\n> > > +}\n> > > +\n> > > +void ViewFinderGL::createVertexShader()\n> > > +{\n> > > + bool bCompile;\n> >\n> > No need to prefix variables with the type name.\n> >\n> > > + /* Create Vertex Shader */\n> > > + pVShader_ = new QOpenGLShader(QOpenGLShader::Vertex, this);\n> > > +\n> > > + bCompile = pVShader_->compileSourceFile(vsrc_);\n> > > + if (!bCompile) {\n> > > + qWarning() << \"[ViewFinderGL]:\" << pVShader_->log();\n> > > + }\n> >\n> > This can simply be written\n> >\n> > if (!pVShader_->compileSourceFile(vsrc_))\n> > qWarning() << \"[ViewFinderGL]:\" << pVShader_->log();\n> >\n> > > +\n> > > + shaderProgram_.addShader(pVShader_);\n> >\n> > Won't this crash if shader compilation failed ? I think\n> > createVertexShader() should return a status as a bool.\n> >\n> > > +}\n> > > +\n> > > +bool ViewFinderGL::createFragmentShader()\n> > > +{\n> > > + bool bCompile;\n> > > +\n> > > + /* Create Fragment Shader */\n> > > + pFShader_ = new QOpenGLShader(QOpenGLShader::Fragment, this);\n> > > +\n> > > + bCompile = pFShader_->compileSourceFile(fsrc_);\n> > > + if (!bCompile) {\n> >\n> > if (!pFShader_->compileSourceFile(fsrc_)) {\n> >\n> > > + qWarning() << \"[ViewFinderGL]:\" << pFShader_->log();\n> > > + return bCompile;\n> >\n> > return false;\n> >\n> > > + }\n> > > +\n> > > + shaderProgram_.addShader(pFShader_);\n> > > +\n> > > + /* Link shader pipeline */\n> > > + if (!shaderProgram_.link()) {\n> > > + qWarning() << \"[ViewFinderGL]:\" << shaderProgram_.log();\n> > > + close();\n> > > + }\n> > > +\n> > > + /* Bind shader pipeline for use */\n> > > + if (!shaderProgram_.bind()) {\n> > > + qWarning() << \"[ViewFinderGL]:\" << shaderProgram_.log();\n> > > + close();\n> > > + }\n> > > +\n> > > + shaderProgram_.enableAttributeArray(ATTRIB_VERTEX);\n> > > + shaderProgram_.enableAttributeArray(ATTRIB_TEXTURE);\n> > > +\n> > > + shaderProgram_.setAttributeBuffer(ATTRIB_VERTEX,\n> > > + GL_FLOAT,\n> > > + 0,\n> > > + 2,\n> > > + 2 * sizeof(GLfloat));\n> > > + shaderProgram_.setAttributeBuffer(ATTRIB_TEXTURE,\n> > > + GL_FLOAT,\n> > > + 8 * sizeof(GLfloat),\n> > > + 2,\n> > > + 2 * sizeof(GLfloat));\n> > > +\n> > > + textureUniformY_ = shaderProgram_.uniformLocation(\"tex_y\");\n> > > + textureUniformU_ = shaderProgram_.uniformLocation(\"tex_u\");\n> > > + textureUniformV_ = shaderProgram_.uniformLocation(\"tex_v\");\n> > > +\n> > > + if (!textureY_.isCreated())\n> > > + textureY_.create();\n> > > +\n> > > + if (!textureU_.isCreated())\n> > > + textureU_.create();\n> > > +\n> > > + if (!textureV_.isCreated())\n> > > + textureV_.create();\n> > > +\n> > > + id_y_ = textureY_.textureId();\n> > > + id_u_ = textureU_.textureId();\n> > > + id_v_ = textureV_.textureId();\n> > > + return true;\n> > > +}\n> > > +\n> > > +void ViewFinderGL::configureTexture(unsigned int id)\n> > > +{\n> > > + glBindTexture(GL_TEXTURE_2D, id);\n> > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);\n> > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);\n> > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S,\n> GL_CLAMP_TO_EDGE);\n> > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T,\n> GL_CLAMP_TO_EDGE);\n> > > +}\n> > > +\n> > > +void ViewFinderGL::removeShader()\n> > > +{\n> > > + if (shaderProgram_.isLinked()) {\n> > > + shaderProgram_.release();\n> > > + shaderProgram_.removeAllShaders();\n> > > + }\n> > > +\n> > > + if (pFShader_)\n> > > + delete pFShader_;\n> > > +\n> > > + if (pVShader_)\n> > > + delete pVShader_;\n> > > +}\n> > > +\n> > > +void ViewFinderGL::initializeGL()\n> > > +{\n> > > + initializeOpenGLFunctions();\n> > > + glEnable(GL_TEXTURE_2D);\n> > > + glDisable(GL_DEPTH_TEST);\n> > > +\n> > > + static const GLfloat vertices[]{\n> > > + -1.0f, -1.0f, -1.0f, +1.0f,\n> > > + +1.0f, +1.0f, +1.0f, -1.0f,\n> > > + 0.0f, 1.0f, 0.0f, 0.0f,\n> > > + 1.0f, 0.0f, 1.0f, 1.0f\n> > > + };\n> >\n> > This is vertex and texture coordinates, not just vertices. How about\n> > writing it as follows ?\n> >\n> > static const GLfloat coordinates[2][4][2] {\n> > {\n> > /* Vertex coordinates */\n> > { -1.0f, -1.0f },\n> > { -1.0f, +1.0f },\n> > { +1.0f, +1.0f },\n> > { +1.0f, -1.0f },\n> > }, {\n> > /* Texture coordinates */\n> > { 0.0f, 1.0f },\n> > { 0.0f, 0.0f },\n> > { 1.0f, 0.0f },\n> > { 1.0f, 1.0f },\n> > },\n> > };\n> >\n> > There's something I don't get though, maybe you can help me understand\n> > it. The vertex coordinates are copied directly to gl_Position in the\n> > vertex shader, so they're essentially expressed in clip space, which I\n> > understand has X pointing towards the right and Y pointing towards the\n> > top. The texture coordinates, if my understand is correct again, have\n> > their origin at the bottom-left corner too. The first vertex, (-1.0,\n> > -1.0), which is at the bottom-left, then maps to texture coordinate\n> > (0.0, 1.0), which is the top-left pixel of the texture. The image should\n> > thus be flipped vertically. Why isn't it ? I'm sure I'm missing\n> > somethign simple.\n>\n> I figured it out. The texture created with glTexImage2D() has (0,0) at\n> byte 0. As the camera captures the image with the top line at the\n> beginning of the buffer, the texture is stored with the top line on row\n> 0. texture coordinate (0.0, 1.0) is thus the bottom-left corner of the\n> texture, not the top-left corner.\n>\n\nYou definitely are an OpenGL expert...:-)\nThe original coordinate mapping is for my camera usage on rockpi4b.\nAnd in my case the camera module is vertical flipped.\nSo...anyway I am now change the coordinate mapping as default below\n\n+ static const GLfloat coordinates[2][4][2] {\n+ {\n+ /* Vertex coordinates */\n+ { -1.0f, -1.0f },\n+ { -1.0f, +1.0f },\n+ { +1.0f, +1.0f },\n+ { +1.0f, -1.0f },\n+ }, {\n+ /* Texture coordinates */\n+ { 1.0f, 0.0f },\n+ { 1.0f, 1.0f },\n+ { 0.0f, 1.0f },\n+ { 0.0f, 0.0f },\n+ },\nPlease let me know if you have any concern.\n\nThanks,\nShow\n\n>\n> > > +\n> > > + vbuf_.create();\n> > > + vbuf_.bind();\n> > > + vbuf_.allocate(vertices, sizeof(vertices));\n> > > +\n> > > + /* Create Vertex Shader */\n> > > + createVertexShader();\n> > > +\n> > > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f);\n> > > +}\n> > > +\n> > > +void ViewFinderGL::doRender()\n> > > +{\n> > > + switch (format_) {\n> > > + case libcamera::formats::NV12:\n> > > + case libcamera::formats::NV21:\n> > > + case libcamera::formats::NV16:\n> > > + case libcamera::formats::NV61:\n> > > + case libcamera::formats::NV24:\n> > > + case libcamera::formats::NV42:\n> > > + /* activate texture 0 */\n> > > + glActiveTexture(GL_TEXTURE0);\n> > > + configureTexture(id_y_);\n> > > + glTexImage2D(GL_TEXTURE_2D,\n> > > + 0,\n> > > + GL_RED,\n> > > + size_.width(),\n> > > + size_.height(),\n> > > + 0,\n> > > + GL_RED,\n> > > + GL_UNSIGNED_BYTE,\n> > > + yuvDataPtr_);\n> > > + glUniform1i(textureUniformY_, 0);\n> >\n> > Would it make sense to use\n> >\n> > shaderProgram_.setUniformValue(textureUniformY_, 0);\n> >\n> > (and similarly below) ?\n> >\n> > > +\n> > > + /* activate texture 1 */\n> > > + glActiveTexture(GL_TEXTURE1);\n> > > + configureTexture(id_u_);\n> > > + glTexImage2D(GL_TEXTURE_2D,\n> > > + 0,\n> > > + GL_RG,\n> > > + size_.width() / horzSubSample_,\n> > > + size_.height() / vertSubSample_,\n> > > + 0,\n> > > + GL_RG,\n> > > + GL_UNSIGNED_BYTE,\n> > > + (char *)yuvDataPtr_ + size_.width() *\n> size_.height());\n> > > + glUniform1i(textureUniformU_, 1);\n> > > + break;\n> >\n> > A blank line here would increase readability. Same below.\n> >\n> > > + case libcamera::formats::YUV420:\n> > > + /* activate texture 0 */\n> > > + glActiveTexture(GL_TEXTURE0);\n> > > + configureTexture(id_y_);\n> > > + glTexImage2D(GL_TEXTURE_2D,\n> > > + 0,\n> > > + GL_RED,\n> > > + size_.width(),\n> > > + size_.height(),\n> > > + 0,\n> > > + GL_RED,\n> > > + GL_UNSIGNED_BYTE,\n> > > + yuvDataPtr_);\n> > > + glUniform1i(textureUniformY_, 0);\n> > > +\n> > > + /* activate texture 1 */\n> > > + glActiveTexture(GL_TEXTURE1);\n> > > + configureTexture(id_u_);\n> > > + glTexImage2D(GL_TEXTURE_2D,\n> > > + 0,\n> > > + GL_RED,\n> > > + size_.width() / horzSubSample_,\n> > > + size_.height() / vertSubSample_,\n> > > + 0,\n> > > + GL_RED,\n> > > + GL_UNSIGNED_BYTE,\n> > > + (char *)yuvDataPtr_ + size_.width() *\n> size_.height());\n> > > + glUniform1i(textureUniformU_, 1);\n> > > +\n> > > + /* activate texture 2 */\n> > > + glActiveTexture(GL_TEXTURE2);\n> > > + configureTexture(id_v_);\n> > > + glTexImage2D(GL_TEXTURE_2D,\n> > > + 0,\n> > > + GL_RED,\n> > > + size_.width() / horzSubSample_,\n> > > + size_.height() / vertSubSample_,\n> > > + 0,\n> > > + GL_RED,\n> > > + GL_UNSIGNED_BYTE,\n> > > + (char *)yuvDataPtr_ + size_.width() *\n> size_.height() * 5 / 4);\n> > > + glUniform1i(textureUniformV_, 2);\n> > > + break;\n> > > + case libcamera::formats::YVU420:\n> > > + /* activate texture 0 */\n> > > + glActiveTexture(GL_TEXTURE0);\n> > > + configureTexture(id_y_);\n> > > + glTexImage2D(GL_TEXTURE_2D,\n> > > + 0,\n> > > + GL_RED,\n> > > + size_.width(),\n> > > + size_.height(),\n> > > + 0,\n> > > + GL_RED,\n> > > + GL_UNSIGNED_BYTE,\n> > > + yuvDataPtr_);\n> > > + glUniform1i(textureUniformY_, 0);\n> > > +\n> > > + /* activate texture 1 */\n> > > + glActiveTexture(GL_TEXTURE2);\n> > > + configureTexture(id_v_);\n> > > + glTexImage2D(GL_TEXTURE_2D,\n> > > + 0,\n> > > + GL_RED,\n> > > + size_.width() / horzSubSample_,\n> > > + size_.height() / vertSubSample_,\n> > > + 0,\n> > > + GL_RED,\n> > > + GL_UNSIGNED_BYTE,\n> > > + (char *)yuvDataPtr_ + size_.width() *\n> size_.height());\n> > > + glUniform1i(textureUniformV_, 1);\n> >\n> > OK, now I understand why the NV_3_planes_UV_f.glsl and\n> > NV_3_planes_VU_f.glsl shaders are identical, you switch the U and V\n> > planes here. I think we should then merge the two files into\n> > NV_3_planes_f_glsl. The above line should become\n> >\n> > glUniform1i(textureUniformU_, 2);\n> >\n> > as you deal with texture 2 here (and a similar change is needed below),\n> > and the two blocks should be swapped as the comments are incorrect (the\n> > comment above refers to texture 1 while the code deals with texture 2).\n> >\n> > > +\n> > > + /* activate texture 2 */\n> > > + glActiveTexture(GL_TEXTURE1);\n> > > + configureTexture(id_u_);\n> > > + glTexImage2D(GL_TEXTURE_2D,\n> > > + 0,\n> > > + GL_RED,\n> > > + size_.width() / horzSubSample_,\n> > > + size_.height() / vertSubSample_,\n> > > + 0,\n> > > + GL_RED,\n> > > + GL_UNSIGNED_BYTE,\n> > > + (char *)yuvDataPtr_ + size_.width() *\n> size_.height() * 5 / 4);\n> > > + glUniform1i(textureUniformU_, 2);\n> >\n> > Please add a break here, let's not rely on implicit fall-through.\n> >\n> > > + default:\n> > > + break;\n> > > + };\n> > > +}\n> > > +\n> > > +void ViewFinderGL::paintGL()\n> > > +{\n> > > + if (pFShader_ == nullptr)\n> >\n> > if (!pfShader_)\n> >\n> > > + createFragmentShader();\n> > > +\n> > > + if (yuvDataPtr_) {\n> > > + glClearColor(0.0, 0.0, 0.0, 1.0);\n> > > + glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);\n> > > +\n> > > + doRender();\n> > > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4);\n> > > + }\n> > > +}\n> > > +\n> > > +void ViewFinderGL::resizeGL(int w, int h)\n> > > +{\n> > > + glViewport(0, 0, w, h);\n> > > +}\n> > > +\n> > > +QSize ViewFinderGL::sizeHint() const\n> > > +{\n> > > + return size_.isValid() ? size_ : QSize(640, 480);\n> > > +}\n> > > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> > > new file mode 100644\n> > > index 0000000..e708c32\n> > > --- /dev/null\n> > > +++ b/src/qcam/viewfinder_gl.h\n> > > @@ -0,0 +1,97 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Linaro\n> > > + *\n> > > + * viewfinder_GL.h - OpenGL Viewfinder for rendering by OpenGL shader\n> > > + *\n> > > + */\n> > > +#ifndef __VIEWFINDER_GL_H__\n> > > +#define __VIEWFINDER_GL_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> >\n> > This header shouldn't be needed.\n> >\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> >\n> > Missing blank line.\n> >\n> > > +#include \"viewfinder.h\"\n> > > +\n> > > +class ViewFinderGL : public QOpenGLWidget,\n> > > + public ViewFinder,\n> > > + protected QOpenGLFunctions\n> > > +{\n> > > + Q_OBJECT\n> > > +\n> > > +public:\n> > > + ViewFinderGL(QWidget *parent = 0);\n> >\n> > = nullptr\n> >\n> > > + ~ViewFinderGL();\n> > > +\n> > > + const QList &nativeFormats() const\n> override;\n> > > +\n> > > + int setFormat(const libcamera::PixelFormat &format, const QSize\n> &size) override;\n> > > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> override;\n> > > + void stop() override;\n> > > +\n> > > + QImage getCurrentImage() override;\n> > > +\n> > > +Q_SIGNALS:\n> > > + void renderComplete(libcamera::FrameBuffer *buffer);\n> > > +\n> > > +protected:\n> > > + void initializeGL() override;\n> > > + void paintGL() override;\n> > > + void resizeGL(int w, int h) override;\n> > > + QSize sizeHint() const override;\n> > > +\n> > > +private:\n> > > + bool isFormatSupport(const libcamera::PixelFormat &format);\n> >\n> > s/isFormatSupport/isFormatSupported/\n> >\n> > > +\n> > > + void configureTexture(unsigned int id);\n> > > + bool createFragmentShader();\n> > > + void createVertexShader();\n> > > + void removeShader();\n> > > + void doRender();\n> > > +\n> > > + /* Captured image size, format and buffer */\n> > > + libcamera::FrameBuffer *buffer_;\n> > > + libcamera::PixelFormat format_;\n> > > + QSize size_;\n> > > +\n> > > + /* OpenGL components for render */\n> >\n> > s/render/rendering/\n> >\n> > > + QOpenGLShader *pFShader_;\n> >\n> > No need to prefix pointers with 'p'. I'd name this fragmentShader_.\n> >\n> > > + QOpenGLShader *pVShader_;\n> >\n> > Same here, vertexShader_.\n> >\n> > > + QOpenGLShaderProgram shaderProgram_;\n> >\n> > Is there a specific reason why pFShader_ and pVShader_ are pointers,\n> > while shaderProgram_ is embedded directly in ViewFinderGL ?\n> >\n> > > +\n> > > + /* Vertex buffer */\n> > > + QOpenGLBuffer vbuf_;\n> > > +\n> > > + /* Fragment and Vertex shader file name */\n> > > + QString fsrc_;\n> >\n> > fragmentShaderSrc_ ?\n> >\n> > > + QString vsrc_;\n> >\n> > And vertexShaderSrc_.\n> >\n> > > +\n> > > + unsigned char *yuvDataPtr_;\n> >\n> > And no need for a Ptr suffix either :-)\n> >\n> > > +\n> > > + /* YUV texture planars and parameters */\n> > > + GLuint id_u_;\n> > > + GLuint id_v_;\n> > > + GLuint id_y_;\n> > > + GLuint textureUniformU_;\n> > > + GLuint textureUniformV_;\n> > > + GLuint textureUniformY_;\n> > > + QOpenGLTexture textureU_;\n> > > + QOpenGLTexture textureV_;\n> > > + QOpenGLTexture textureY_;\n> > > + unsigned int horzSubSample_;\n> > > + unsigned int vertSubSample_;\n> > > +\n> > > + QImage image_;\n> >\n> > This is never used.\n> >\n> > > + QMutex mutex_; /* Prevent concurrent access to image_ */\n> > > +};\n> > > +#endif /* __VIEWFINDER_GL_H__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 AC854C3B5B\n\tfor ;\n\tThu, 10 Sep 2020 09:38:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 140B762CE1;\n\tThu, 10 Sep 2020 11:38:48 +0200 (CEST)","from mail-pj1-x1044.google.com (mail-pj1-x1044.google.com\n\t[IPv6:2607:f8b0:4864:20::1044])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A378603EE\n\tfor ;\n\tThu, 10 Sep 2020 11:38:45 +0200 (CEST)","by mail-pj1-x1044.google.com with SMTP id u3so2763784pjr.3\n\tfor ;\n\tThu, 10 Sep 2020 02:38:45 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"x9cOrBBw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=JfMKpnXv2Yawi9dn6iavmp1Rfu/Gm/YFzeiMxzy70fM=;\n\tb=x9cOrBBwUeTrsENowwbf2A3EDxCpGbF81xzwLPSM675fzFB5ub2rkh2/inaPZK+lbU\n\teoulmnMHgSIXGL56a2J+ErNqFNhKcScpLBc/Y7Ow1PXw3D6HX/zBWDKbQm0OkSOGHZlH\n\tbXxiLDQxx7PR90ExU6OZZJoHXLEwsq1xPRegssTTKk5vZCXDS7z95wsEi/cEWAi0fzrw\n\tyGIqwOANgiVBJG2ZrkDJYFHorl5364Aw509mA+yx5KZg9wnhvHIRrHOPEI3PtdAZVulX\n\tbgIbTEsooJAXrBhq2W9IRxXm0HfzLAvXV63NQCFCOlfM9/Dg6Rr23OFr4prtivVwVXfj\n\tqadQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=JfMKpnXv2Yawi9dn6iavmp1Rfu/Gm/YFzeiMxzy70fM=;\n\tb=dkhWimXYooX4BzFRX0YY3KhqrJVd0F77Mwcygz3HtUzI5qYhZQuAGQ6bIUvejJQVse\n\t7NY5ZGW/9Zsa3j/5aR0tNi6V7jI8M4PITT+rfiix1rdWccuuWh2cw+euupdRNPU9ahFN\n\tx3nT2bzOjT8uSYi5Qs+8u8BvDZ3De0sslFiXRyXA7v/msppHuXphhhNyGNDYoDD8qhv5\n\tPvmWPbs6+0l/Fxx+y9yd4AykchRWchyCdjBEZ7+YkVkhPnjCrmU0AoJc46I7gdTeA8VL\n\tKzd2pYM24WprGGbKpeZszLm9agQFL9QJLgSZ4dNYDYtVqMHlFgMOCGAR56pyigbFKZGw\n\t9a4g==","X-Gm-Message-State":"AOAM530hLxsd2Rc1ut9tY/sM9qCgZIWiC+kmxePKmJu7bHrij72RPP83\n\t9rMCDMpC3uZqoZTK1lggF0nH2lnKKnb7aADMEs26Euw1hIQu2A==","X-Google-Smtp-Source":"ABdhPJzDiw0/5eYQWCjJop8fCVqhsblbDKCBlWCSwJzZPKWenJ+ZjMFVDNznnd9Blobv82plYEoiB+QiKi/Bdh5mOPk=","X-Received":"by 2002:a17:90b:110a:: with SMTP id\n\tgi10mr4501283pjb.104.1599730723093; \n\tThu, 10 Sep 2020 02:38:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20200904084316.7319-1-show.liu@linaro.org>\n\t<20200904084316.7319-5-show.liu@linaro.org>\n\t<20200906161807.GA6047@pendragon.ideasonboard.com>\n\t<20200906231001.GA15818@pendragon.ideasonboard.com>","In-Reply-To":"<20200906231001.GA15818@pendragon.ideasonboard.com>","From":"Show Liu ","Date":"Thu, 10 Sep 2020 17:38:31 +0800","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v5 3/4] qcam: add viewfinderGL class\n\tto accelerate the format convert","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":"multipart/mixed;\n\tboundary=\"===============8033594468860638017==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12463,"web_url":"https://patchwork.libcamera.org/comment/12463/","msgid":"<20200912011342.GJ6808@pendragon.ideasonboard.com>","date":"2020-09-12T01:13:42","subject":"Re: [libcamera-devel] [PATCH v5 3/4] qcam: add viewfinderGL class\n\tto accelerate the format convert","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Show,\n\nOn Thu, Sep 10, 2020 at 05:38:31PM +0800, Show Liu wrote:\n> On Mon, Sep 7, 2020 at 7:10 AM Laurent Pinchart wrote:\n> > On Sun, Sep 06, 2020 at 07:18:07PM +0300, Laurent Pinchart wrote:\n> > > On Fri, Sep 04, 2020 at 04:43:15PM +0800, Show Liu wrote:\n> > > > the viewfinderGL accelerates the format conversion by\n> > > > using OpenGL ES shader\n> > > >\n> > > > Signed-off-by: Show Liu \n> > > > ---\n> > > > src/qcam/meson.build | 2 +\n> > > > src/qcam/viewfinder_gl.cpp | 441 +++++++++++++++++++++++++++++++++++++\n> > > > src/qcam/viewfinder_gl.h | 97 ++++++++\n> > > > 3 files changed, 540 insertions(+)\n> > > > create mode 100644 src/qcam/viewfinder_gl.cpp\n> > > > create mode 100644 src/qcam/viewfinder_gl.h\n> > > >\n> > > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > > > index a4bad0a..32c0fc3 100644\n> > > > --- a/src/qcam/meson.build\n> > > > +++ b/src/qcam/meson.build\n> > > > @@ -7,11 +7,13 @@ qcam_sources = files([\n> > > > 'main.cpp',\n> > > > 'main_window.cpp',\n> > > > 'viewfinder_qt.cpp',\n> > > > + 'viewfinder_gl.cpp',\n> > >\n> > > Let's keep files alphabetically sorted.\n> > >\n> > > > ])\n> > > >\n> > > > qcam_moc_headers = files([\n> > > > 'main_window.h',\n> > > > 'viewfinder_qt.h',\n> > > > + 'viewfinder_gl.h',\n> > >\n> > > Here too.\n> > >\n> > > > ])\n> > > >\n> > > > qcam_resources = files([\n> > >\n> > > You need to set the minimum Qt version to 5.4, as QOpenGLWidget wasn't\n> > > available before that.\n> > >\n> > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > > index 32c0fc3e0f6b..9d3f189a896b 100644\n> > > --- a/src/qcam/meson.build\n> > > +++ b/src/qcam/meson.build\n> > > @@ -25,7 +25,8 @@ qt5 = import('qt5')\n> > > qt5_dep = dependency('qt5',\n> > > method : 'pkg-config',\n> > > modules : ['Core', 'Gui', 'Widgets'],\n> > > - required : get_option('qcam'))\n> > > + required : get_option('qcam'),\n> > > + version : '>=5.4')\n> > >\n> > > if qt5_dep.found()\n> > > qcam_deps = [\n> > >\n> > > Furthermore, Qt can be compiled without OpenGL support, in which case\n> > > this patch will fail to compile. The following change should address it.\n> > >\n> > > diff --git a/meson.build b/meson.build\n> > > index b6c99ba8e0eb..5f7d619a79d0 100644\n> > > --- a/meson.build\n> > > +++ b/meson.build\n> > > @@ -26,6 +26,7 @@ libcamera_version = libcamera_git_version.split('+')[0]\n> > >\n> > > # Configure the build environment.\n> > > cc = meson.get_compiler('c')\n> > > +cxx = meson.get_compiler('cpp')\n> > > config_h = configuration_data()\n> > >\n> > > if cc.has_header_symbol('execinfo.h', 'backtrace')\n> > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > > index 32c0fc3e0f6b..9bb48c0d06c5 100644\n> > > --- a/src/qcam/meson.build\n> > > +++ b/src/qcam/meson.build\n> > > @@ -7,18 +7,15 @@ qcam_sources = files([\n> > > 'main.cpp',\n> > > 'main_window.cpp',\n> > > 'viewfinder_qt.cpp',\n> > > - 'viewfinder_gl.cpp',\n> > > ])\n> > >\n> > > qcam_moc_headers = files([\n> > > 'main_window.h',\n> > > 'viewfinder_qt.h',\n> > > - 'viewfinder_gl.h',\n> > > ])\n> > >\n> > > qcam_resources = files([\n> > > 'assets/feathericons/feathericons.qrc',\n> > > - 'assets/shader/shaders.qrc'\n> > > ])\n> > >\n> > > qt5 = import('qt5')\n> > > @@ -44,6 +42,19 @@ if qt5_dep.found()\n> > > ])\n> > > endif\n> > >\n> > > + if cxx.has_header_symbol('QOpenGLWidget', 'QOpenGLWidget',\n> > > + dependencies : qt5_dep, args : '-fPIC')\n> > > + qcam_sources += files([\n> > > + 'viewfinder_gl.cpp',\n> > > + ])\n> > > + qcam_moc_headers += files([\n> > > + 'viewfinder_gl.h',\n> > > + ])\n> > > + qcam_resources += files([\n> > > + 'assets/shader/shaders.qrc'\n> > > + ])\n> > > + endif\n> > > +\n> > > # gcc 9 introduced a deprecated-copy warning that is triggered by Qt until\n> > > # Qt 5.13. clang 10 introduced the same warning, but detects more issues\n> > > # that are not fixed in Qt yet. Disable the warning manually in both cases.\n> > >\n> > > Patch 4/4 will need to be updated to with conditional compilation on\n> > > QT_NO_OPENGL.\n> > >\n> > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > > > new file mode 100644\n> > > > index 0000000..5591916\n> > > > --- /dev/null\n> > > > +++ b/src/qcam/viewfinder_gl.cpp\n> > > > @@ -0,0 +1,441 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Linaro\n> > > > + *\n> > > > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader\n> > > > + */\n> > > > +\n> > > > +#include \"viewfinder_gl.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#define ATTRIB_VERTEX 0\n> > > > +#define ATTRIB_TEXTURE 1\n> > >\n> > > Is there a guarantee that attribute locations match the declaration\n> > > order in the shader program ? Wouldn't it be better to use\n> > > shaderProgram.attributeLocation() to retrieve the attribute locations by\n> > > name (before linking), or shaderProgram.bindAttributeLocation() to set\n> > > them explicitly (after linking) ?\n> > >\n> > > > +\n> > > > +static const QList supportFormats{\n> > >\n> > > s/supportFormats/supportedFormats/\n> > >\n> > > > + libcamera::formats::NV12,\n> > > > + libcamera::formats::NV21,\n> > > > + libcamera::formats::NV16,\n> > > > + libcamera::formats::NV61,\n> > > > + libcamera::formats::NV24,\n> > > > + libcamera::formats::NV42,\n> > > > + libcamera::formats::YUV420,\n> > > > + libcamera::formats::YVU420\n> > > > +};\n> > > > +\n> > > > +ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > > > + : QOpenGLWidget(parent),\n> > > > + buffer_(nullptr),\n> > > > + pFShader_(nullptr),\n> > > > + pVShader_(nullptr),\n> > > > + vbuf_(QOpenGLBuffer::VertexBuffer),\n> > > > + yuvDataPtr_(nullptr),\n> > > > + textureU_(QOpenGLTexture::Target2D),\n> > > > + textureV_(QOpenGLTexture::Target2D),\n> > > > + textureY_(QOpenGLTexture::Target2D)\n> > >\n> > > Feel free to have multiple members per line if desired.\n> > >\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +ViewFinderGL::~ViewFinderGL()\n> > > > +{\n> > > > + removeShader();\n> > > > +\n> > > > + if (textureY_.isCreated())\n> > > > + textureY_.destroy();\n> > > > +\n> > > > + if (textureU_.isCreated())\n> > > > + textureU_.destroy();\n> > > > +\n> > > > + if (textureV_.isCreated())\n> > > > + textureV_.destroy();\n> > >\n> > > Are these needed, or does the QOpenGLTexture destructor destroy the\n> > > textures ?\n> > >\n> > > > +\n> > > > + vbuf_.destroy();\n> > >\n> > > Same question for vbuf_.\n> > >\n> > > > +}\n> > > > +\n> > > > +const QList &ViewFinderGL::nativeFormats() const\n> > > > +{\n> > > > + return (::supportFormats);\n> > >\n> > > No need for parentheses or an explicit namespace.\n> > >\n> > > return supportedFormats;\n> > >\n> > > > +}\n> > > > +\n> > > > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> > > > + const QSize &size)\n> > > > +{\n> > > > + int ret = 0;\n> > > > +\n> > > > + if (isFormatSupport(format)) {\n> > > > + format_ = format;\n> > > > + size_ = size;\n> > > > + } else {\n> > > > + ret = -1;\n> > > > + }\n> > > > + updateGeometry();\n> > >\n> > > When the format change, don't you need to recreate the shaders ?\n> > >\n> > > > + return ret;\n> > > > +}\n> > > > +\n> > > > +void ViewFinderGL::stop()\n> > > > +{\n> > > > + if (buffer_) {\n> > > > + renderComplete(buffer_);\n> > > > + buffer_ = nullptr;\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +QImage ViewFinderGL::getCurrentImage()\n> > > > +{\n> > > > + QMutexLocker locker(&mutex_);\n> > > > +\n> > > > + return (grabFramebuffer());\n> > >\n> > > return grabFrameBuffer();\n> > >\n> > > > +}\n> > > > +\n> > > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > +{\n> > > > + if (buffer->planes().size() != 1) {\n> > > > + qWarning() << \"Multi-planar buffers are not supported\";\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + if (buffer_)\n> > > > + renderComplete(buffer_);\n> > > > +\n> > > > + unsigned char *memory = static_cast(map->memory);\n> > > > + if (memory) {\n> > >\n> > > Can memory be null ?\n> > >\n> > > > + yuvDataPtr_ = memory;\n> > > > + update();\n> > > > + buffer_ = buffer;\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +bool ViewFinderGL::isFormatSupport(const libcamera::PixelFormat &format)\n> > > > +{\n> > >\n> > > As this function sets internal members based on the format, I would call\n> > > it selectFormat(), it does more than just checking if the format is\n> > > supported.\n> > >\n> > > > + bool ret = true;\n> > > > + switch (format) {\n> > > > + case libcamera::formats::NV12:\n> > > > + horzSubSample_ = 2;\n> > > > + vertSubSample_ = 2;\n> > > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > > + fsrc_ = \":NV_2_planes_UV_f.glsl\";\n> > > > + break;\n> > > > + case libcamera::formats::NV21:\n> > > > + horzSubSample_ = 2;\n> > > > + vertSubSample_ = 2;\n> > > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > > + fsrc_ = \":NV_2_planes_VU_f.glsl\";\n> > > > + break;\n> > > > + case libcamera::formats::NV16:\n> > > > + horzSubSample_ = 2;\n> > > > + vertSubSample_ = 1;\n> > > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > > + fsrc_ = \":NV_2_planes_UV_f.glsl\";\n> > > > + break;\n> > > > + case libcamera::formats::NV61:\n> > > > + horzSubSample_ = 2;\n> > > > + vertSubSample_ = 1;\n> > > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > > + fsrc_ = \":NV_2_planes_VU_f.glsl\";\n> > > > + break;\n> > > > + case libcamera::formats::NV24:\n> > > > + horzSubSample_ = 1;\n> > > > + vertSubSample_ = 1;\n> > > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > > + fsrc_ = \":NV_2_planes_UV_f.glsl\";\n> > > > + break;\n> > > > + case libcamera::formats::NV42:\n> > > > + horzSubSample_ = 1;\n> > > > + vertSubSample_ = 1;\n> > > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > > + fsrc_ = \":NV_2_planes_VU_f.glsl\";\n> > > > + break;\n> > > > + case libcamera::formats::YUV420:\n> > > > + horzSubSample_ = 2;\n> > > > + vertSubSample_ = 2;\n> > > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > > + fsrc_ = \":NV_3_planes_UV_f.glsl\";\n> > > > + break;\n> > > > + case libcamera::formats::YVU420:\n> > > > + horzSubSample_ = 2;\n> > > > + vertSubSample_ = 2;\n> > > > + vsrc_ = \":NV_vertex_shader.glsl\";\n> > > > + fsrc_ = \":NV_3_planes_VU_f.glsl\";\n> > > > + break;\n> > > > + default:\n> > > > + ret = false;\n> > > > + qWarning() << \"[ViewFinderGL]:\"\n> > > > + << \"format not support yet.\";\n> > >\n> > > s/support yet./supported/\n> > >\n> > > > + break;\n> > > > + };\n> > > > +\n> > > > + return ret;\n> > > > +}\n> > > > +\n> > > > +void ViewFinderGL::createVertexShader()\n> > > > +{\n> > > > + bool bCompile;\n> > >\n> > > No need to prefix variables with the type name.\n> > >\n> > > > + /* Create Vertex Shader */\n> > > > + pVShader_ = new QOpenGLShader(QOpenGLShader::Vertex, this);\n> > > > +\n> > > > + bCompile = pVShader_->compileSourceFile(vsrc_);\n> > > > + if (!bCompile) {\n> > > > + qWarning() << \"[ViewFinderGL]:\" << pVShader_->log();\n> > > > + }\n> > >\n> > > This can simply be written\n> > >\n> > > if (!pVShader_->compileSourceFile(vsrc_))\n> > > qWarning() << \"[ViewFinderGL]:\" << pVShader_->log();\n> > >\n> > > > +\n> > > > + shaderProgram_.addShader(pVShader_);\n> > >\n> > > Won't this crash if shader compilation failed ? I think\n> > > createVertexShader() should return a status as a bool.\n> > >\n> > > > +}\n> > > > +\n> > > > +bool ViewFinderGL::createFragmentShader()\n> > > > +{\n> > > > + bool bCompile;\n> > > > +\n> > > > + /* Create Fragment Shader */\n> > > > + pFShader_ = new QOpenGLShader(QOpenGLShader::Fragment, this);\n> > > > +\n> > > > + bCompile = pFShader_->compileSourceFile(fsrc_);\n> > > > + if (!bCompile) {\n> > >\n> > > if (!pFShader_->compileSourceFile(fsrc_)) {\n> > >\n> > > > + qWarning() << \"[ViewFinderGL]:\" << pFShader_->log();\n> > > > + return bCompile;\n> > >\n> > > return false;\n> > >\n> > > > + }\n> > > > +\n> > > > + shaderProgram_.addShader(pFShader_);\n> > > > +\n> > > > + /* Link shader pipeline */\n> > > > + if (!shaderProgram_.link()) {\n> > > > + qWarning() << \"[ViewFinderGL]:\" << shaderProgram_.log();\n> > > > + close();\n> > > > + }\n> > > > +\n> > > > + /* Bind shader pipeline for use */\n> > > > + if (!shaderProgram_.bind()) {\n> > > > + qWarning() << \"[ViewFinderGL]:\" << shaderProgram_.log();\n> > > > + close();\n> > > > + }\n> > > > +\n> > > > + shaderProgram_.enableAttributeArray(ATTRIB_VERTEX);\n> > > > + shaderProgram_.enableAttributeArray(ATTRIB_TEXTURE);\n> > > > +\n> > > > + shaderProgram_.setAttributeBuffer(ATTRIB_VERTEX,\n> > > > + GL_FLOAT,\n> > > > + 0,\n> > > > + 2,\n> > > > + 2 * sizeof(GLfloat));\n> > > > + shaderProgram_.setAttributeBuffer(ATTRIB_TEXTURE,\n> > > > + GL_FLOAT,\n> > > > + 8 * sizeof(GLfloat),\n> > > > + 2,\n> > > > + 2 * sizeof(GLfloat));\n> > > > +\n> > > > + textureUniformY_ = shaderProgram_.uniformLocation(\"tex_y\");\n> > > > + textureUniformU_ = shaderProgram_.uniformLocation(\"tex_u\");\n> > > > + textureUniformV_ = shaderProgram_.uniformLocation(\"tex_v\");\n> > > > +\n> > > > + if (!textureY_.isCreated())\n> > > > + textureY_.create();\n> > > > +\n> > > > + if (!textureU_.isCreated())\n> > > > + textureU_.create();\n> > > > +\n> > > > + if (!textureV_.isCreated())\n> > > > + textureV_.create();\n> > > > +\n> > > > + id_y_ = textureY_.textureId();\n> > > > + id_u_ = textureU_.textureId();\n> > > > + id_v_ = textureV_.textureId();\n> > > > + return true;\n> > > > +}\n> > > > +\n> > > > +void ViewFinderGL::configureTexture(unsigned int id)\n> > > > +{\n> > > > + glBindTexture(GL_TEXTURE_2D, id);\n> > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);\n> > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);\n> > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);\n> > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);\n> > > > +}\n> > > > +\n> > > > +void ViewFinderGL::removeShader()\n> > > > +{\n> > > > + if (shaderProgram_.isLinked()) {\n> > > > + shaderProgram_.release();\n> > > > + shaderProgram_.removeAllShaders();\n> > > > + }\n> > > > +\n> > > > + if (pFShader_)\n> > > > + delete pFShader_;\n> > > > +\n> > > > + if (pVShader_)\n> > > > + delete pVShader_;\n> > > > +}\n> > > > +\n> > > > +void ViewFinderGL::initializeGL()\n> > > > +{\n> > > > + initializeOpenGLFunctions();\n> > > > + glEnable(GL_TEXTURE_2D);\n> > > > + glDisable(GL_DEPTH_TEST);\n> > > > +\n> > > > + static const GLfloat vertices[]{\n> > > > + -1.0f, -1.0f, -1.0f, +1.0f,\n> > > > + +1.0f, +1.0f, +1.0f, -1.0f,\n> > > > + 0.0f, 1.0f, 0.0f, 0.0f,\n> > > > + 1.0f, 0.0f, 1.0f, 1.0f\n> > > > + };\n> > >\n> > > This is vertex and texture coordinates, not just vertices. How about\n> > > writing it as follows ?\n> > >\n> > > static const GLfloat coordinates[2][4][2] {\n> > > {\n> > > /* Vertex coordinates */\n> > > { -1.0f, -1.0f },\n> > > { -1.0f, +1.0f },\n> > > { +1.0f, +1.0f },\n> > > { +1.0f, -1.0f },\n> > > }, {\n> > > /* Texture coordinates */\n> > > { 0.0f, 1.0f },\n> > > { 0.0f, 0.0f },\n> > > { 1.0f, 0.0f },\n> > > { 1.0f, 1.0f },\n> > > },\n> > > };\n> > >\n> > > There's something I don't get though, maybe you can help me understand\n> > > it. The vertex coordinates are copied directly to gl_Position in the\n> > > vertex shader, so they're essentially expressed in clip space, which I\n> > > understand has X pointing towards the right and Y pointing towards the\n> > > top. The texture coordinates, if my understand is correct again, have\n> > > their origin at the bottom-left corner too. The first vertex, (-1.0,\n> > > -1.0), which is at the bottom-left, then maps to texture coordinate\n> > > (0.0, 1.0), which is the top-left pixel of the texture. The image should\n> > > thus be flipped vertically. Why isn't it ? I'm sure I'm missing\n> > > somethign simple.\n> >\n> > I figured it out. The texture created with glTexImage2D() has (0,0) at\n> > byte 0. As the camera captures the image with the top line at the\n> > beginning of the buffer, the texture is stored with the top line on row\n> > 0. texture coordinate (0.0, 1.0) is thus the bottom-left corner of the\n> > texture, not the top-left corner.\n> \n> You definitely are an OpenGL expert...:-)\n> The original coordinate mapping is for my camera usage on rockpi4b.\n> And in my case the camera module is vertical flipped.\n> So...anyway I am now change the coordinate mapping as default below\n> \n> + static const GLfloat coordinates[2][4][2] {\n> + {\n> + /* Vertex coordinates */\n> + { -1.0f, -1.0f },\n> + { -1.0f, +1.0f },\n> + { +1.0f, +1.0f },\n> + { +1.0f, -1.0f },\n> + }, {\n> + /* Texture coordinates */\n> + { 1.0f, 0.0f },\n> + { 1.0f, 1.0f },\n> + { 0.0f, 1.0f },\n> + { 0.0f, 0.0f },\n> + },\n>\n> Please let me know if you have any concern.\n\nI could be mistaken, but I think this will rotate the image by 180°.\n\nPosition of the vertices:\n\n\t1 +-------+ 2\n\t | |\n\t | |\n\t | |\n\t0 +-------+ 3\n\nTexture coordinates:\n\n\t3 +-------+ 0\n\t | --- |\n\t | |- |\n\t | | |\n\t2 +-------+ 1\n\nMapping the texture on the screen:\n\n\t1 +-------+ 2\n\t | | |\n\t | -| |\n\t | --- |\n\t0 +-------+ 3\n\n> > > > +\n> > > > + vbuf_.create();\n> > > > + vbuf_.bind();\n> > > > + vbuf_.allocate(vertices, sizeof(vertices));\n> > > > +\n> > > > + /* Create Vertex Shader */\n> > > > + createVertexShader();\n> > > > +\n> > > > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f);\n> > > > +}\n> > > > +\n> > > > +void ViewFinderGL::doRender()\n> > > > +{\n> > > > + switch (format_) {\n> > > > + case libcamera::formats::NV12:\n> > > > + case libcamera::formats::NV21:\n> > > > + case libcamera::formats::NV16:\n> > > > + case libcamera::formats::NV61:\n> > > > + case libcamera::formats::NV24:\n> > > > + case libcamera::formats::NV42:\n> > > > + /* activate texture 0 */\n> > > > + glActiveTexture(GL_TEXTURE0);\n> > > > + configureTexture(id_y_);\n> > > > + glTexImage2D(GL_TEXTURE_2D,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + size_.width(),\n> > > > + size_.height(),\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + GL_UNSIGNED_BYTE,\n> > > > + yuvDataPtr_);\n> > > > + glUniform1i(textureUniformY_, 0);\n> > >\n> > > Would it make sense to use\n> > >\n> > > shaderProgram_.setUniformValue(textureUniformY_, 0);\n> > >\n> > > (and similarly below) ?\n> > >\n> > > > +\n> > > > + /* activate texture 1 */\n> > > > + glActiveTexture(GL_TEXTURE1);\n> > > > + configureTexture(id_u_);\n> > > > + glTexImage2D(GL_TEXTURE_2D,\n> > > > + 0,\n> > > > + GL_RG,\n> > > > + size_.width() / horzSubSample_,\n> > > > + size_.height() / vertSubSample_,\n> > > > + 0,\n> > > > + GL_RG,\n> > > > + GL_UNSIGNED_BYTE,\n> > > > + (char *)yuvDataPtr_ + size_.width() * size_.height());\n> > > > + glUniform1i(textureUniformU_, 1);\n> > > > + break;\n> > >\n> > > A blank line here would increase readability. Same below.\n> > >\n> > > > + case libcamera::formats::YUV420:\n> > > > + /* activate texture 0 */\n> > > > + glActiveTexture(GL_TEXTURE0);\n> > > > + configureTexture(id_y_);\n> > > > + glTexImage2D(GL_TEXTURE_2D,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + size_.width(),\n> > > > + size_.height(),\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + GL_UNSIGNED_BYTE,\n> > > > + yuvDataPtr_);\n> > > > + glUniform1i(textureUniformY_, 0);\n> > > > +\n> > > > + /* activate texture 1 */\n> > > > + glActiveTexture(GL_TEXTURE1);\n> > > > + configureTexture(id_u_);\n> > > > + glTexImage2D(GL_TEXTURE_2D,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + size_.width() / horzSubSample_,\n> > > > + size_.height() / vertSubSample_,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + GL_UNSIGNED_BYTE,\n> > > > + (char *)yuvDataPtr_ + size_.width() * size_.height());\n> > > > + glUniform1i(textureUniformU_, 1);\n> > > > +\n> > > > + /* activate texture 2 */\n> > > > + glActiveTexture(GL_TEXTURE2);\n> > > > + configureTexture(id_v_);\n> > > > + glTexImage2D(GL_TEXTURE_2D,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + size_.width() / horzSubSample_,\n> > > > + size_.height() / vertSubSample_,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + GL_UNSIGNED_BYTE,\n> > > > + (char *)yuvDataPtr_ + size_.width() * size_.height() * 5 / 4);\n> > > > + glUniform1i(textureUniformV_, 2);\n> > > > + break;\n> > > > + case libcamera::formats::YVU420:\n> > > > + /* activate texture 0 */\n> > > > + glActiveTexture(GL_TEXTURE0);\n> > > > + configureTexture(id_y_);\n> > > > + glTexImage2D(GL_TEXTURE_2D,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + size_.width(),\n> > > > + size_.height(),\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + GL_UNSIGNED_BYTE,\n> > > > + yuvDataPtr_);\n> > > > + glUniform1i(textureUniformY_, 0);\n> > > > +\n> > > > + /* activate texture 1 */\n> > > > + glActiveTexture(GL_TEXTURE2);\n> > > > + configureTexture(id_v_);\n> > > > + glTexImage2D(GL_TEXTURE_2D,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + size_.width() / horzSubSample_,\n> > > > + size_.height() / vertSubSample_,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + GL_UNSIGNED_BYTE,\n> > > > + (char *)yuvDataPtr_ + size_.width() * size_.height());\n> > > > + glUniform1i(textureUniformV_, 1);\n> > >\n> > > OK, now I understand why the NV_3_planes_UV_f.glsl and\n> > > NV_3_planes_VU_f.glsl shaders are identical, you switch the U and V\n> > > planes here. I think we should then merge the two files into\n> > > NV_3_planes_f_glsl. The above line should become\n> > >\n> > > glUniform1i(textureUniformU_, 2);\n> > >\n> > > as you deal with texture 2 here (and a similar change is needed below),\n> > > and the two blocks should be swapped as the comments are incorrect (the\n> > > comment above refers to texture 1 while the code deals with texture 2).\n> > >\n> > > > +\n> > > > + /* activate texture 2 */\n> > > > + glActiveTexture(GL_TEXTURE1);\n> > > > + configureTexture(id_u_);\n> > > > + glTexImage2D(GL_TEXTURE_2D,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + size_.width() / horzSubSample_,\n> > > > + size_.height() / vertSubSample_,\n> > > > + 0,\n> > > > + GL_RED,\n> > > > + GL_UNSIGNED_BYTE,\n> > > > + (char *)yuvDataPtr_ + size_.width() * size_.height() * 5 / 4);\n> > > > + glUniform1i(textureUniformU_, 2);\n> > >\n> > > Please add a break here, let's not rely on implicit fall-through.\n> > >\n> > > > + default:\n> > > > + break;\n> > > > + };\n> > > > +}\n> > > > +\n> > > > +void ViewFinderGL::paintGL()\n> > > > +{\n> > > > + if (pFShader_ == nullptr)\n> > >\n> > > if (!pfShader_)\n> > >\n> > > > + createFragmentShader();\n> > > > +\n> > > > + if (yuvDataPtr_) {\n> > > > + glClearColor(0.0, 0.0, 0.0, 1.0);\n> > > > + glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);\n> > > > +\n> > > > + doRender();\n> > > > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4);\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +void ViewFinderGL::resizeGL(int w, int h)\n> > > > +{\n> > > > + glViewport(0, 0, w, h);\n> > > > +}\n> > > > +\n> > > > +QSize ViewFinderGL::sizeHint() const\n> > > > +{\n> > > > + return size_.isValid() ? size_ : QSize(640, 480);\n> > > > +}\n> > > > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> > > > new file mode 100644\n> > > > index 0000000..e708c32\n> > > > --- /dev/null\n> > > > +++ b/src/qcam/viewfinder_gl.h\n> > > > @@ -0,0 +1,97 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Linaro\n> > > > + *\n> > > > + * viewfinder_GL.h - OpenGL Viewfinder for rendering by OpenGL shader\n> > > > + *\n> > > > + */\n> > > > +#ifndef __VIEWFINDER_GL_H__\n> > > > +#define __VIEWFINDER_GL_H__\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > >\n> > > This header shouldn't be needed.\n> > >\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > >\n> > > Missing blank line.\n> > >\n> > > > +#include \"viewfinder.h\"\n> > > > +\n> > > > +class ViewFinderGL : public QOpenGLWidget,\n> > > > + public ViewFinder,\n> > > > + protected QOpenGLFunctions\n> > > > +{\n> > > > + Q_OBJECT\n> > > > +\n> > > > +public:\n> > > > + ViewFinderGL(QWidget *parent = 0);\n> > >\n> > > = nullptr\n> > >\n> > > > + ~ViewFinderGL();\n> > > > +\n> > > > + const QList &nativeFormats() const override;\n> > > > +\n> > > > + int setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > > > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > > + void stop() override;\n> > > > +\n> > > > + QImage getCurrentImage() override;\n> > > > +\n> > > > +Q_SIGNALS:\n> > > > + void renderComplete(libcamera::FrameBuffer *buffer);\n> > > > +\n> > > > +protected:\n> > > > + void initializeGL() override;\n> > > > + void paintGL() override;\n> > > > + void resizeGL(int w, int h) override;\n> > > > + QSize sizeHint() const override;\n> > > > +\n> > > > +private:\n> > > > + bool isFormatSupport(const libcamera::PixelFormat &format);\n> > >\n> > > s/isFormatSupport/isFormatSupported/\n> > >\n> > > > +\n> > > > + void configureTexture(unsigned int id);\n> > > > + bool createFragmentShader();\n> > > > + void createVertexShader();\n> > > > + void removeShader();\n> > > > + void doRender();\n> > > > +\n> > > > + /* Captured image size, format and buffer */\n> > > > + libcamera::FrameBuffer *buffer_;\n> > > > + libcamera::PixelFormat format_;\n> > > > + QSize size_;\n> > > > +\n> > > > + /* OpenGL components for render */\n> > >\n> > > s/render/rendering/\n> > >\n> > > > + QOpenGLShader *pFShader_;\n> > >\n> > > No need to prefix pointers with 'p'. I'd name this fragmentShader_.\n> > >\n> > > > + QOpenGLShader *pVShader_;\n> > >\n> > > Same here, vertexShader_.\n> > >\n> > > > + QOpenGLShaderProgram shaderProgram_;\n> > >\n> > > Is there a specific reason why pFShader_ and pVShader_ are pointers,\n> > > while shaderProgram_ is embedded directly in ViewFinderGL ?\n> > >\n> > > > +\n> > > > + /* Vertex buffer */\n> > > > + QOpenGLBuffer vbuf_;\n> > > > +\n> > > > + /* Fragment and Vertex shader file name */\n> > > > + QString fsrc_;\n> > >\n> > > fragmentShaderSrc_ ?\n> > >\n> > > > + QString vsrc_;\n> > >\n> > > And vertexShaderSrc_.\n> > >\n> > > > +\n> > > > + unsigned char *yuvDataPtr_;\n> > >\n> > > And no need for a Ptr suffix either :-)\n> > >\n> > > > +\n> > > > + /* YUV texture planars and parameters */\n> > > > + GLuint id_u_;\n> > > > + GLuint id_v_;\n> > > > + GLuint id_y_;\n> > > > + GLuint textureUniformU_;\n> > > > + GLuint textureUniformV_;\n> > > > + GLuint textureUniformY_;\n> > > > + QOpenGLTexture textureU_;\n> > > > + QOpenGLTexture textureV_;\n> > > > + QOpenGLTexture textureY_;\n> > > > + unsigned int horzSubSample_;\n> > > > + unsigned int vertSubSample_;\n> > > > +\n> > > > + QImage image_;\n> > >\n> > > This is never used.\n> > >\n> > > > + QMutex mutex_; /* Prevent concurrent access to image_ */\n> > > > +};\n> > > > +#endif /* __VIEWFINDER_GL_H__ */","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 91218BF01C\n\tfor ;\n\tSat, 12 Sep 2020 01:14:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C88E62DA1;\n\tSat, 12 Sep 2020 03:14:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B10D62C43\n\tfor ;\n\tSat, 12 Sep 2020 03:14:11 +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 550A226B;\n\tSat, 12 Sep 2020 03:14:10 +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=\"dFdvJV0x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599873250;\n\tbh=4MR2FZmWX85EDfYqL6+Lytn3ijLEpT7YkGuqI6U55Gs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dFdvJV0xK19cEIU4cDDabFOBsXyGnkCMyxyMy0dwD2Nfh574+TmpgFZSzxDEGsmOx\n\tmd3SGkDry85pLan5P/ZSeCwH59n3AQxRAn4mWxFJXEXzaTOHxTsF0IcZWpj8mOlsk6\n\tXuBuqP5ED+cnoa6xFsTyrORC9DDaQUo3d1JJJxq0=","Date":"Sat, 12 Sep 2020 04:13:42 +0300","From":"Laurent Pinchart ","To":"Show Liu ","Message-ID":"<20200912011342.GJ6808@pendragon.ideasonboard.com>","References":"<20200904084316.7319-1-show.liu@linaro.org>\n\t<20200904084316.7319-5-show.liu@linaro.org>\n\t<20200906161807.GA6047@pendragon.ideasonboard.com>\n\t<20200906231001.GA15818@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 3/4] qcam: add viewfinderGL class\n\tto accelerate the format convert","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]