<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 7, 2020 at 7:10 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sun, Sep 06, 2020 at 07:18:07PM +0300, Laurent Pinchart wrote:<br>
> Hi Show,<br>
> <br>
> Thank you for the patch.<br>
> <br>
> On Fri, Sep 04, 2020 at 04:43:15PM +0800, Show Liu wrote:<br>
> > the viewfinderGL accelerates the format conversion by<br>
> > using OpenGL ES shader<br>
> > <br>
> > Signed-off-by: Show Liu <<a href="mailto:show.liu@linaro.org" target="_blank">show.liu@linaro.org</a>><br>
> > ---<br>
> > src/qcam/meson.build | 2 +<br>
> > src/qcam/viewfinder_gl.cpp | 441 +++++++++++++++++++++++++++++++++++++<br>
> > src/qcam/viewfinder_gl.h | 97 ++++++++<br>
> > 3 files changed, 540 insertions(+)<br>
> > create mode 100644 src/qcam/viewfinder_gl.cpp<br>
> > create mode 100644 src/qcam/viewfinder_gl.h<br>
> > <br>
> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build<br>
> > index a4bad0a..32c0fc3 100644<br>
> > --- a/src/qcam/meson.build<br>
> > +++ b/src/qcam/meson.build<br>
> > @@ -7,11 +7,13 @@ qcam_sources = files([<br>
> > 'main.cpp',<br>
> > 'main_window.cpp',<br>
> > 'viewfinder_qt.cpp',<br>
> > + 'viewfinder_gl.cpp',<br>
> <br>
> Let's keep files alphabetically sorted.<br>
> <br>
> > ])<br>
> > <br>
> > qcam_moc_headers = files([<br>
> > 'main_window.h',<br>
> > 'viewfinder_qt.h',<br>
> > + 'viewfinder_gl.h',<br>
> <br>
> Here too.<br>
> <br>
> > ])<br>
> > <br>
> > qcam_resources = files([<br>
> <br>
> You need to set the minimum Qt version to 5.4, as QOpenGLWidget wasn't<br>
> available before that.<br>
> <br>
> diff --git a/src/qcam/meson.build b/src/qcam/meson.build<br>
> index 32c0fc3e0f6b..9d3f189a896b 100644<br>
> --- a/src/qcam/meson.build<br>
> +++ b/src/qcam/meson.build<br>
> @@ -25,7 +25,8 @@ qt5 = import('qt5')<br>
> qt5_dep = dependency('qt5',<br>
> method : 'pkg-config',<br>
> modules : ['Core', 'Gui', 'Widgets'],<br>
> - required : get_option('qcam'))<br>
> + required : get_option('qcam'),<br>
> + version : '>=5.4')<br>
> <br>
> if qt5_dep.found()<br>
> qcam_deps = [<br>
> <br>
> Furthermore, Qt can be compiled without OpenGL support, in which case<br>
> this patch will fail to compile. The following change should address it.<br>
> <br>
> diff --git a/meson.build b/meson.build<br>
> index b6c99ba8e0eb..5f7d619a79d0 100644<br>
> --- a/meson.build<br>
> +++ b/meson.build<br>
> @@ -26,6 +26,7 @@ libcamera_version = libcamera_git_version.split('+')[0]<br>
> <br>
> # Configure the build environment.<br>
> cc = meson.get_compiler('c')<br>
> +cxx = meson.get_compiler('cpp')<br>
> config_h = configuration_data()<br>
> <br>
> if cc.has_header_symbol('execinfo.h', 'backtrace')<br>
> diff --git a/src/qcam/meson.build b/src/qcam/meson.build<br>
> index 32c0fc3e0f6b..9bb48c0d06c5 100644<br>
> --- a/src/qcam/meson.build<br>
> +++ b/src/qcam/meson.build<br>
> @@ -7,18 +7,15 @@ qcam_sources = files([<br>
> 'main.cpp',<br>
> 'main_window.cpp',<br>
> 'viewfinder_qt.cpp',<br>
> - 'viewfinder_gl.cpp',<br>
> ])<br>
> <br>
> qcam_moc_headers = files([<br>
> 'main_window.h',<br>
> 'viewfinder_qt.h',<br>
> - 'viewfinder_gl.h',<br>
> ])<br>
> <br>
> qcam_resources = files([<br>
> 'assets/feathericons/feathericons.qrc',<br>
> - 'assets/shader/shaders.qrc'<br>
> ])<br>
> <br>
> qt5 = import('qt5')<br>
> @@ -44,6 +42,19 @@ if qt5_dep.found()<br>
> ])<br>
> endif<br>
> <br>
> + if cxx.has_header_symbol('QOpenGLWidget', 'QOpenGLWidget',<br>
> + dependencies : qt5_dep, args : '-fPIC')<br>
> + qcam_sources += files([<br>
> + 'viewfinder_gl.cpp',<br>
> + ])<br>
> + qcam_moc_headers += files([<br>
> + 'viewfinder_gl.h',<br>
> + ])<br>
> + qcam_resources += files([<br>
> + 'assets/shader/shaders.qrc'<br>
> + ])<br>
> + endif<br>
> +<br>
> # gcc 9 introduced a deprecated-copy warning that is triggered by Qt until<br>
> # Qt 5.13. clang 10 introduced the same warning, but detects more issues<br>
> # that are not fixed in Qt yet. Disable the warning manually in both cases.<br>
> <br>
> Patch 4/4 will need to be updated to with conditional compilation on<br>
> QT_NO_OPENGL.<br>
> <br>
> > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp<br>
> > new file mode 100644<br>
> > index 0000000..5591916<br>
> > --- /dev/null<br>
> > +++ b/src/qcam/viewfinder_gl.cpp<br>
> > @@ -0,0 +1,441 @@<br>
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */<br>
> > +/*<br>
> > + * Copyright (C) 2020, Linaro<br>
> > + *<br>
> > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader<br>
> > + */<br>
> > +<br>
> > +#include "viewfinder_gl.h"<br>
> > +<br>
> > +#include <QImage><br>
> > +<br>
> > +#include <libcamera/formats.h><br>
> > +<br>
> > +#define ATTRIB_VERTEX 0<br>
> > +#define ATTRIB_TEXTURE 1<br>
> <br>
> Is there a guarantee that attribute locations match the declaration<br>
> order in the shader program ? Wouldn't it be better to use<br>
> shaderProgram.attributeLocation() to retrieve the attribute locations by<br>
> name (before linking), or shaderProgram.bindAttributeLocation() to set<br>
> them explicitly (after linking) ?<br>
> <br>
> > +<br>
> > +static const QList<libcamera::PixelFormat> supportFormats{<br>
> <br>
> s/supportFormats/supportedFormats/<br>
> <br>
> > + libcamera::formats::NV12,<br>
> > + libcamera::formats::NV21,<br>
> > + libcamera::formats::NV16,<br>
> > + libcamera::formats::NV61,<br>
> > + libcamera::formats::NV24,<br>
> > + libcamera::formats::NV42,<br>
> > + libcamera::formats::YUV420,<br>
> > + libcamera::formats::YVU420<br>
> > +};<br>
> > +<br>
> > +ViewFinderGL::ViewFinderGL(QWidget *parent)<br>
> > + : QOpenGLWidget(parent),<br>
> > + buffer_(nullptr),<br>
> > + pFShader_(nullptr),<br>
> > + pVShader_(nullptr),<br>
> > + vbuf_(QOpenGLBuffer::VertexBuffer),<br>
> > + yuvDataPtr_(nullptr),<br>
> > + textureU_(QOpenGLTexture::Target2D),<br>
> > + textureV_(QOpenGLTexture::Target2D),<br>
> > + textureY_(QOpenGLTexture::Target2D)<br>
> <br>
> Feel free to have multiple members per line if desired.<br>
> <br>
> > +{<br>
> > +}<br>
> > +<br>
> > +ViewFinderGL::~ViewFinderGL()<br>
> > +{<br>
> > + removeShader();<br>
> > +<br>
> > + if (textureY_.isCreated())<br>
> > + textureY_.destroy();<br>
> > +<br>
> > + if (textureU_.isCreated())<br>
> > + textureU_.destroy();<br>
> > +<br>
> > + if (textureV_.isCreated())<br>
> > + textureV_.destroy();<br>
> <br>
> Are these needed, or does the QOpenGLTexture destructor destroy the<br>
> textures ?<br>
> <br>
> > +<br>
> > + vbuf_.destroy();<br>
> <br>
> Same question for vbuf_.<br>
> <br>
> > +}<br>
> > +<br>
> > +const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const<br>
> > +{<br>
> > + return (::supportFormats);<br>
> <br>
> No need for parentheses or an explicit namespace.<br>
> <br>
> return supportedFormats;<br>
> <br>
> > +}<br>
> > +<br>
> > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,<br>
> > + const QSize &size)<br>
> > +{<br>
> > + int ret = 0;<br>
> > +<br>
> > + if (isFormatSupport(format)) {<br>
> > + format_ = format;<br>
> > + size_ = size;<br>
> > + } else {<br>
> > + ret = -1;<br>
> > + }<br>
> > + updateGeometry();<br>
> <br>
> When the format change, don't you need to recreate the shaders ?<br>
> <br>
> > + return ret;<br>
> > +}<br>
> > +<br>
> > +void ViewFinderGL::stop()<br>
> > +{<br>
> > + if (buffer_) {<br>
> > + renderComplete(buffer_);<br>
> > + buffer_ = nullptr;<br>
> > + }<br>
> > +}<br>
> > +<br>
> > +QImage ViewFinderGL::getCurrentImage()<br>
> > +{<br>
> > + QMutexLocker locker(&mutex_);<br>
> > +<br>
> > + return (grabFramebuffer());<br>
> <br>
> return grabFrameBuffer();<br>
> <br>
> > +}<br>
> > +<br>
> > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)<br>
> > +{<br>
> > + if (buffer->planes().size() != 1) {<br>
> > + qWarning() << "Multi-planar buffers are not supported";<br>
> > + return;<br>
> > + }<br>
> > +<br>
> > + if (buffer_)<br>
> > + renderComplete(buffer_);<br>
> > +<br>
> > + unsigned char *memory = static_cast<unsigned char *>(map->memory);<br>
> > + if (memory) {<br>
> <br>
> Can memory be null ?<br>
> <br>
> > + yuvDataPtr_ = memory;<br>
> > + update();<br>
> > + buffer_ = buffer;<br>
> > + }<br>
> > +}<br>
> > +<br>
> > +bool ViewFinderGL::isFormatSupport(const libcamera::PixelFormat &format)<br>
> > +{<br>
> <br>
> As this function sets internal members based on the format, I would call<br>
> it selectFormat(), it does more than just checking if the format is<br>
> supported.<br>
> <br>
> > + bool ret = true;<br>
> > + switch (format) {<br>
> > + case libcamera::formats::NV12:<br>
> > + horzSubSample_ = 2;<br>
> > + vertSubSample_ = 2;<br>
> > + vsrc_ = ":NV_vertex_shader.glsl";<br>
> > + fsrc_ = ":NV_2_planes_UV_f.glsl";<br>
> > + break;<br>
> > + case libcamera::formats::NV21:<br>
> > + horzSubSample_ = 2;<br>
> > + vertSubSample_ = 2;<br>
> > + vsrc_ = ":NV_vertex_shader.glsl";<br>
> > + fsrc_ = ":NV_2_planes_VU_f.glsl";<br>
> > + break;<br>
> > + case libcamera::formats::NV16:<br>
> > + horzSubSample_ = 2;<br>
> > + vertSubSample_ = 1;<br>
> > + vsrc_ = ":NV_vertex_shader.glsl";<br>
> > + fsrc_ = ":NV_2_planes_UV_f.glsl";<br>
> > + break;<br>
> > + case libcamera::formats::NV61:<br>
> > + horzSubSample_ = 2;<br>
> > + vertSubSample_ = 1;<br>
> > + vsrc_ = ":NV_vertex_shader.glsl";<br>
> > + fsrc_ = ":NV_2_planes_VU_f.glsl";<br>
> > + break;<br>
> > + case libcamera::formats::NV24:<br>
> > + horzSubSample_ = 1;<br>
> > + vertSubSample_ = 1;<br>
> > + vsrc_ = ":NV_vertex_shader.glsl";<br>
> > + fsrc_ = ":NV_2_planes_UV_f.glsl";<br>
> > + break;<br>
> > + case libcamera::formats::NV42:<br>
> > + horzSubSample_ = 1;<br>
> > + vertSubSample_ = 1;<br>
> > + vsrc_ = ":NV_vertex_shader.glsl";<br>
> > + fsrc_ = ":NV_2_planes_VU_f.glsl";<br>
> > + break;<br>
> > + case libcamera::formats::YUV420:<br>
> > + horzSubSample_ = 2;<br>
> > + vertSubSample_ = 2;<br>
> > + vsrc_ = ":NV_vertex_shader.glsl";<br>
> > + fsrc_ = ":NV_3_planes_UV_f.glsl";<br>
> > + break;<br>
> > + case libcamera::formats::YVU420:<br>
> > + horzSubSample_ = 2;<br>
> > + vertSubSample_ = 2;<br>
> > + vsrc_ = ":NV_vertex_shader.glsl";<br>
> > + fsrc_ = ":NV_3_planes_VU_f.glsl";<br>
> > + break;<br>
> > + default:<br>
> > + ret = false;<br>
> > + qWarning() << "[ViewFinderGL]:"<br>
> > + << "format not support yet.";<br>
> <br>
> s/support yet./supported/<br>
> <br>
> > + break;<br>
> > + };<br>
> > +<br>
> > + return ret;<br>
> > +}<br>
> > +<br>
> > +void ViewFinderGL::createVertexShader()<br>
> > +{<br>
> > + bool bCompile;<br>
> <br>
> No need to prefix variables with the type name.<br>
> <br>
> > + /* Create Vertex Shader */<br>
> > + pVShader_ = new QOpenGLShader(QOpenGLShader::Vertex, this);<br>
> > +<br>
> > + bCompile = pVShader_->compileSourceFile(vsrc_);<br>
> > + if (!bCompile) {<br>
> > + qWarning() << "[ViewFinderGL]:" << pVShader_->log();<br>
> > + }<br>
> <br>
> This can simply be written<br>
> <br>
> if (!pVShader_->compileSourceFile(vsrc_))<br>
> qWarning() << "[ViewFinderGL]:" << pVShader_->log();<br>
> <br>
> > +<br>
> > + shaderProgram_.addShader(pVShader_);<br>
> <br>
> Won't this crash if shader compilation failed ? I think<br>
> createVertexShader() should return a status as a bool.<br>
> <br>
> > +}<br>
> > +<br>
> > +bool ViewFinderGL::createFragmentShader()<br>
> > +{<br>
> > + bool bCompile;<br>
> > +<br>
> > + /* Create Fragment Shader */<br>
> > + pFShader_ = new QOpenGLShader(QOpenGLShader::Fragment, this);<br>
> > +<br>
> > + bCompile = pFShader_->compileSourceFile(fsrc_);<br>
> > + if (!bCompile) {<br>
> <br>
> if (!pFShader_->compileSourceFile(fsrc_)) {<br>
> <br>
> > + qWarning() << "[ViewFinderGL]:" << pFShader_->log();<br>
> > + return bCompile;<br>
> <br>
> return false;<br>
> <br>
> > + }<br>
> > +<br>
> > + shaderProgram_.addShader(pFShader_);<br>
> > +<br>
> > + /* Link shader pipeline */<br>
> > + if (!shaderProgram_.link()) {<br>
> > + qWarning() << "[ViewFinderGL]:" << shaderProgram_.log();<br>
> > + close();<br>
> > + }<br>
> > +<br>
> > + /* Bind shader pipeline for use */<br>
> > + if (!shaderProgram_.bind()) {<br>
> > + qWarning() << "[ViewFinderGL]:" << shaderProgram_.log();<br>
> > + close();<br>
> > + }<br>
> > +<br>
> > + shaderProgram_.enableAttributeArray(ATTRIB_VERTEX);<br>
> > + shaderProgram_.enableAttributeArray(ATTRIB_TEXTURE);<br>
> > +<br>
> > + shaderProgram_.setAttributeBuffer(ATTRIB_VERTEX,<br>
> > + GL_FLOAT,<br>
> > + 0,<br>
> > + 2,<br>
> > + 2 * sizeof(GLfloat));<br>
> > + shaderProgram_.setAttributeBuffer(ATTRIB_TEXTURE,<br>
> > + GL_FLOAT,<br>
> > + 8 * sizeof(GLfloat),<br>
> > + 2,<br>
> > + 2 * sizeof(GLfloat));<br>
> > +<br>
> > + textureUniformY_ = shaderProgram_.uniformLocation("tex_y");<br>
> > + textureUniformU_ = shaderProgram_.uniformLocation("tex_u");<br>
> > + textureUniformV_ = shaderProgram_.uniformLocation("tex_v");<br>
> > +<br>
> > + if (!textureY_.isCreated())<br>
> > + textureY_.create();<br>
> > +<br>
> > + if (!textureU_.isCreated())<br>
> > + textureU_.create();<br>
> > +<br>
> > + if (!textureV_.isCreated())<br>
> > + textureV_.create();<br>
> > +<br>
> > + id_y_ = textureY_.textureId();<br>
> > + id_u_ = textureU_.textureId();<br>
> > + id_v_ = textureV_.textureId();<br>
> > + return true;<br>
> > +}<br>
> > +<br>
> > +void ViewFinderGL::configureTexture(unsigned int id)<br>
> > +{<br>
> > + glBindTexture(GL_TEXTURE_2D, id);<br>
> > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);<br>
> > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);<br>
> > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);<br>
> > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);<br>
> > +}<br>
> > +<br>
> > +void ViewFinderGL::removeShader()<br>
> > +{<br>
> > + if (shaderProgram_.isLinked()) {<br>
> > + shaderProgram_.release();<br>
> > + shaderProgram_.removeAllShaders();<br>
> > + }<br>
> > +<br>
> > + if (pFShader_)<br>
> > + delete pFShader_;<br>
> > +<br>
> > + if (pVShader_)<br>
> > + delete pVShader_;<br>
> > +}<br>
> > +<br>
> > +void ViewFinderGL::initializeGL()<br>
> > +{<br>
> > + initializeOpenGLFunctions();<br>
> > + glEnable(GL_TEXTURE_2D);<br>
> > + glDisable(GL_DEPTH_TEST);<br>
> > +<br>
> > + static const GLfloat vertices[]{<br>
> > + -1.0f, -1.0f, -1.0f, +1.0f,<br>
> > + +1.0f, +1.0f, +1.0f, -1.0f,<br>
> > + 0.0f, 1.0f, 0.0f, 0.0f,<br>
> > + 1.0f, 0.0f, 1.0f, 1.0f<br>
> > + };<br>
> <br>
> This is vertex and texture coordinates, not just vertices. How about<br>
> writing it as follows ?<br>
> <br>
> static const GLfloat coordinates[2][4][2] {<br>
> {<br>
> /* Vertex coordinates */<br>
> { -1.0f, -1.0f },<br>
> { -1.0f, +1.0f },<br>
> { +1.0f, +1.0f },<br>
> { +1.0f, -1.0f },<br>
> }, {<br>
> /* Texture coordinates */<br>
> { 0.0f, 1.0f },<br>
> { 0.0f, 0.0f },<br>
> { 1.0f, 0.0f },<br>
> { 1.0f, 1.0f },<br>
> },<br>
> };<br>
> <br>
> There's something I don't get though, maybe you can help me understand<br>
> it. The vertex coordinates are copied directly to gl_Position in the<br>
> vertex shader, so they're essentially expressed in clip space, which I<br>
> understand has X pointing towards the right and Y pointing towards the<br>
> top. The texture coordinates, if my understand is correct again, have<br>
> their origin at the bottom-left corner too. The first vertex, (-1.0,<br>
> -1.0), which is at the bottom-left, then maps to texture coordinate<br>
> (0.0, 1.0), which is the top-left pixel of the texture. The image should<br>
> thus be flipped vertically. Why isn't it ? I'm sure I'm missing<br>
> somethign simple.<br>
<br>
I figured it out. The texture created with glTexImage2D() has (0,0) at<br>
byte 0. As the camera captures the image with the top line at the<br>
beginning of the buffer, the texture is stored with the top line on row<br>
0. texture coordinate (0.0, 1.0) is thus the bottom-left corner of the<br>
texture, not the top-left corner.<br></blockquote><div><br></div><div>You definitely are an OpenGL expert...:-)</div><div>The original coordinate mapping is for my camera usage on rockpi4b.</div><div>And in my case the camera module is vertical flipped.</div><div>So...anyway I am now change the coordinate mapping as default below</div><div><br></div><div>+ static const GLfloat coordinates[2][4][2] {<br>+ {<br>+ /* Vertex coordinates */<br>+ { -1.0f, -1.0f },<br>+ { -1.0f, +1.0f },<br>+ { +1.0f, +1.0f },<br>+ { +1.0f, -1.0f },<br>+ }, {<br>+ /* Texture coordinates */<br>+ { 1.0f, 0.0f },<br>+ { 1.0f, 1.0f },<br>+ { 0.0f, 1.0f },<br>+ { 0.0f, 0.0f },<br>+ },<br></div><div>Please let me know if you have any concern.</div><div><br></div><div>Thanks,</div><div>Show</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > +<br>
> > + vbuf_.create();<br>
> > + vbuf_.bind();<br>
> > + vbuf_.allocate(vertices, sizeof(vertices));<br>
> > +<br>
> > + /* Create Vertex Shader */<br>
> > + createVertexShader();<br>
> > +<br>
> > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f);<br>
> > +}<br>
> > +<br>
> > +void ViewFinderGL::doRender()<br>
> > +{<br>
> > + switch (format_) {<br>
> > + case libcamera::formats::NV12:<br>
> > + case libcamera::formats::NV21:<br>
> > + case libcamera::formats::NV16:<br>
> > + case libcamera::formats::NV61:<br>
> > + case libcamera::formats::NV24:<br>
> > + case libcamera::formats::NV42:<br>
> > + /* activate texture 0 */<br>
> > + glActiveTexture(GL_TEXTURE0);<br>
> > + configureTexture(id_y_);<br>
> > + glTexImage2D(GL_TEXTURE_2D,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + size_.width(),<br>
> > + size_.height(),<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + GL_UNSIGNED_BYTE,<br>
> > + yuvDataPtr_);<br>
> > + glUniform1i(textureUniformY_, 0);<br>
> <br>
> Would it make sense to use<br>
> <br>
> shaderProgram_.setUniformValue(textureUniformY_, 0);<br>
> <br>
> (and similarly below) ?<br>
> <br>
> > +<br>
> > + /* activate texture 1 */<br>
> > + glActiveTexture(GL_TEXTURE1);<br>
> > + configureTexture(id_u_);<br>
> > + glTexImage2D(GL_TEXTURE_2D,<br>
> > + 0,<br>
> > + GL_RG,<br>
> > + size_.width() / horzSubSample_,<br>
> > + size_.height() / vertSubSample_,<br>
> > + 0,<br>
> > + GL_RG,<br>
> > + GL_UNSIGNED_BYTE,<br>
> > + (char *)yuvDataPtr_ + size_.width() * size_.height());<br>
> > + glUniform1i(textureUniformU_, 1);<br>
> > + break;<br>
> <br>
> A blank line here would increase readability. Same below.<br>
> <br>
> > + case libcamera::formats::YUV420:<br>
> > + /* activate texture 0 */<br>
> > + glActiveTexture(GL_TEXTURE0);<br>
> > + configureTexture(id_y_);<br>
> > + glTexImage2D(GL_TEXTURE_2D,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + size_.width(),<br>
> > + size_.height(),<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + GL_UNSIGNED_BYTE,<br>
> > + yuvDataPtr_);<br>
> > + glUniform1i(textureUniformY_, 0);<br>
> > +<br>
> > + /* activate texture 1 */<br>
> > + glActiveTexture(GL_TEXTURE1);<br>
> > + configureTexture(id_u_);<br>
> > + glTexImage2D(GL_TEXTURE_2D,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + size_.width() / horzSubSample_,<br>
> > + size_.height() / vertSubSample_,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + GL_UNSIGNED_BYTE,<br>
> > + (char *)yuvDataPtr_ + size_.width() * size_.height());<br>
> > + glUniform1i(textureUniformU_, 1);<br>
> > +<br>
> > + /* activate texture 2 */<br>
> > + glActiveTexture(GL_TEXTURE2);<br>
> > + configureTexture(id_v_);<br>
> > + glTexImage2D(GL_TEXTURE_2D,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + size_.width() / horzSubSample_,<br>
> > + size_.height() / vertSubSample_,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + GL_UNSIGNED_BYTE,<br>
> > + (char *)yuvDataPtr_ + size_.width() * size_.height() * 5 / 4);<br>
> > + glUniform1i(textureUniformV_, 2);<br>
> > + break;<br>
> > + case libcamera::formats::YVU420:<br>
> > + /* activate texture 0 */<br>
> > + glActiveTexture(GL_TEXTURE0);<br>
> > + configureTexture(id_y_);<br>
> > + glTexImage2D(GL_TEXTURE_2D,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + size_.width(),<br>
> > + size_.height(),<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + GL_UNSIGNED_BYTE,<br>
> > + yuvDataPtr_);<br>
> > + glUniform1i(textureUniformY_, 0);<br>
> > +<br>
> > + /* activate texture 1 */<br>
> > + glActiveTexture(GL_TEXTURE2);<br>
> > + configureTexture(id_v_);<br>
> > + glTexImage2D(GL_TEXTURE_2D,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + size_.width() / horzSubSample_,<br>
> > + size_.height() / vertSubSample_,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + GL_UNSIGNED_BYTE,<br>
> > + (char *)yuvDataPtr_ + size_.width() * size_.height());<br>
> > + glUniform1i(textureUniformV_, 1);<br>
> <br>
> OK, now I understand why the NV_3_planes_UV_f.glsl and<br>
> NV_3_planes_VU_f.glsl shaders are identical, you switch the U and V<br>
> planes here. I think we should then merge the two files into<br>
> NV_3_planes_f_glsl. The above line should become<br>
> <br>
> glUniform1i(textureUniformU_, 2);<br>
> <br>
> as you deal with texture 2 here (and a similar change is needed below),<br>
> and the two blocks should be swapped as the comments are incorrect (the<br>
> comment above refers to texture 1 while the code deals with texture 2).<br>
> <br>
> > +<br>
> > + /* activate texture 2 */<br>
> > + glActiveTexture(GL_TEXTURE1);<br>
> > + configureTexture(id_u_);<br>
> > + glTexImage2D(GL_TEXTURE_2D,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + size_.width() / horzSubSample_,<br>
> > + size_.height() / vertSubSample_,<br>
> > + 0,<br>
> > + GL_RED,<br>
> > + GL_UNSIGNED_BYTE,<br>
> > + (char *)yuvDataPtr_ + size_.width() * size_.height() * 5 / 4);<br>
> > + glUniform1i(textureUniformU_, 2);<br>
> <br>
> Please add a break here, let's not rely on implicit fall-through.<br>
> <br>
> > + default:<br>
> > + break;<br>
> > + };<br>
> > +}<br>
> > +<br>
> > +void ViewFinderGL::paintGL()<br>
> > +{<br>
> > + if (pFShader_ == nullptr)<br>
> <br>
> if (!pfShader_)<br>
> <br>
> > + createFragmentShader();<br>
> > +<br>
> > + if (yuvDataPtr_) {<br>
> > + glClearColor(0.0, 0.0, 0.0, 1.0);<br>
> > + glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);<br>
> > +<br>
> > + doRender();<br>
> > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4);<br>
> > + }<br>
> > +}<br>
> > +<br>
> > +void ViewFinderGL::resizeGL(int w, int h)<br>
> > +{<br>
> > + glViewport(0, 0, w, h);<br>
> > +}<br>
> > +<br>
> > +QSize ViewFinderGL::sizeHint() const<br>
> > +{<br>
> > + return size_.isValid() ? size_ : QSize(640, 480);<br>
> > +}<br>
> > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h<br>
> > new file mode 100644<br>
> > index 0000000..e708c32<br>
> > --- /dev/null<br>
> > +++ b/src/qcam/viewfinder_gl.h<br>
> > @@ -0,0 +1,97 @@<br>
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */<br>
> > +/*<br>
> > + * Copyright (C) 2020, Linaro<br>
> > + *<br>
> > + * viewfinder_GL.h - OpenGL Viewfinder for rendering by OpenGL shader<br>
> > + *<br>
> > + */<br>
> > +#ifndef __VIEWFINDER_GL_H__<br>
> > +#define __VIEWFINDER_GL_H__<br>
> > +<br>
> > +#include <QImage><br>
> > +#include <QMutex><br>
> > +#include <QObject><br>
> <br>
> This header shouldn't be needed.<br>
> <br>
> > +#include <QOpenGLBuffer><br>
> > +#include <QOpenGLFunctions><br>
> > +#include <QOpenGLShader><br>
> > +#include <QOpenGLShaderProgram><br>
> > +#include <QOpenGLTexture><br>
> > +#include <QOpenGLWidget><br>
> > +#include <QSize><br>
> > +<br>
> > +#include <libcamera/buffer.h><br>
> > +#include <libcamera/formats.h><br>
> <br>
> Missing blank line.<br>
> <br>
> > +#include "viewfinder.h"<br>
> > +<br>
> > +class ViewFinderGL : public QOpenGLWidget,<br>
> > + public ViewFinder,<br>
> > + protected QOpenGLFunctions<br>
> > +{<br>
> > + Q_OBJECT<br>
> > +<br>
> > +public:<br>
> > + ViewFinderGL(QWidget *parent = 0);<br>
> <br>
> = nullptr<br>
> <br>
> > + ~ViewFinderGL();<br>
> > +<br>
> > + const QList<libcamera::PixelFormat> &nativeFormats() const override;<br>
> > +<br>
> > + int setFormat(const libcamera::PixelFormat &format, const QSize &size) override;<br>
> > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;<br>
> > + void stop() override;<br>
> > +<br>
> > + QImage getCurrentImage() override;<br>
> > +<br>
> > +Q_SIGNALS:<br>
> > + void renderComplete(libcamera::FrameBuffer *buffer);<br>
> > +<br>
> > +protected:<br>
> > + void initializeGL() override;<br>
> > + void paintGL() override;<br>
> > + void resizeGL(int w, int h) override;<br>
> > + QSize sizeHint() const override;<br>
> > +<br>
> > +private:<br>
> > + bool isFormatSupport(const libcamera::PixelFormat &format);<br>
> <br>
> s/isFormatSupport/isFormatSupported/<br>
> <br>
> > +<br>
> > + void configureTexture(unsigned int id);<br>
> > + bool createFragmentShader();<br>
> > + void createVertexShader();<br>
> > + void removeShader();<br>
> > + void doRender();<br>
> > +<br>
> > + /* Captured image size, format and buffer */<br>
> > + libcamera::FrameBuffer *buffer_;<br>
> > + libcamera::PixelFormat format_;<br>
> > + QSize size_;<br>
> > +<br>
> > + /* OpenGL components for render */<br>
> <br>
> s/render/rendering/<br>
> <br>
> > + QOpenGLShader *pFShader_;<br>
> <br>
> No need to prefix pointers with 'p'. I'd name this fragmentShader_.<br>
> <br>
> > + QOpenGLShader *pVShader_;<br>
> <br>
> Same here, vertexShader_.<br>
> <br>
> > + QOpenGLShaderProgram shaderProgram_;<br>
> <br>
> Is there a specific reason why pFShader_ and pVShader_ are pointers,<br>
> while shaderProgram_ is embedded directly in ViewFinderGL ?<br>
> <br>
> > +<br>
> > + /* Vertex buffer */<br>
> > + QOpenGLBuffer vbuf_;<br>
> > +<br>
> > + /* Fragment and Vertex shader file name */<br>
> > + QString fsrc_;<br>
> <br>
> fragmentShaderSrc_ ?<br>
> <br>
> > + QString vsrc_;<br>
> <br>
> And vertexShaderSrc_.<br>
> <br>
> > +<br>
> > + unsigned char *yuvDataPtr_;<br>
> <br>
> And no need for a Ptr suffix either :-)<br>
> <br>
> > +<br>
> > + /* YUV texture planars and parameters */<br>
> > + GLuint id_u_;<br>
> > + GLuint id_v_;<br>
> > + GLuint id_y_;<br>
> > + GLuint textureUniformU_;<br>
> > + GLuint textureUniformV_;<br>
> > + GLuint textureUniformY_;<br>
> > + QOpenGLTexture textureU_;<br>
> > + QOpenGLTexture textureV_;<br>
> > + QOpenGLTexture textureY_;<br>
> > + unsigned int horzSubSample_;<br>
> > + unsigned int vertSubSample_;<br>
> > +<br>
> > + QImage image_;<br>
> <br>
> This is never used.<br>
> <br>
> > + QMutex mutex_; /* Prevent concurrent access to image_ */<br>
> > +};<br>
> > +#endif /* __VIEWFINDER_GL_H__ */<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>