Message ID | 20200624073705.14737-2-show.liu@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Show, Thank you, I'm quite excited to see this update, and I know Niklas is keen for this to get in too, as he uses a Rockchip target mostly so this will improve the performances for him quite a lot. Most of my comments below are stylistic, (but theres a couple of other topics too), so as suggested below, could you install the checkstyle.py hook as a git commit hook please? I personally prefer having it as a 'post-commit' hook, rather than a pre-commit so that it's easier to 'ignore' suggestions that I don't care for ;-) -- Kieran On 24/06/2020 08:37, Show Liu wrote: > The patch is to render the NV family YUV formats by OpenGL shader. > V3: refine the fragment shader for better pixel color. > > Signed-off-by: Show Liu <show.liu@linaro.org> > --- > src/qcam/main.cpp | 2 + > src/qcam/main_window.cpp | 19 ++- > src/qcam/main_window.h | 3 +- > src/qcam/meson.build | 2 + > src/qcam/shader.h | 104 ++++++++++++ > src/qcam/viewfinder.cpp | 18 +- > src/qcam/viewfinder.h | 23 ++- > src/qcam/viewfinderGL.cpp | 335 ++++++++++++++++++++++++++++++++++++++ > src/qcam/viewfinderGL.h | 101 ++++++++++++ > 9 files changed, 593 insertions(+), 14 deletions(-) > create mode 100644 src/qcam/shader.h > create mode 100644 src/qcam/viewfinderGL.cpp > create mode 100644 src/qcam/viewfinderGL.h > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > index b3468cb..a3ea471 100644 > --- a/src/qcam/main.cpp > +++ b/src/qcam/main.cpp > @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc, char *argv[]) > "help"); > parser.addOption(OptStream, &streamKeyValue, > "Set configuration of a camera stream", "stream", true); > + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats frame via OpenGL shader", > + "opengl"); (Just a question to everyone) - Should we default to the OpenGL viewfinder if open-gl is available, and fall back to the QWidget version otherwise? (We can always do such actions on top/later if we choose.) > > OptionsParser::Options options = parser.parse(argc, argv); > if (options.isSet(OptHelp)) > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 7bc1360..6edf370 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -28,6 +28,9 @@ > #include <libcamera/version.h> > > #include "dng_writer.h" > +#include "main_window.h" > +#include "viewfinder.h" > +#include "viewfinderGL.h" > > using namespace libcamera; > > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) > setWindowTitle(title_); > connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle())); > > - viewfinder_ = new ViewFinder(this); > - connect(viewfinder_, &ViewFinder::renderComplete, > - this, &MainWindow::queueRequest); > - setCentralWidget(viewfinder_); > + if (options_.isSet(OptOpenGL)) { > + viewfinder_ = new ViewFinderGL(this); > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_), &ViewFinderGL::renderComplete, > + this, &MainWindow::queueRequest); checkstyle.py highlights that the indentation of the second line should be pulled back, and I agree: connect(dynamic_cast<ViewFinderGL *>(viewfinder_), &ViewFinderGL::renderComplete, - this, &MainWindow::queueRequest); + this, &MainWindow::queueRequest); > + setCentralWidget(dynamic_cast<ViewFinderGL *>(viewfinder_)); Does the setCentralWidget need to have the dynamic_cast? Or can it just be a call to setCentralWidget(viewfinder_); after this conditional block? Perhaps if the base viewfinder class had a method to call which handled the connect, that could remove casting requirements there - but it would probably end up needing to deal with complex template things for the signal/slot parsing ... so I suspect that part isn't worth it... > + } else { > + viewfinder_ = new ViewFinder(this); > + connect(dynamic_cast<ViewFinder *>(viewfinder_), &ViewFinder::renderComplete, > + this, &MainWindow::queueRequest); Same indentation fault here. If you add the checkstyle.py hook to your libcamera sources, you'll get these notifications as you commit (you can then decide if checkstyle was right or not ;D): $ cp utils/hooks/post-commit .git/hooks/post-commit > + setCentralWidget(dynamic_cast<ViewFinder *>(viewfinder_)); > + } > + > adjustSize(); > > /* Hotplug/unplug support */ > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > index 4606fe4..a852ef4 100644 > --- a/src/qcam/main_window.h > +++ b/src/qcam/main_window.h > @@ -38,6 +38,7 @@ enum { > OptCamera = 'c', > OptHelp = 'h', > OptStream = 's', > + OptOpenGL = 'o', > }; > > class CaptureRequest > @@ -102,7 +103,7 @@ private: > QAction *startStopAction_; > QComboBox *cameraCombo_; > QAction *saveRaw_; > - ViewFinder *viewfinder_; > + ViewFinderHandler *viewfinder_; Handler? I guess I'll see below, but I think I'd expect the base class interface to be 'ViewFinder', and then make specific derived implementations of that? > > QIcon iconPlay_; > QIcon iconStop_; > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > index 045db52..6a58947 100644 > --- a/src/qcam/meson.build > +++ b/src/qcam/meson.build > @@ -7,11 +7,13 @@ qcam_sources = files([ > 'main.cpp', > 'main_window.cpp', > 'viewfinder.cpp', > + 'viewfinderGL.cpp' > ]) > > qcam_moc_headers = files([ > 'main_window.h', > 'viewfinder.h', > + 'viewfinderGL.h' > ]) > > qcam_resources = files([ > diff --git a/src/qcam/shader.h b/src/qcam/shader.h > new file mode 100644 > index 0000000..f54c264 > --- /dev/null > +++ b/src/qcam/shader.h > @@ -0,0 +1,104 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2020, Linaro > + * > + * shader.h - YUV format converter shader source code > + */ > +#ifndef __SHADER_H__ > +#define __SHADER_H__ > + > +/* Vertex shader for NV formats */ > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" > + "attribute vec2 textureIn;\n" > + "varying vec2 textureOut;\n" > + "void main(void)\n" > + "{\n" > + "gl_Position = vertexIn;\n" > + "textureOut = textureIn;\n" > + "}\n"; I'd pull that indentation back so that they all match, including moving the first line to a new line to match... char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" "attribute vec2 textureIn;\n" "varying vec2 textureOut;\n" "void main(void)\n" "{\n" "gl_Position = vertexIn;\n" "textureOut = textureIn;\n" "}\n"; > + > +/* Fragment shader for NV12, NV16 and NV24 */ > +char NV_2_planes_UV[] = "#ifdef GL_ES\n" And ofcourse be consistent with the code block indentation style throughout. checkstyle might complain whatever you do here, so I would take checkstyle with a pinch of salt and just make sure the file is consitent and well laid out. > + "precision highp float;\n" > + "#endif\n" > + "varying vec2 textureOut;\n" > + "uniform sampler2D tex_y;\n" > + "uniform sampler2D tex_u;\n" > + "void main(void)\n" > + "{\n" > + "vec3 yuv;\n" > + "vec3 rgb;\n" > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > + " vec3(0.0, -0.390625, 2.015625),\n" > + " vec3(1.5975625, -0.8125, 0.0));\n" > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n" > + "rgb = convert_mat * yuv;\n" > + "gl_FragColor = vec4(rgb, 1.0);\n" > + "}\n"; > + > +/* Fragment shader for NV21, NV61 and NV42 */ > +char NV_2_planes_VU[] = "#ifdef GL_ES\n" > + "precision highp float;\n" > + "#endif\n" > + "varying vec2 textureOut;\n" > + "uniform sampler2D tex_y;\n" > + "uniform sampler2D tex_u;\n" > + "void main(void)\n" > + "{\n" > + "vec3 yuv;\n" > + "vec3 rgb;\n" > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > + " vec3(0.0, -0.390625, 2.015625),\n" > + " vec3(1.5975625, -0.8125, 0.0));\n" > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n" > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > + "rgb = convert_mat * yuv;\n" > + "gl_FragColor = vec4(rgb, 1.0);\n" > + "}\n"; > + > +/* Fragment shader for YUV420 */ > +char NV_3_planes_UV[] = "#ifdef GL_ES\n" > + "precision highp float;\n" > + "#endif\n" > + "varying vec2 textureOut;\n" > + "uniform sampler2D tex_y;\n" > + "uniform sampler2D tex_u;\n" > + "uniform sampler2D tex_v;\n" > + "void main(void)\n" > + "{\n" > + "vec3 yuv;\n" > + "vec3 rgb;\n" > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > + " vec3(0.0, -0.390625, 2.015625),\n" > + " vec3(1.5975625, -0.8125, 0.0));\n" > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n" > + "rgb = convert_mat * yuv;\n" > + "gl_FragColor = vec4(rgb, 1);\n" > + "}\n"; > + > +/* Fragment shader for YVU420 */ > +char NV_3_planes_VU[] = "precision highp float;\n" > + "#endif\n" > + "varying vec2 textureOut;\n" > + "uniform sampler2D tex_y;\n" > + "uniform sampler2D tex_u;\n" > + "uniform sampler2D tex_v;\n" > + "void main(void)\n" > + "{\n" > + "vec3 yuv;\n" > + "vec3 rgb;\n" > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > + " vec3(0.0, -0.390625, 2.015625),\n" > + " vec3(1.5975625, -0.8125, 0.0));\n" > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n" > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > + "rgb = convert_mat * yuv;\n" > + "gl_FragColor = vec4(rgb, 1);\n" > + "}\n"; > +#endif // __SHADER_H__ I can't comment on the shaders without a lot of research into shaders... so I'm going to say "Yay, if this black magic works, then it works for me" ;-D > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp > index dcf8a15..d3a2422 100644 > --- a/src/qcam/viewfinder.cpp > +++ b/src/qcam/viewfinder.cpp > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats > { libcamera::formats::BGR888, QImage::Format_RGB888 }, > }; > > -ViewFinder::ViewFinder(QWidget *parent) > - : QWidget(parent), buffer_(nullptr) > +ViewFinderHandler::ViewFinderHandler() > { > - icon_ = QIcon(":camera-off.svg"); > } > > -ViewFinder::~ViewFinder() > +ViewFinderHandler::~ViewFinderHandler() > { > } > > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() const > +const QList<libcamera::PixelFormat> &ViewFinderHandler::nativeFormats() const > { > static const QList<libcamera::PixelFormat> formats = ::nativeFormats.keys(); > return formats; > } > > +ViewFinder::ViewFinder(QWidget *parent) > + : QWidget(parent), buffer_(nullptr) > +{ > + icon_ = QIcon(":camera-off.svg"); > +} > + > +ViewFinder::~ViewFinder() > +{ > +} > + > int ViewFinder::setFormat(const libcamera::PixelFormat &format, > const QSize &size) > { > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > index 26a1320..741d694 100644 > --- a/src/qcam/viewfinder.h > +++ b/src/qcam/viewfinder.h > @@ -13,6 +13,8 @@ > #include <QList> > #include <QImage> > #include <QMutex> > +#include <QOpenGLWidget> > +#include <QOpenGLFunctions> Shouldn't those includes be in src/qcam/viewfinderGL.cpp or src/qcam/viewfinderGL.h only? Also - they should be alphabetical order. (Checkstyle will highlight that for you). > #include <QSize> > #include <QWidget> > > @@ -28,7 +30,23 @@ struct MappedBuffer { > size_t size; > }; > > -class ViewFinder : public QWidget > +class ViewFinderHandler I think it's just naming, but I would have called this ViewFinder,... > +{ > +public: > + ViewFinderHandler(); > + virtual ~ViewFinderHandler(); > + > + const QList<libcamera::PixelFormat> &nativeFormats() const; > + > + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) =0; > + virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) =0; > + virtual void stop() =0; > + > + virtual QImage getCurrentImage() =0; on each of those, a space after the = looks better: functionName() = 0; > + > +}; > + > +class ViewFinder : public QWidget, public ViewFinderHandler And this, ViewFinderQT or ViewFinderQWidget? (probably the later..) To keep consistent with coding style, I think the viewfinder.h would then only contain the base interface, and create a new file+header for the QWidget version. Or maybe that's over kill, as the QWidget version would be the 'default' ViewFinder anyway ... > { > Q_OBJECT > > @@ -36,8 +54,6 @@ public: > ViewFinder(QWidget *parent); > ~ViewFinder(); > > - const QList<libcamera::PixelFormat> &nativeFormats() const; > - > int setFormat(const libcamera::PixelFormat &format, const QSize &size); > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > void stop(); > @@ -67,5 +83,4 @@ private: > QImage image_; > QMutex mutex_; /* Prevent concurrent access to image_ */ > }; > - > #endif /* __QCAM_VIEWFINDER__ */ > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp > new file mode 100644 > index 0000000..7b47beb > --- /dev/null > +++ b/src/qcam/viewfinderGL.cpp > @@ -0,0 +1,335 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2020, Linaro > + * > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader > + */ > + > +#include "shader.h" > +#include "viewfinderGL.h" > + > +#include <QImage> QImage is provided in viewfinder.h ... And do you actually use it in here? > + > +#include <libcamera/formats.h> > + > +#define ATTRIB_VERTEX 0 > +#define ATTRIB_TEXTURE 1 > + > +ViewFinderGL::ViewFinderGL(QWidget *parent) > + : QOpenGLWidget(parent), > + glBuffer(QOpenGLBuffer::VertexBuffer), > + pFShader(nullptr), > + pVShader(nullptr), > + textureU(QOpenGLTexture::Target2D), > + textureV(QOpenGLTexture::Target2D), > + textureY(QOpenGLTexture::Target2D), > + yuvDataPtr(nullptr) Checkstyle will tell you to align those vertically with the first QOpenGLWidget(parent), rather than the ':'. > + > +{ > +} > + > +ViewFinderGL::~ViewFinderGL() > +{ > + removeShader(); > + > + if(textureY.isCreated()) Space after if: "if ()" > + textureY.destroy(); > + > + if(textureU.isCreated()) > + textureU.destroy(); > + > + if(textureV.isCreated()) > + textureV.destroy(); For each of those ;-) There's plenty more too, but I'll let you find them with checkstyle. > + > + glBuffer.destroy(); > +} > + > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > + const QSize &size) > +{ > + format_ = format; > + size_ = size; > + > + updateGeometry(); > + return 0; > +} > + > +void ViewFinderGL::stop() > +{ > + if (buffer_) { > + renderComplete(buffer_); > + buffer_ = nullptr; > + } > +} > + > +QImage ViewFinderGL::getCurrentImage() > +{ > + QMutexLocker locker(&mutex_); > + > + return(grabFramebuffer()); I think that could be : return grabFrameBuffer(); > +} > + > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map) > +{ > + if (buffer->planes().size() != 1) { > + qWarning() << "Multi-planar buffers are not supported"; > + return; > + } > + > + unsigned char *memory = static_cast<unsigned char *>(map->memory); > + if(memory) { > + yuvDataPtr = memory; > + update(); > + buffer_ = buffer; > + } > + > + if (buffer) > + renderComplete(buffer); > +} > + > +void ViewFinderGL::updateFrame(unsigned char *buffer) > +{ > + if(buffer) { > + yuvDataPtr = buffer; > + update(); > + } > +} > + > +void ViewFinderGL::setFrameSize(int width, int height) Can width/height be negative? Maybe the should be unsigned int? What about using a QSize too rather than individually passing width/height? I can't see what calls setFrameSize(), is this an override from > +{ > + if(width > 0 && height > 0) { > + width_ = width; > + height_ = height; > + } In fact, don't we already have a size_ in the class? (which could also be in the base class too most likely... > +} > + > +char *ViewFinderGL::selectFragmentShader(unsigned int format) > +{ > + char *fsrc = nullptr; > + > + switch(format) { > + case libcamera::formats::NV12: > + horzSubSample_ = 2; > + vertSubSample_ = 2; > + fsrc = NV_2_planes_UV; > + break; > + case libcamera::formats::NV21: > + horzSubSample_ = 2; > + vertSubSample_ = 2; > + fsrc = NV_2_planes_VU; > + break; > + case libcamera::formats::NV16: > + horzSubSample_ = 2; > + vertSubSample_ = 1; > + fsrc = NV_2_planes_UV; > + break; > + case libcamera::formats::NV61: > + horzSubSample_ = 2; > + vertSubSample_ = 1; > + fsrc = NV_2_planes_VU; > + break; > + case libcamera::formats::NV24: > + horzSubSample_ = 1; > + vertSubSample_ = 1; > + fsrc = NV_2_planes_UV; > + break; > + case libcamera::formats::NV42: > + horzSubSample_ = 1; > + vertSubSample_ = 1; > + fsrc = NV_2_planes_VU; > + break; > + case libcamera::formats::YUV420: > + horzSubSample_ = 2; > + vertSubSample_ = 2; > + fsrc = NV_3_planes_UV; > + break; > + default: > + break; > + }; > + > + if(fsrc == nullptr) { > + qDebug() << __func__ << "[ERROR]:" <<" No suitable fragment shader can be select."; 'selected' > + } > + return fsrc; > +} > + > +void ViewFinderGL::createFragmentShader() > +{ > + bool bCompile; > + > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this); > + char *fsrc = selectFragmentShader(format_); > + > + bCompile = pFShader->compileSourceCode(fsrc); > + if(!bCompile) > + { > + qDebug() << __func__ << ":" << pFShader->log(); > + } > + > + shaderProgram.addShader(pFShader); > + > + // Link shader pipeline Even though it's C++, we usually use C style comments... > + if (!shaderProgram.link()) { > + qDebug() << __func__ << ": shader program link failed.\n" << shaderProgram.log(); > + close(); > + } > + > + // Bind shader pipeline for use > + if (!shaderProgram.bind()) { > + qDebug() << __func__ << ": shader program binding failed.\n" << shaderProgram.log(); > + close(); > + } > + > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX); > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE); > + > + shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat)); > + shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat)); > + > + textureUniformY = shaderProgram.uniformLocation("tex_y"); > + textureUniformU = shaderProgram.uniformLocation("tex_u"); > + textureUniformV = shaderProgram.uniformLocation("tex_v"); > + > + if(!textureY.isCreated()) > + textureY.create(); > + > + if(!textureU.isCreated()) > + textureU.create(); > + > + if(!textureV.isCreated()) > + textureV.create(); > + > + id_y = textureY.textureId(); > + id_u = textureU.textureId(); > + id_v = textureV.textureId(); > +} > + > +void ViewFinderGL::configureTexture(unsigned int id) > +{ > + glBindTexture(GL_TEXTURE_2D, id); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); > +} > + > +void ViewFinderGL::removeShader() > +{ > + if (shaderProgram.isLinked()) { > + shaderProgram.release(); > + shaderProgram.removeAllShaders(); > + } > + > + if(pFShader) > + delete pFShader; > + > + if(pVShader) > + delete pVShader; > +} > + > +void ViewFinderGL::initializeGL() > +{ > + bool bCompile; > + > + initializeOpenGLFunctions(); > + glEnable(GL_TEXTURE_2D); > + glDisable(GL_DEPTH_TEST); > + > + static const GLfloat vertices[] { > + -1.0f,-1.0f, > + -1.0f,+1.0f, > + +1.0f,+1.0f, > + +1.0f,-1.0f, > + 0.0f,1.0f, > + 0.0f,0.0f, > + 1.0f,0.0f, > + 1.0f,1.0f, When you get here, checkstyle will suggest putting this all in one vertical column. Ignore it... (checkstyle is just advice, and in this case your layout is better). Though I would add a space at least after the first ',' on each line to make the columns clearer., Or go further and align the 'f's... ? > + }; > + > + glBuffer.create(); > + glBuffer.bind(); > + glBuffer.allocate(vertices,sizeof(vertices)); > + > + /* Create Vertex Shader */ > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this); > + char *vsrc = NV_Vertex_shader; > + > + bCompile = pVShader->compileSourceCode(vsrc); > + if(!bCompile) { > + qDebug() << __func__<< ":" << pVShader->log(); > + } > + > + shaderProgram.addShader(pVShader); > + > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f); > +} > + > +void ViewFinderGL::render() > +{ > + switch(format_) { > + case libcamera::formats::NV12: > + case libcamera::formats::NV21: > + case libcamera::formats::NV16: > + case libcamera::formats::NV61: > + case libcamera::formats::NV24: > + case libcamera::formats::NV42: > + /* activate texture 0 */ > + glActiveTexture(GL_TEXTURE0); > + configureTexture(id_y); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > + glUniform1i(textureUniformY, 0); > + > + /* activate texture 1 */ > + glActiveTexture(GL_TEXTURE1); > + configureTexture(id_u); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > + glUniform1i(textureUniformU, 1); > + break; > + case libcamera::formats::YUV420: > + /* activate texture 0 */ > + glActiveTexture(GL_TEXTURE0); > + configureTexture(id_y); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > + glUniform1i(textureUniformY, 0); > + > + /* activate texture 1 */ > + glActiveTexture(GL_TEXTURE1); > + configureTexture(id_u); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > + glUniform1i(textureUniformU, 1); > + > + /* activate texture 2 */ > + glActiveTexture(GL_TEXTURE2); > + configureTexture(id_v); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()*5/4); > + glUniform1i(textureUniformV, 2); > + break; > + default: > + break; > + }; > +} > + > +void ViewFinderGL::paintGL() > +{ > + if(pFShader == nullptr) > + createFragmentShader(); > + > + if(yuvDataPtr) > + { > + glClearColor(0.0, 0.0, 0.0, 1.0); > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT ); > + > + render(); > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); > + } > +} > + > +void ViewFinderGL::resizeGL(int w, int h) > +{ > + glViewport(0,0,w,h); > +} > + > +QSize ViewFinderGL::sizeHint() const > +{ > + return size_.isValid() ? size_ : QSize(640, 480); > +} I wonder if this sizeHint should go to the base class? Does ViewFinderGL use it? I think it's there to provide a required overload for the QWidget ... if it's common it could go to the base, and if it's not used, it could get dropped. > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h > new file mode 100644 > index 0000000..08662ca > --- /dev/null > +++ b/src/qcam/viewfinderGL.h > @@ -0,0 +1,101 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2020, Linaro > + * > + * viewfinderGL.h - Render YUV format frame by OpenGL shader > + */ > +#ifndef __VIEWFINDERGL_H__ Perhaps to make this clearer use this: __VIEWFINDER_GL_H__ ? An underscore between the ViewFinder and the GL would separate the base, and the derived names... > +#define __VIEWFINDERGL_H__ > + > +#include <fcntl.h> > +#include <string.h> > +#include <unistd.h> > + > +#include <iomanip> > +#include <iostream> > +#include <sstream> > + > +#include <QImage> > +#include <QOpenGLBuffer> > +#include <QOpenGLFunctions> > +#include <QOpenGLShader> > +#include <QOpenGLShaderProgram> > +#include <QSize> > +#include <QOpenGLTexture> > +#include <QOpenGLWidget> > + > +#include <libcamera/buffer.h> > +#include <libcamera/pixel_format.h> > +#include "viewfinder.h" > + > +class ViewFinderGL : public QOpenGLWidget, > + public ViewFinderHandler, > + protected QOpenGLFunctions That indentation looks off, I think they should be aligned vertically at least. > +{ > + Q_OBJECT > + > +public: > + ViewFinderGL(QWidget *parent = 0); > + ~ViewFinderGL(); > + > + int setFormat(const libcamera::PixelFormat &format, const QSize &size); > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > + void stop(); > + > + QImage getCurrentImage(); Aha - that's why we need QImage, I think that's part of the save routine perhaps isn't it ... > + > + void setFrameSize(int width, int height); I can't see what calls setFrameSize... is it redundant? Or is it an override used externally or such. If so I think it needs to be marked as accordingly as an override? > + void updateFrame(unsigned char *buffer); Same here... What calls updateFrame()? > + > + char *selectFragmentShader(unsigned int format); > + void createFragmentShader(); > + void render(); > + > +Q_SIGNALS: > + void renderComplete(libcamera::FrameBuffer *buffer); > + > +protected: > + void initializeGL() Q_DECL_OVERRIDE; > + void paintGL() Q_DECL_OVERRIDE; > + void resizeGL(int w, int h) Q_DECL_OVERRIDE; > + QSize sizeHint() const override; is sizeHint() used? > + > +private: > + > + void configureTexture(unsigned int id); > + void removeShader(); > + > + /* Captured image size, format and buffer */ > + libcamera::FrameBuffer *buffer_; > + libcamera::PixelFormat format_; > + QOpenGLBuffer glBuffer; glBuffer_; > + QSize size_; > + > + GLuint id_u; > + GLuint id_v; > + GLuint id_y; > + > + QMutex mutex_; /* Prevent concurrent access to image_ */ > + All of the member variables below should have a '_' suffix... > + QOpenGLShader *pFShader; > + QOpenGLShader *pVShader; > + QOpenGLShaderProgram shaderProgram; > + > + GLuint textureUniformU; > + GLuint textureUniformV; > + GLuint textureUniformY; > + > + QOpenGLTexture textureU; > + QOpenGLTexture textureV; > + QOpenGLTexture textureY; > + > + unsigned int width_; > + unsigned int height_; There is already a QSize size_, do these duplicate that purpose? > + > + unsigned char* yuvDataPtr; > + > + /* NV parameters */ > + unsigned int horzSubSample_ ; Extra space? > + unsigned int vertSubSample_; > +}; > +#endif // __VIEWFINDERGL_H__ >
Hi Kieran, Thanks for your review. Your suggestions and opinions are really good for me. I will try to hook the checkstyle.py and check my patch first. I believe that will fix most of problems below. Sorry for the inconvenience. - Show Kieran Bingham <kieran.bingham@ideasonboard.com> 於 2020年6月24日 週三 下午5:34 寫道: > Hi Show, > > Thank you, I'm quite excited to see this update, and I know Niklas is > keen for this to get in too, as he uses a Rockchip target mostly so this > will improve the performances for him quite a lot. > > Most of my comments below are stylistic, (but theres a couple of other > topics too), so as suggested below, could you install the checkstyle.py > hook as a git commit hook please? > Th > > I personally prefer having it as a 'post-commit' hook, rather than a > pre-commit so that it's easier to 'ignore' suggestions that I don't care > for ;-) > > -- > Kieran > > > On 24/06/2020 08:37, Show Liu wrote: > > The patch is to render the NV family YUV formats by OpenGL shader. > > V3: refine the fragment shader for better pixel color. > > > > Signed-off-by: Show Liu <show.liu@linaro.org> > > --- > > src/qcam/main.cpp | 2 + > > src/qcam/main_window.cpp | 19 ++- > > src/qcam/main_window.h | 3 +- > > src/qcam/meson.build | 2 + > > src/qcam/shader.h | 104 ++++++++++++ > > src/qcam/viewfinder.cpp | 18 +- > > src/qcam/viewfinder.h | 23 ++- > > src/qcam/viewfinderGL.cpp | 335 ++++++++++++++++++++++++++++++++++++++ > > src/qcam/viewfinderGL.h | 101 ++++++++++++ > > 9 files changed, 593 insertions(+), 14 deletions(-) > > create mode 100644 src/qcam/shader.h > > create mode 100644 src/qcam/viewfinderGL.cpp > > create mode 100644 src/qcam/viewfinderGL.h > > > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > > index b3468cb..a3ea471 100644 > > --- a/src/qcam/main.cpp > > +++ b/src/qcam/main.cpp > > @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc, char > *argv[]) > > "help"); > > parser.addOption(OptStream, &streamKeyValue, > > "Set configuration of a camera stream", "stream", > true); > > + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats frame > via OpenGL shader", > > + "opengl"); > > (Just a question to everyone) - Should we default to the OpenGL > viewfinder if open-gl is available, and fall back to the QWidget version > otherwise? (We can always do such actions on top/later if we choose.) > > > > > > OptionsParser::Options options = parser.parse(argc, argv); > > if (options.isSet(OptHelp)) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 7bc1360..6edf370 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -28,6 +28,9 @@ > > #include <libcamera/version.h> > > > > #include "dng_writer.h" > > +#include "main_window.h" > > +#include "viewfinder.h" > > +#include "viewfinderGL.h" > > > > using namespace libcamera; > > > > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm, const > OptionsParser::Options &options) > > setWindowTitle(title_); > > connect(&titleTimer_, SIGNAL(timeout()), this, > SLOT(updateTitle())); > > > > - viewfinder_ = new ViewFinder(this); > > - connect(viewfinder_, &ViewFinder::renderComplete, > > - this, &MainWindow::queueRequest); > > - setCentralWidget(viewfinder_); > > + if (options_.isSet(OptOpenGL)) { > > + viewfinder_ = new ViewFinderGL(this); > > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_), > &ViewFinderGL::renderComplete, > > + this, &MainWindow::queueRequest); > > checkstyle.py highlights that the indentation of the second line should > be pulled back, and I agree: > connect(dynamic_cast<ViewFinderGL *>(viewfinder_), > &ViewFinderGL::renderComplete, > - this, &MainWindow::queueRequest); > + this, &MainWindow::queueRequest); > > > > > + setCentralWidget(dynamic_cast<ViewFinderGL > *>(viewfinder_)); > > Does the setCentralWidget need to have the dynamic_cast? Or can it just > be a call to > > setCentralWidget(viewfinder_); I got build error if use above if no dynamic_cast. > > after this conditional block? > > > Perhaps if the base viewfinder class had a method to call which handled > the connect, that could remove casting requirements there - but it would > probably end up needing to deal with complex template things for the > signal/slot parsing ... so I suspect that part isn't worth it... > I can trying to refine this parts. > > > > + } else { > > + viewfinder_ = new ViewFinder(this); > > + connect(dynamic_cast<ViewFinder *>(viewfinder_), > &ViewFinder::renderComplete, > > + this, &MainWindow::queueRequest); > > Same indentation fault here. > > If you add the checkstyle.py hook to your libcamera sources, you'll get > these notifications as you commit (you can then decide if checkstyle was > right or not ;D): > > $ cp utils/hooks/post-commit .git/hooks/post-commit > > > + setCentralWidget(dynamic_cast<ViewFinder *>(viewfinder_)); > > + } > > + > > adjustSize(); > > > > /* Hotplug/unplug support */ > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > index 4606fe4..a852ef4 100644 > > --- a/src/qcam/main_window.h > > +++ b/src/qcam/main_window.h > > @@ -38,6 +38,7 @@ enum { > > OptCamera = 'c', > > OptHelp = 'h', > > OptStream = 's', > > + OptOpenGL = 'o', > > }; > > > > class CaptureRequest > > @@ -102,7 +103,7 @@ private: > > QAction *startStopAction_; > > QComboBox *cameraCombo_; > > QAction *saveRaw_; > > - ViewFinder *viewfinder_; > > + ViewFinderHandler *viewfinder_; > > Handler? > > I guess I'll see below, but I think I'd expect the base class interface > to be 'ViewFinder', and then make specific derived implementations of that? > Ok I will think about how to refine this too. > > > > > QIcon iconPlay_; > > QIcon iconStop_; > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > > index 045db52..6a58947 100644 > > --- a/src/qcam/meson.build > > +++ b/src/qcam/meson.build > > @@ -7,11 +7,13 @@ qcam_sources = files([ > > 'main.cpp', > > 'main_window.cpp', > > 'viewfinder.cpp', > > + 'viewfinderGL.cpp' > > ]) > > > > qcam_moc_headers = files([ > > 'main_window.h', > > 'viewfinder.h', > > + 'viewfinderGL.h' > > ]) > > > > qcam_resources = files([ > > diff --git a/src/qcam/shader.h b/src/qcam/shader.h > > new file mode 100644 > > index 0000000..f54c264 > > --- /dev/null > > +++ b/src/qcam/shader.h > > @@ -0,0 +1,104 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Linaro > > + * > > + * shader.h - YUV format converter shader source code > > + */ > > +#ifndef __SHADER_H__ > > +#define __SHADER_H__ > > + > > +/* Vertex shader for NV formats */ > > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" > > + "attribute vec2 textureIn;\n" > > + "varying vec2 textureOut;\n" > > + "void main(void)\n" > > + "{\n" > > + "gl_Position = vertexIn;\n" > > + "textureOut = textureIn;\n" > > + "}\n"; > > I'd pull that indentation back so that they all match, including moving > the first line to a new line to match... > > char NV_Vertex_shader[] = > "attribute vec4 vertexIn;\n" > "attribute vec2 textureIn;\n" > "varying vec2 textureOut;\n" > "void main(void)\n" > "{\n" > "gl_Position = vertexIn;\n" > "textureOut = textureIn;\n" > "}\n"; > > > > > + > > +/* Fragment shader for NV12, NV16 and NV24 */ > > +char NV_2_planes_UV[] = "#ifdef GL_ES\n" > > And ofcourse be consistent with the code block indentation style > throughout. > > checkstyle might complain whatever you do here, so I would take > checkstyle with a pinch of salt and just make sure the file is consitent > and well laid out. > > > > > + "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, > 1.1640625),\n" > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > + "}\n"; > > + > > +/* Fragment shader for NV21, NV61 and NV42 */ > > +char NV_2_planes_VU[] = "#ifdef GL_ES\n" > > + "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, > 1.1640625),\n" > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n" > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > + "}\n"; > > + > > +/* Fragment shader for YUV420 */ > > +char NV_3_planes_UV[] = "#ifdef GL_ES\n" > > + "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "uniform sampler2D tex_v;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, > 1.1640625),\n" > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1);\n" > > + "}\n"; > > + > > +/* Fragment shader for YVU420 */ > > +char NV_3_planes_VU[] = "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "uniform sampler2D tex_v;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, > 1.1640625),\n" > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n" > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1);\n" > > + "}\n"; > > +#endif // __SHADER_H__ > > I can't comment on the shaders without a lot of research into shaders... > so I'm going to say "Yay, if this black magic works, then it works for > me" ;-D > > > > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp > > index dcf8a15..d3a2422 100644 > > --- a/src/qcam/viewfinder.cpp > > +++ b/src/qcam/viewfinder.cpp > > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat, > QImage::Format> nativeFormats > > { libcamera::formats::BGR888, QImage::Format_RGB888 }, > > }; > > > > -ViewFinder::ViewFinder(QWidget *parent) > > - : QWidget(parent), buffer_(nullptr) > > +ViewFinderHandler::ViewFinderHandler() > > { > > - icon_ = QIcon(":camera-off.svg"); > > } > > > > -ViewFinder::~ViewFinder() > > +ViewFinderHandler::~ViewFinderHandler() > > { > > } > > > > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() const > > +const QList<libcamera::PixelFormat> &ViewFinderHandler::nativeFormats() > const > > { > > static const QList<libcamera::PixelFormat> formats = > ::nativeFormats.keys(); > > return formats; > > } > > > > +ViewFinder::ViewFinder(QWidget *parent) > > + : QWidget(parent), buffer_(nullptr) > > +{ > > + icon_ = QIcon(":camera-off.svg"); > > +} > > + > > +ViewFinder::~ViewFinder() > > +{ > > +} > > + > > int ViewFinder::setFormat(const libcamera::PixelFormat &format, > > const QSize &size) > > { > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > > index 26a1320..741d694 100644 > > --- a/src/qcam/viewfinder.h > > +++ b/src/qcam/viewfinder.h > > @@ -13,6 +13,8 @@ > > #include <QList> > > #include <QImage> > > #include <QMutex> > > +#include <QOpenGLWidget> > > +#include <QOpenGLFunctions> > > Shouldn't those includes be in > src/qcam/viewfinderGL.cpp or src/qcam/viewfinderGL.h only? > > Also - they should be alphabetical order. > (Checkstyle will highlight that for you). > > > #include <QSize> > > #include <QWidget> > > > > @@ -28,7 +30,23 @@ struct MappedBuffer { > > size_t size; > > }; > > > > -class ViewFinder : public QWidget > > +class ViewFinderHandler > > I think it's just naming, but I would have called this ViewFinder,... > > > +{ > > +public: > > + ViewFinderHandler(); > > + virtual ~ViewFinderHandler(); > > + > > + const QList<libcamera::PixelFormat> &nativeFormats() const; > > + > > + virtual int setFormat(const libcamera::PixelFormat &format, const > QSize &size) =0; > > + virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer > *map) =0; > > + virtual void stop() =0; > > + > > + virtual QImage getCurrentImage() =0; > > on each of those, a space after the = looks better: > functionName() = 0; > > > > + > > +}; > > + > > +class ViewFinder : public QWidget, public ViewFinderHandler > > And this, ViewFinderQT or ViewFinderQWidget? (probably the later..) > > To keep consistent with coding style, I think the viewfinder.h would > then only contain the base interface, and create a new file+header for > the QWidget version. > > Or maybe that's over kill, as the QWidget version would be the 'default' > ViewFinder anyway ... > > > > { > > Q_OBJECT > > > > @@ -36,8 +54,6 @@ public: > > ViewFinder(QWidget *parent); > > ~ViewFinder(); > > > > - const QList<libcamera::PixelFormat> &nativeFormats() const; > > - > > int setFormat(const libcamera::PixelFormat &format, const QSize > &size); > > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > void stop(); > > @@ -67,5 +83,4 @@ private: > > QImage image_; > > QMutex mutex_; /* Prevent concurrent access to image_ */ > > }; > > - > > #endif /* __QCAM_VIEWFINDER__ */ > > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp > > new file mode 100644 > > index 0000000..7b47beb > > --- /dev/null > > +++ b/src/qcam/viewfinderGL.cpp > > @@ -0,0 +1,335 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Linaro > > + * > > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader > > + */ > > + > > +#include "shader.h" > > +#include "viewfinderGL.h" > > + > > +#include <QImage> > > QImage is provided in viewfinder.h ... And do you actually use it in here? > > > > + > > +#include <libcamera/formats.h> > > + > > +#define ATTRIB_VERTEX 0 > > +#define ATTRIB_TEXTURE 1 > > + > > +ViewFinderGL::ViewFinderGL(QWidget *parent) > > + : QOpenGLWidget(parent), > > + glBuffer(QOpenGLBuffer::VertexBuffer), > > + pFShader(nullptr), > > + pVShader(nullptr), > > + textureU(QOpenGLTexture::Target2D), > > + textureV(QOpenGLTexture::Target2D), > > + textureY(QOpenGLTexture::Target2D), > > + yuvDataPtr(nullptr) > > Checkstyle will tell you to align those vertically with the first > QOpenGLWidget(parent), rather than the ':'. > > > > + > > +{ > > +} > > + > > +ViewFinderGL::~ViewFinderGL() > > +{ > > + removeShader(); > > + > > + if(textureY.isCreated()) > > Space after if: "if ()" > > > + textureY.destroy(); > > + > > + if(textureU.isCreated()) > > + textureU.destroy(); > > + > > + if(textureV.isCreated()) > > + textureV.destroy(); > > For each of those ;-) > > There's plenty more too, but I'll let you find them with checkstyle. > > > > + > > + glBuffer.destroy(); > > +} > > + > > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > > + const QSize &size) > > +{ > > + format_ = format; > > + size_ = size; > > + > > + updateGeometry(); > > + return 0; > > +} > > + > > +void ViewFinderGL::stop() > > +{ > > + if (buffer_) { > > + renderComplete(buffer_); > > + buffer_ = nullptr; > > + } > > +} > > + > > +QImage ViewFinderGL::getCurrentImage() > > +{ > > + QMutexLocker locker(&mutex_); > > + > > + return(grabFramebuffer()); > > I think that could be : > > return grabFrameBuffer(); > > > +} > > + > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer > *map) > > +{ > > + if (buffer->planes().size() != 1) { > > + qWarning() << "Multi-planar buffers are not supported"; > > + return; > > + } > > + > > + unsigned char *memory = static_cast<unsigned char *>(map->memory); > > + if(memory) { > > + yuvDataPtr = memory; > > + update(); > > + buffer_ = buffer; > > + } > > + > > + if (buffer) > > + renderComplete(buffer); > > +} > > + > > +void ViewFinderGL::updateFrame(unsigned char *buffer) > > +{ > > + if(buffer) { > > + yuvDataPtr = buffer; > > + update(); > > + } > > +} > > + > > +void ViewFinderGL::setFrameSize(int width, int height) > > Can width/height be negative? Maybe the should be unsigned int? > > What about using a QSize too rather than individually passing width/height? > > I can't see what calls setFrameSize(), is this an override from > > > +{ > > + if(width > 0 && height > 0) { > > + width_ = width; > > + height_ = height; > > + } > > In fact, don't we already have a size_ in the class? (which could also > be in the base class too most likely... > Sure, I will use QSize instead. > > > > +} > > + > > +char *ViewFinderGL::selectFragmentShader(unsigned int format) > > +{ > > + char *fsrc = nullptr; > > + > > + switch(format) { > > + case libcamera::formats::NV12: > > + horzSubSample_ = 2; > > + vertSubSample_ = 2; > > + fsrc = NV_2_planes_UV; > > + break; > > + case libcamera::formats::NV21: > > + horzSubSample_ = 2; > > + vertSubSample_ = 2; > > + fsrc = NV_2_planes_VU; > > + break; > > + case libcamera::formats::NV16: > > + horzSubSample_ = 2; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_UV; > > + break; > > + case libcamera::formats::NV61: > > + horzSubSample_ = 2; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_VU; > > + break; > > + case libcamera::formats::NV24: > > + horzSubSample_ = 1; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_UV; > > + break; > > + case libcamera::formats::NV42: > > + horzSubSample_ = 1; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_VU; > > + break; > > + case libcamera::formats::YUV420: > > + horzSubSample_ = 2; > > + vertSubSample_ = 2; > > + fsrc = NV_3_planes_UV; > > + break; > > + default: > > + break; > > + }; > > + > > + if(fsrc == nullptr) { > > + qDebug() << __func__ << "[ERROR]:" <<" No suitable > fragment shader can be select."; > > 'selected' > Ok. Will fix it. > > > > + } > > + return fsrc; > > +} > > + > > +void ViewFinderGL::createFragmentShader() > > +{ > > + bool bCompile; > > + > > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this); > > + char *fsrc = selectFragmentShader(format_); > > + > > + bCompile = pFShader->compileSourceCode(fsrc); > > + if(!bCompile) > > + { > > + qDebug() << __func__ << ":" << pFShader->log(); > > + } > > + > > + shaderProgram.addShader(pFShader); > > + > > + // Link shader pipeline > > Even though it's C++, we usually use C style comments... > > > > + if (!shaderProgram.link()) { > > + qDebug() << __func__ << ": shader program link failed.\n" > << shaderProgram.log(); > > + close(); > > + } > > + > > + // Bind shader pipeline for use > > + if (!shaderProgram.bind()) { > > + qDebug() << __func__ << ": shader program binding > failed.\n" << shaderProgram.log(); > > + close(); > > + } > > + > > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX); > > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE); > > + > > + > shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat)); > > + > shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat)); > > + > > + textureUniformY = shaderProgram.uniformLocation("tex_y"); > > + textureUniformU = shaderProgram.uniformLocation("tex_u"); > > + textureUniformV = shaderProgram.uniformLocation("tex_v"); > > + > > + if(!textureY.isCreated()) > > + textureY.create(); > > + > > + if(!textureU.isCreated()) > > + textureU.create(); > > + > > + if(!textureV.isCreated()) > > + textureV.create(); > > + > > + id_y = textureY.textureId(); > > + id_u = textureU.textureId(); > > + id_v = textureV.textureId(); > > +} > > + > > +void ViewFinderGL::configureTexture(unsigned int id) > > +{ > > + glBindTexture(GL_TEXTURE_2D, id); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, > GL_CLAMP_TO_EDGE); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, > GL_CLAMP_TO_EDGE); > > +} > > + > > +void ViewFinderGL::removeShader() > > +{ > > + if (shaderProgram.isLinked()) { > > + shaderProgram.release(); > > + shaderProgram.removeAllShaders(); > > + } > > + > > + if(pFShader) > > + delete pFShader; > > + > > + if(pVShader) > > + delete pVShader; > > +} > > + > > +void ViewFinderGL::initializeGL() > > +{ > > + bool bCompile; > > + > > + initializeOpenGLFunctions(); > > + glEnable(GL_TEXTURE_2D); > > + glDisable(GL_DEPTH_TEST); > > + > > + static const GLfloat vertices[] { > > + -1.0f,-1.0f, > > + -1.0f,+1.0f, > > + +1.0f,+1.0f, > > + +1.0f,-1.0f, > > + 0.0f,1.0f, > > + 0.0f,0.0f, > > + 1.0f,0.0f, > > + 1.0f,1.0f, > > When you get here, checkstyle will suggest putting this all in one > vertical column. > > Ignore it... (checkstyle is just advice, and in this case your layout is > better). > > Though I would add a space at least after the first ',' on each line to > make the columns clearer., Or go further and align the 'f's... ? > > > + }; > > + > > + glBuffer.create(); > > + glBuffer.bind(); > > + glBuffer.allocate(vertices,sizeof(vertices)); > > + > > + /* Create Vertex Shader */ > > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this); > > + char *vsrc = NV_Vertex_shader; > > + > > + bCompile = pVShader->compileSourceCode(vsrc); > > + if(!bCompile) { > > + qDebug() << __func__<< ":" << pVShader->log(); > > + } > > + > > + shaderProgram.addShader(pVShader); > > + > > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f); > > +} > > + > > +void ViewFinderGL::render() > > +{ > > + switch(format_) { > > + case libcamera::formats::NV12: > > + case libcamera::formats::NV21: > > + case libcamera::formats::NV16: > > + case libcamera::formats::NV61: > > + case libcamera::formats::NV24: > > + case libcamera::formats::NV42: > > + /* activate texture 0 */ > > + glActiveTexture(GL_TEXTURE0); > > + configureTexture(id_y); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), > size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > + glUniform1i(textureUniformY, 0); > > + > > + /* activate texture 1 */ > > + glActiveTexture(GL_TEXTURE1); > > + configureTexture(id_u); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, > GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > + glUniform1i(textureUniformU, 1); > > + break; > > + case libcamera::formats::YUV420: > > + /* activate texture 0 */ > > + glActiveTexture(GL_TEXTURE0); > > + configureTexture(id_y); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), > size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > + glUniform1i(textureUniformY, 0); > > + > > + /* activate texture 1 */ > > + glActiveTexture(GL_TEXTURE1); > > + configureTexture(id_u); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, > GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > + glUniform1i(textureUniformU, 1); > > + > > + /* activate texture 2 */ > > + glActiveTexture(GL_TEXTURE2); > > + configureTexture(id_v); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, > GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()*5/4); > > + glUniform1i(textureUniformV, 2); > > + break; > > + default: > > + break; > > + }; > > +} > > + > > +void ViewFinderGL::paintGL() > > +{ > > + if(pFShader == nullptr) > > + createFragmentShader(); > > + > > + if(yuvDataPtr) > > + { > > + glClearColor(0.0, 0.0, 0.0, 1.0); > > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT ); > > + > > + render(); > > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); > > + } > > +} > > + > > +void ViewFinderGL::resizeGL(int w, int h) > > +{ > > + glViewport(0,0,w,h); > > +} > > + > > +QSize ViewFinderGL::sizeHint() const > > +{ > > + return size_.isValid() ? size_ : QSize(640, 480); > > +} > > I wonder if this sizeHint should go to the base class? > > Does ViewFinderGL use it? I think it's there to provide a required > overload for the QWidget ... if it's common it could go to the base, and > if it's not used, it could get dropped. > Got it. > > > > > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h > > new file mode 100644 > > index 0000000..08662ca > > --- /dev/null > > +++ b/src/qcam/viewfinderGL.h > > @@ -0,0 +1,101 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Linaro > > + * > > + * viewfinderGL.h - Render YUV format frame by OpenGL shader > > + */ > > +#ifndef __VIEWFINDERGL_H__ > > Perhaps to make this clearer use this: __VIEWFINDER_GL_H__ ? > An underscore between the ViewFinder and the GL would separate the base, > and the derived names... > Ok, I will fix this. > > > +#define __VIEWFINDERGL_H__ > > + > > +#include <fcntl.h> > > +#include <string.h> > > +#include <unistd.h> > > + > > +#include <iomanip> > > +#include <iostream> > > +#include <sstream> > > + > > +#include <QImage> > > +#include <QOpenGLBuffer> > > +#include <QOpenGLFunctions> > > +#include <QOpenGLShader> > > +#include <QOpenGLShaderProgram> > > +#include <QSize> > > +#include <QOpenGLTexture> > > +#include <QOpenGLWidget> > > + > > +#include <libcamera/buffer.h> > > +#include <libcamera/pixel_format.h> > > +#include "viewfinder.h" > > + > > +class ViewFinderGL : public QOpenGLWidget, > > + public ViewFinderHandler, > > + protected QOpenGLFunctions > > That indentation looks off, I think they should be aligned vertically at > least. > Sure. > > > > +{ > > + Q_OBJECT > > + > > +public: > > + ViewFinderGL(QWidget *parent = 0); > > + ~ViewFinderGL(); > > + > > + int setFormat(const libcamera::PixelFormat &format, const QSize > &size); > > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > + void stop(); > > + > > + QImage getCurrentImage(); > > Aha - that's why we need QImage, I think that's part of the save routine > perhaps isn't it ... > > > + > > + void setFrameSize(int width, int height); > > I can't see what calls setFrameSize... is it redundant? Or is it an > override used externally or such. If so I think it needs to be marked as > accordingly as an override? > > > + void updateFrame(unsigned char *buffer); > > Same here... What calls updateFrame()? > Actually, only NV12, I can verify with Libcamera on rock pi 4, I use an external application to verify other shaders and above functions are for that. I will remove that in next version. > > > > + > > + char *selectFragmentShader(unsigned int format); > > + void createFragmentShader(); > > + void render(); > > + > > +Q_SIGNALS: > > + void renderComplete(libcamera::FrameBuffer *buffer); > > + > > +protected: > > + void initializeGL() Q_DECL_OVERRIDE; > > + void paintGL() Q_DECL_OVERRIDE; > > + void resizeGL(int w, int h) Q_DECL_OVERRIDE; > > + QSize sizeHint() const override; > > is sizeHint() used? > > > > + > > +private: > > + > > + void configureTexture(unsigned int id); > > + void removeShader(); > > + > > + /* Captured image size, format and buffer */ > > + libcamera::FrameBuffer *buffer_; > > + libcamera::PixelFormat format_; > > + QOpenGLBuffer glBuffer; > > glBuffer_; > Will fix it. > > > > > + QSize size_; > > + > > + GLuint id_u; > > + GLuint id_v; > > + GLuint id_y; > > + > > + QMutex mutex_; /* Prevent concurrent access to image_ */ > > + > > All of the member variables below should have a '_' suffix... > > > + QOpenGLShader *pFShader; > > + QOpenGLShader *pVShader; > > + QOpenGLShaderProgram shaderProgram; > > + > > + GLuint textureUniformU; > > + GLuint textureUniformV; > > + GLuint textureUniformY; > > + > > + QOpenGLTexture textureU; > > + QOpenGLTexture textureV; > > + QOpenGLTexture textureY; > > + > > + unsigned int width_; > > + unsigned int height_; > > There is already a QSize size_, do these duplicate that purpose? > Yeah, I will use size_ instead. > > > + > > + unsigned char* yuvDataPtr; > > + > > + /* NV parameters */ > > + unsigned int horzSubSample_ ; > > Extra space? > Ok will fix it. > > > + unsigned int vertSubSample_; > > +}; > > +#endif // __VIEWFINDERGL_H__ > > > > > > -- > Regards > -- > Kieran > Thank you for your comments again. I will post next version asap. -- Regards -- Show >
Hi Show, On 25/06/2020 09:43, Show Liu wrote: > Hi Kieran, > > Thanks for your review. > Your suggestions and opinions are really good for me. > > I will try to hook the checkstyle.py and check my patch first. I believe > that will fix most of problems below. Sorry for the inconvenience. No problem, or inconvenience, that's why we (well Laurent) made checkstyle.py :-D Looking forward to your update. Kieran > > - Show > > > > > Kieran Bingham <kieran.bingham@ideasonboard.com > <mailto:kieran.bingham@ideasonboard.com>> 於 2020年6月24日 週三 下午5:34 > 寫道: > > Hi Show, > > Thank you, I'm quite excited to see this update, and I know Niklas is > keen for this to get in too, as he uses a Rockchip target mostly so this > will improve the performances for him quite a lot. > > Most of my comments below are stylistic, (but theres a couple of other > topics too), so as suggested below, could you install the checkstyle.py > hook as a git commit hook please? > > > Th > > > I personally prefer having it as a 'post-commit' hook, rather than a > pre-commit so that it's easier to 'ignore' suggestions that I don't care > for ;-) > > -- > Kieran > > > On 24/06/2020 08:37, Show Liu wrote: > > The patch is to render the NV family YUV formats by OpenGL shader. > > V3: refine the fragment shader for better pixel color. > > > > Signed-off-by: Show Liu <show.liu@linaro.org > <mailto:show.liu@linaro.org>> > > --- > > src/qcam/main.cpp | 2 + > > src/qcam/main_window.cpp | 19 ++- > > src/qcam/main_window.h | 3 +- > > src/qcam/meson.build | 2 + > > src/qcam/shader.h | 104 ++++++++++++ > > src/qcam/viewfinder.cpp | 18 +- > > src/qcam/viewfinder.h | 23 ++- > > src/qcam/viewfinderGL.cpp | 335 > ++++++++++++++++++++++++++++++++++++++ > > src/qcam/viewfinderGL.h | 101 ++++++++++++ > > 9 files changed, 593 insertions(+), 14 deletions(-) > > create mode 100644 src/qcam/shader.h > > create mode 100644 src/qcam/viewfinderGL.cpp > > create mode 100644 src/qcam/viewfinderGL.h > > > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > > index b3468cb..a3ea471 100644 > > --- a/src/qcam/main.cpp > > +++ b/src/qcam/main.cpp > > @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc, > char *argv[]) > > "help"); > > parser.addOption(OptStream, &streamKeyValue, > > "Set configuration of a camera stream", > "stream", true); > > + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats > frame via OpenGL shader", > > + "opengl"); > > (Just a question to everyone) - Should we default to the OpenGL > viewfinder if open-gl is available, and fall back to the QWidget version > otherwise? (We can always do such actions on top/later if we choose.) > > > > > > OptionsParser::Options options = parser.parse(argc, argv); > > if (options.isSet(OptHelp)) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 7bc1360..6edf370 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -28,6 +28,9 @@ > > #include <libcamera/version.h> > > > > #include "dng_writer.h" > > +#include "main_window.h" > > +#include "viewfinder.h" > > +#include "viewfinderGL.h" > > > > using namespace libcamera; > > > > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm, > const OptionsParser::Options &options) > > setWindowTitle(title_); > > connect(&titleTimer_, SIGNAL(timeout()), this, > SLOT(updateTitle())); > > > > - viewfinder_ = new ViewFinder(this); > > - connect(viewfinder_, &ViewFinder::renderComplete, > > - this, &MainWindow::queueRequest); > > - setCentralWidget(viewfinder_); > > + if (options_.isSet(OptOpenGL)) { > > + viewfinder_ = new ViewFinderGL(this); > > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_), > &ViewFinderGL::renderComplete, > > + this, &MainWindow::queueRequest); > > checkstyle.py highlights that the indentation of the second line should > be pulled back, and I agree: > connect(dynamic_cast<ViewFinderGL *>(viewfinder_), > &ViewFinderGL::renderComplete, > - this, &MainWindow::queueRequest); > + this, &MainWindow::queueRequest); > > > > > + setCentralWidget(dynamic_cast<ViewFinderGL > *>(viewfinder_)); > > Does the setCentralWidget need to have the dynamic_cast? Or can it just > be a call to > > setCentralWidget(viewfinder_); > > I got build error if use above if no dynamic_cast. > > > after this conditional block? > > > Perhaps if the base viewfinder class had a method to call which handled > the connect, that could remove casting requirements there - but it would > probably end up needing to deal with complex template things for the > signal/slot parsing ... so I suspect that part isn't worth it... > > > I can trying to refine this parts. > > > > > + } else { > > + viewfinder_ = new ViewFinder(this); > > + connect(dynamic_cast<ViewFinder *>(viewfinder_), > &ViewFinder::renderComplete, > > + this, &MainWindow::queueRequest); > > Same indentation fault here. > > If you add the checkstyle.py hook to your libcamera sources, you'll get > these notifications as you commit (you can then decide if checkstyle was > right or not ;D): > > $ cp utils/hooks/post-commit .git/hooks/post-commit > > > + setCentralWidget(dynamic_cast<ViewFinder > *>(viewfinder_)); > > + } > > + > > adjustSize(); > > > > /* Hotplug/unplug support */ > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > index 4606fe4..a852ef4 100644 > > --- a/src/qcam/main_window.h > > +++ b/src/qcam/main_window.h > > @@ -38,6 +38,7 @@ enum { > > OptCamera = 'c', > > OptHelp = 'h', > > OptStream = 's', > > + OptOpenGL = 'o', > > }; > > > > class CaptureRequest > > @@ -102,7 +103,7 @@ private: > > QAction *startStopAction_; > > QComboBox *cameraCombo_; > > QAction *saveRaw_; > > - ViewFinder *viewfinder_; > > + ViewFinderHandler *viewfinder_; > > Handler? > > I guess I'll see below, but I think I'd expect the base class interface > to be 'ViewFinder', and then make specific derived implementations > of that? > > Ok I will think about how to refine this too. > > > > > > QIcon iconPlay_; > > QIcon iconStop_; > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > > index 045db52..6a58947 100644 > > --- a/src/qcam/meson.build > > +++ b/src/qcam/meson.build > > @@ -7,11 +7,13 @@ qcam_sources = files([ > > 'main.cpp', > > 'main_window.cpp', > > 'viewfinder.cpp', > > + 'viewfinderGL.cpp' > > ]) > > > > qcam_moc_headers = files([ > > 'main_window.h', > > 'viewfinder.h', > > + 'viewfinderGL.h' > > ]) > > > > qcam_resources = files([ > > diff --git a/src/qcam/shader.h b/src/qcam/shader.h > > new file mode 100644 > > index 0000000..f54c264 > > --- /dev/null > > +++ b/src/qcam/shader.h > > @@ -0,0 +1,104 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Linaro > > + * > > + * shader.h - YUV format converter shader source code > > + */ > > +#ifndef __SHADER_H__ > > +#define __SHADER_H__ > > + > > +/* Vertex shader for NV formats */ > > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" > > + "attribute vec2 textureIn;\n" > > + "varying vec2 textureOut;\n" > > + "void main(void)\n" > > + "{\n" > > + "gl_Position = vertexIn;\n" > > + "textureOut = textureIn;\n" > > + "}\n"; > > I'd pull that indentation back so that they all match, including moving > the first line to a new line to match... > > char NV_Vertex_shader[] = > "attribute vec4 vertexIn;\n" > "attribute vec2 textureIn;\n" > "varying vec2 textureOut;\n" > "void main(void)\n" > "{\n" > "gl_Position = vertexIn;\n" > "textureOut = textureIn;\n" > "}\n"; > > > > > + > > +/* Fragment shader for NV12, NV16 and NV24 */ > > +char NV_2_planes_UV[] = "#ifdef GL_ES\n" > > And ofcourse be consistent with the code block indentation style > throughout. > > checkstyle might complain whatever you do here, so I would take > checkstyle with a pinch of salt and just make sure the file is consitent > and well laid out. > > > > > + "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, > 1.1640625, 1.1640625),\n" > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > + "}\n"; > > + > > +/* Fragment shader for NV21, NV61 and NV42 */ > > +char NV_2_planes_VU[] = "#ifdef GL_ES\n" > > + "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, > 1.1640625, 1.1640625),\n" > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n" > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > + "}\n"; > > + > > +/* Fragment shader for YUV420 */ > > +char NV_3_planes_UV[] = "#ifdef GL_ES\n" > > + "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "uniform sampler2D tex_v;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, > 1.1640625, 1.1640625),\n" > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1);\n" > > + "}\n"; > > + > > +/* Fragment shader for YVU420 */ > > +char NV_3_planes_VU[] = "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "uniform sampler2D tex_v;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, > 1.1640625, 1.1640625),\n" > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n" > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1);\n" > > + "}\n"; > > +#endif // __SHADER_H__ > > I can't comment on the shaders without a lot of research into shaders... > so I'm going to say "Yay, if this black magic works, then it works for > me" ;-D > > > > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp > > index dcf8a15..d3a2422 100644 > > --- a/src/qcam/viewfinder.cpp > > +++ b/src/qcam/viewfinder.cpp > > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat, > QImage::Format> nativeFormats > > { libcamera::formats::BGR888, QImage::Format_RGB888 }, > > }; > > > > -ViewFinder::ViewFinder(QWidget *parent) > > - : QWidget(parent), buffer_(nullptr) > > +ViewFinderHandler::ViewFinderHandler() > > { > > - icon_ = QIcon(":camera-off.svg"); > > } > > > > -ViewFinder::~ViewFinder() > > +ViewFinderHandler::~ViewFinderHandler() > > { > > } > > > > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() > const > > +const QList<libcamera::PixelFormat> > &ViewFinderHandler::nativeFormats() const > > { > > static const QList<libcamera::PixelFormat> formats = > ::nativeFormats.keys(); > > return formats; > > } > > > > +ViewFinder::ViewFinder(QWidget *parent) > > + : QWidget(parent), buffer_(nullptr) > > +{ > > + icon_ = QIcon(":camera-off.svg"); > > +} > > + > > +ViewFinder::~ViewFinder() > > +{ > > +} > > + > > int ViewFinder::setFormat(const libcamera::PixelFormat &format, > > const QSize &size) > > { > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > > index 26a1320..741d694 100644 > > --- a/src/qcam/viewfinder.h > > +++ b/src/qcam/viewfinder.h > > @@ -13,6 +13,8 @@ > > #include <QList> > > #include <QImage> > > #include <QMutex> > > +#include <QOpenGLWidget> > > +#include <QOpenGLFunctions> > > Shouldn't those includes be in > src/qcam/viewfinderGL.cpp or src/qcam/viewfinderGL.h only? > > Also - they should be alphabetical order. > (Checkstyle will highlight that for you). > > > #include <QSize> > > #include <QWidget> > > > > @@ -28,7 +30,23 @@ struct MappedBuffer { > > size_t size; > > }; > > > > -class ViewFinder : public QWidget > > +class ViewFinderHandler > > I think it's just naming, but I would have called this ViewFinder,... > > > +{ > > +public: > > + ViewFinderHandler(); > > + virtual ~ViewFinderHandler(); > > + > > + const QList<libcamera::PixelFormat> &nativeFormats() const; > > + > > + virtual int setFormat(const libcamera::PixelFormat &format, > const QSize &size) =0; > > + virtual void render(libcamera::FrameBuffer *buffer, > MappedBuffer *map) =0; > > + virtual void stop() =0; > > + > > + virtual QImage getCurrentImage() =0; > > on each of those, a space after the = looks better: > functionName() = 0; > > > > + > > +}; > > + > > +class ViewFinder : public QWidget, public ViewFinderHandler > > And this, ViewFinderQT or ViewFinderQWidget? (probably the later..) > > To keep consistent with coding style, I think the viewfinder.h would > then only contain the base interface, and create a new file+header for > the QWidget version. > > Or maybe that's over kill, as the QWidget version would be the 'default' > ViewFinder anyway ... > > > > { > > Q_OBJECT > > > > @@ -36,8 +54,6 @@ public: > > ViewFinder(QWidget *parent); > > ~ViewFinder(); > > > > - const QList<libcamera::PixelFormat> &nativeFormats() const; > > - > > int setFormat(const libcamera::PixelFormat &format, const > QSize &size); > > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > void stop(); > > @@ -67,5 +83,4 @@ private: > > QImage image_; > > QMutex mutex_; /* Prevent concurrent access to image_ */ > > }; > > - > > #endif /* __QCAM_VIEWFINDER__ */ > > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp > > new file mode 100644 > > index 0000000..7b47beb > > --- /dev/null > > +++ b/src/qcam/viewfinderGL.cpp > > @@ -0,0 +1,335 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Linaro > > + * > > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader > > + */ > > + > > +#include "shader.h" > > +#include "viewfinderGL.h" > > + > > +#include <QImage> > > QImage is provided in viewfinder.h ... And do you actually use it in > here? > > > > + > > +#include <libcamera/formats.h> > > + > > +#define ATTRIB_VERTEX 0 > > +#define ATTRIB_TEXTURE 1 > > + > > +ViewFinderGL::ViewFinderGL(QWidget *parent) > > + : QOpenGLWidget(parent), > > + glBuffer(QOpenGLBuffer::VertexBuffer), > > + pFShader(nullptr), > > + pVShader(nullptr), > > + textureU(QOpenGLTexture::Target2D), > > + textureV(QOpenGLTexture::Target2D), > > + textureY(QOpenGLTexture::Target2D), > > + yuvDataPtr(nullptr) > > Checkstyle will tell you to align those vertically with the first > QOpenGLWidget(parent), rather than the ':'. > > > > + > > +{ > > +} > > + > > +ViewFinderGL::~ViewFinderGL() > > +{ > > + removeShader(); > > + > > + if(textureY.isCreated()) > > Space after if: "if ()" > > > + textureY.destroy(); > > + > > + if(textureU.isCreated()) > > + textureU.destroy(); > > + > > + if(textureV.isCreated()) > > + textureV.destroy(); > > For each of those ;-) > > There's plenty more too, but I'll let you find them with checkstyle. > > > > + > > + glBuffer.destroy(); > > +} > > + > > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > > + const QSize &size) > > +{ > > + format_ = format; > > + size_ = size; > > + > > + updateGeometry(); > > + return 0; > > +} > > + > > +void ViewFinderGL::stop() > > +{ > > + if (buffer_) { > > + renderComplete(buffer_); > > + buffer_ = nullptr; > > + } > > +} > > + > > +QImage ViewFinderGL::getCurrentImage() > > +{ > > + QMutexLocker locker(&mutex_); > > + > > + return(grabFramebuffer()); > > I think that could be : > > return grabFrameBuffer(); > > > +} > > + > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, > MappedBuffer *map) > > +{ > > + if (buffer->planes().size() != 1) { > > + qWarning() << "Multi-planar buffers are not supported"; > > + return; > > + } > > + > > + unsigned char *memory = static_cast<unsigned char > *>(map->memory); > > + if(memory) { > > + yuvDataPtr = memory; > > + update(); > > + buffer_ = buffer; > > + } > > + > > + if (buffer) > > + renderComplete(buffer); > > +} > > + > > +void ViewFinderGL::updateFrame(unsigned char *buffer) > > +{ > > + if(buffer) { > > + yuvDataPtr = buffer; > > + update(); > > + } > > +} > > + > > +void ViewFinderGL::setFrameSize(int width, int height) > > Can width/height be negative? Maybe the should be unsigned int? > > What about using a QSize too rather than individually passing > width/height? > > I can't see what calls setFrameSize(), is this an override from > > > +{ > > + if(width > 0 && height > 0) { > > + width_ = width; > > + height_ = height; > > + } > > In fact, don't we already have a size_ in the class? (which could also > be in the base class too most likely... > > Sure, I will use QSize instead. > > > > > +} > > + > > +char *ViewFinderGL::selectFragmentShader(unsigned int format) > > +{ > > + char *fsrc = nullptr; > > + > > + switch(format) { > > + case libcamera::formats::NV12: > > + horzSubSample_ = 2; > > + vertSubSample_ = 2; > > + fsrc = NV_2_planes_UV; > > + break; > > + case libcamera::formats::NV21: > > + horzSubSample_ = 2; > > + vertSubSample_ = 2; > > + fsrc = NV_2_planes_VU; > > + break; > > + case libcamera::formats::NV16: > > + horzSubSample_ = 2; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_UV; > > + break; > > + case libcamera::formats::NV61: > > + horzSubSample_ = 2; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_VU; > > + break; > > + case libcamera::formats::NV24: > > + horzSubSample_ = 1; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_UV; > > + break; > > + case libcamera::formats::NV42: > > + horzSubSample_ = 1; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_VU; > > + break; > > + case libcamera::formats::YUV420: > > + horzSubSample_ = 2; > > + vertSubSample_ = 2; > > + fsrc = NV_3_planes_UV; > > + break; > > + default: > > + break; > > + }; > > + > > + if(fsrc == nullptr) { > > + qDebug() << __func__ << "[ERROR]:" <<" No suitable > fragment shader can be select."; > > 'selected' > > Ok. Will fix it. > > > > > + } > > + return fsrc; > > +} > > + > > +void ViewFinderGL::createFragmentShader() > > +{ > > + bool bCompile; > > + > > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this); > > + char *fsrc = selectFragmentShader(format_); > > + > > + bCompile = pFShader->compileSourceCode(fsrc); > > + if(!bCompile) > > + { > > + qDebug() << __func__ << ":" << pFShader->log(); > > + } > > + > > + shaderProgram.addShader(pFShader); > > + > > + // Link shader pipeline > > Even though it's C++, we usually use C style comments... > > > > + if (!shaderProgram.link()) { > > + qDebug() << __func__ << ": shader program link > failed.\n" << shaderProgram.log(); > > + close(); > > + } > > + > > + // Bind shader pipeline for use > > + if (!shaderProgram.bind()) { > > + qDebug() << __func__ << ": shader program binding > failed.\n" << shaderProgram.log(); > > + close(); > > + } > > + > > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX); > > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE); > > + > > + > shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat)); > > + > shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat)); > > + > > + textureUniformY = shaderProgram.uniformLocation("tex_y"); > > + textureUniformU = shaderProgram.uniformLocation("tex_u"); > > + textureUniformV = shaderProgram.uniformLocation("tex_v"); > > + > > + if(!textureY.isCreated()) > > + textureY.create(); > > + > > + if(!textureU.isCreated()) > > + textureU.create(); > > + > > + if(!textureV.isCreated()) > > + textureV.create(); > > + > > + id_y = textureY.textureId(); > > + id_u = textureU.textureId(); > > + id_v = textureV.textureId(); > > +} > > + > > +void ViewFinderGL::configureTexture(unsigned int id) > > +{ > > + glBindTexture(GL_TEXTURE_2D, id); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, > GL_LINEAR); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, > GL_LINEAR); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, > GL_CLAMP_TO_EDGE); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, > GL_CLAMP_TO_EDGE); > > +} > > + > > +void ViewFinderGL::removeShader() > > +{ > > + if (shaderProgram.isLinked()) { > > + shaderProgram.release(); > > + shaderProgram.removeAllShaders(); > > + } > > + > > + if(pFShader) > > + delete pFShader; > > + > > + if(pVShader) > > + delete pVShader; > > +} > > + > > +void ViewFinderGL::initializeGL() > > +{ > > + bool bCompile; > > + > > + initializeOpenGLFunctions(); > > + glEnable(GL_TEXTURE_2D); > > + glDisable(GL_DEPTH_TEST); > > + > > + static const GLfloat vertices[] { > > + -1.0f,-1.0f, > > + -1.0f,+1.0f, > > + +1.0f,+1.0f, > > + +1.0f,-1.0f, > > + 0.0f,1.0f, > > + 0.0f,0.0f, > > + 1.0f,0.0f, > > + 1.0f,1.0f, > > When you get here, checkstyle will suggest putting this all in one > vertical column. > > Ignore it... (checkstyle is just advice, and in this case your layout is > better). > > Though I would add a space at least after the first ',' on each line to > make the columns clearer., Or go further and align the 'f's... ? > > > + }; > > + > > + glBuffer.create(); > > + glBuffer.bind(); > > + glBuffer.allocate(vertices,sizeof(vertices)); > > + > > + /* Create Vertex Shader */ > > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this); > > + char *vsrc = NV_Vertex_shader; > > + > > +�� bCompile = pVShader->compileSourceCode(vsrc); > > + if(!bCompile) { > > + qDebug() << __func__<< ":" << pVShader->log(); > > + } > > + > > + shaderProgram.addShader(pVShader); > > + > > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f); > > +} > > + > > +void ViewFinderGL::render() > > +{ > > + switch(format_) { > > + case libcamera::formats::NV12: > > + case libcamera::formats::NV21: > > + case libcamera::formats::NV16: > > + case libcamera::formats::NV61: > > + case libcamera::formats::NV24: > > + case libcamera::formats::NV42: > > + /* activate texture 0 */ > > + glActiveTexture(GL_TEXTURE0); > > + configureTexture(id_y); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, > size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > + glUniform1i(textureUniformY, 0); > > + > > + /* activate texture 1 */ > > + glActiveTexture(GL_TEXTURE1); > > + configureTexture(id_u); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, > GL_RG, GL_UNSIGNED_BYTE, > (char*)yuvDataPtr+size_.width()*size_.height()); > > + glUniform1i(textureUniformU, 1); > > + break; > > + case libcamera::formats::YUV420: > > + /* activate texture 0 */ > > + glActiveTexture(GL_TEXTURE0); > > + configureTexture(id_y); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, > size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > + glUniform1i(textureUniformY, 0); > > + > > + /* activate texture 1 */ > > + glActiveTexture(GL_TEXTURE1); > > + configureTexture(id_u); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, > GL_RG, GL_UNSIGNED_BYTE, > (char*)yuvDataPtr+size_.width()*size_.height()); > > + glUniform1i(textureUniformU, 1); > > + > > + /* activate texture 2 */ > > + glActiveTexture(GL_TEXTURE2); > > + configureTexture(id_v); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, > GL_RG, GL_UNSIGNED_BYTE, > (char*)yuvDataPtr+size_.width()*size_.height()*5/4); > > + glUniform1i(textureUniformV, 2); > > + break; > > + default: > > + break; > > + }; > > +} > > + > > +void ViewFinderGL::paintGL() > > +{ > > + if(pFShader == nullptr) > > + createFragmentShader(); > > + > > + if(yuvDataPtr) > > + { > > + glClearColor(0.0, 0.0, 0.0, 1.0); > > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT ); > > + > > + render(); > > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); > > + } > > +} > > + > > +void ViewFinderGL::resizeGL(int w, int h) > > +{ > > + glViewport(0,0,w,h); > > +} > > + > > +QSize ViewFinderGL::sizeHint() const > > +{ > > + return size_.isValid() ? size_ : QSize(640, 480); > > +} > > I wonder if this sizeHint should go to the base class? > > Does ViewFinderGL use it? I think it's there to provide a required > overload for the QWidget ... if it's common it could go to the base, and > if it's not used, it could get dropped. > > Got it. > > > > > > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h > > new file mode 100644 > > index 0000000..08662ca > > --- /dev/null > > +++ b/src/qcam/viewfinderGL.h > > @@ -0,0 +1,101 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Linaro > > + * > > + * viewfinderGL.h - Render YUV format frame by OpenGL shader > > + */ > > +#ifndef __VIEWFINDERGL_H__ > > Perhaps to make this clearer use this: __VIEWFINDER_GL_H__ ? > An underscore between the ViewFinder and the GL would separate the base, > and the derived names... > Ok, I will fix this. > > > +#define __VIEWFINDERGL_H__ > > + > > +#include <fcntl.h> > > +#include <string.h> > > +#include <unistd.h> > > + > > +#include <iomanip> > > +#include <iostream> > > +#include <sstream> > > + > > +#include <QImage> > > +#include <QOpenGLBuffer> > > +#include <QOpenGLFunctions> > > +#include <QOpenGLShader> > > +#include <QOpenGLShaderProgram> > > +#include <QSize> > > +#include <QOpenGLTexture> > > +#include <QOpenGLWidget> > > + > > +#include <libcamera/buffer.h> > > +#include <libcamera/pixel_format.h> > > +#include "viewfinder.h" > > + > > +class ViewFinderGL : public QOpenGLWidget, > > + public ViewFinderHandler, > > + protected QOpenGLFunctions > > That indentation looks off, I think they should be aligned vertically at > least. > > Sure. > > > > > +{ > > + Q_OBJECT > > + > > +public: > > + ViewFinderGL(QWidget *parent = 0); > > + ~ViewFinderGL(); > > + > > + int setFormat(const libcamera::PixelFormat &format, const > QSize &size); > > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > + void stop(); > > + > > + QImage getCurrentImage(); > > Aha - that's why we need QImage, I think that's part of the save routine > perhaps isn't it ... > > > + > > + void setFrameSize(int width, int height); > > I can't see what calls setFrameSize... is it redundant? Or is it an > override used externally or such. If so I think it needs to be marked as > accordingly as an override? > > > + void updateFrame(unsigned char *buffer); > > Same here... What calls updateFrame()? > > Actually, only NV12, I can verify with Libcamera on rock pi 4, I use an > external application to verify other shaders and above functions are for > that. > I will remove that in next version. > > > > > + > > + char *selectFragmentShader(unsigned int format); > > + void createFragmentShader(); > > + void render(); > > + > > +Q_SIGNALS: > > + void renderComplete(libcamera::FrameBuffer *buffer); > > + > > +protected: > > + void initializeGL() Q_DECL_OVERRIDE; > > + void paintGL() Q_DECL_OVERRIDE; > > + void resizeGL(int w, int h) Q_DECL_OVERRIDE; > > + QSize sizeHint() const override; > > is sizeHint() used? > > > > + > > +private: > > + > > + void configureTexture(unsigned int id); > > + void removeShader(); > > + > > + /* Captured image size, format and buffer */ > > + libcamera::FrameBuffer *buffer_; > > + libcamera::PixelFormat format_; > > + QOpenGLBuffer glBuffer; > > glBuffer_; > > Will fix it. > > > > > > + QSize size_; > > + > > + GLuint id_u; > > + GLuint id_v; > > + GLuint id_y; > > + > > + QMutex mutex_; /* Prevent concurrent access to image_ */ > > + > > All of the member variables below should have a '_' suffix... > > > + QOpenGLShader *pFShader; > > + QOpenGLShader *pVShader; > > + QOpenGLShaderProgram shaderProgram; > > + > > + GLuint textureUniformU; > > + GLuint textureUniformV; > > + GLuint textureUniformY; > > + > > + QOpenGLTexture textureU; > > + QOpenGLTexture textureV; > > + QOpenGLTexture textureY; > > + > > + unsigned int width_; > > + unsigned int height_; > > There is already a QSize size_, do these duplicate that purpose? > > Yeah, I will use size_ instead. > > > > + > > + unsigned char* yuvDataPtr; > > + > > + /* NV parameters */ > > + unsigned int horzSubSample_ ; > > Extra space? > > Ok will fix it. > > > > + unsigned int vertSubSample_; > > +}; > > +#endif // __VIEWFINDERGL_H__ > > > > > > -- > Regards > -- > Kieran > > > Thank you for your comments again. > I will post next version asap. > > -- > Regards > -- > Show >
Hi Show, Thanks for your work. I really like this version! The structure is almost there and much better then previous versions. As Kieran points out there are style errors that checkstyle.py will help you point out so I will ignore them in this review. On 2020-06-24 15:37:05 +0800, Show Liu wrote: > The patch is to render the NV family YUV formats by OpenGL shader. > V3: refine the fragment shader for better pixel color. > > Signed-off-by: Show Liu <show.liu@linaro.org> > --- > src/qcam/main.cpp | 2 + > src/qcam/main_window.cpp | 19 ++- > src/qcam/main_window.h | 3 +- > src/qcam/meson.build | 2 + > src/qcam/shader.h | 104 ++++++++++++ > src/qcam/viewfinder.cpp | 18 +- > src/qcam/viewfinder.h | 23 ++- > src/qcam/viewfinderGL.cpp | 335 ++++++++++++++++++++++++++++++++++++++ > src/qcam/viewfinderGL.h | 101 ++++++++++++ > 9 files changed, 593 insertions(+), 14 deletions(-) > create mode 100644 src/qcam/shader.h > create mode 100644 src/qcam/viewfinderGL.cpp > create mode 100644 src/qcam/viewfinderGL.h > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > index b3468cb..a3ea471 100644 > --- a/src/qcam/main.cpp > +++ b/src/qcam/main.cpp > @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc, char *argv[]) > "help"); > parser.addOption(OptStream, &streamKeyValue, > "Set configuration of a camera stream", "stream", true); > + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats frame via OpenGL shader", > + "opengl"); > > OptionsParser::Options options = parser.parse(argc, argv); > if (options.isSet(OptHelp)) > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 7bc1360..6edf370 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -28,6 +28,9 @@ > #include <libcamera/version.h> > > #include "dng_writer.h" > +#include "main_window.h" > +#include "viewfinder.h" > +#include "viewfinderGL.h" > > using namespace libcamera; > > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) > setWindowTitle(title_); > connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle())); > > - viewfinder_ = new ViewFinder(this); > - connect(viewfinder_, &ViewFinder::renderComplete, > - this, &MainWindow::queueRequest); > - setCentralWidget(viewfinder_); > + if (options_.isSet(OptOpenGL)) { > + viewfinder_ = new ViewFinderGL(this); > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_), &ViewFinderGL::renderComplete, > + this, &MainWindow::queueRequest); > + setCentralWidget(dynamic_cast<ViewFinderGL *>(viewfinder_)); > + } else { > + viewfinder_ = new ViewFinder(this); > + connect(dynamic_cast<ViewFinder *>(viewfinder_), &ViewFinder::renderComplete, > + this, &MainWindow::queueRequest); > + setCentralWidget(dynamic_cast<ViewFinder *>(viewfinder_)); > + } I understand that one can't inherit from QObject twice, but this looks odd. Is there someway the base class could inherit from QObject or possibly QWidget and the derived classes hide their specilization somehow? I'm no Qt export so maybe I'm asking for something impossible or really stupid. > + > adjustSize(); > > /* Hotplug/unplug support */ > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > index 4606fe4..a852ef4 100644 > --- a/src/qcam/main_window.h > +++ b/src/qcam/main_window.h > @@ -38,6 +38,7 @@ enum { > OptCamera = 'c', > OptHelp = 'h', > OptStream = 's', > + OptOpenGL = 'o', > }; > > class CaptureRequest > @@ -102,7 +103,7 @@ private: > QAction *startStopAction_; > QComboBox *cameraCombo_; > QAction *saveRaw_; > - ViewFinder *viewfinder_; > + ViewFinderHandler *viewfinder_; > > QIcon iconPlay_; > QIcon iconStop_; > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > index 045db52..6a58947 100644 > --- a/src/qcam/meson.build > +++ b/src/qcam/meson.build > @@ -7,11 +7,13 @@ qcam_sources = files([ > 'main.cpp', > 'main_window.cpp', > 'viewfinder.cpp', > + 'viewfinderGL.cpp' > ]) > > qcam_moc_headers = files([ > 'main_window.h', > 'viewfinder.h', > + 'viewfinderGL.h' > ]) > > qcam_resources = files([ > diff --git a/src/qcam/shader.h b/src/qcam/shader.h > new file mode 100644 > index 0000000..f54c264 > --- /dev/null > +++ b/src/qcam/shader.h > @@ -0,0 +1,104 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2020, Linaro > + * > + * shader.h - YUV format converter shader source code > + */ > +#ifndef __SHADER_H__ > +#define __SHADER_H__ I think the content of this file should be moved inside viewfinderGL.cpp > + > +/* Vertex shader for NV formats */ > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" could this (and bellow) be made static const ? > + "attribute vec2 textureIn;\n" > + "varying vec2 textureOut;\n" > + "void main(void)\n" > + "{\n" > + "gl_Position = vertexIn;\n" > + "textureOut = textureIn;\n" > + "}\n"; > + > +/* Fragment shader for NV12, NV16 and NV24 */ > +char NV_2_planes_UV[] = "#ifdef GL_ES\n" > + "precision highp float;\n" > + "#endif\n" > + "varying vec2 textureOut;\n" > + "uniform sampler2D tex_y;\n" > + "uniform sampler2D tex_u;\n" > + "void main(void)\n" > + "{\n" > + "vec3 yuv;\n" > + "vec3 rgb;\n" > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > + " vec3(0.0, -0.390625, 2.015625),\n" > + " vec3(1.5975625, -0.8125, 0.0));\n" > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n" > + "rgb = convert_mat * yuv;\n" > + "gl_FragColor = vec4(rgb, 1.0);\n" > + "}\n"; > + > +/* Fragment shader for NV21, NV61 and NV42 */ > +char NV_2_planes_VU[] = "#ifdef GL_ES\n" > + "precision highp float;\n" > + "#endif\n" > + "varying vec2 textureOut;\n" > + "uniform sampler2D tex_y;\n" > + "uniform sampler2D tex_u;\n" > + "void main(void)\n" > + "{\n" > + "vec3 yuv;\n" > + "vec3 rgb;\n" > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > + " vec3(0.0, -0.390625, 2.015625),\n" > + " vec3(1.5975625, -0.8125, 0.0));\n" > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n" > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > + "rgb = convert_mat * yuv;\n" > + "gl_FragColor = vec4(rgb, 1.0);\n" > + "}\n"; > + > +/* Fragment shader for YUV420 */ > +char NV_3_planes_UV[] = "#ifdef GL_ES\n" > + "precision highp float;\n" > + "#endif\n" > + "varying vec2 textureOut;\n" > + "uniform sampler2D tex_y;\n" > + "uniform sampler2D tex_u;\n" > + "uniform sampler2D tex_v;\n" > + "void main(void)\n" > + "{\n" > + "vec3 yuv;\n" > + "vec3 rgb;\n" > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > + " vec3(0.0, -0.390625, 2.015625),\n" > + " vec3(1.5975625, -0.8125, 0.0));\n" > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n" > + "rgb = convert_mat * yuv;\n" > + "gl_FragColor = vec4(rgb, 1);\n" > + "}\n"; > + > +/* Fragment shader for YVU420 */ > +char NV_3_planes_VU[] = "precision highp float;\n" > + "#endif\n" > + "varying vec2 textureOut;\n" > + "uniform sampler2D tex_y;\n" > + "uniform sampler2D tex_u;\n" > + "uniform sampler2D tex_v;\n" > + "void main(void)\n" > + "{\n" > + "vec3 yuv;\n" > + "vec3 rgb;\n" > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > + " vec3(0.0, -0.390625, 2.015625),\n" > + " vec3(1.5975625, -0.8125, 0.0));\n" > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n" > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > + "rgb = convert_mat * yuv;\n" > + "gl_FragColor = vec4(rgb, 1);\n" > + "}\n"; > +#endif // __SHADER_H__ > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp > index dcf8a15..d3a2422 100644 > --- a/src/qcam/viewfinder.cpp > +++ b/src/qcam/viewfinder.cpp > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats > { libcamera::formats::BGR888, QImage::Format_RGB888 }, > }; > > -ViewFinder::ViewFinder(QWidget *parent) > - : QWidget(parent), buffer_(nullptr) > +ViewFinderHandler::ViewFinderHandler() > { > - icon_ = QIcon(":camera-off.svg"); > } > > -ViewFinder::~ViewFinder() > +ViewFinderHandler::~ViewFinderHandler() > { > } > > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() const > +const QList<libcamera::PixelFormat> &ViewFinderHandler::nativeFormats() const > { > static const QList<libcamera::PixelFormat> formats = ::nativeFormats.keys(); > return formats; > } > > +ViewFinder::ViewFinder(QWidget *parent) > + : QWidget(parent), buffer_(nullptr) > +{ > + icon_ = QIcon(":camera-off.svg"); > +} I agree with Kieran here I think the base class should be named ViewFinder and the two derived classes ViewFinderQt and ViewFinderGL or something similar. I also think you should move the base class to its own .h file (and .cpp file if needed). > + > +ViewFinder::~ViewFinder() > +{ > +} > + > int ViewFinder::setFormat(const libcamera::PixelFormat &format, > const QSize &size) > { > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > index 26a1320..741d694 100644 > --- a/src/qcam/viewfinder.h > +++ b/src/qcam/viewfinder.h > @@ -13,6 +13,8 @@ > #include <QList> > #include <QImage> > #include <QMutex> > +#include <QOpenGLWidget> > +#include <QOpenGLFunctions> > #include <QSize> > #include <QWidget> > > @@ -28,7 +30,23 @@ struct MappedBuffer { > size_t size; > }; > > -class ViewFinder : public QWidget > +class ViewFinderHandler > +{ > +public: > + ViewFinderHandler(); > + virtual ~ViewFinderHandler(); > + > + const QList<libcamera::PixelFormat> &nativeFormats() const; > + > + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) =0; > + virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) =0; > + virtual void stop() =0; > + > + virtual QImage getCurrentImage() =0; > + > +}; > + > +class ViewFinder : public QWidget, public ViewFinderHandler > { > Q_OBJECT > > @@ -36,8 +54,6 @@ public: > ViewFinder(QWidget *parent); > ~ViewFinder(); > > - const QList<libcamera::PixelFormat> &nativeFormats() const; > - > int setFormat(const libcamera::PixelFormat &format, const QSize &size); > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > void stop(); > @@ -67,5 +83,4 @@ private: > QImage image_; > QMutex mutex_; /* Prevent concurrent access to image_ */ > }; > - > #endif /* __QCAM_VIEWFINDER__ */ > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp > new file mode 100644 > index 0000000..7b47beb > --- /dev/null > +++ b/src/qcam/viewfinderGL.cpp > @@ -0,0 +1,335 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2020, Linaro > + * > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader > + */ > + > +#include "shader.h" > +#include "viewfinderGL.h" > + > +#include <QImage> > + > +#include <libcamera/formats.h> > + > +#define ATTRIB_VERTEX 0 > +#define ATTRIB_TEXTURE 1 > + > +ViewFinderGL::ViewFinderGL(QWidget *parent) > + : QOpenGLWidget(parent), > + glBuffer(QOpenGLBuffer::VertexBuffer), > + pFShader(nullptr), > + pVShader(nullptr), > + textureU(QOpenGLTexture::Target2D), > + textureV(QOpenGLTexture::Target2D), > + textureY(QOpenGLTexture::Target2D), > + yuvDataPtr(nullptr) > + > +{ > +} > + > +ViewFinderGL::~ViewFinderGL() > +{ > + removeShader(); > + > + if(textureY.isCreated()) > + textureY.destroy(); > + > + if(textureU.isCreated()) > + textureU.destroy(); > + > + if(textureV.isCreated()) > + textureV.destroy(); > + > + glBuffer.destroy(); > +} > + > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > + const QSize &size) > +{ > + format_ = format; > + size_ = size; > + > + updateGeometry(); > + return 0; > +} > + > +void ViewFinderGL::stop() > +{ > + if (buffer_) { > + renderComplete(buffer_); > + buffer_ = nullptr; > + } > +} > + > +QImage ViewFinderGL::getCurrentImage() > +{ > + QMutexLocker locker(&mutex_); > + > + return(grabFramebuffer()); > +} > + > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map) > +{ > + if (buffer->planes().size() != 1) { > + qWarning() << "Multi-planar buffers are not supported"; > + return; > + } > + > + unsigned char *memory = static_cast<unsigned char *>(map->memory); > + if(memory) { > + yuvDataPtr = memory; > + update(); > + buffer_ = buffer; > + } > + > + if (buffer) > + renderComplete(buffer); > +} > + > +void ViewFinderGL::updateFrame(unsigned char *buffer) > +{ > + if(buffer) { > + yuvDataPtr = buffer; > + update(); > + } > +} > + > +void ViewFinderGL::setFrameSize(int width, int height) > +{ > + if(width > 0 && height > 0) { > + width_ = width; > + height_ = height; > + } > +} > + > +char *ViewFinderGL::selectFragmentShader(unsigned int format) > +{ > + char *fsrc = nullptr; > + > + switch(format) { > + case libcamera::formats::NV12: > + horzSubSample_ = 2; > + vertSubSample_ = 2; > + fsrc = NV_2_planes_UV; > + break; > + case libcamera::formats::NV21: > + horzSubSample_ = 2; > + vertSubSample_ = 2; > + fsrc = NV_2_planes_VU; > + break; > + case libcamera::formats::NV16: > + horzSubSample_ = 2; > + vertSubSample_ = 1; > + fsrc = NV_2_planes_UV; > + break; > + case libcamera::formats::NV61: > + horzSubSample_ = 2; > + vertSubSample_ = 1; > + fsrc = NV_2_planes_VU; > + break; > + case libcamera::formats::NV24: > + horzSubSample_ = 1; > + vertSubSample_ = 1; > + fsrc = NV_2_planes_UV; > + break; > + case libcamera::formats::NV42: > + horzSubSample_ = 1; > + vertSubSample_ = 1; > + fsrc = NV_2_planes_VU; > + break; > + case libcamera::formats::YUV420: > + horzSubSample_ = 2; > + vertSubSample_ = 2; > + fsrc = NV_3_planes_UV; > + break; > + default: > + break; > + }; > + > + if(fsrc == nullptr) { > + qDebug() << __func__ << "[ERROR]:" <<" No suitable fragment shader can be select."; > + } > + return fsrc; > +} > + > +void ViewFinderGL::createFragmentShader() > +{ > + bool bCompile; > + > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this); > + char *fsrc = selectFragmentShader(format_); > + > + bCompile = pFShader->compileSourceCode(fsrc); > + if(!bCompile) > + { > + qDebug() << __func__ << ":" << pFShader->log(); > + } > + > + shaderProgram.addShader(pFShader); > + > + // Link shader pipeline > + if (!shaderProgram.link()) { > + qDebug() << __func__ << ": shader program link failed.\n" << shaderProgram.log(); > + close(); > + } > + > + // Bind shader pipeline for use > + if (!shaderProgram.bind()) { > + qDebug() << __func__ << ": shader program binding failed.\n" << shaderProgram.log(); > + close(); > + } > + > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX); > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE); > + > + shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat)); > + shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat)); > + > + textureUniformY = shaderProgram.uniformLocation("tex_y"); > + textureUniformU = shaderProgram.uniformLocation("tex_u"); > + textureUniformV = shaderProgram.uniformLocation("tex_v"); > + > + if(!textureY.isCreated()) > + textureY.create(); > + > + if(!textureU.isCreated()) > + textureU.create(); > + > + if(!textureV.isCreated()) > + textureV.create(); > + > + id_y = textureY.textureId(); > + id_u = textureU.textureId(); > + id_v = textureV.textureId(); > +} > + > +void ViewFinderGL::configureTexture(unsigned int id) > +{ > + glBindTexture(GL_TEXTURE_2D, id); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); > +} > + > +void ViewFinderGL::removeShader() > +{ > + if (shaderProgram.isLinked()) { > + shaderProgram.release(); > + shaderProgram.removeAllShaders(); > + } > + > + if(pFShader) > + delete pFShader; > + > + if(pVShader) > + delete pVShader; > +} > + > +void ViewFinderGL::initializeGL() > +{ > + bool bCompile; > + > + initializeOpenGLFunctions(); > + glEnable(GL_TEXTURE_2D); > + glDisable(GL_DEPTH_TEST); > + > + static const GLfloat vertices[] { > + -1.0f,-1.0f, > + -1.0f,+1.0f, > + +1.0f,+1.0f, > + +1.0f,-1.0f, > + 0.0f,1.0f, > + 0.0f,0.0f, > + 1.0f,0.0f, > + 1.0f,1.0f, > + }; > + > + glBuffer.create(); > + glBuffer.bind(); > + glBuffer.allocate(vertices,sizeof(vertices)); > + > + /* Create Vertex Shader */ > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this); > + char *vsrc = NV_Vertex_shader; > + > + bCompile = pVShader->compileSourceCode(vsrc); > + if(!bCompile) { > + qDebug() << __func__<< ":" << pVShader->log(); > + } > + > + shaderProgram.addShader(pVShader); > + > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f); > +} > + > +void ViewFinderGL::render() > +{ > + switch(format_) { > + case libcamera::formats::NV12: > + case libcamera::formats::NV21: > + case libcamera::formats::NV16: > + case libcamera::formats::NV61: > + case libcamera::formats::NV24: > + case libcamera::formats::NV42: > + /* activate texture 0 */ > + glActiveTexture(GL_TEXTURE0); > + configureTexture(id_y); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > + glUniform1i(textureUniformY, 0); > + > + /* activate texture 1 */ > + glActiveTexture(GL_TEXTURE1); > + configureTexture(id_u); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > + glUniform1i(textureUniformU, 1); > + break; > + case libcamera::formats::YUV420: > + /* activate texture 0 */ > + glActiveTexture(GL_TEXTURE0); > + configureTexture(id_y); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > + glUniform1i(textureUniformY, 0); > + > + /* activate texture 1 */ > + glActiveTexture(GL_TEXTURE1); > + configureTexture(id_u); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > + glUniform1i(textureUniformU, 1); > + > + /* activate texture 2 */ > + glActiveTexture(GL_TEXTURE2); > + configureTexture(id_v); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()*5/4); > + glUniform1i(textureUniformV, 2); > + break; > + default: > + break; > + }; > +} > + > +void ViewFinderGL::paintGL() > +{ > + if(pFShader == nullptr) > + createFragmentShader(); > + > + if(yuvDataPtr) > + { > + glClearColor(0.0, 0.0, 0.0, 1.0); > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT ); > + > + render(); > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); > + } > +} > + > +void ViewFinderGL::resizeGL(int w, int h) > +{ > + glViewport(0,0,w,h); > +} > + > +QSize ViewFinderGL::sizeHint() const > +{ > + return size_.isValid() ? size_ : QSize(640, 480); > +} > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h > new file mode 100644 > index 0000000..08662ca > --- /dev/null > +++ b/src/qcam/viewfinderGL.h > @@ -0,0 +1,101 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2020, Linaro > + * > + * viewfinderGL.h - Render YUV format frame by OpenGL shader > + */ > +#ifndef __VIEWFINDERGL_H__ > +#define __VIEWFINDERGL_H__ > + > +#include <fcntl.h> > +#include <string.h> > +#include <unistd.h> > + > +#include <iomanip> > +#include <iostream> > +#include <sstream> > + > +#include <QImage> > +#include <QOpenGLBuffer> > +#include <QOpenGLFunctions> > +#include <QOpenGLShader> > +#include <QOpenGLShaderProgram> > +#include <QSize> > +#include <QOpenGLTexture> > +#include <QOpenGLWidget> > + > +#include <libcamera/buffer.h> > +#include <libcamera/pixel_format.h> > +#include "viewfinder.h" > + > +class ViewFinderGL : public QOpenGLWidget, > + public ViewFinderHandler, > + protected QOpenGLFunctions > +{ > + Q_OBJECT > + > +public: > + ViewFinderGL(QWidget *parent = 0); > + ~ViewFinderGL(); > + > + int setFormat(const libcamera::PixelFormat &format, const QSize &size); > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > + void stop(); > + > + QImage getCurrentImage(); > + > + void setFrameSize(int width, int height); > + void updateFrame(unsigned char *buffer); > + > + char *selectFragmentShader(unsigned int format); > + void createFragmentShader(); > + void render(); > + > +Q_SIGNALS: > + void renderComplete(libcamera::FrameBuffer *buffer); > + > +protected: > + void initializeGL() Q_DECL_OVERRIDE; > + void paintGL() Q_DECL_OVERRIDE; > + void resizeGL(int w, int h) Q_DECL_OVERRIDE; > + QSize sizeHint() const override; > + > +private: > + > + void configureTexture(unsigned int id); > + void removeShader(); > + > + /* Captured image size, format and buffer */ > + libcamera::FrameBuffer *buffer_; > + libcamera::PixelFormat format_; > + QOpenGLBuffer glBuffer; > + QSize size_; > + > + GLuint id_u; > + GLuint id_v; > + GLuint id_y; > + > + QMutex mutex_; /* Prevent concurrent access to image_ */ > + > + QOpenGLShader *pFShader; > + QOpenGLShader *pVShader; > + QOpenGLShaderProgram shaderProgram; > + > + GLuint textureUniformU; > + GLuint textureUniformV; > + GLuint textureUniformY; > + > + QOpenGLTexture textureU; > + QOpenGLTexture textureV; > + QOpenGLTexture textureY; > + > + unsigned int width_; > + unsigned int height_; > + > + unsigned char* yuvDataPtr; > + > + /* NV parameters */ > + unsigned int horzSubSample_ ; > + unsigned int vertSubSample_; > +}; > +#endif // __VIEWFINDERGL_H__ > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Tue, Jun 30, 2020 at 05:03:43PM +0200, Niklas Söderlund wrote: > Hi Show, > > Thanks for your work. Likewise :-) > I really like this version! The structure is almost there and much > better then previous versions. As Kieran points out there are style > errors that checkstyle.py will help you point out so I will ignore them > in this review. > > On 2020-06-24 15:37:05 +0800, Show Liu wrote: > > The patch is to render the NV family YUV formats by OpenGL shader. > > V3: refine the fragment shader for better pixel color. > > > > Signed-off-by: Show Liu <show.liu@linaro.org> > > --- > > src/qcam/main.cpp | 2 + > > src/qcam/main_window.cpp | 19 ++- > > src/qcam/main_window.h | 3 +- > > src/qcam/meson.build | 2 + > > src/qcam/shader.h | 104 ++++++++++++ > > src/qcam/viewfinder.cpp | 18 +- > > src/qcam/viewfinder.h | 23 ++- > > src/qcam/viewfinderGL.cpp | 335 ++++++++++++++++++++++++++++++++++++++ > > src/qcam/viewfinderGL.h | 101 ++++++++++++ > > 9 files changed, 593 insertions(+), 14 deletions(-) > > create mode 100644 src/qcam/shader.h > > create mode 100644 src/qcam/viewfinderGL.cpp > > create mode 100644 src/qcam/viewfinderGL.h > > > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > > index b3468cb..a3ea471 100644 > > --- a/src/qcam/main.cpp > > +++ b/src/qcam/main.cpp > > @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc, char *argv[]) > > "help"); > > parser.addOption(OptStream, &streamKeyValue, > > "Set configuration of a camera stream", "stream", true); > > + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats frame via OpenGL shader", > > + "opengl"); Should we default to OpenGL when possible, and add an option to force a particular backend ? Maybe -r/--render={gles,qt} > > > > OptionsParser::Options options = parser.parse(argc, argv); > > if (options.isSet(OptHelp)) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 7bc1360..6edf370 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -28,6 +28,9 @@ > > #include <libcamera/version.h> > > > > #include "dng_writer.h" > > +#include "main_window.h" > > +#include "viewfinder.h" > > +#include "viewfinderGL.h" According to the name scheme we use, I think this should be viewfinder_gl.h. > > > > using namespace libcamera; > > > > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) > > setWindowTitle(title_); > > connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle())); > > > > - viewfinder_ = new ViewFinder(this); > > - connect(viewfinder_, &ViewFinder::renderComplete, > > - this, &MainWindow::queueRequest); > > - setCentralWidget(viewfinder_); > > + if (options_.isSet(OptOpenGL)) { > > + viewfinder_ = new ViewFinderGL(this); > > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_), &ViewFinderGL::renderComplete, > > + this, &MainWindow::queueRequest); > > + setCentralWidget(dynamic_cast<ViewFinderGL *>(viewfinder_)); > > + } else { > > + viewfinder_ = new ViewFinder(this); > > + connect(dynamic_cast<ViewFinder *>(viewfinder_), &ViewFinder::renderComplete, > > + this, &MainWindow::queueRequest); > > + setCentralWidget(dynamic_cast<ViewFinder *>(viewfinder_)); > > + } > > I understand that one can't inherit from QObject twice, but this looks > odd. Is there someway the base class could inherit from QObject or > possibly QWidget and the derived classes hide their specilization > somehow? I'm no Qt export so maybe I'm asking for something impossible > or really stupid. If we assume all subclasses of Viewfinder will be QWidget, we could do if (options_.isSet(OptOpenGL)) viewfinder_ = new ViewFinderGL(this); else viewfinder_ = new ViewFinder(this); QWidget *vf>idget = dynamic_cast<QWidget>(viewfinder_); connect(vfWidget, &ViewFinderHandler::renderComplete, this, &MainWindow::queueRequest); setCentralWidget(vfWidget); With ViewFinderHandler::renderComplete() being a signal in the base class. > > + > > adjustSize(); > > > > /* Hotplug/unplug support */ > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > index 4606fe4..a852ef4 100644 > > --- a/src/qcam/main_window.h > > +++ b/src/qcam/main_window.h > > @@ -38,6 +38,7 @@ enum { > > OptCamera = 'c', > > OptHelp = 'h', > > OptStream = 's', > > + OptOpenGL = 'o', > > }; > > > > class CaptureRequest > > @@ -102,7 +103,7 @@ private: > > QAction *startStopAction_; > > QComboBox *cameraCombo_; > > QAction *saveRaw_; > > - ViewFinder *viewfinder_; > > + ViewFinderHandler *viewfinder_; I'd split this patch in two, with one patch that renames the existing ViewFinder to ViewFinderQt and creates a ViewFinder base class (which you call ViewFinderHandler here), and a second patch that adds ViewFinderGL. > > > > QIcon iconPlay_; > > QIcon iconStop_; > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > > index 045db52..6a58947 100644 > > --- a/src/qcam/meson.build > > +++ b/src/qcam/meson.build > > @@ -7,11 +7,13 @@ qcam_sources = files([ > > 'main.cpp', > > 'main_window.cpp', > > 'viewfinder.cpp', > > + 'viewfinderGL.cpp' > > ]) > > > > qcam_moc_headers = files([ > > 'main_window.h', > > 'viewfinder.h', > > + 'viewfinderGL.h' > > ]) > > > > qcam_resources = files([ > > diff --git a/src/qcam/shader.h b/src/qcam/shader.h > > new file mode 100644 > > index 0000000..f54c264 > > --- /dev/null > > +++ b/src/qcam/shader.h > > @@ -0,0 +1,104 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Linaro > > + * > > + * shader.h - YUV format converter shader source code > > + */ > > +#ifndef __SHADER_H__ > > +#define __SHADER_H__ > > I think the content of this file should be moved inside viewfinderGL.cpp Or maybe to viewfinder_gl_shader.cpp if we want to keep it separate. Header files should only contain the declarations in any case, not the actual variable contents. Ideally we should store the shaders in files of their own, not in a C array, and have them pulled in as Qt resources (https://doc.qt.io/qt-5/resources.html#using-resources-in-the-application). They can be read through a QFile. It's something we can do on top of this patch. > > + > > +/* Vertex shader for NV formats */ > > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" > > could this (and bellow) be made static const ? > > > + "attribute vec2 textureIn;\n" > > + "varying vec2 textureOut;\n" > > + "void main(void)\n" > > + "{\n" > > + "gl_Position = vertexIn;\n" > > + "textureOut = textureIn;\n" > > + "}\n"; > > + > > +/* Fragment shader for NV12, NV16 and NV24 */ > > +char NV_2_planes_UV[] = "#ifdef GL_ES\n" > > + "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > > + " vec3(0.0, -0.390625, 2.015625),\n" > > + " vec3(1.5975625, -0.8125, 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > + "}\n"; > > + > > +/* Fragment shader for NV21, NV61 and NV42 */ > > +char NV_2_planes_VU[] = "#ifdef GL_ES\n" > > + "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > > + " vec3(0.0, -0.390625, 2.015625),\n" > > + " vec3(1.5975625, -0.8125, 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n" > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > + "}\n"; > > + > > +/* Fragment shader for YUV420 */ > > +char NV_3_planes_UV[] = "#ifdef GL_ES\n" > > + "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "uniform sampler2D tex_v;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > > + " vec3(0.0, -0.390625, 2.015625),\n" > > + " vec3(1.5975625, -0.8125, 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1);\n" > > + "}\n"; > > + > > +/* Fragment shader for YVU420 */ > > +char NV_3_planes_VU[] = "precision highp float;\n" > > + "#endif\n" > > + "varying vec2 textureOut;\n" > > + "uniform sampler2D tex_y;\n" > > + "uniform sampler2D tex_u;\n" > > + "uniform sampler2D tex_v;\n" > > + "void main(void)\n" > > + "{\n" > > + "vec3 yuv;\n" > > + "vec3 rgb;\n" > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > > + " vec3(0.0, -0.390625, 2.015625),\n" > > + " vec3(1.5975625, -0.8125, 0.0));\n" > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n" > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > + "rgb = convert_mat * yuv;\n" > > + "gl_FragColor = vec4(rgb, 1);\n" > > + "}\n"; > > +#endif // __SHADER_H__ > > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp > > index dcf8a15..d3a2422 100644 > > --- a/src/qcam/viewfinder.cpp > > +++ b/src/qcam/viewfinder.cpp > > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats > > { libcamera::formats::BGR888, QImage::Format_RGB888 }, > > }; > > > > -ViewFinder::ViewFinder(QWidget *parent) > > - : QWidget(parent), buffer_(nullptr) > > +ViewFinderHandler::ViewFinderHandler() > > { > > - icon_ = QIcon(":camera-off.svg"); > > } You don't need an empty constructor in the base class, you can just drop it. > > > > -ViewFinder::~ViewFinder() > > +ViewFinderHandler::~ViewFinderHandler() > > { > > } > > > > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() const > > +const QList<libcamera::PixelFormat> &ViewFinderHandler::nativeFormats() const > > { > > static const QList<libcamera::PixelFormat> formats = ::nativeFormats.keys(); > > return formats; > > } I expect native formats to be different for ViewFinderQt and ViewFinderGL, so I'd make this a pure virtual function with implementations in the derived classes. ViewFinderGL should report all the formats for which you have conversion shaders. > > > > +ViewFinder::ViewFinder(QWidget *parent) > > + : QWidget(parent), buffer_(nullptr) > > +{ > > + icon_ = QIcon(":camera-off.svg"); > > +} > > I agree with Kieran here I think the base class should be named > ViewFinder and the two derived classes ViewFinderQt and ViewFinderGL or > something similar. Seems we all agree then :-) > I also think you should move the base class to its own .h file (and .cpp > file if needed). Agreed. > > + > > +ViewFinder::~ViewFinder() > > +{ > > +} > > + > > int ViewFinder::setFormat(const libcamera::PixelFormat &format, > > const QSize &size) > > { > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > > index 26a1320..741d694 100644 > > --- a/src/qcam/viewfinder.h > > +++ b/src/qcam/viewfinder.h > > @@ -13,6 +13,8 @@ > > #include <QList> > > #include <QImage> > > #include <QMutex> > > +#include <QOpenGLWidget> > > +#include <QOpenGLFunctions> > > #include <QSize> > > #include <QWidget> > > > > @@ -28,7 +30,23 @@ struct MappedBuffer { > > size_t size; > > }; > > > > -class ViewFinder : public QWidget > > +class ViewFinderHandler > > +{ > > +public: > > + ViewFinderHandler(); > > + virtual ~ViewFinderHandler(); > > + > > + const QList<libcamera::PixelFormat> &nativeFormats() const; > > + > > + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) =0; > > + virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) =0; > > + virtual void stop() =0; > > + > > + virtual QImage getCurrentImage() =0; > > + > > +}; > > + > > +class ViewFinder : public QWidget, public ViewFinderHandler > > { > > Q_OBJECT > > > > @@ -36,8 +54,6 @@ public: > > ViewFinder(QWidget *parent); > > ~ViewFinder(); > > > > - const QList<libcamera::PixelFormat> &nativeFormats() const; > > - > > int setFormat(const libcamera::PixelFormat &format, const QSize &size); > > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > void stop(); > > @@ -67,5 +83,4 @@ private: > > QImage image_; > > QMutex mutex_; /* Prevent concurrent access to image_ */ > > }; > > - This blank line should be kept. > > #endif /* __QCAM_VIEWFINDER__ */ > > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp > > new file mode 100644 > > index 0000000..7b47beb > > --- /dev/null > > +++ b/src/qcam/viewfinderGL.cpp > > @@ -0,0 +1,335 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Linaro > > + * > > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader > > + */ > > + > > +#include "shader.h" > > +#include "viewfinderGL.h" > > + > > +#include <QImage> > > + > > +#include <libcamera/formats.h> > > + > > +#define ATTRIB_VERTEX 0 > > +#define ATTRIB_TEXTURE 1 > > + > > +ViewFinderGL::ViewFinderGL(QWidget *parent) > > + : QOpenGLWidget(parent), > > + glBuffer(QOpenGLBuffer::VertexBuffer), > > + pFShader(nullptr), > > + pVShader(nullptr), > > + textureU(QOpenGLTexture::Target2D), > > + textureV(QOpenGLTexture::Target2D), > > + textureY(QOpenGLTexture::Target2D), > > + yuvDataPtr(nullptr) > > + > > +{ > > +} > > + > > +ViewFinderGL::~ViewFinderGL() > > +{ > > + removeShader(); > > + > > + if(textureY.isCreated()) > > + textureY.destroy(); > > + > > + if(textureU.isCreated()) > > + textureU.destroy(); > > + > > + if(textureV.isCreated()) > > + textureV.destroy(); > > + > > + glBuffer.destroy(); > > +} > > + > > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > > + const QSize &size) > > +{ > > + format_ = format; > > + size_ = size; > > + > > + updateGeometry(); > > + return 0; > > +} > > + > > +void ViewFinderGL::stop() > > +{ > > + if (buffer_) { > > + renderComplete(buffer_); > > + buffer_ = nullptr; > > + } > > +} > > + > > +QImage ViewFinderGL::getCurrentImage() > > +{ > > + QMutexLocker locker(&mutex_); > > + > > + return(grabFramebuffer()); > > +} > > + > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map) > > +{ > > + if (buffer->planes().size() != 1) { > > + qWarning() << "Multi-planar buffers are not supported"; > > + return; > > + } > > + > > + unsigned char *memory = static_cast<unsigned char *>(map->memory); > > + if(memory) { Can memory be null here ? > > + yuvDataPtr = memory; > > + update(); > > + buffer_ = buffer; > > + } > > + > > + if (buffer) > > + renderComplete(buffer); Here's you're supposed to signal completion of the previous buffer, not the current buffer. That's why there's a std::swap() in the existing ViewFinder::render(). > > +} > > + > > +void ViewFinderGL::updateFrame(unsigned char *buffer) > > +{ > > + if(buffer) { > > + yuvDataPtr = buffer; > > + update(); > > + } > > +} > > + > > +void ViewFinderGL::setFrameSize(int width, int height) > > +{ > > + if(width > 0 && height > 0) { > > + width_ = width; > > + height_ = height; > > + } > > +} > > + > > +char *ViewFinderGL::selectFragmentShader(unsigned int format) > > +{ > > + char *fsrc = nullptr; > > + > > + switch(format) { > > + case libcamera::formats::NV12: > > + horzSubSample_ = 2; > > + vertSubSample_ = 2; > > + fsrc = NV_2_planes_UV; > > + break; > > + case libcamera::formats::NV21: > > + horzSubSample_ = 2; > > + vertSubSample_ = 2; > > + fsrc = NV_2_planes_VU; > > + break; > > + case libcamera::formats::NV16: > > + horzSubSample_ = 2; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_UV; > > + break; > > + case libcamera::formats::NV61: > > + horzSubSample_ = 2; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_VU; > > + break; > > + case libcamera::formats::NV24: > > + horzSubSample_ = 1; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_UV; > > + break; > > + case libcamera::formats::NV42: > > + horzSubSample_ = 1; > > + vertSubSample_ = 1; > > + fsrc = NV_2_planes_VU; > > + break; > > + case libcamera::formats::YUV420: > > + horzSubSample_ = 2; > > + vertSubSample_ = 2; > > + fsrc = NV_3_planes_UV; > > + break; > > + default: > > + break; > > + }; > > + > > + if(fsrc == nullptr) { > > + qDebug() << __func__ << "[ERROR]:" <<" No suitable fragment shader can be select."; > > + } > > + return fsrc; > > +} > > + > > +void ViewFinderGL::createFragmentShader() > > +{ > > + bool bCompile; > > + > > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this); > > + char *fsrc = selectFragmentShader(format_); > > + > > + bCompile = pFShader->compileSourceCode(fsrc); > > + if(!bCompile) > > + { > > + qDebug() << __func__ << ":" << pFShader->log(); > > + } > > + > > + shaderProgram.addShader(pFShader); > > + > > + // Link shader pipeline > > + if (!shaderProgram.link()) { > > + qDebug() << __func__ << ": shader program link failed.\n" << shaderProgram.log(); > > + close(); > > + } > > + > > + // Bind shader pipeline for use > > + if (!shaderProgram.bind()) { > > + qDebug() << __func__ << ": shader program binding failed.\n" << shaderProgram.log(); > > + close(); > > + } > > + > > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX); > > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE); > > + > > + shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat)); > > + shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat)); > > + > > + textureUniformY = shaderProgram.uniformLocation("tex_y"); > > + textureUniformU = shaderProgram.uniformLocation("tex_u"); > > + textureUniformV = shaderProgram.uniformLocation("tex_v"); > > + > > + if(!textureY.isCreated()) > > + textureY.create(); > > + > > + if(!textureU.isCreated()) > > + textureU.create(); > > + > > + if(!textureV.isCreated()) > > + textureV.create(); > > + > > + id_y = textureY.textureId(); > > + id_u = textureU.textureId(); > > + id_v = textureV.textureId(); > > +} > > + > > +void ViewFinderGL::configureTexture(unsigned int id) > > +{ > > + glBindTexture(GL_TEXTURE_2D, id); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); > > +} > > + > > +void ViewFinderGL::removeShader() > > +{ > > + if (shaderProgram.isLinked()) { > > + shaderProgram.release(); > > + shaderProgram.removeAllShaders(); > > + } > > + > > + if(pFShader) > > + delete pFShader; > > + > > + if(pVShader) > > + delete pVShader; If we stop and restart the stream with a different format, the previous shader will be used, as removeShader() isn't called. I don't think that's right. I believe you need to call removeShader() at the beginning of setFormat(). > > +} > > + > > +void ViewFinderGL::initializeGL() > > +{ > > + bool bCompile; > > + > > + initializeOpenGLFunctions(); > > + glEnable(GL_TEXTURE_2D); > > + glDisable(GL_DEPTH_TEST); > > + > > + static const GLfloat vertices[] { > > + -1.0f,-1.0f, > > + -1.0f,+1.0f, > > + +1.0f,+1.0f, > > + +1.0f,-1.0f, > > + 0.0f,1.0f, > > + 0.0f,0.0f, > > + 1.0f,0.0f, > > + 1.0f,1.0f, > > + }; > > + > > + glBuffer.create(); > > + glBuffer.bind(); > > + glBuffer.allocate(vertices,sizeof(vertices)); > > + > > + /* Create Vertex Shader */ > > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this); > > + char *vsrc = NV_Vertex_shader; > > + > > + bCompile = pVShader->compileSourceCode(vsrc); > > + if(!bCompile) { > > + qDebug() << __func__<< ":" << pVShader->log(); > > + } > > + > > + shaderProgram.addShader(pVShader); > > + > > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f); > > +} > > + > > +void ViewFinderGL::render() > > +{ > > + switch(format_) { > > + case libcamera::formats::NV12: > > + case libcamera::formats::NV21: > > + case libcamera::formats::NV16: > > + case libcamera::formats::NV61: > > + case libcamera::formats::NV24: > > + case libcamera::formats::NV42: > > + /* activate texture 0 */ > > + glActiveTexture(GL_TEXTURE0); > > + configureTexture(id_y); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > + glUniform1i(textureUniformY, 0); > > + > > + /* activate texture 1 */ > > + glActiveTexture(GL_TEXTURE1); > > + configureTexture(id_u); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > + glUniform1i(textureUniformU, 1); > > + break; > > + case libcamera::formats::YUV420: > > + /* activate texture 0 */ > > + glActiveTexture(GL_TEXTURE0); > > + configureTexture(id_y); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > + glUniform1i(textureUniformY, 0); > > + > > + /* activate texture 1 */ > > + glActiveTexture(GL_TEXTURE1); > > + configureTexture(id_u); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > + glUniform1i(textureUniformU, 1); > > + > > + /* activate texture 2 */ > > + glActiveTexture(GL_TEXTURE2); > > + configureTexture(id_v); > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()*5/4); > > + glUniform1i(textureUniformV, 2); > > + break; > > + default: > > + break; > > + }; > > +} > > + > > +void ViewFinderGL::paintGL() > > +{ > > + if(pFShader == nullptr) We tend to write if (!pFShader) > > + createFragmentShader(); I wonder if we could do this in setFormat(). > > + > > + if(yuvDataPtr) > > + { > > + glClearColor(0.0, 0.0, 0.0, 1.0); > > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT ); > > + > > + render(); > > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); > > + } > > +} > > + > > +void ViewFinderGL::resizeGL(int w, int h) > > +{ > > + glViewport(0,0,w,h); > > +} > > + > > +QSize ViewFinderGL::sizeHint() const > > +{ > > + return size_.isValid() ? size_ : QSize(640, 480); > > +} > > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h > > new file mode 100644 > > index 0000000..08662ca > > --- /dev/null > > +++ b/src/qcam/viewfinderGL.h > > @@ -0,0 +1,101 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2020, Linaro > > + * > > + * viewfinderGL.h - Render YUV format frame by OpenGL shader > > + */ > > +#ifndef __VIEWFINDERGL_H__ > > +#define __VIEWFINDERGL_H__ > > + > > +#include <fcntl.h> > > +#include <string.h> > > +#include <unistd.h> > > + > > +#include <iomanip> > > +#include <iostream> > > +#include <sstream> > > + Do you need all these headers in the .h file ? They seem to belong to the .cpp file. > > +#include <QImage> > > +#include <QOpenGLBuffer> > > +#include <QOpenGLFunctions> > > +#include <QOpenGLShader> > > +#include <QOpenGLShaderProgram> > > +#include <QSize> > > +#include <QOpenGLTexture> > > +#include <QOpenGLWidget> > > + > > +#include <libcamera/buffer.h> > > +#include <libcamera/pixel_format.h> Blank line missing here. > > +#include "viewfinder.h" > > + > > +class ViewFinderGL : public QOpenGLWidget, > > + public ViewFinderHandler, > > + protected QOpenGLFunctions > > +{ > > + Q_OBJECT > > + > > +public: > > + ViewFinderGL(QWidget *parent = 0); > > + ~ViewFinderGL(); > > + > > + int setFormat(const libcamera::PixelFormat &format, const QSize &size); You should add "override" to qualify all overridden functions. int setFormat(const libcamera::PixelFormat &format, const QSize &size) override; > > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > + void stop(); > > + > > + QImage getCurrentImage(); > > + > > + void setFrameSize(int width, int height); > > + void updateFrame(unsigned char *buffer); Those two functions seem unused. > > + > > + char *selectFragmentShader(unsigned int format); > > + void createFragmentShader(); And these two functions can be private. > > + void render(); > > + > > +Q_SIGNALS: > > + void renderComplete(libcamera::FrameBuffer *buffer); > > + > > +protected: > > + void initializeGL() Q_DECL_OVERRIDE; You can use "override" directly, we know the compiler supports it. > > + void paintGL() Q_DECL_OVERRIDE; > > + void resizeGL(int w, int h) Q_DECL_OVERRIDE; > > + QSize sizeHint() const override; > > + > > +private: > > + > > + void configureTexture(unsigned int id); > > + void removeShader(); > > + > > + /* Captured image size, format and buffer */ > > + libcamera::FrameBuffer *buffer_; > > + libcamera::PixelFormat format_; > > + QOpenGLBuffer glBuffer; > > + QSize size_; > > + > > + GLuint id_u; > > + GLuint id_v; > > + GLuint id_y; > > + > > + QMutex mutex_; /* Prevent concurrent access to image_ */ > > + > > + QOpenGLShader *pFShader; > > + QOpenGLShader *pVShader; > > + QOpenGLShaderProgram shaderProgram; > > + > > + GLuint textureUniformU; > > + GLuint textureUniformV; > > + GLuint textureUniformY; > > + > > + QOpenGLTexture textureU; > > + QOpenGLTexture textureV; > > + QOpenGLTexture textureY; > > + > > + unsigned int width_; > > + unsigned int height_; > > + > > + unsigned char* yuvDataPtr; > > + > > + /* NV parameters */ > > + unsigned int horzSubSample_ ; > > + unsigned int vertSubSample_; > > +}; > > +#endif // __VIEWFINDERGL_H__
Hi Niklas, Laurent, Thanks for the review. On Wed, Jul 1, 2020 at 12:26 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hello, > > On Tue, Jun 30, 2020 at 05:03:43PM +0200, Niklas Söderlund wrote: > > Hi Show, > > > > Thanks for your work. > > Likewise :-) > > > I really like this version! The structure is almost there and much > > better then previous versions. As Kieran points out there are style > > errors that checkstyle.py will help you point out so I will ignore them > > in this review. > Thank you.:-) > > > > On 2020-06-24 15:37:05 +0800, Show Liu wrote: > > > The patch is to render the NV family YUV formats by OpenGL shader. > > > V3: refine the fragment shader for better pixel color. > > > > > > Signed-off-by: Show Liu <show.liu@linaro.org> > > > --- > > > src/qcam/main.cpp | 2 + > > > src/qcam/main_window.cpp | 19 ++- > > > src/qcam/main_window.h | 3 +- > > > src/qcam/meson.build | 2 + > > > src/qcam/shader.h | 104 ++++++++++++ > > > src/qcam/viewfinder.cpp | 18 +- > > > src/qcam/viewfinder.h | 23 ++- > > > src/qcam/viewfinderGL.cpp | 335 ++++++++++++++++++++++++++++++++++++++ > > > src/qcam/viewfinderGL.h | 101 ++++++++++++ > > > 9 files changed, 593 insertions(+), 14 deletions(-) > > > create mode 100644 src/qcam/shader.h > > > create mode 100644 src/qcam/viewfinderGL.cpp > > > create mode 100644 src/qcam/viewfinderGL.h > > > > > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > > > index b3468cb..a3ea471 100644 > > > --- a/src/qcam/main.cpp > > > +++ b/src/qcam/main.cpp > > > @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc, char > *argv[]) > > > "help"); > > > parser.addOption(OptStream, &streamKeyValue, > > > "Set configuration of a camera stream", "stream", > true); > > > + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats frame > via OpenGL shader", > > > + "opengl"); > > Should we default to OpenGL when possible, and add an option to force a > particular backend ? Maybe -r/--render={gles,qt} > I'd like to mention why I set OpenGL rendering as an option. First, this depends on the "GPU enabled" platforms, and it always needs some additional steps to make sure the GPU is ready. Second, as I said this patch is only for NV family YUV formats at present, and I believe it just covers very small parts of camera stream formats. That's why I am still trying to make it support more formats as I can.:-) > > > > > > > OptionsParser::Options options = parser.parse(argc, argv); > > > if (options.isSet(OptHelp)) > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > > index 7bc1360..6edf370 100644 > > > --- a/src/qcam/main_window.cpp > > > +++ b/src/qcam/main_window.cpp > > > @@ -28,6 +28,9 @@ > > > #include <libcamera/version.h> > > > > > > #include "dng_writer.h" > > > +#include "main_window.h" > > > +#include "viewfinder.h" > > > +#include "viewfinderGL.h" > > According to the name scheme we use, I think this should be > viewfinder_gl.h. OK. will fix it. > > > > > > > using namespace libcamera; > > > > > > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm, const > OptionsParser::Options &options) > > > setWindowTitle(title_); > > > connect(&titleTimer_, SIGNAL(timeout()), this, > SLOT(updateTitle())); > > > > > > - viewfinder_ = new ViewFinder(this); > > > - connect(viewfinder_, &ViewFinder::renderComplete, > > > - this, &MainWindow::queueRequest); > > > - setCentralWidget(viewfinder_); > > > + if (options_.isSet(OptOpenGL)) { > > > + viewfinder_ = new ViewFinderGL(this); > > > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_), > &ViewFinderGL::renderComplete, > > > + this, &MainWindow::queueRequest); > > > + setCentralWidget(dynamic_cast<ViewFinderGL > *>(viewfinder_)); > > > + } else { > > > + viewfinder_ = new ViewFinder(this); > > > + connect(dynamic_cast<ViewFinder *>(viewfinder_), > &ViewFinder::renderComplete, > > > + this, &MainWindow::queueRequest); > > > + setCentralWidget(dynamic_cast<ViewFinder *>(viewfinder_)); > > > + } > > > > I understand that one can't inherit from QObject twice, but this looks > > odd. Is there someway the base class could inherit from QObject or > > possibly QWidget and the derived classes hide their specilization > > somehow? I'm no Qt export so maybe I'm asking for something impossible > > or really stupid. > No, I really appreciate your opinions, it'll help me to make this better. :) > > If we assume all subclasses of Viewfinder will be QWidget, we could do > > if (options_.isSet(OptOpenGL)) > viewfinder_ = new ViewFinderGL(this); > else > viewfinder_ = new ViewFinder(this); > > QWidget *vf>idget = dynamic_cast<QWidget>(viewfinder_); > connect(vfWidget, &ViewFinderHandler::renderComplete, > this, &MainWindow::queueRequest); > setCentralWidget(vfWidget); > > With ViewFinderHandler::renderComplete() being a signal in the base > class. > I will try this way in the next version. > > > > + > > > adjustSize(); > > > > > > /* Hotplug/unplug support */ > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > > index 4606fe4..a852ef4 100644 > > > --- a/src/qcam/main_window.h > > > +++ b/src/qcam/main_window.h > > > @@ -38,6 +38,7 @@ enum { > > > OptCamera = 'c', > > > OptHelp = 'h', > > > OptStream = 's', > > > + OptOpenGL = 'o', > > > }; > > > > > > class CaptureRequest > > > @@ -102,7 +103,7 @@ private: > > > QAction *startStopAction_; > > > QComboBox *cameraCombo_; > > > QAction *saveRaw_; > > > - ViewFinder *viewfinder_; > > > + ViewFinderHandler *viewfinder_; > > I'd split this patch in two, with one patch that renames the existing > ViewFinder to ViewFinderQt and creates a ViewFinder base class (which > you call ViewFinderHandler here), and a second patch that adds > ViewFinderGL. OK. will do. > > > > > > > QIcon iconPlay_; > > > QIcon iconStop_; > > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > > > index 045db52..6a58947 100644 > > > --- a/src/qcam/meson.build > > > +++ b/src/qcam/meson.build > > > @@ -7,11 +7,13 @@ qcam_sources = files([ > > > 'main.cpp', > > > 'main_window.cpp', > > > 'viewfinder.cpp', > > > + 'viewfinderGL.cpp' > > > ]) > > > > > > qcam_moc_headers = files([ > > > 'main_window.h', > > > 'viewfinder.h', > > > + 'viewfinderGL.h' > > > ]) > > > > > > qcam_resources = files([ > > > diff --git a/src/qcam/shader.h b/src/qcam/shader.h > > > new file mode 100644 > > > index 0000000..f54c264 > > > --- /dev/null > > > +++ b/src/qcam/shader.h > > > @@ -0,0 +1,104 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * Copyright (C) 2020, Linaro > > > + * > > > + * shader.h - YUV format converter shader source code > > > + */ > > > +#ifndef __SHADER_H__ > > > +#define __SHADER_H__ > > > > I think the content of this file should be moved inside viewfinderGL.cpp > > Or maybe to viewfinder_gl_shader.cpp if we want to keep it separate. > Header files should only contain the declarations in any case, not the > actual variable contents. > > Ideally we should store the shaders in files of their own, not in a C > array, and have them pulled in as Qt resources > (https://doc.qt.io/qt-5/resources.html#using-resources-in-the-application > ). > They can be read through a QFile. It's something we can do on top of > this patch. > Actually, QOpenGLShader/QOpenGLShaderProgram are provide some methods to build/compile the shader source code from file directly but if I load it from file which means additional shader installation steps are required and also there are some potential risks if the shader is not in the right position. That's why I push them into one header file. Forgive my lazy ...:-P Anyway refine this part in the next version. > > > + > > > +/* Vertex shader for NV formats */ > > > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" > > > > could this (and bellow) be made static const ? > > > > > + "attribute vec2 textureIn;\n" > > > + "varying vec2 textureOut;\n" > > > + "void main(void)\n" > > > + "{\n" > > > + "gl_Position = vertexIn;\n" > > > + "textureOut = textureIn;\n" > > > + "}\n"; > > > + > > > +/* Fragment shader for NV12, NV16 and NV24 */ > > > +char NV_2_planes_UV[] = "#ifdef GL_ES\n" > > > + "precision highp float;\n" > > > + "#endif\n" > > > + "varying vec2 textureOut;\n" > > > + "uniform sampler2D tex_y;\n" > > > + "uniform sampler2D tex_u;\n" > > > + "void main(void)\n" > > > + "{\n" > > > + "vec3 yuv;\n" > > > + "vec3 rgb;\n" > > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, > 1.1640625),\n" > > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n" > > > + "rgb = convert_mat * yuv;\n" > > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > > + "}\n"; > > > + > > > +/* Fragment shader for NV21, NV61 and NV42 */ > > > +char NV_2_planes_VU[] = "#ifdef GL_ES\n" > > > + "precision highp float;\n" > > > + "#endif\n" > > > + "varying vec2 textureOut;\n" > > > + "uniform sampler2D tex_y;\n" > > > + "uniform sampler2D tex_u;\n" > > > + "void main(void)\n" > > > + "{\n" > > > + "vec3 yuv;\n" > > > + "vec3 rgb;\n" > > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, > 1.1640625),\n" > > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n" > > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > > + "rgb = convert_mat * yuv;\n" > > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > > + "}\n"; > > > + > > > +/* Fragment shader for YUV420 */ > > > +char NV_3_planes_UV[] = "#ifdef GL_ES\n" > > > + "precision highp float;\n" > > > + "#endif\n" > > > + "varying vec2 textureOut;\n" > > > + "uniform sampler2D tex_y;\n" > > > + "uniform sampler2D tex_u;\n" > > > + "uniform sampler2D tex_v;\n" > > > + "void main(void)\n" > > > + "{\n" > > > + "vec3 yuv;\n" > > > + "vec3 rgb;\n" > > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, > 1.1640625),\n" > > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n" > > > + "rgb = convert_mat * yuv;\n" > > > + "gl_FragColor = vec4(rgb, 1);\n" > > > + "}\n"; > > > + > > > +/* Fragment shader for YVU420 */ > > > +char NV_3_planes_VU[] = "precision highp float;\n" > > > + "#endif\n" > > > + "varying vec2 textureOut;\n" > > > + "uniform sampler2D tex_y;\n" > > > + "uniform sampler2D tex_u;\n" > > > + "uniform sampler2D tex_v;\n" > > > + "void main(void)\n" > > > + "{\n" > > > + "vec3 yuv;\n" > > > + "vec3 rgb;\n" > > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, > 1.1640625),\n" > > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n" > > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > > + "rgb = convert_mat * yuv;\n" > > > + "gl_FragColor = vec4(rgb, 1);\n" > > > + "}\n"; > > > +#endif // __SHADER_H__ > > > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp > > > index dcf8a15..d3a2422 100644 > > > --- a/src/qcam/viewfinder.cpp > > > +++ b/src/qcam/viewfinder.cpp > > > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat, > QImage::Format> nativeFormats > > > { libcamera::formats::BGR888, QImage::Format_RGB888 }, > > > }; > > > > > > -ViewFinder::ViewFinder(QWidget *parent) > > > - : QWidget(parent), buffer_(nullptr) > > > +ViewFinderHandler::ViewFinderHandler() > > > { > > > - icon_ = QIcon(":camera-off.svg"); > > > } > > You don't need an empty constructor in the base class, you can just drop > it. > > > > > > > -ViewFinder::~ViewFinder() > > > +ViewFinderHandler::~ViewFinderHandler() > > > { > > > } > > > > > > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() const > > > +const QList<libcamera::PixelFormat> > &ViewFinderHandler::nativeFormats() const > > > { > > > static const QList<libcamera::PixelFormat> formats = > ::nativeFormats.keys(); > > > return formats; > > > } > > I expect native formats to be different for ViewFinderQt and > ViewFinderGL, so I'd make this a pure virtual function with > implementations in the derived classes. ViewFinderGL should report all > the formats for which you have conversion shaders. > ok, I will rewrite this. > > > > > > > +ViewFinder::ViewFinder(QWidget *parent) > > > + : QWidget(parent), buffer_(nullptr) > > > +{ > > > + icon_ = QIcon(":camera-off.svg"); > > > +} > > > > I agree with Kieran here I think the base class should be named > > ViewFinder and the two derived classes ViewFinderQt and ViewFinderGL or > > something similar. > > Seems we all agree then :-) > > > I also think you should move the base class to its own .h file (and .cpp > > file if needed). > > Agreed. > I will modify it accordingly. > > > > + > > > +ViewFinder::~ViewFinder() > > > +{ > > > +} > > > + > > > int ViewFinder::setFormat(const libcamera::PixelFormat &format, > > > const QSize &size) > > > { > > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > > > index 26a1320..741d694 100644 > > > --- a/src/qcam/viewfinder.h > > > +++ b/src/qcam/viewfinder.h > > > @@ -13,6 +13,8 @@ > > > #include <QList> > > > #include <QImage> > > > #include <QMutex> > > > +#include <QOpenGLWidget> > > > +#include <QOpenGLFunctions> > > > #include <QSize> > > > #include <QWidget> > > > > > > @@ -28,7 +30,23 @@ struct MappedBuffer { > > > size_t size; > > > }; > > > > > > -class ViewFinder : public QWidget > > > +class ViewFinderHandler > > > +{ > > > +public: > > > + ViewFinderHandler(); > > > + virtual ~ViewFinderHandler(); > > > + > > > + const QList<libcamera::PixelFormat> &nativeFormats() const; > > > + > > > + virtual int setFormat(const libcamera::PixelFormat &format, const > QSize &size) =0; > > > + virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer > *map) =0; > > > + virtual void stop() =0; > > > + > > > + virtual QImage getCurrentImage() =0; > > > + > > > +}; > > > + > > > +class ViewFinder : public QWidget, public ViewFinderHandler > > > { > > > Q_OBJECT > > > > > > @@ -36,8 +54,6 @@ public: > > > ViewFinder(QWidget *parent); > > > ~ViewFinder(); > > > > > > - const QList<libcamera::PixelFormat> &nativeFormats() const; > > > - > > > int setFormat(const libcamera::PixelFormat &format, const QSize > &size); > > > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > > void stop(); > > > @@ -67,5 +83,4 @@ private: > > > QImage image_; > > > QMutex mutex_; /* Prevent concurrent access to image_ */ > > > }; > > > - > > This blank line should be kept. > sure, will fix it. > > > > #endif /* __QCAM_VIEWFINDER__ */ > > > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp > > > new file mode 100644 > > > index 0000000..7b47beb > > > --- /dev/null > > > +++ b/src/qcam/viewfinderGL.cpp > > > @@ -0,0 +1,335 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * Copyright (C) 2020, Linaro > > > + * > > > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader > > > + */ > > > + > > > +#include "shader.h" > > > +#include "viewfinderGL.h" > > > + > > > +#include <QImage> > > > + > > > +#include <libcamera/formats.h> > > > + > > > +#define ATTRIB_VERTEX 0 > > > +#define ATTRIB_TEXTURE 1 > > > + > > > +ViewFinderGL::ViewFinderGL(QWidget *parent) > > > + : QOpenGLWidget(parent), > > > + glBuffer(QOpenGLBuffer::VertexBuffer), > > > + pFShader(nullptr), > > > + pVShader(nullptr), > > > + textureU(QOpenGLTexture::Target2D), > > > + textureV(QOpenGLTexture::Target2D), > > > + textureY(QOpenGLTexture::Target2D), > > > + yuvDataPtr(nullptr) > > > + > > > +{ > > > +} > > > + > > > +ViewFinderGL::~ViewFinderGL() > > > +{ > > > + removeShader(); > > > + > > > + if(textureY.isCreated()) > > > + textureY.destroy(); > > > + > > > + if(textureU.isCreated()) > > > + textureU.destroy(); > > > + > > > + if(textureV.isCreated()) > > > + textureV.destroy(); > > > + > > > + glBuffer.destroy(); > > > +} > > > + > > > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > > > + const QSize &size) > > > +{ > > > + format_ = format; > > > + size_ = size; > > > + > > > + updateGeometry(); > > > + return 0; > > > +} > > > + > > > +void ViewFinderGL::stop() > > > +{ > > > + if (buffer_) { > > > + renderComplete(buffer_); > > > + buffer_ = nullptr; > > > + } > > > +} > > > + > > > +QImage ViewFinderGL::getCurrentImage() > > > +{ > > > + QMutexLocker locker(&mutex_); > > > + > > > + return(grabFramebuffer()); > > > +} > > > + > > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, > MappedBuffer *map) > > > +{ > > > + if (buffer->planes().size() != 1) { > > > + qWarning() << "Multi-planar buffers are not supported"; > > > + return; > > > + } > > > + > > > + unsigned char *memory = static_cast<unsigned char *>(map->memory); > > > + if(memory) { > > Can memory be null here ? > I will rewrite this. > > > > + yuvDataPtr = memory; > > > + update(); > > > + buffer_ = buffer; > > > + } > > > + > > > + if (buffer) > > > + renderComplete(buffer); > > Here's you're supposed to signal completion of the previous buffer, not > the current buffer. That's why there's a std::swap() in the existing > ViewFinder::render(). Is That means I should emit renderComplete(buffer_); after update(); then buffer_ = buffer; ... I don't quite understand what you mean, would you please explain it in detail? Forgive my stupid..:-P > > > > +} > > > + > > > +void ViewFinderGL::updateFrame(unsigned char *buffer) > > > +{ > > > + if(buffer) { > > > + yuvDataPtr = buffer; > > > + update(); > > > + } > > > +} > > > + > > > +void ViewFinderGL::setFrameSize(int width, int height) > > > +{ > > > + if(width > 0 && height > 0) { > > > + width_ = width; > > > + height_ = height; > > > + } > > > +} > > > + > > > +char *ViewFinderGL::selectFragmentShader(unsigned int format) > > > +{ > > > + char *fsrc = nullptr; > > > + > > > + switch(format) { > > > + case libcamera::formats::NV12: > > > + horzSubSample_ = 2; > > > + vertSubSample_ = 2; > > > + fsrc = NV_2_planes_UV; > > > + break; > > > + case libcamera::formats::NV21: > > > + horzSubSample_ = 2; > > > + vertSubSample_ = 2; > > > + fsrc = NV_2_planes_VU; > > > + break; > > > + case libcamera::formats::NV16: > > > + horzSubSample_ = 2; > > > + vertSubSample_ = 1; > > > + fsrc = NV_2_planes_UV; > > > + break; > > > + case libcamera::formats::NV61: > > > + horzSubSample_ = 2; > > > + vertSubSample_ = 1; > > > + fsrc = NV_2_planes_VU; > > > + break; > > > + case libcamera::formats::NV24: > > > + horzSubSample_ = 1; > > > + vertSubSample_ = 1; > > > + fsrc = NV_2_planes_UV; > > > + break; > > > + case libcamera::formats::NV42: > > > + horzSubSample_ = 1; > > > + vertSubSample_ = 1; > > > + fsrc = NV_2_planes_VU; > > > + break; > > > + case libcamera::formats::YUV420: > > > + horzSubSample_ = 2; > > > + vertSubSample_ = 2; > > > + fsrc = NV_3_planes_UV; > > > + break; > > > + default: > > > + break; > > > + }; > > > + > > > + if(fsrc == nullptr) { > > > + qDebug() << __func__ << "[ERROR]:" <<" No suitable > fragment shader can be select."; > > > + } > > > + return fsrc; > > > +} > > > + > > > +void ViewFinderGL::createFragmentShader() > > > +{ > > > + bool bCompile; > > > + > > > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this); > > > + char *fsrc = selectFragmentShader(format_); > > > + > > > + bCompile = pFShader->compileSourceCode(fsrc); > > > + if(!bCompile) > > > + { > > > + qDebug() << __func__ << ":" << pFShader->log(); > > > + } > > > + > > > + shaderProgram.addShader(pFShader); > > > + > > > + // Link shader pipeline > > > + if (!shaderProgram.link()) { > > > + qDebug() << __func__ << ": shader program link failed.\n" > << shaderProgram.log(); > > > + close(); > > > + } > > > + > > > + // Bind shader pipeline for use > > > + if (!shaderProgram.bind()) { > > > + qDebug() << __func__ << ": shader program binding > failed.\n" << shaderProgram.log(); > > > + close(); > > > + } > > > + > > > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX); > > > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE); > > > + > > > + > shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat)); > > > + > shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat)); > > > + > > > + textureUniformY = shaderProgram.uniformLocation("tex_y"); > > > + textureUniformU = shaderProgram.uniformLocation("tex_u"); > > > + textureUniformV = shaderProgram.uniformLocation("tex_v"); > > > + > > > + if(!textureY.isCreated()) > > > + textureY.create(); > > > + > > > + if(!textureU.isCreated()) > > > + textureU.create(); > > > + > > > + if(!textureV.isCreated()) > > > + textureV.create(); > > > + > > > + id_y = textureY.textureId(); > > > + id_u = textureU.textureId(); > > > + id_v = textureV.textureId(); > > > +} > > > + > > > +void ViewFinderGL::configureTexture(unsigned int id) > > > +{ > > > + glBindTexture(GL_TEXTURE_2D, id); > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, > GL_CLAMP_TO_EDGE); > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, > GL_CLAMP_TO_EDGE); > > > +} > > > + > > > +void ViewFinderGL::removeShader() > > > +{ > > > + if (shaderProgram.isLinked()) { > > > + shaderProgram.release(); > > > + shaderProgram.removeAllShaders(); > > > + } > > > + > > > + if(pFShader) > > > + delete pFShader; > > > + > > > + if(pVShader) > > > + delete pVShader; > > If we stop and restart the stream with a different format, the previous > shader will be used, as removeShader() isn't called. I don't think > that's right. I believe you need to call removeShader() at the beginning > of setFormat(). > Yes. you're right. I will refine the setFormat() function. > > > > +} > > > + > > > +void ViewFinderGL::initializeGL() > > > +{ > > > + bool bCompile; > > > + > > > + initializeOpenGLFunctions(); > > > + glEnable(GL_TEXTURE_2D); > > > + glDisable(GL_DEPTH_TEST); > > > + > > > + static const GLfloat vertices[] { > > > + -1.0f,-1.0f, > > > + -1.0f,+1.0f, > > > + +1.0f,+1.0f, > > > + +1.0f,-1.0f, > > > + 0.0f,1.0f, > > > + 0.0f,0.0f, > > > + 1.0f,0.0f, > > > + 1.0f,1.0f, > > > + }; > > > + > > > + glBuffer.create(); > > > + glBuffer.bind(); > > > + glBuffer.allocate(vertices,sizeof(vertices)); > > > + > > > + /* Create Vertex Shader */ > > > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this); > > > + char *vsrc = NV_Vertex_shader; > > > + > > > + bCompile = pVShader->compileSourceCode(vsrc); > > > + if(!bCompile) { > > > + qDebug() << __func__<< ":" << pVShader->log(); > > > + } > > > + > > > + shaderProgram.addShader(pVShader); > > > + > > > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f); > > > +} > > > + > > > +void ViewFinderGL::render() > > > +{ > > > + switch(format_) { > > > + case libcamera::formats::NV12: > > > + case libcamera::formats::NV21: > > > + case libcamera::formats::NV16: > > > + case libcamera::formats::NV61: > > > + case libcamera::formats::NV24: > > > + case libcamera::formats::NV42: > > > + /* activate texture 0 */ > > > + glActiveTexture(GL_TEXTURE0); > > > + configureTexture(id_y); > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), > size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > > + glUniform1i(textureUniformY, 0); > > > + > > > + /* activate texture 1 */ > > > + glActiveTexture(GL_TEXTURE1); > > > + configureTexture(id_u); > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, > GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > > + glUniform1i(textureUniformU, 1); > > > + break; > > > + case libcamera::formats::YUV420: > > > + /* activate texture 0 */ > > > + glActiveTexture(GL_TEXTURE0); > > > + configureTexture(id_y); > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), > size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > > + glUniform1i(textureUniformY, 0); > > > + > > > + /* activate texture 1 */ > > > + glActiveTexture(GL_TEXTURE1); > > > + configureTexture(id_u); > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, > GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > > + glUniform1i(textureUniformU, 1); > > > + > > > + /* activate texture 2 */ > > > + glActiveTexture(GL_TEXTURE2); > > > + configureTexture(id_v); > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, > GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()*5/4); > > > + glUniform1i(textureUniformV, 2); > > > + break; > > > + default: > > > + break; > > > + }; > > > +} > > > + > > > +void ViewFinderGL::paintGL() > > > +{ > > > + if(pFShader == nullptr) > > We tend to write > if (!pFShader) > ok will fix it. > > > > + createFragmentShader(); > > I wonder if we could do this in setFormat(). > That should be ok, I will try to move this when setting Format. > > > > + > > > + if(yuvDataPtr) > > > + { > > > + glClearColor(0.0, 0.0, 0.0, 1.0); > > > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT ); > > > + > > > + render(); > > > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); > > > + } > > > +} > > > + > > > +void ViewFinderGL::resizeGL(int w, int h) > > > +{ > > > + glViewport(0,0,w,h); > > > +} > > > + > > > +QSize ViewFinderGL::sizeHint() const > > > +{ > > > + return size_.isValid() ? size_ : QSize(640, 480); > > > +} > > > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h > > > new file mode 100644 > > > index 0000000..08662ca > > > --- /dev/null > > > +++ b/src/qcam/viewfinderGL.h > > > @@ -0,0 +1,101 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * Copyright (C) 2020, Linaro > > > + * > > > + * viewfinderGL.h - Render YUV format frame by OpenGL shader > > > + */ > > > +#ifndef __VIEWFINDERGL_H__ > > > +#define __VIEWFINDERGL_H__ > > > + > > > +#include <fcntl.h> > > > +#include <string.h> > > > +#include <unistd.h> > > > + > > > +#include <iomanip> > > > +#include <iostream> > > > +#include <sstream> > > > + > > Do you need all these headers in the .h file ? They seem to belong to > the .cpp file. > OK I will check or just remove it. > > > > +#include <QImage> > > > +#include <QOpenGLBuffer> > > > +#include <QOpenGLFunctions> > > > +#include <QOpenGLShader> > > > +#include <QOpenGLShaderProgram> > > > +#include <QSize> > > > +#include <QOpenGLTexture> > > > +#include <QOpenGLWidget> > > > + > > > +#include <libcamera/buffer.h> > > > +#include <libcamera/pixel_format.h> > > Blank line missing here. > sure, will do . > > > > +#include "viewfinder.h" > > > + > > > +class ViewFinderGL : public QOpenGLWidget, > > > + public ViewFinderHandler, > > > + protected QOpenGLFunctions > > > +{ > > > + Q_OBJECT > > > + > > > +public: > > > + ViewFinderGL(QWidget *parent = 0); > > > + ~ViewFinderGL(); > > > + > > > + int setFormat(const libcamera::PixelFormat &format, const QSize > &size); > > You should add "override" to qualify all overridden functions. > > int setFormat(const libcamera::PixelFormat &format, > const QSize &size) override; > ok will do. > > > > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > > + void stop(); > > > + > > > + QImage getCurrentImage(); > > > + > > > + void setFrameSize(int width, int height); > > > + void updateFrame(unsigned char *buffer); > > Those two functions seem unused. sure, I will remove it. > > > > + > > > + char *selectFragmentShader(unsigned int format); > > > + void createFragmentShader(); > > And these two functions can be private. > will move to private. > > > > + void render(); > > > + > > > +Q_SIGNALS: > > > + void renderComplete(libcamera::FrameBuffer *buffer); > > > + > > > +protected: > > > + void initializeGL() Q_DECL_OVERRIDE; > > You can use "override" directly, we know the compiler supports it. > ok, I will fix it up. Best Regards, Show Liu > > > > + void paintGL() Q_DECL_OVERRIDE; > > > + void resizeGL(int w, int h) Q_DECL_OVERRIDE; > > > + QSize sizeHint() const override; > > > + > > > +private: > > > + > > > + void configureTexture(unsigned int id); > > > + void removeShader(); > > > + > > > + /* Captured image size, format and buffer */ > > > + libcamera::FrameBuffer *buffer_; > > > + libcamera::PixelFormat format_; > > > + QOpenGLBuffer glBuffer; > > > + QSize size_; > > > + > > > + GLuint id_u; > > > + GLuint id_v; > > > + GLuint id_y; > > > + > > > + QMutex mutex_; /* Prevent concurrent access to image_ */ > > > + > > > + QOpenGLShader *pFShader; > > > + QOpenGLShader *pVShader; > > > + QOpenGLShaderProgram shaderProgram; > > > + > > > + GLuint textureUniformU; > > > + GLuint textureUniformV; > > > + GLuint textureUniformY; > > > + > > > + QOpenGLTexture textureU; > > > + QOpenGLTexture textureV; > > > + QOpenGLTexture textureY; > > > + > > > + unsigned int width_; > > > + unsigned int height_; > > > + > > > + unsigned char* yuvDataPtr; > > > + > > > + /* NV parameters */ > > > + unsigned int horzSubSample_ ; > > > + unsigned int vertSubSample_; > > > +}; > > > +#endif // __VIEWFINDERGL_H__ > > -- > Regards, > > Laurent Pinchart >
Hi Show, On Fri, Jul 03, 2020 at 03:46:32PM +0800, Show Liu wrote: > On Wed, Jul 1, 2020 at 12:26 AM Laurent Pinchart wrote: > > On Tue, Jun 30, 2020 at 05:03:43PM +0200, Niklas Söderlund wrote: > > > Hi Show, > > > > > > Thanks for your work. > > > > Likewise :-) > > > > > I really like this version! The structure is almost there and much > > > better then previous versions. As Kieran points out there are style > > > errors that checkstyle.py will help you point out so I will ignore them > > > in this review. > > Thank you.:-) > > > > On 2020-06-24 15:37:05 +0800, Show Liu wrote: > > > > The patch is to render the NV family YUV formats by OpenGL shader. > > > > V3: refine the fragment shader for better pixel color. > > > > > > > > Signed-off-by: Show Liu <show.liu@linaro.org> > > > > --- > > > > src/qcam/main.cpp | 2 + > > > > src/qcam/main_window.cpp | 19 ++- > > > > src/qcam/main_window.h | 3 +- > > > > src/qcam/meson.build | 2 + > > > > src/qcam/shader.h | 104 ++++++++++++ > > > > src/qcam/viewfinder.cpp | 18 +- > > > > src/qcam/viewfinder.h | 23 ++- > > > > src/qcam/viewfinderGL.cpp | 335 ++++++++++++++++++++++++++++++++++++++ > > > > src/qcam/viewfinderGL.h | 101 ++++++++++++ > > > > 9 files changed, 593 insertions(+), 14 deletions(-) > > > > create mode 100644 src/qcam/shader.h > > > > create mode 100644 src/qcam/viewfinderGL.cpp > > > > create mode 100644 src/qcam/viewfinderGL.h > > > > > > > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > > > > index b3468cb..a3ea471 100644 > > > > --- a/src/qcam/main.cpp > > > > +++ b/src/qcam/main.cpp > > > > @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc, char *argv[]) > > > > "help"); > > > > parser.addOption(OptStream, &streamKeyValue, > > > > "Set configuration of a camera stream", "stream", true); > > > > + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats frame via OpenGL shader", > > > > + "opengl"); > > > > Should we default to OpenGL when possible, and add an option to force a > > particular backend ? Maybe -r/--render={gles,qt} > > I'd like to mention why I set OpenGL rendering as an option. > First, this depends on the "GPU enabled" platforms, > and it always needs some additional steps to make sure the GPU is ready. > Second, as I said this patch is only for NV family YUV formats at present, > and I believe it just covers very small parts of camera stream formats. > That's why I am still trying to make it support more formats as I can.:-) That's a good point, defaulting to the Qt renderer for now may be best. I'd still prefer a -r/--render option if possible though, to make it easier to change the default and to add more renderers later if needed. > > > > > > > > OptionsParser::Options options = parser.parse(argc, argv); > > > > if (options.isSet(OptHelp)) > > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > > > index 7bc1360..6edf370 100644 > > > > --- a/src/qcam/main_window.cpp > > > > +++ b/src/qcam/main_window.cpp > > > > @@ -28,6 +28,9 @@ > > > > #include <libcamera/version.h> > > > > > > > > #include "dng_writer.h" > > > > +#include "main_window.h" > > > > +#include "viewfinder.h" > > > > +#include "viewfinderGL.h" > > > > According to the name scheme we use, I think this should be > > viewfinder_gl.h. > > OK. will fix it. > > > > > using namespace libcamera; > > > > > > > > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) > > > > setWindowTitle(title_); > > > > connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle())); > > > > > > > > - viewfinder_ = new ViewFinder(this); > > > > - connect(viewfinder_, &ViewFinder::renderComplete, > > > > - this, &MainWindow::queueRequest); > > > > - setCentralWidget(viewfinder_); > > > > + if (options_.isSet(OptOpenGL)) { > > > > + viewfinder_ = new ViewFinderGL(this); > > > > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_), &ViewFinderGL::renderComplete, > > > > + this, &MainWindow::queueRequest); > > > > + setCentralWidget(dynamic_cast<ViewFinderGL *>(viewfinder_)); > > > > + } else { > > > > + viewfinder_ = new ViewFinder(this); > > > > + connect(dynamic_cast<ViewFinder *>(viewfinder_), &ViewFinder::renderComplete, > > > > + this, &MainWindow::queueRequest); > > > > + setCentralWidget(dynamic_cast<ViewFinder *>(viewfinder_)); > > > > + } > > > > > > I understand that one can't inherit from QObject twice, but this looks > > > odd. Is there someway the base class could inherit from QObject or > > > possibly QWidget and the derived classes hide their specilization > > > somehow? I'm no Qt export so maybe I'm asking for something impossible > > > or really stupid. > > No, I really appreciate your opinions, it'll help me to make this better. :) > > > If we assume all subclasses of Viewfinder will be QWidget, we could do > > > > if (options_.isSet(OptOpenGL)) > > viewfinder_ = new ViewFinderGL(this); > > else > > viewfinder_ = new ViewFinder(this); > > > > QWidget *vf>idget = dynamic_cast<QWidget>(viewfinder_); > > connect(vfWidget, &ViewFinderHandler::renderComplete, > > this, &MainWindow::queueRequest); > > setCentralWidget(vfWidget); > > > > With ViewFinderHandler::renderComplete() being a signal in the base > > class. > > I will try this way in the next version. > > > > > + > > > > adjustSize(); > > > > > > > > /* Hotplug/unplug support */ > > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > > > index 4606fe4..a852ef4 100644 > > > > --- a/src/qcam/main_window.h > > > > +++ b/src/qcam/main_window.h > > > > @@ -38,6 +38,7 @@ enum { > > > > OptCamera = 'c', > > > > OptHelp = 'h', > > > > OptStream = 's', > > > > + OptOpenGL = 'o', > > > > }; > > > > > > > > class CaptureRequest > > > > @@ -102,7 +103,7 @@ private: > > > > QAction *startStopAction_; > > > > QComboBox *cameraCombo_; > > > > QAction *saveRaw_; > > > > - ViewFinder *viewfinder_; > > > > + ViewFinderHandler *viewfinder_; > > > > I'd split this patch in two, with one patch that renames the existing > > ViewFinder to ViewFinderQt and creates a ViewFinder base class (which > > you call ViewFinderHandler here), and a second patch that adds > > ViewFinderGL. > > OK. will do. > > > > > QIcon iconPlay_; > > > > QIcon iconStop_; > > > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > > > > index 045db52..6a58947 100644 > > > > --- a/src/qcam/meson.build > > > > +++ b/src/qcam/meson.build > > > > @@ -7,11 +7,13 @@ qcam_sources = files([ > > > > 'main.cpp', > > > > 'main_window.cpp', > > > > 'viewfinder.cpp', > > > > + 'viewfinderGL.cpp' > > > > ]) > > > > > > > > qcam_moc_headers = files([ > > > > 'main_window.h', > > > > 'viewfinder.h', > > > > + 'viewfinderGL.h' > > > > ]) > > > > > > > > qcam_resources = files([ > > > > diff --git a/src/qcam/shader.h b/src/qcam/shader.h > > > > new file mode 100644 > > > > index 0000000..f54c264 > > > > --- /dev/null > > > > +++ b/src/qcam/shader.h > > > > @@ -0,0 +1,104 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > +/* > > > > + * Copyright (C) 2020, Linaro > > > > + * > > > > + * shader.h - YUV format converter shader source code > > > > + */ > > > > +#ifndef __SHADER_H__ > > > > +#define __SHADER_H__ > > > > > > I think the content of this file should be moved inside viewfinderGL.cpp > > > > Or maybe to viewfinder_gl_shader.cpp if we want to keep it separate. > > Header files should only contain the declarations in any case, not the > > actual variable contents. > > > > Ideally we should store the shaders in files of their own, not in a C > > array, and have them pulled in as Qt resources > > (https://doc.qt.io/qt-5/resources.html#using-resources-in-the-application > > ). > > They can be read through a QFile. It's something we can do on top of > > this patch. > > Actually, QOpenGLShader/QOpenGLShaderProgram are provide some methods > to build/compile the shader source code from file directly but if I load it > from file which means > additional shader installation steps are required and also there are some > potential risks > if the shader is not in the right position. That's why I push them into one > header file. > Forgive my lazy ...:-P > Anyway refine this part in the next version. I agree with you that storing shaders in files that need to be installed isn't very nice, for the reasons you described. However, if you store them in files in the source tree that are then compiled as Qt resources, the data will be in the same binary, and always accessible. I think that would be a good middle-ground, making the shader sources nicer to read, modify and review, and still easy to use in the code through QFile. If QFile::map() works on resources, that will be very simple, otherwise QFile::readAll() will give you a QByteArray with the data. > > > > + > > > > +/* Vertex shader for NV formats */ > > > > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" > > > > > > could this (and bellow) be made static const ? > > > > > > > + "attribute vec2 textureIn;\n" > > > > + "varying vec2 textureOut;\n" > > > > + "void main(void)\n" > > > > + "{\n" > > > > + "gl_Position = vertexIn;\n" > > > > + "textureOut = textureIn;\n" > > > > + "}\n"; > > > > + > > > > +/* Fragment shader for NV12, NV16 and NV24 */ > > > > +char NV_2_planes_UV[] = "#ifdef GL_ES\n" > > > > + "precision highp float;\n" > > > > + "#endif\n" > > > > + "varying vec2 textureOut;\n" > > > > + "uniform sampler2D tex_y;\n" > > > > + "uniform sampler2D tex_u;\n" > > > > + "void main(void)\n" > > > > + "{\n" > > > > + "vec3 yuv;\n" > > > > + "vec3 rgb;\n" > > > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > > > > + " vec3(0.0, -0.390625, 2.015625),\n" > > > > + " vec3(1.5975625, -0.8125, 0.0));\n" > > > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > > > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n" > > > > + "rgb = convert_mat * yuv;\n" > > > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > > > + "}\n"; > > > > + > > > > +/* Fragment shader for NV21, NV61 and NV42 */ > > > > +char NV_2_planes_VU[] = "#ifdef GL_ES\n" > > > > + "precision highp float;\n" > > > > + "#endif\n" > > > > + "varying vec2 textureOut;\n" > > > > + "uniform sampler2D tex_y;\n" > > > > + "uniform sampler2D tex_u;\n" > > > > + "void main(void)\n" > > > > + "{\n" > > > > + "vec3 yuv;\n" > > > > + "vec3 rgb;\n" > > > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > > > > + " vec3(0.0, -0.390625, 2.015625),\n" > > > > + " vec3(1.5975625, -0.8125, 0.0));\n" > > > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > > > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n" > > > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > > > + "rgb = convert_mat * yuv;\n" > > > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > > > + "}\n"; > > > > + > > > > +/* Fragment shader for YUV420 */ > > > > +char NV_3_planes_UV[] = "#ifdef GL_ES\n" > > > > + "precision highp float;\n" > > > > + "#endif\n" > > > > + "varying vec2 textureOut;\n" > > > > + "uniform sampler2D tex_y;\n" > > > > + "uniform sampler2D tex_u;\n" > > > > + "uniform sampler2D tex_v;\n" > > > > + "void main(void)\n" > > > > + "{\n" > > > > + "vec3 yuv;\n" > > > > + "vec3 rgb;\n" > > > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > > > > + " vec3(0.0, -0.390625, 2.015625),\n" > > > > + " vec3(1.5975625, -0.8125, 0.0));\n" > > > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > > > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n" > > > > + "rgb = convert_mat * yuv;\n" > > > > + "gl_FragColor = vec4(rgb, 1);\n" > > > > + "}\n"; > > > > + > > > > +/* Fragment shader for YVU420 */ > > > > +char NV_3_planes_VU[] = "precision highp float;\n" > > > > + "#endif\n" > > > > + "varying vec2 textureOut;\n" > > > > + "uniform sampler2D tex_y;\n" > > > > + "uniform sampler2D tex_u;\n" > > > > + "uniform sampler2D tex_v;\n" > > > > + "void main(void)\n" > > > > + "{\n" > > > > + "vec3 yuv;\n" > > > > + "vec3 rgb;\n" > > > > + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" > > > > + " vec3(0.0, -0.390625, 2.015625),\n" > > > > + " vec3(1.5975625, -0.8125, 0.0));\n" > > > > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" > > > > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n" > > > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > > > + "rgb = convert_mat * yuv;\n" > > > > + "gl_FragColor = vec4(rgb, 1);\n" > > > > + "}\n"; > > > > +#endif // __SHADER_H__ > > > > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp > > > > index dcf8a15..d3a2422 100644 > > > > --- a/src/qcam/viewfinder.cpp > > > > +++ b/src/qcam/viewfinder.cpp > > > > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats > > > > { libcamera::formats::BGR888, QImage::Format_RGB888 }, > > > > }; > > > > > > > > -ViewFinder::ViewFinder(QWidget *parent) > > > > - : QWidget(parent), buffer_(nullptr) > > > > +ViewFinderHandler::ViewFinderHandler() > > > > { > > > > - icon_ = QIcon(":camera-off.svg"); > > > > } > > > > You don't need an empty constructor in the base class, you can just drop > > it. > > > > > > > > > > -ViewFinder::~ViewFinder() > > > > +ViewFinderHandler::~ViewFinderHandler() > > > > { > > > > } > > > > > > > > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() const > > > > +const QList<libcamera::PixelFormat> &ViewFinderHandler::nativeFormats() const > > > > { > > > > static const QList<libcamera::PixelFormat> formats = ::nativeFormats.keys(); > > > > return formats; > > > > } > > > > I expect native formats to be different for ViewFinderQt and > > ViewFinderGL, so I'd make this a pure virtual function with > > implementations in the derived classes. ViewFinderGL should report all > > the formats for which you have conversion shaders. > > ok, I will rewrite this. > > > > > > > > > +ViewFinder::ViewFinder(QWidget *parent) > > > > + : QWidget(parent), buffer_(nullptr) > > > > +{ > > > > + icon_ = QIcon(":camera-off.svg"); > > > > +} > > > > > > I agree with Kieran here I think the base class should be named > > > ViewFinder and the two derived classes ViewFinderQt and ViewFinderGL or > > > something similar. > > > > Seems we all agree then :-) > > > > > I also think you should move the base class to its own .h file (and .cpp > > > file if needed). > > > > Agreed. > > I will modify it accordingly. > > > > > + > > > > +ViewFinder::~ViewFinder() > > > > +{ > > > > +} > > > > + > > > > int ViewFinder::setFormat(const libcamera::PixelFormat &format, > > > > const QSize &size) > > > > { > > > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > > > > index 26a1320..741d694 100644 > > > > --- a/src/qcam/viewfinder.h > > > > +++ b/src/qcam/viewfinder.h > > > > @@ -13,6 +13,8 @@ > > > > #include <QList> > > > > #include <QImage> > > > > #include <QMutex> > > > > +#include <QOpenGLWidget> > > > > +#include <QOpenGLFunctions> > > > > #include <QSize> > > > > #include <QWidget> > > > > > > > > @@ -28,7 +30,23 @@ struct MappedBuffer { > > > > size_t size; > > > > }; > > > > > > > > -class ViewFinder : public QWidget > > > > +class ViewFinderHandler > > > > +{ > > > > +public: > > > > + ViewFinderHandler(); > > > > + virtual ~ViewFinderHandler(); > > > > + > > > > + const QList<libcamera::PixelFormat> &nativeFormats() const; > > > > + > > > > + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) =0; > > > > + virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) =0; > > > > + virtual void stop() =0; > > > > + > > > > + virtual QImage getCurrentImage() =0; > > > > + > > > > +}; > > > > + > > > > +class ViewFinder : public QWidget, public ViewFinderHandler > > > > { > > > > Q_OBJECT > > > > > > > > @@ -36,8 +54,6 @@ public: > > > > ViewFinder(QWidget *parent); > > > > ~ViewFinder(); > > > > > > > > - const QList<libcamera::PixelFormat> &nativeFormats() const; > > > > - > > > > int setFormat(const libcamera::PixelFormat &format, const QSize &size); > > > > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > > > void stop(); > > > > @@ -67,5 +83,4 @@ private: > > > > QImage image_; > > > > QMutex mutex_; /* Prevent concurrent access to image_ */ > > > > }; > > > > - > > > > This blank line should be kept. > > sure, will fix it. > > > > > #endif /* __QCAM_VIEWFINDER__ */ > > > > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp > > > > new file mode 100644 > > > > index 0000000..7b47beb > > > > --- /dev/null > > > > +++ b/src/qcam/viewfinderGL.cpp > > > > @@ -0,0 +1,335 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > +/* > > > > + * Copyright (C) 2020, Linaro > > > > + * > > > > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader > > > > + */ > > > > + > > > > +#include "shader.h" > > > > +#include "viewfinderGL.h" > > > > + > > > > +#include <QImage> > > > > + > > > > +#include <libcamera/formats.h> > > > > + > > > > +#define ATTRIB_VERTEX 0 > > > > +#define ATTRIB_TEXTURE 1 > > > > + > > > > +ViewFinderGL::ViewFinderGL(QWidget *parent) > > > > + : QOpenGLWidget(parent), > > > > + glBuffer(QOpenGLBuffer::VertexBuffer), > > > > + pFShader(nullptr), > > > > + pVShader(nullptr), > > > > + textureU(QOpenGLTexture::Target2D), > > > > + textureV(QOpenGLTexture::Target2D), > > > > + textureY(QOpenGLTexture::Target2D), > > > > + yuvDataPtr(nullptr) > > > > + > > > > +{ > > > > +} > > > > + > > > > +ViewFinderGL::~ViewFinderGL() > > > > +{ > > > > + removeShader(); > > > > + > > > > + if(textureY.isCreated()) > > > > + textureY.destroy(); > > > > + > > > > + if(textureU.isCreated()) > > > > + textureU.destroy(); > > > > + > > > > + if(textureV.isCreated()) > > > > + textureV.destroy(); > > > > + > > > > + glBuffer.destroy(); > > > > +} > > > > + > > > > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > > > > + const QSize &size) > > > > +{ > > > > + format_ = format; > > > > + size_ = size; > > > > + > > > > + updateGeometry(); > > > > + return 0; > > > > +} > > > > + > > > > +void ViewFinderGL::stop() > > > > +{ > > > > + if (buffer_) { > > > > + renderComplete(buffer_); > > > > + buffer_ = nullptr; > > > > + } > > > > +} > > > > + > > > > +QImage ViewFinderGL::getCurrentImage() > > > > +{ > > > > + QMutexLocker locker(&mutex_); > > > > + > > > > + return(grabFramebuffer()); > > > > +} > > > > + > > > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map) > > > > +{ > > > > + if (buffer->planes().size() != 1) { > > > > + qWarning() << "Multi-planar buffers are not supported"; > > > > + return; > > > > + } > > > > + > > > > + unsigned char *memory = static_cast<unsigned char *>(map->memory); > > > > + if(memory) { > > > > Can memory be null here ? > > I will rewrite this. > > > > > + yuvDataPtr = memory; > > > > + update(); > > > > + buffer_ = buffer; > > > > + } > > > > + > > > > + if (buffer) > > > > + renderComplete(buffer); > > > > Here's you're supposed to signal completion of the previous buffer, not > > the current buffer. That's why there's a std::swap() in the existing > > ViewFinder::render(). > > Is That means I should > emit renderComplete(buffer_); after update(); > then > buffer_ = buffer; > ... > I don't quite understand what you mean, would you please explain it in > detail? > Forgive my stupid..:-P A stupid person couldn't have sent this patch in the first place :-) The ViewFinder component performs rendering asynchronously. It receives a libcamera buffer in render(), and is tasked to display it on the screen. The render() function returns before the rendering is complete. We thus need to tell the caller when the rendering is complete, to let it reuse the buffer once ViewFinder doesn't need it anymore. That's what renderComplete() is for. With the code above, you signal that the buffer has been rendered right after calling update(). That's not true, rendering is still in progress at that point. You can only give the buffer back to the caller with renderComplete() once a new buffer has been received to be rendered. That's why the current ViewFinder implementation has std::swap(buffer_, buffer); After this call, buffer contains the old buffer, and we then call if (buffer) renderComplete(buffer); to give the old buffer back. We need to check here if buffer is null, as when the first buffer is rendered, there's no old buffer to be given back. > > > > +} > > > > + > > > > +void ViewFinderGL::updateFrame(unsigned char *buffer) > > > > +{ > > > > + if(buffer) { > > > > + yuvDataPtr = buffer; > > > > + update(); > > > > + } > > > > +} > > > > + > > > > +void ViewFinderGL::setFrameSize(int width, int height) > > > > +{ > > > > + if(width > 0 && height > 0) { > > > > + width_ = width; > > > > + height_ = height; > > > > + } > > > > +} > > > > + > > > > +char *ViewFinderGL::selectFragmentShader(unsigned int format) > > > > +{ > > > > + char *fsrc = nullptr; > > > > + > > > > + switch(format) { > > > > + case libcamera::formats::NV12: > > > > + horzSubSample_ = 2; > > > > + vertSubSample_ = 2; > > > > + fsrc = NV_2_planes_UV; > > > > + break; > > > > + case libcamera::formats::NV21: > > > > + horzSubSample_ = 2; > > > > + vertSubSample_ = 2; > > > > + fsrc = NV_2_planes_VU; > > > > + break; > > > > + case libcamera::formats::NV16: > > > > + horzSubSample_ = 2; > > > > + vertSubSample_ = 1; > > > > + fsrc = NV_2_planes_UV; > > > > + break; > > > > + case libcamera::formats::NV61: > > > > + horzSubSample_ = 2; > > > > + vertSubSample_ = 1; > > > > + fsrc = NV_2_planes_VU; > > > > + break; > > > > + case libcamera::formats::NV24: > > > > + horzSubSample_ = 1; > > > > + vertSubSample_ = 1; > > > > + fsrc = NV_2_planes_UV; > > > > + break; > > > > + case libcamera::formats::NV42: > > > > + horzSubSample_ = 1; > > > > + vertSubSample_ = 1; > > > > + fsrc = NV_2_planes_VU; > > > > + break; > > > > + case libcamera::formats::YUV420: > > > > + horzSubSample_ = 2; > > > > + vertSubSample_ = 2; > > > > + fsrc = NV_3_planes_UV; > > > > + break; > > > > + default: > > > > + break; > > > > + }; > > > > + > > > > + if(fsrc == nullptr) { > > > > + qDebug() << __func__ << "[ERROR]:" <<" No suitable fragment shader can be select."; > > > > + } > > > > + return fsrc; > > > > +} > > > > + > > > > +void ViewFinderGL::createFragmentShader() > > > > +{ > > > > + bool bCompile; > > > > + > > > > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this); > > > > + char *fsrc = selectFragmentShader(format_); > > > > + > > > > + bCompile = pFShader->compileSourceCode(fsrc); > > > > + if(!bCompile) > > > > + { > > > > + qDebug() << __func__ << ":" << pFShader->log(); > > > > + } > > > > + > > > > + shaderProgram.addShader(pFShader); > > > > + > > > > + // Link shader pipeline > > > > + if (!shaderProgram.link()) { > > > > + qDebug() << __func__ << ": shader program link failed.\n" << shaderProgram.log(); > > > > + close(); > > > > + } > > > > + > > > > + // Bind shader pipeline for use > > > > + if (!shaderProgram.bind()) { > > > > + qDebug() << __func__ << ": shader program binding failed.\n" << shaderProgram.log(); > > > > + close(); > > > > + } > > > > + > > > > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX); > > > > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE); > > > > + > > > > + shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat)); > > > > + shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat)); > > > > + > > > > + textureUniformY = shaderProgram.uniformLocation("tex_y"); > > > > + textureUniformU = shaderProgram.uniformLocation("tex_u"); > > > > + textureUniformV = shaderProgram.uniformLocation("tex_v"); > > > > + > > > > + if(!textureY.isCreated()) > > > > + textureY.create(); > > > > + > > > > + if(!textureU.isCreated()) > > > > + textureU.create(); > > > > + > > > > + if(!textureV.isCreated()) > > > > + textureV.create(); > > > > + > > > > + id_y = textureY.textureId(); > > > > + id_u = textureU.textureId(); > > > > + id_v = textureV.textureId(); > > > > +} > > > > + > > > > +void ViewFinderGL::configureTexture(unsigned int id) > > > > +{ > > > > + glBindTexture(GL_TEXTURE_2D, id); > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); > > > > +} > > > > + > > > > +void ViewFinderGL::removeShader() > > > > +{ > > > > + if (shaderProgram.isLinked()) { > > > > + shaderProgram.release(); > > > > + shaderProgram.removeAllShaders(); > > > > + } > > > > + > > > > + if(pFShader) > > > > + delete pFShader; > > > > + > > > > + if(pVShader) > > > > + delete pVShader; > > > > If we stop and restart the stream with a different format, the previous > > shader will be used, as removeShader() isn't called. I don't think > > that's right. I believe you need to call removeShader() at the beginning > > of setFormat(). > > Yes. you're right. I will refine the setFormat() function. > > > > > +} > > > > + > > > > +void ViewFinderGL::initializeGL() > > > > +{ > > > > + bool bCompile; > > > > + > > > > + initializeOpenGLFunctions(); > > > > + glEnable(GL_TEXTURE_2D); > > > > + glDisable(GL_DEPTH_TEST); > > > > + > > > > + static const GLfloat vertices[] { > > > > + -1.0f,-1.0f, > > > > + -1.0f,+1.0f, > > > > + +1.0f,+1.0f, > > > > + +1.0f,-1.0f, > > > > + 0.0f,1.0f, > > > > + 0.0f,0.0f, > > > > + 1.0f,0.0f, > > > > + 1.0f,1.0f, > > > > + }; > > > > + > > > > + glBuffer.create(); > > > > + glBuffer.bind(); > > > > + glBuffer.allocate(vertices,sizeof(vertices)); > > > > + > > > > + /* Create Vertex Shader */ > > > > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this); > > > > + char *vsrc = NV_Vertex_shader; > > > > + > > > > + bCompile = pVShader->compileSourceCode(vsrc); > > > > + if(!bCompile) { > > > > + qDebug() << __func__<< ":" << pVShader->log(); > > > > + } > > > > + > > > > + shaderProgram.addShader(pVShader); > > > > + > > > > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f); > > > > +} > > > > + > > > > +void ViewFinderGL::render() > > > > +{ > > > > + switch(format_) { > > > > + case libcamera::formats::NV12: > > > > + case libcamera::formats::NV21: > > > > + case libcamera::formats::NV16: > > > > + case libcamera::formats::NV61: > > > > + case libcamera::formats::NV24: > > > > + case libcamera::formats::NV42: > > > > + /* activate texture 0 */ > > > > + glActiveTexture(GL_TEXTURE0); > > > > + configureTexture(id_y); > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > > > + glUniform1i(textureUniformY, 0); > > > > + > > > > + /* activate texture 1 */ > > > > + glActiveTexture(GL_TEXTURE1); > > > > + configureTexture(id_u); > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > > > + glUniform1i(textureUniformU, 1); > > > > + break; > > > > + case libcamera::formats::YUV420: > > > > + /* activate texture 0 */ > > > > + glActiveTexture(GL_TEXTURE0); > > > > + configureTexture(id_y); > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > > > + glUniform1i(textureUniformY, 0); > > > > + > > > > + /* activate texture 1 */ > > > > + glActiveTexture(GL_TEXTURE1); > > > > + configureTexture(id_u); > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > > > + glUniform1i(textureUniformU, 1); > > > > + > > > > + /* activate texture 2 */ > > > > + glActiveTexture(GL_TEXTURE2); > > > > + configureTexture(id_v); > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()*5/4); > > > > + glUniform1i(textureUniformV, 2); > > > > + break; > > > > + default: > > > > + break; > > > > + }; > > > > +} > > > > + > > > > +void ViewFinderGL::paintGL() > > > > +{ > > > > + if(pFShader == nullptr) > > > > We tend to write > > > > if (!pFShader) > > ok will fix it. > > > > > + createFragmentShader(); > > > > I wonder if we could do this in setFormat(). > > That should be ok, I will try to move this when setting Format. > > > > > + > > > > + if(yuvDataPtr) > > > > + { > > > > + glClearColor(0.0, 0.0, 0.0, 1.0); > > > > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT ); > > > > + > > > > + render(); > > > > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); > > > > + } > > > > +} > > > > + > > > > +void ViewFinderGL::resizeGL(int w, int h) > > > > +{ > > > > + glViewport(0,0,w,h); > > > > +} > > > > + > > > > +QSize ViewFinderGL::sizeHint() const > > > > +{ > > > > + return size_.isValid() ? size_ : QSize(640, 480); > > > > +} > > > > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h > > > > new file mode 100644 > > > > index 0000000..08662ca > > > > --- /dev/null > > > > +++ b/src/qcam/viewfinderGL.h > > > > @@ -0,0 +1,101 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > +/* > > > > + * Copyright (C) 2020, Linaro > > > > + * > > > > + * viewfinderGL.h - Render YUV format frame by OpenGL shader > > > > + */ > > > > +#ifndef __VIEWFINDERGL_H__ > > > > +#define __VIEWFINDERGL_H__ > > > > + > > > > +#include <fcntl.h> > > > > +#include <string.h> > > > > +#include <unistd.h> > > > > + > > > > +#include <iomanip> > > > > +#include <iostream> > > > > +#include <sstream> > > > > + > > > > Do you need all these headers in the .h file ? They seem to belong to > > the .cpp file. > > OK I will check or just remove it. > > > > > +#include <QImage> > > > > +#include <QOpenGLBuffer> > > > > +#include <QOpenGLFunctions> > > > > +#include <QOpenGLShader> > > > > +#include <QOpenGLShaderProgram> > > > > +#include <QSize> > > > > +#include <QOpenGLTexture> > > > > +#include <QOpenGLWidget> > > > > + > > > > +#include <libcamera/buffer.h> > > > > +#include <libcamera/pixel_format.h> > > > > Blank line missing here. > > sure, will do . > > > > > +#include "viewfinder.h" > > > > + > > > > +class ViewFinderGL : public QOpenGLWidget, > > > > + public ViewFinderHandler, > > > > + protected QOpenGLFunctions > > > > +{ > > > > + Q_OBJECT > > > > + > > > > +public: > > > > + ViewFinderGL(QWidget *parent = 0); > > > > + ~ViewFinderGL(); > > > > + > > > > + int setFormat(const libcamera::PixelFormat &format, const QSize &size); > > > > You should add "override" to qualify all overridden functions. > > > > int setFormat(const libcamera::PixelFormat &format, > > const QSize &size) override; > > ok will do. > > > > > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > > > + void stop(); > > > > + > > > > + QImage getCurrentImage(); > > > > + > > > > + void setFrameSize(int width, int height); > > > > + void updateFrame(unsigned char *buffer); > > > > Those two functions seem unused. > > sure, I will remove it. > > > > > + > > > > + char *selectFragmentShader(unsigned int format); > > > > + void createFragmentShader(); > > > > And these two functions can be private. > > will move to private. > > > > > + void render(); > > > > + > > > > +Q_SIGNALS: > > > > + void renderComplete(libcamera::FrameBuffer *buffer); > > > > + > > > > +protected: > > > > + void initializeGL() Q_DECL_OVERRIDE; > > > > You can use "override" directly, we know the compiler supports it. > > ok, I will fix it up. > > > > > + void paintGL() Q_DECL_OVERRIDE; > > > > + void resizeGL(int w, int h) Q_DECL_OVERRIDE; > > > > + QSize sizeHint() const override; > > > > + > > > > +private: > > > > + > > > > + void configureTexture(unsigned int id); > > > > + void removeShader(); > > > > + > > > > + /* Captured image size, format and buffer */ > > > > + libcamera::FrameBuffer *buffer_; > > > > + libcamera::PixelFormat format_; > > > > + QOpenGLBuffer glBuffer; > > > > + QSize size_; > > > > + > > > > + GLuint id_u; > > > > + GLuint id_v; > > > > + GLuint id_y; > > > > + > > > > + QMutex mutex_; /* Prevent concurrent access to image_ */ > > > > + > > > > + QOpenGLShader *pFShader; > > > > + QOpenGLShader *pVShader; > > > > + QOpenGLShaderProgram shaderProgram; > > > > + > > > > + GLuint textureUniformU; > > > > + GLuint textureUniformV; > > > > + GLuint textureUniformY; > > > > + > > > > + QOpenGLTexture textureU; > > > > + QOpenGLTexture textureV; > > > > + QOpenGLTexture textureY; > > > > + > > > > + unsigned int width_; > > > > + unsigned int height_; > > > > + > > > > + unsigned char* yuvDataPtr; > > > > + > > > > + /* NV parameters */ > > > > + unsigned int horzSubSample_ ; > > > > + unsigned int vertSubSample_; > > > > +}; > > > > +#endif // __VIEWFINDERGL_H__
Hi Laurent, On Fri, Jul 3, 2020 at 6:14 PM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Show, > > On Fri, Jul 03, 2020 at 03:46:32PM +0800, Show Liu wrote: > > On Wed, Jul 1, 2020 at 12:26 AM Laurent Pinchart wrote: > > > On Tue, Jun 30, 2020 at 05:03:43PM +0200, Niklas Söderlund wrote: > > > > Hi Show, > > > > > > > > Thanks for your work. > > > > > > Likewise :-) > > > > > > > I really like this version! The structure is almost there and much > > > > better then previous versions. As Kieran points out there are style > > > > errors that checkstyle.py will help you point out so I will ignore > them > > > > in this review. > > > > Thank you.:-) > > > > > > On 2020-06-24 15:37:05 +0800, Show Liu wrote: > > > > > The patch is to render the NV family YUV formats by OpenGL shader. > > > > > V3: refine the fragment shader for better pixel color. > > > > > > > > > > Signed-off-by: Show Liu <show.liu@linaro.org> > > > > > --- > > > > > src/qcam/main.cpp | 2 + > > > > > src/qcam/main_window.cpp | 19 ++- > > > > > src/qcam/main_window.h | 3 +- > > > > > src/qcam/meson.build | 2 + > > > > > src/qcam/shader.h | 104 ++++++++++++ > > > > > src/qcam/viewfinder.cpp | 18 +- > > > > > src/qcam/viewfinder.h | 23 ++- > > > > > src/qcam/viewfinderGL.cpp | 335 > ++++++++++++++++++++++++++++++++++++++ > > > > > src/qcam/viewfinderGL.h | 101 ++++++++++++ > > > > > 9 files changed, 593 insertions(+), 14 deletions(-) > > > > > create mode 100644 src/qcam/shader.h > > > > > create mode 100644 src/qcam/viewfinderGL.cpp > > > > > create mode 100644 src/qcam/viewfinderGL.h > > > > > > > > > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > > > > > index b3468cb..a3ea471 100644 > > > > > --- a/src/qcam/main.cpp > > > > > +++ b/src/qcam/main.cpp > > > > > @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc, > char *argv[]) > > > > > "help"); > > > > > parser.addOption(OptStream, &streamKeyValue, > > > > > "Set configuration of a camera stream", > "stream", true); > > > > > + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats > frame via OpenGL shader", > > > > > + "opengl"); > > > > > > Should we default to OpenGL when possible, and add an option to force a > > > particular backend ? Maybe -r/--render={gles,qt} > > > > I'd like to mention why I set OpenGL rendering as an option. > > First, this depends on the "GPU enabled" platforms, > > and it always needs some additional steps to make sure the GPU is ready. > > Second, as I said this patch is only for NV family YUV formats at > present, > > and I believe it just covers very small parts of camera stream formats. > > That's why I am still trying to make it support more formats as I can.:-) > > That's a good point, defaulting to the Qt renderer for now may be best. > I'd still prefer a -r/--render option if possible though, to make it > easier to change the default and to add more renderers later if needed. > OK. Understood. I will try this in the next version. > > > > > > > > > > > OptionsParser::Options options = parser.parse(argc, argv); > > > > > if (options.isSet(OptHelp)) > > > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > > > > index 7bc1360..6edf370 100644 > > > > > --- a/src/qcam/main_window.cpp > > > > > +++ b/src/qcam/main_window.cpp > > > > > @@ -28,6 +28,9 @@ > > > > > #include <libcamera/version.h> > > > > > > > > > > #include "dng_writer.h" > > > > > +#include "main_window.h" > > > > > +#include "viewfinder.h" > > > > > +#include "viewfinderGL.h" > > > > > > According to the name scheme we use, I think this should be > > > viewfinder_gl.h. > > > > OK. will fix it. > > > > > > > using namespace libcamera; > > > > > > > > > > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm, > const OptionsParser::Options &options) > > > > > setWindowTitle(title_); > > > > > connect(&titleTimer_, SIGNAL(timeout()), this, > SLOT(updateTitle())); > > > > > > > > > > - viewfinder_ = new ViewFinder(this); > > > > > - connect(viewfinder_, &ViewFinder::renderComplete, > > > > > - this, &MainWindow::queueRequest); > > > > > - setCentralWidget(viewfinder_); > > > > > + if (options_.isSet(OptOpenGL)) { > > > > > + viewfinder_ = new ViewFinderGL(this); > > > > > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_), > &ViewFinderGL::renderComplete, > > > > > + this, &MainWindow::queueRequest); > > > > > + setCentralWidget(dynamic_cast<ViewFinderGL > *>(viewfinder_)); > > > > > + } else { > > > > > + viewfinder_ = new ViewFinder(this); > > > > > + connect(dynamic_cast<ViewFinder *>(viewfinder_), > &ViewFinder::renderComplete, > > > > > + this, &MainWindow::queueRequest); > > > > > + setCentralWidget(dynamic_cast<ViewFinder > *>(viewfinder_)); > > > > > + } > > > > > > > > I understand that one can't inherit from QObject twice, but this > looks > > > > odd. Is there someway the base class could inherit from QObject or > > > > possibly QWidget and the derived classes hide their specilization > > > > somehow? I'm no Qt export so maybe I'm asking for something > impossible > > > > or really stupid. > > > > No, I really appreciate your opinions, it'll help me to make this > better. :) > > > > > If we assume all subclasses of Viewfinder will be QWidget, we could do > > > > > > if (options_.isSet(OptOpenGL)) > > > viewfinder_ = new ViewFinderGL(this); > > > else > > > viewfinder_ = new ViewFinder(this); > > > > > > QWidget *vf>idget = dynamic_cast<QWidget>(viewfinder_); > > > connect(vfWidget, &ViewFinderHandler::renderComplete, > > > this, &MainWindow::queueRequest); > > > setCentralWidget(vfWidget); > > > > > > With ViewFinderHandler::renderComplete() being a signal in the base > > > class. > > > > I will try this way in the next version. > > > > > > > + > > > > > adjustSize(); > > > > > > > > > > /* Hotplug/unplug support */ > > > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > > > > index 4606fe4..a852ef4 100644 > > > > > --- a/src/qcam/main_window.h > > > > > +++ b/src/qcam/main_window.h > > > > > @@ -38,6 +38,7 @@ enum { > > > > > OptCamera = 'c', > > > > > OptHelp = 'h', > > > > > OptStream = 's', > > > > > + OptOpenGL = 'o', > > > > > }; > > > > > > > > > > class CaptureRequest > > > > > @@ -102,7 +103,7 @@ private: > > > > > QAction *startStopAction_; > > > > > QComboBox *cameraCombo_; > > > > > QAction *saveRaw_; > > > > > - ViewFinder *viewfinder_; > > > > > + ViewFinderHandler *viewfinder_; > > > > > > I'd split this patch in two, with one patch that renames the existing > > > ViewFinder to ViewFinderQt and creates a ViewFinder base class (which > > > you call ViewFinderHandler here), and a second patch that adds > > > ViewFinderGL. > > > > OK. will do. > > > > > > > QIcon iconPlay_; > > > > > QIcon iconStop_; > > > > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > > > > > index 045db52..6a58947 100644 > > > > > --- a/src/qcam/meson.build > > > > > +++ b/src/qcam/meson.build > > > > > @@ -7,11 +7,13 @@ qcam_sources = files([ > > > > > 'main.cpp', > > > > > 'main_window.cpp', > > > > > 'viewfinder.cpp', > > > > > + 'viewfinderGL.cpp' > > > > > ]) > > > > > > > > > > qcam_moc_headers = files([ > > > > > 'main_window.h', > > > > > 'viewfinder.h', > > > > > + 'viewfinderGL.h' > > > > > ]) > > > > > > > > > > qcam_resources = files([ > > > > > diff --git a/src/qcam/shader.h b/src/qcam/shader.h > > > > > new file mode 100644 > > > > > index 0000000..f54c264 > > > > > --- /dev/null > > > > > +++ b/src/qcam/shader.h > > > > > @@ -0,0 +1,104 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2020, Linaro > > > > > + * > > > > > + * shader.h - YUV format converter shader source code > > > > > + */ > > > > > +#ifndef __SHADER_H__ > > > > > +#define __SHADER_H__ > > > > > > > > I think the content of this file should be moved inside > viewfinderGL.cpp > > > > > > Or maybe to viewfinder_gl_shader.cpp if we want to keep it separate. > > > Header files should only contain the declarations in any case, not the > > > actual variable contents. > > > > > > Ideally we should store the shaders in files of their own, not in a C > > > array, and have them pulled in as Qt resources > > > ( > https://doc.qt.io/qt-5/resources.html#using-resources-in-the-application > > > ). > > > They can be read through a QFile. It's something we can do on top of > > > this patch. > > > > Actually, QOpenGLShader/QOpenGLShaderProgram are provide some methods > > to build/compile the shader source code from file directly but if I load > it > > from file which means > > additional shader installation steps are required and also there are some > > potential risks > > if the shader is not in the right position. That's why I push them into > one > > header file. > > Forgive my lazy ...:-P > > Anyway refine this part in the next version. > > I agree with you that storing shaders in files that need to be installed > isn't very nice, for the reasons you described. However, if you store > them in files in the source tree that are then compiled as Qt resources, > the data will be in the same binary, and always accessible. I think that > would be a good middle-ground, making the shader sources nicer to read, > modify and review, and still easy to use in the code through QFile. If > QFile::map() works on resources, that will be very simple, otherwise > QFile::readAll() will give you a QByteArray with the data. > OK. sounds good. I will try this in the next version too. > > > > > > + > > > > > +/* Vertex shader for NV formats */ > > > > > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" > > > > > > > > could this (and bellow) be made static const ? > > > > > > > > > + "attribute vec2 textureIn;\n" > > > > > + "varying vec2 textureOut;\n" > > > > > + "void main(void)\n" > > > > > + "{\n" > > > > > + "gl_Position = vertexIn;\n" > > > > > + "textureOut = textureIn;\n" > > > > > + "}\n"; > > > > > + > > > > > +/* Fragment shader for NV12, NV16 and NV24 */ > > > > > +char NV_2_planes_UV[] = "#ifdef GL_ES\n" > > > > > + "precision highp float;\n" > > > > > + "#endif\n" > > > > > + "varying vec2 textureOut;\n" > > > > > + "uniform sampler2D tex_y;\n" > > > > > + "uniform sampler2D tex_u;\n" > > > > > + "void main(void)\n" > > > > > + "{\n" > > > > > + "vec3 yuv;\n" > > > > > + "vec3 rgb;\n" > > > > > + "mat3 convert_mat = mat3(vec3(1.1640625, > 1.1640625, 1.1640625),\n" > > > > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > > > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > > > > + "yuv.x = texture2D(tex_y, textureOut).r - > 0.0625;\n" > > > > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > > > > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n" > > > > > + "rgb = convert_mat * yuv;\n" > > > > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > > > > + "}\n"; > > > > > + > > > > > +/* Fragment shader for NV21, NV61 and NV42 */ > > > > > +char NV_2_planes_VU[] = "#ifdef GL_ES\n" > > > > > + "precision highp float;\n" > > > > > + "#endif\n" > > > > > + "varying vec2 textureOut;\n" > > > > > + "uniform sampler2D tex_y;\n" > > > > > + "uniform sampler2D tex_u;\n" > > > > > + "void main(void)\n" > > > > > + "{\n" > > > > > + "vec3 yuv;\n" > > > > > + "vec3 rgb;\n" > > > > > + "mat3 convert_mat = mat3(vec3(1.1640625, > 1.1640625, 1.1640625),\n" > > > > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > > > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > > > > + "yuv.x = texture2D(tex_y, textureOut).r - > 0.0625;\n" > > > > > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n" > > > > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > > > > + "rgb = convert_mat * yuv;\n" > > > > > + "gl_FragColor = vec4(rgb, 1.0);\n" > > > > > + "}\n"; > > > > > + > > > > > +/* Fragment shader for YUV420 */ > > > > > +char NV_3_planes_UV[] = "#ifdef GL_ES\n" > > > > > + "precision highp float;\n" > > > > > + "#endif\n" > > > > > + "varying vec2 textureOut;\n" > > > > > + "uniform sampler2D tex_y;\n" > > > > > + "uniform sampler2D tex_u;\n" > > > > > + "uniform sampler2D tex_v;\n" > > > > > + "void main(void)\n" > > > > > + "{\n" > > > > > + "vec3 yuv;\n" > > > > > + "vec3 rgb;\n" > > > > > + "mat3 convert_mat = mat3(vec3(1.1640625, > 1.1640625, 1.1640625),\n" > > > > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > > > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > > > > + "yuv.x = texture2D(tex_y, textureOut).r - > 0.0625;\n" > > > > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" > > > > > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n" > > > > > + "rgb = convert_mat * yuv;\n" > > > > > + "gl_FragColor = vec4(rgb, 1);\n" > > > > > + "}\n"; > > > > > + > > > > > +/* Fragment shader for YVU420 */ > > > > > +char NV_3_planes_VU[] = "precision highp float;\n" > > > > > + "#endif\n" > > > > > + "varying vec2 textureOut;\n" > > > > > + "uniform sampler2D tex_y;\n" > > > > > + "uniform sampler2D tex_u;\n" > > > > > + "uniform sampler2D tex_v;\n" > > > > > + "void main(void)\n" > > > > > + "{\n" > > > > > + "vec3 yuv;\n" > > > > > + "vec3 rgb;\n" > > > > > + "mat3 convert_mat = mat3(vec3(1.1640625, > 1.1640625, 1.1640625),\n" > > > > > + " vec3(0.0, -0.390625, > 2.015625),\n" > > > > > + " vec3(1.5975625, -0.8125, > 0.0));\n" > > > > > + "yuv.x = texture2D(tex_y, textureOut).r - > 0.0625;\n" > > > > > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n" > > > > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" > > > > > + "rgb = convert_mat * yuv;\n" > > > > > + "gl_FragColor = vec4(rgb, 1);\n" > > > > > + "}\n"; > > > > > +#endif // __SHADER_H__ > > > > > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp > > > > > index dcf8a15..d3a2422 100644 > > > > > --- a/src/qcam/viewfinder.cpp > > > > > +++ b/src/qcam/viewfinder.cpp > > > > > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat, > QImage::Format> nativeFormats > > > > > { libcamera::formats::BGR888, QImage::Format_RGB888 }, > > > > > }; > > > > > > > > > > -ViewFinder::ViewFinder(QWidget *parent) > > > > > - : QWidget(parent), buffer_(nullptr) > > > > > +ViewFinderHandler::ViewFinderHandler() > > > > > { > > > > > - icon_ = QIcon(":camera-off.svg"); > > > > > } > > > > > > You don't need an empty constructor in the base class, you can just > drop > > > it. > > > > > > > > > > > > > -ViewFinder::~ViewFinder() > > > > > +ViewFinderHandler::~ViewFinderHandler() > > > > > { > > > > > } > > > > > > > > > > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() > const > > > > > +const QList<libcamera::PixelFormat> > &ViewFinderHandler::nativeFormats() const > > > > > { > > > > > static const QList<libcamera::PixelFormat> formats = > ::nativeFormats.keys(); > > > > > return formats; > > > > > } > > > > > > I expect native formats to be different for ViewFinderQt and > > > ViewFinderGL, so I'd make this a pure virtual function with > > > implementations in the derived classes. ViewFinderGL should report all > > > the formats for which you have conversion shaders. > > > > ok, I will rewrite this. > > > > > > > > > > > > +ViewFinder::ViewFinder(QWidget *parent) > > > > > + : QWidget(parent), buffer_(nullptr) > > > > > +{ > > > > > + icon_ = QIcon(":camera-off.svg"); > > > > > +} > > > > > > > > I agree with Kieran here I think the base class should be named > > > > ViewFinder and the two derived classes ViewFinderQt and ViewFinderGL > or > > > > something similar. > > > > > > Seems we all agree then :-) > > > > > > > I also think you should move the base class to its own .h file (and > .cpp > > > > file if needed). > > > > > > Agreed. > > > > I will modify it accordingly. > > > > > > > + > > > > > +ViewFinder::~ViewFinder() > > > > > +{ > > > > > +} > > > > > + > > > > > int ViewFinder::setFormat(const libcamera::PixelFormat &format, > > > > > const QSize &size) > > > > > { > > > > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > > > > > index 26a1320..741d694 100644 > > > > > --- a/src/qcam/viewfinder.h > > > > > +++ b/src/qcam/viewfinder.h > > > > > @@ -13,6 +13,8 @@ > > > > > #include <QList> > > > > > #include <QImage> > > > > > #include <QMutex> > > > > > +#include <QOpenGLWidget> > > > > > +#include <QOpenGLFunctions> > > > > > #include <QSize> > > > > > #include <QWidget> > > > > > > > > > > @@ -28,7 +30,23 @@ struct MappedBuffer { > > > > > size_t size; > > > > > }; > > > > > > > > > > -class ViewFinder : public QWidget > > > > > +class ViewFinderHandler > > > > > +{ > > > > > +public: > > > > > + ViewFinderHandler(); > > > > > + virtual ~ViewFinderHandler(); > > > > > + > > > > > + const QList<libcamera::PixelFormat> &nativeFormats() const; > > > > > + > > > > > + virtual int setFormat(const libcamera::PixelFormat &format, > const QSize &size) =0; > > > > > + virtual void render(libcamera::FrameBuffer *buffer, > MappedBuffer *map) =0; > > > > > + virtual void stop() =0; > > > > > + > > > > > + virtual QImage getCurrentImage() =0; > > > > > + > > > > > +}; > > > > > + > > > > > +class ViewFinder : public QWidget, public ViewFinderHandler > > > > > { > > > > > Q_OBJECT > > > > > > > > > > @@ -36,8 +54,6 @@ public: > > > > > ViewFinder(QWidget *parent); > > > > > ~ViewFinder(); > > > > > > > > > > - const QList<libcamera::PixelFormat> &nativeFormats() const; > > > > > - > > > > > int setFormat(const libcamera::PixelFormat &format, const > QSize &size); > > > > > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > > > > void stop(); > > > > > @@ -67,5 +83,4 @@ private: > > > > > QImage image_; > > > > > QMutex mutex_; /* Prevent concurrent access to image_ */ > > > > > }; > > > > > - > > > > > > This blank line should be kept. > > > > sure, will fix it. > > > > > > > #endif /* __QCAM_VIEWFINDER__ */ > > > > > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp > > > > > new file mode 100644 > > > > > index 0000000..7b47beb > > > > > --- /dev/null > > > > > +++ b/src/qcam/viewfinderGL.cpp > > > > > @@ -0,0 +1,335 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2020, Linaro > > > > > + * > > > > > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader > > > > > + */ > > > > > + > > > > > +#include "shader.h" > > > > > +#include "viewfinderGL.h" > > > > > + > > > > > +#include <QImage> > > > > > + > > > > > +#include <libcamera/formats.h> > > > > > + > > > > > +#define ATTRIB_VERTEX 0 > > > > > +#define ATTRIB_TEXTURE 1 > > > > > + > > > > > +ViewFinderGL::ViewFinderGL(QWidget *parent) > > > > > + : QOpenGLWidget(parent), > > > > > + glBuffer(QOpenGLBuffer::VertexBuffer), > > > > > + pFShader(nullptr), > > > > > + pVShader(nullptr), > > > > > + textureU(QOpenGLTexture::Target2D), > > > > > + textureV(QOpenGLTexture::Target2D), > > > > > + textureY(QOpenGLTexture::Target2D), > > > > > + yuvDataPtr(nullptr) > > > > > + > > > > > +{ > > > > > +} > > > > > + > > > > > +ViewFinderGL::~ViewFinderGL() > > > > > +{ > > > > > + removeShader(); > > > > > + > > > > > + if(textureY.isCreated()) > > > > > + textureY.destroy(); > > > > > + > > > > > + if(textureU.isCreated()) > > > > > + textureU.destroy(); > > > > > + > > > > > + if(textureV.isCreated()) > > > > > + textureV.destroy(); > > > > > + > > > > > + glBuffer.destroy(); > > > > > +} > > > > > + > > > > > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > > > > > + const QSize &size) > > > > > +{ > > > > > + format_ = format; > > > > > + size_ = size; > > > > > + > > > > > + updateGeometry(); > > > > > + return 0; > > > > > +} > > > > > + > > > > > +void ViewFinderGL::stop() > > > > > +{ > > > > > + if (buffer_) { > > > > > + renderComplete(buffer_); > > > > > + buffer_ = nullptr; > > > > > + } > > > > > +} > > > > > + > > > > > +QImage ViewFinderGL::getCurrentImage() > > > > > +{ > > > > > + QMutexLocker locker(&mutex_); > > > > > + > > > > > + return(grabFramebuffer()); > > > > > +} > > > > > + > > > > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, > MappedBuffer *map) > > > > > +{ > > > > > + if (buffer->planes().size() != 1) { > > > > > + qWarning() << "Multi-planar buffers are not supported"; > > > > > + return; > > > > > + } > > > > > + > > > > > + unsigned char *memory = static_cast<unsigned char > *>(map->memory); > > > > > + if(memory) { > > > > > > Can memory be null here ? > > > > I will rewrite this. > > > > > > > + yuvDataPtr = memory; > > > > > + update(); > > > > > + buffer_ = buffer; > > > > > + } > > > > > + > > > > > + if (buffer) > > > > > + renderComplete(buffer); > > > > > > Here's you're supposed to signal completion of the previous buffer, not > > > the current buffer. That's why there's a std::swap() in the existing > > > ViewFinder::render(). > > > > Is That means I should > > emit renderComplete(buffer_); after update(); > > then > > buffer_ = buffer; > > ... > > I don't quite understand what you mean, would you please explain it in > > detail? > > Forgive my stupid..:-P > > A stupid person couldn't have sent this patch in the first place :-) > > The ViewFinder component performs rendering asynchronously. It receives > a libcamera buffer in render(), and is tasked to display it on the > screen. The render() function returns before the rendering is complete. > We thus need to tell the caller when the rendering is complete, to let > it reuse the buffer once ViewFinder doesn't need it anymore. That's what > renderComplete() is for. > > With the code above, you signal that the buffer has been rendered right > after calling update(). That's not true, rendering is still in progress > at that point. You can only give the buffer back to the caller with > renderComplete() once a new buffer has been received to be rendered. > That's why the current ViewFinder implementation has > > std::swap(buffer_, buffer); > > After this call, buffer contains the old buffer, and we then call > > if (buffer) > renderComplete(buffer); > > to give the old buffer back. We need to check here if buffer is null, as > when the first buffer is rendered, there's no old buffer to be given > back. > Thanks for the detailed explanation. I totally misunderstood this portion before. BR, Show Liu > > > > > > +} > > > > > + > > > > > +void ViewFinderGL::updateFrame(unsigned char *buffer) > > > > > +{ > > > > > + if(buffer) { > > > > > + yuvDataPtr = buffer; > > > > > + update(); > > > > > + } > > > > > +} > > > > > + > > > > > +void ViewFinderGL::setFrameSize(int width, int height) > > > > > +{ > > > > > + if(width > 0 && height > 0) { > > > > > + width_ = width; > > > > > + height_ = height; > > > > > + } > > > > > +} > > > > > + > > > > > +char *ViewFinderGL::selectFragmentShader(unsigned int format) > > > > > +{ > > > > > + char *fsrc = nullptr; > > > > > + > > > > > + switch(format) { > > > > > + case libcamera::formats::NV12: > > > > > + horzSubSample_ = 2; > > > > > + vertSubSample_ = 2; > > > > > + fsrc = NV_2_planes_UV; > > > > > + break; > > > > > + case libcamera::formats::NV21: > > > > > + horzSubSample_ = 2; > > > > > + vertSubSample_ = 2; > > > > > + fsrc = NV_2_planes_VU; > > > > > + break; > > > > > + case libcamera::formats::NV16: > > > > > + horzSubSample_ = 2; > > > > > + vertSubSample_ = 1; > > > > > + fsrc = NV_2_planes_UV; > > > > > + break; > > > > > + case libcamera::formats::NV61: > > > > > + horzSubSample_ = 2; > > > > > + vertSubSample_ = 1; > > > > > + fsrc = NV_2_planes_VU; > > > > > + break; > > > > > + case libcamera::formats::NV24: > > > > > + horzSubSample_ = 1; > > > > > + vertSubSample_ = 1; > > > > > + fsrc = NV_2_planes_UV; > > > > > + break; > > > > > + case libcamera::formats::NV42: > > > > > + horzSubSample_ = 1; > > > > > + vertSubSample_ = 1; > > > > > + fsrc = NV_2_planes_VU; > > > > > + break; > > > > > + case libcamera::formats::YUV420: > > > > > + horzSubSample_ = 2; > > > > > + vertSubSample_ = 2; > > > > > + fsrc = NV_3_planes_UV; > > > > > + break; > > > > > + default: > > > > > + break; > > > > > + }; > > > > > + > > > > > + if(fsrc == nullptr) { > > > > > + qDebug() << __func__ << "[ERROR]:" <<" No suitable > fragment shader can be select."; > > > > > + } > > > > > + return fsrc; > > > > > +} > > > > > + > > > > > +void ViewFinderGL::createFragmentShader() > > > > > +{ > > > > > + bool bCompile; > > > > > + > > > > > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this); > > > > > + char *fsrc = selectFragmentShader(format_); > > > > > + > > > > > + bCompile = pFShader->compileSourceCode(fsrc); > > > > > + if(!bCompile) > > > > > + { > > > > > + qDebug() << __func__ << ":" << pFShader->log(); > > > > > + } > > > > > + > > > > > + shaderProgram.addShader(pFShader); > > > > > + > > > > > + // Link shader pipeline > > > > > + if (!shaderProgram.link()) { > > > > > + qDebug() << __func__ << ": shader program link > failed.\n" << shaderProgram.log(); > > > > > + close(); > > > > > + } > > > > > + > > > > > + // Bind shader pipeline for use > > > > > + if (!shaderProgram.bind()) { > > > > > + qDebug() << __func__ << ": shader program binding > failed.\n" << shaderProgram.log(); > > > > > + close(); > > > > > + } > > > > > + > > > > > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX); > > > > > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE); > > > > > + > > > > > + > shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat)); > > > > > + > shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat)); > > > > > + > > > > > + textureUniformY = shaderProgram.uniformLocation("tex_y"); > > > > > + textureUniformU = shaderProgram.uniformLocation("tex_u"); > > > > > + textureUniformV = shaderProgram.uniformLocation("tex_v"); > > > > > + > > > > > + if(!textureY.isCreated()) > > > > > + textureY.create(); > > > > > + > > > > > + if(!textureU.isCreated()) > > > > > + textureU.create(); > > > > > + > > > > > + if(!textureV.isCreated()) > > > > > + textureV.create(); > > > > > + > > > > > + id_y = textureY.textureId(); > > > > > + id_u = textureU.textureId(); > > > > > + id_v = textureV.textureId(); > > > > > +} > > > > > + > > > > > +void ViewFinderGL::configureTexture(unsigned int id) > > > > > +{ > > > > > + glBindTexture(GL_TEXTURE_2D, id); > > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, > GL_LINEAR); > > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, > GL_LINEAR); > > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, > GL_CLAMP_TO_EDGE); > > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, > GL_CLAMP_TO_EDGE); > > > > > +} > > > > > + > > > > > +void ViewFinderGL::removeShader() > > > > > +{ > > > > > + if (shaderProgram.isLinked()) { > > > > > + shaderProgram.release(); > > > > > + shaderProgram.removeAllShaders(); > > > > > + } > > > > > + > > > > > + if(pFShader) > > > > > + delete pFShader; > > > > > + > > > > > + if(pVShader) > > > > > + delete pVShader; > > > > > > If we stop and restart the stream with a different format, the previous > > > shader will be used, as removeShader() isn't called. I don't think > > > that's right. I believe you need to call removeShader() at the > beginning > > > of setFormat(). > > > > Yes. you're right. I will refine the setFormat() function. > > > > > > > +} > > > > > + > > > > > +void ViewFinderGL::initializeGL() > > > > > +{ > > > > > + bool bCompile; > > > > > + > > > > > + initializeOpenGLFunctions(); > > > > > + glEnable(GL_TEXTURE_2D); > > > > > + glDisable(GL_DEPTH_TEST); > > > > > + > > > > > + static const GLfloat vertices[] { > > > > > + -1.0f,-1.0f, > > > > > + -1.0f,+1.0f, > > > > > + +1.0f,+1.0f, > > > > > + +1.0f,-1.0f, > > > > > + 0.0f,1.0f, > > > > > + 0.0f,0.0f, > > > > > + 1.0f,0.0f, > > > > > + 1.0f,1.0f, > > > > > + }; > > > > > + > > > > > + glBuffer.create(); > > > > > + glBuffer.bind(); > > > > > + glBuffer.allocate(vertices,sizeof(vertices)); > > > > > + > > > > > + /* Create Vertex Shader */ > > > > > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this); > > > > > + char *vsrc = NV_Vertex_shader; > > > > > + > > > > > + bCompile = pVShader->compileSourceCode(vsrc); > > > > > + if(!bCompile) { > > > > > + qDebug() << __func__<< ":" << pVShader->log(); > > > > > + } > > > > > + > > > > > + shaderProgram.addShader(pVShader); > > > > > + > > > > > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f); > > > > > +} > > > > > + > > > > > +void ViewFinderGL::render() > > > > > +{ > > > > > + switch(format_) { > > > > > + case libcamera::formats::NV12: > > > > > + case libcamera::formats::NV21: > > > > > + case libcamera::formats::NV16: > > > > > + case libcamera::formats::NV61: > > > > > + case libcamera::formats::NV24: > > > > > + case libcamera::formats::NV42: > > > > > + /* activate texture 0 */ > > > > > + glActiveTexture(GL_TEXTURE0); > > > > > + configureTexture(id_y); > > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), > size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > > > > + glUniform1i(textureUniformY, 0); > > > > > + > > > > > + /* activate texture 1 */ > > > > > + glActiveTexture(GL_TEXTURE1); > > > > > + configureTexture(id_u); > > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, > GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > > > > + glUniform1i(textureUniformU, 1); > > > > > + break; > > > > > + case libcamera::formats::YUV420: > > > > > + /* activate texture 0 */ > > > > > + glActiveTexture(GL_TEXTURE0); > > > > > + configureTexture(id_y); > > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), > size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); > > > > > + glUniform1i(textureUniformY, 0); > > > > > + > > > > > + /* activate texture 1 */ > > > > > + glActiveTexture(GL_TEXTURE1); > > > > > + configureTexture(id_u); > > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, > GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); > > > > > + glUniform1i(textureUniformU, 1); > > > > > + > > > > > + /* activate texture 2 */ > > > > > + glActiveTexture(GL_TEXTURE2); > > > > > + configureTexture(id_v); > > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, > size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, > GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()*5/4); > > > > > + glUniform1i(textureUniformV, 2); > > > > > + break; > > > > > + default: > > > > > + break; > > > > > + }; > > > > > +} > > > > > + > > > > > +void ViewFinderGL::paintGL() > > > > > +{ > > > > > + if(pFShader == nullptr) > > > > > > We tend to write > > > > > > if (!pFShader) > > > > ok will fix it. > > > > > > > + createFragmentShader(); > > > > > > I wonder if we could do this in setFormat(). > > > > That should be ok, I will try to move this when setting Format. > > > > > > > + > > > > > + if(yuvDataPtr) > > > > > + { > > > > > + glClearColor(0.0, 0.0, 0.0, 1.0); > > > > > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT ); > > > > > + > > > > > + render(); > > > > > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); > > > > > + } > > > > > +} > > > > > + > > > > > +void ViewFinderGL::resizeGL(int w, int h) > > > > > +{ > > > > > + glViewport(0,0,w,h); > > > > > +} > > > > > + > > > > > +QSize ViewFinderGL::sizeHint() const > > > > > +{ > > > > > + return size_.isValid() ? size_ : QSize(640, 480); > > > > > +} > > > > > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h > > > > > new file mode 100644 > > > > > index 0000000..08662ca > > > > > --- /dev/null > > > > > +++ b/src/qcam/viewfinderGL.h > > > > > @@ -0,0 +1,101 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2020, Linaro > > > > > + * > > > > > + * viewfinderGL.h - Render YUV format frame by OpenGL shader > > > > > + */ > > > > > +#ifndef __VIEWFINDERGL_H__ > > > > > +#define __VIEWFINDERGL_H__ > > > > > + > > > > > +#include <fcntl.h> > > > > > +#include <string.h> > > > > > +#include <unistd.h> > > > > > + > > > > > +#include <iomanip> > > > > > +#include <iostream> > > > > > +#include <sstream> > > > > > + > > > > > > Do you need all these headers in the .h file ? They seem to belong to > > > the .cpp file. > > > > OK I will check or just remove it. > > > > > > > +#include <QImage> > > > > > +#include <QOpenGLBuffer> > > > > > +#include <QOpenGLFunctions> > > > > > +#include <QOpenGLShader> > > > > > +#include <QOpenGLShaderProgram> > > > > > +#include <QSize> > > > > > +#include <QOpenGLTexture> > > > > > +#include <QOpenGLWidget> > > > > > + > > > > > +#include <libcamera/buffer.h> > > > > > +#include <libcamera/pixel_format.h> > > > > > > Blank line missing here. > > > > sure, will do . > > > > > > > +#include "viewfinder.h" > > > > > + > > > > > +class ViewFinderGL : public QOpenGLWidget, > > > > > + public ViewFinderHandler, > > > > > + protected QOpenGLFunctions > > > > > +{ > > > > > + Q_OBJECT > > > > > + > > > > > +public: > > > > > + ViewFinderGL(QWidget *parent = 0); > > > > > + ~ViewFinderGL(); > > > > > + > > > > > + int setFormat(const libcamera::PixelFormat &format, const > QSize &size); > > > > > > You should add "override" to qualify all overridden functions. > > > > > > int setFormat(const libcamera::PixelFormat &format, > > > const QSize &size) override; > > > > ok will do. > > > > > > > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); > > > > > + void stop(); > > > > > + > > > > > + QImage getCurrentImage(); > > > > > + > > > > > + void setFrameSize(int width, int height); > > > > > + void updateFrame(unsigned char *buffer); > > > > > > Those two functions seem unused. > > > > sure, I will remove it. > > > > > > > + > > > > > + char *selectFragmentShader(unsigned int format); > > > > > + void createFragmentShader(); > > > > > > And these two functions can be private. > > > > will move to private. > > > > > > > + void render(); > > > > > + > > > > > +Q_SIGNALS: > > > > > + void renderComplete(libcamera::FrameBuffer *buffer); > > > > > + > > > > > +protected: > > > > > + void initializeGL() Q_DECL_OVERRIDE; > > > > > > You can use "override" directly, we know the compiler supports it. > > > > ok, I will fix it up. > > > > > > > + void paintGL() Q_DECL_OVERRIDE; > > > > > + void resizeGL(int w, int h) Q_DECL_OVERRIDE; > > > > > + QSize sizeHint() const override; > > > > > + > > > > > +private: > > > > > + > > > > > + void configureTexture(unsigned int id); > > > > > + void removeShader(); > > > > > + > > > > > + /* Captured image size, format and buffer */ > > > > > + libcamera::FrameBuffer *buffer_; > > > > > + libcamera::PixelFormat format_; > > > > > + QOpenGLBuffer glBuffer; > > > > > + QSize size_; > > > > > + > > > > > + GLuint id_u; > > > > > + GLuint id_v; > > > > > + GLuint id_y; > > > > > + > > > > > + QMutex mutex_; /* Prevent concurrent access to image_ */ > > > > > + > > > > > + QOpenGLShader *pFShader; > > > > > + QOpenGLShader *pVShader; > > > > > + QOpenGLShaderProgram shaderProgram; > > > > > + > > > > > + GLuint textureUniformU; > > > > > + GLuint textureUniformV; > > > > > + GLuint textureUniformY; > > > > > + > > > > > + QOpenGLTexture textureU; > > > > > + QOpenGLTexture textureV; > > > > > + QOpenGLTexture textureY; > > > > > + > > > > > + unsigned int width_; > > > > > + unsigned int height_; > > > > > + > > > > > + unsigned char* yuvDataPtr; > > > > > + > > > > > + /* NV parameters */ > > > > > + unsigned int horzSubSample_ ; > > > > > + unsigned int vertSubSample_; > > > > > +}; > > > > > +#endif // __VIEWFINDERGL_H__ > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp index b3468cb..a3ea471 100644 --- a/src/qcam/main.cpp +++ b/src/qcam/main.cpp @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc, char *argv[]) "help"); parser.addOption(OptStream, &streamKeyValue, "Set configuration of a camera stream", "stream", true); + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats frame via OpenGL shader", + "opengl"); OptionsParser::Options options = parser.parse(argc, argv); if (options.isSet(OptHelp)) diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 7bc1360..6edf370 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -28,6 +28,9 @@ #include <libcamera/version.h> #include "dng_writer.h" +#include "main_window.h" +#include "viewfinder.h" +#include "viewfinderGL.h" using namespace libcamera; @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) setWindowTitle(title_); connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle())); - viewfinder_ = new ViewFinder(this); - connect(viewfinder_, &ViewFinder::renderComplete, - this, &MainWindow::queueRequest); - setCentralWidget(viewfinder_); + if (options_.isSet(OptOpenGL)) { + viewfinder_ = new ViewFinderGL(this); + connect(dynamic_cast<ViewFinderGL *>(viewfinder_), &ViewFinderGL::renderComplete, + this, &MainWindow::queueRequest); + setCentralWidget(dynamic_cast<ViewFinderGL *>(viewfinder_)); + } else { + viewfinder_ = new ViewFinder(this); + connect(dynamic_cast<ViewFinder *>(viewfinder_), &ViewFinder::renderComplete, + this, &MainWindow::queueRequest); + setCentralWidget(dynamic_cast<ViewFinder *>(viewfinder_)); + } + adjustSize(); /* Hotplug/unplug support */ diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h index 4606fe4..a852ef4 100644 --- a/src/qcam/main_window.h +++ b/src/qcam/main_window.h @@ -38,6 +38,7 @@ enum { OptCamera = 'c', OptHelp = 'h', OptStream = 's', + OptOpenGL = 'o', }; class CaptureRequest @@ -102,7 +103,7 @@ private: QAction *startStopAction_; QComboBox *cameraCombo_; QAction *saveRaw_; - ViewFinder *viewfinder_; + ViewFinderHandler *viewfinder_; QIcon iconPlay_; QIcon iconStop_; diff --git a/src/qcam/meson.build b/src/qcam/meson.build index 045db52..6a58947 100644 --- a/src/qcam/meson.build +++ b/src/qcam/meson.build @@ -7,11 +7,13 @@ qcam_sources = files([ 'main.cpp', 'main_window.cpp', 'viewfinder.cpp', + 'viewfinderGL.cpp' ]) qcam_moc_headers = files([ 'main_window.h', 'viewfinder.h', + 'viewfinderGL.h' ]) qcam_resources = files([ diff --git a/src/qcam/shader.h b/src/qcam/shader.h new file mode 100644 index 0000000..f54c264 --- /dev/null +++ b/src/qcam/shader.h @@ -0,0 +1,104 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2020, Linaro + * + * shader.h - YUV format converter shader source code + */ +#ifndef __SHADER_H__ +#define __SHADER_H__ + +/* Vertex shader for NV formats */ +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n" + "attribute vec2 textureIn;\n" + "varying vec2 textureOut;\n" + "void main(void)\n" + "{\n" + "gl_Position = vertexIn;\n" + "textureOut = textureIn;\n" + "}\n"; + +/* Fragment shader for NV12, NV16 and NV24 */ +char NV_2_planes_UV[] = "#ifdef GL_ES\n" + "precision highp float;\n" + "#endif\n" + "varying vec2 textureOut;\n" + "uniform sampler2D tex_y;\n" + "uniform sampler2D tex_u;\n" + "void main(void)\n" + "{\n" + "vec3 yuv;\n" + "vec3 rgb;\n" + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" + " vec3(0.0, -0.390625, 2.015625),\n" + " vec3(1.5975625, -0.8125, 0.0));\n" + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n" + "rgb = convert_mat * yuv;\n" + "gl_FragColor = vec4(rgb, 1.0);\n" + "}\n"; + +/* Fragment shader for NV21, NV61 and NV42 */ +char NV_2_planes_VU[] = "#ifdef GL_ES\n" + "precision highp float;\n" + "#endif\n" + "varying vec2 textureOut;\n" + "uniform sampler2D tex_y;\n" + "uniform sampler2D tex_u;\n" + "void main(void)\n" + "{\n" + "vec3 yuv;\n" + "vec3 rgb;\n" + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" + " vec3(0.0, -0.390625, 2.015625),\n" + " vec3(1.5975625, -0.8125, 0.0));\n" + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n" + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" + "rgb = convert_mat * yuv;\n" + "gl_FragColor = vec4(rgb, 1.0);\n" + "}\n"; + +/* Fragment shader for YUV420 */ +char NV_3_planes_UV[] = "#ifdef GL_ES\n" + "precision highp float;\n" + "#endif\n" + "varying vec2 textureOut;\n" + "uniform sampler2D tex_y;\n" + "uniform sampler2D tex_u;\n" + "uniform sampler2D tex_v;\n" + "void main(void)\n" + "{\n" + "vec3 yuv;\n" + "vec3 rgb;\n" + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" + " vec3(0.0, -0.390625, 2.015625),\n" + " vec3(1.5975625, -0.8125, 0.0));\n" + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n" + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n" + "rgb = convert_mat * yuv;\n" + "gl_FragColor = vec4(rgb, 1);\n" + "}\n"; + +/* Fragment shader for YVU420 */ +char NV_3_planes_VU[] = "precision highp float;\n" + "#endif\n" + "varying vec2 textureOut;\n" + "uniform sampler2D tex_y;\n" + "uniform sampler2D tex_u;\n" + "uniform sampler2D tex_v;\n" + "void main(void)\n" + "{\n" + "vec3 yuv;\n" + "vec3 rgb;\n" + "mat3 convert_mat = mat3(vec3(1.1640625, 1.1640625, 1.1640625),\n" + " vec3(0.0, -0.390625, 2.015625),\n" + " vec3(1.5975625, -0.8125, 0.0));\n" + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n" + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n" + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n" + "rgb = convert_mat * yuv;\n" + "gl_FragColor = vec4(rgb, 1);\n" + "}\n"; +#endif // __SHADER_H__ diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp index dcf8a15..d3a2422 100644 --- a/src/qcam/viewfinder.cpp +++ b/src/qcam/viewfinder.cpp @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats { libcamera::formats::BGR888, QImage::Format_RGB888 }, }; -ViewFinder::ViewFinder(QWidget *parent) - : QWidget(parent), buffer_(nullptr) +ViewFinderHandler::ViewFinderHandler() { - icon_ = QIcon(":camera-off.svg"); } -ViewFinder::~ViewFinder() +ViewFinderHandler::~ViewFinderHandler() { } -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats() const +const QList<libcamera::PixelFormat> &ViewFinderHandler::nativeFormats() const { static const QList<libcamera::PixelFormat> formats = ::nativeFormats.keys(); return formats; } +ViewFinder::ViewFinder(QWidget *parent) + : QWidget(parent), buffer_(nullptr) +{ + icon_ = QIcon(":camera-off.svg"); +} + +ViewFinder::~ViewFinder() +{ +} + int ViewFinder::setFormat(const libcamera::PixelFormat &format, const QSize &size) { diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h index 26a1320..741d694 100644 --- a/src/qcam/viewfinder.h +++ b/src/qcam/viewfinder.h @@ -13,6 +13,8 @@ #include <QList> #include <QImage> #include <QMutex> +#include <QOpenGLWidget> +#include <QOpenGLFunctions> #include <QSize> #include <QWidget> @@ -28,7 +30,23 @@ struct MappedBuffer { size_t size; }; -class ViewFinder : public QWidget +class ViewFinderHandler +{ +public: + ViewFinderHandler(); + virtual ~ViewFinderHandler(); + + const QList<libcamera::PixelFormat> &nativeFormats() const; + + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) =0; + virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) =0; + virtual void stop() =0; + + virtual QImage getCurrentImage() =0; + +}; + +class ViewFinder : public QWidget, public ViewFinderHandler { Q_OBJECT @@ -36,8 +54,6 @@ public: ViewFinder(QWidget *parent); ~ViewFinder(); - const QList<libcamera::PixelFormat> &nativeFormats() const; - int setFormat(const libcamera::PixelFormat &format, const QSize &size); void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); void stop(); @@ -67,5 +83,4 @@ private: QImage image_; QMutex mutex_; /* Prevent concurrent access to image_ */ }; - #endif /* __QCAM_VIEWFINDER__ */ diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp new file mode 100644 index 0000000..7b47beb --- /dev/null +++ b/src/qcam/viewfinderGL.cpp @@ -0,0 +1,335 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2020, Linaro + * + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader + */ + +#include "shader.h" +#include "viewfinderGL.h" + +#include <QImage> + +#include <libcamera/formats.h> + +#define ATTRIB_VERTEX 0 +#define ATTRIB_TEXTURE 1 + +ViewFinderGL::ViewFinderGL(QWidget *parent) + : QOpenGLWidget(parent), + glBuffer(QOpenGLBuffer::VertexBuffer), + pFShader(nullptr), + pVShader(nullptr), + textureU(QOpenGLTexture::Target2D), + textureV(QOpenGLTexture::Target2D), + textureY(QOpenGLTexture::Target2D), + yuvDataPtr(nullptr) + +{ +} + +ViewFinderGL::~ViewFinderGL() +{ + removeShader(); + + if(textureY.isCreated()) + textureY.destroy(); + + if(textureU.isCreated()) + textureU.destroy(); + + if(textureV.isCreated()) + textureV.destroy(); + + glBuffer.destroy(); +} + +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, + const QSize &size) +{ + format_ = format; + size_ = size; + + updateGeometry(); + return 0; +} + +void ViewFinderGL::stop() +{ + if (buffer_) { + renderComplete(buffer_); + buffer_ = nullptr; + } +} + +QImage ViewFinderGL::getCurrentImage() +{ + QMutexLocker locker(&mutex_); + + return(grabFramebuffer()); +} + +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map) +{ + if (buffer->planes().size() != 1) { + qWarning() << "Multi-planar buffers are not supported"; + return; + } + + unsigned char *memory = static_cast<unsigned char *>(map->memory); + if(memory) { + yuvDataPtr = memory; + update(); + buffer_ = buffer; + } + + if (buffer) + renderComplete(buffer); +} + +void ViewFinderGL::updateFrame(unsigned char *buffer) +{ + if(buffer) { + yuvDataPtr = buffer; + update(); + } +} + +void ViewFinderGL::setFrameSize(int width, int height) +{ + if(width > 0 && height > 0) { + width_ = width; + height_ = height; + } +} + +char *ViewFinderGL::selectFragmentShader(unsigned int format) +{ + char *fsrc = nullptr; + + switch(format) { + case libcamera::formats::NV12: + horzSubSample_ = 2; + vertSubSample_ = 2; + fsrc = NV_2_planes_UV; + break; + case libcamera::formats::NV21: + horzSubSample_ = 2; + vertSubSample_ = 2; + fsrc = NV_2_planes_VU; + break; + case libcamera::formats::NV16: + horzSubSample_ = 2; + vertSubSample_ = 1; + fsrc = NV_2_planes_UV; + break; + case libcamera::formats::NV61: + horzSubSample_ = 2; + vertSubSample_ = 1; + fsrc = NV_2_planes_VU; + break; + case libcamera::formats::NV24: + horzSubSample_ = 1; + vertSubSample_ = 1; + fsrc = NV_2_planes_UV; + break; + case libcamera::formats::NV42: + horzSubSample_ = 1; + vertSubSample_ = 1; + fsrc = NV_2_planes_VU; + break; + case libcamera::formats::YUV420: + horzSubSample_ = 2; + vertSubSample_ = 2; + fsrc = NV_3_planes_UV; + break; + default: + break; + }; + + if(fsrc == nullptr) { + qDebug() << __func__ << "[ERROR]:" <<" No suitable fragment shader can be select."; + } + return fsrc; +} + +void ViewFinderGL::createFragmentShader() +{ + bool bCompile; + + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this); + char *fsrc = selectFragmentShader(format_); + + bCompile = pFShader->compileSourceCode(fsrc); + if(!bCompile) + { + qDebug() << __func__ << ":" << pFShader->log(); + } + + shaderProgram.addShader(pFShader); + + // Link shader pipeline + if (!shaderProgram.link()) { + qDebug() << __func__ << ": shader program link failed.\n" << shaderProgram.log(); + close(); + } + + // Bind shader pipeline for use + if (!shaderProgram.bind()) { + qDebug() << __func__ << ": shader program binding failed.\n" << shaderProgram.log(); + close(); + } + + shaderProgram.enableAttributeArray(ATTRIB_VERTEX); + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE); + + shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat)); + shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat)); + + textureUniformY = shaderProgram.uniformLocation("tex_y"); + textureUniformU = shaderProgram.uniformLocation("tex_u"); + textureUniformV = shaderProgram.uniformLocation("tex_v"); + + if(!textureY.isCreated()) + textureY.create(); + + if(!textureU.isCreated()) + textureU.create(); + + if(!textureV.isCreated()) + textureV.create(); + + id_y = textureY.textureId(); + id_u = textureU.textureId(); + id_v = textureV.textureId(); +} + +void ViewFinderGL::configureTexture(unsigned int id) +{ + glBindTexture(GL_TEXTURE_2D, id); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); +} + +void ViewFinderGL::removeShader() +{ + if (shaderProgram.isLinked()) { + shaderProgram.release(); + shaderProgram.removeAllShaders(); + } + + if(pFShader) + delete pFShader; + + if(pVShader) + delete pVShader; +} + +void ViewFinderGL::initializeGL() +{ + bool bCompile; + + initializeOpenGLFunctions(); + glEnable(GL_TEXTURE_2D); + glDisable(GL_DEPTH_TEST); + + static const GLfloat vertices[] { + -1.0f,-1.0f, + -1.0f,+1.0f, + +1.0f,+1.0f, + +1.0f,-1.0f, + 0.0f,1.0f, + 0.0f,0.0f, + 1.0f,0.0f, + 1.0f,1.0f, + }; + + glBuffer.create(); + glBuffer.bind(); + glBuffer.allocate(vertices,sizeof(vertices)); + + /* Create Vertex Shader */ + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this); + char *vsrc = NV_Vertex_shader; + + bCompile = pVShader->compileSourceCode(vsrc); + if(!bCompile) { + qDebug() << __func__<< ":" << pVShader->log(); + } + + shaderProgram.addShader(pVShader); + + glClearColor(1.0f, 1.0f, 1.0f, 0.0f); +} + +void ViewFinderGL::render() +{ + switch(format_) { + case libcamera::formats::NV12: + case libcamera::formats::NV21: + case libcamera::formats::NV16: + case libcamera::formats::NV61: + case libcamera::formats::NV24: + case libcamera::formats::NV42: + /* activate texture 0 */ + glActiveTexture(GL_TEXTURE0); + configureTexture(id_y); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); + glUniform1i(textureUniformY, 0); + + /* activate texture 1 */ + glActiveTexture(GL_TEXTURE1); + configureTexture(id_u); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); + glUniform1i(textureUniformU, 1); + break; + case libcamera::formats::YUV420: + /* activate texture 0 */ + glActiveTexture(GL_TEXTURE0); + configureTexture(id_y); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr); + glUniform1i(textureUniformY, 0); + + /* activate texture 1 */ + glActiveTexture(GL_TEXTURE1); + configureTexture(id_u); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()); + glUniform1i(textureUniformU, 1); + + /* activate texture 2 */ + glActiveTexture(GL_TEXTURE2); + configureTexture(id_v); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG, size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG, GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()*5/4); + glUniform1i(textureUniformV, 2); + break; + default: + break; + }; +} + +void ViewFinderGL::paintGL() +{ + if(pFShader == nullptr) + createFragmentShader(); + + if(yuvDataPtr) + { + glClearColor(0.0, 0.0, 0.0, 1.0); + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT ); + + render(); + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); + } +} + +void ViewFinderGL::resizeGL(int w, int h) +{ + glViewport(0,0,w,h); +} + +QSize ViewFinderGL::sizeHint() const +{ + return size_.isValid() ? size_ : QSize(640, 480); +} diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h new file mode 100644 index 0000000..08662ca --- /dev/null +++ b/src/qcam/viewfinderGL.h @@ -0,0 +1,101 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2020, Linaro + * + * viewfinderGL.h - Render YUV format frame by OpenGL shader + */ +#ifndef __VIEWFINDERGL_H__ +#define __VIEWFINDERGL_H__ + +#include <fcntl.h> +#include <string.h> +#include <unistd.h> + +#include <iomanip> +#include <iostream> +#include <sstream> + +#include <QImage> +#include <QOpenGLBuffer> +#include <QOpenGLFunctions> +#include <QOpenGLShader> +#include <QOpenGLShaderProgram> +#include <QSize> +#include <QOpenGLTexture> +#include <QOpenGLWidget> + +#include <libcamera/buffer.h> +#include <libcamera/pixel_format.h> +#include "viewfinder.h" + +class ViewFinderGL : public QOpenGLWidget, + public ViewFinderHandler, + protected QOpenGLFunctions +{ + Q_OBJECT + +public: + ViewFinderGL(QWidget *parent = 0); + ~ViewFinderGL(); + + int setFormat(const libcamera::PixelFormat &format, const QSize &size); + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map); + void stop(); + + QImage getCurrentImage(); + + void setFrameSize(int width, int height); + void updateFrame(unsigned char *buffer); + + char *selectFragmentShader(unsigned int format); + void createFragmentShader(); + void render(); + +Q_SIGNALS: + void renderComplete(libcamera::FrameBuffer *buffer); + +protected: + void initializeGL() Q_DECL_OVERRIDE; + void paintGL() Q_DECL_OVERRIDE; + void resizeGL(int w, int h) Q_DECL_OVERRIDE; + QSize sizeHint() const override; + +private: + + void configureTexture(unsigned int id); + void removeShader(); + + /* Captured image size, format and buffer */ + libcamera::FrameBuffer *buffer_; + libcamera::PixelFormat format_; + QOpenGLBuffer glBuffer; + QSize size_; + + GLuint id_u; + GLuint id_v; + GLuint id_y; + + QMutex mutex_; /* Prevent concurrent access to image_ */ + + QOpenGLShader *pFShader; + QOpenGLShader *pVShader; + QOpenGLShaderProgram shaderProgram; + + GLuint textureUniformU; + GLuint textureUniformV; + GLuint textureUniformY; + + QOpenGLTexture textureU; + QOpenGLTexture textureV; + QOpenGLTexture textureY; + + unsigned int width_; + unsigned int height_; + + unsigned char* yuvDataPtr; + + /* NV parameters */ + unsigned int horzSubSample_ ; + unsigned int vertSubSample_; +}; +#endif // __VIEWFINDERGL_H__
The patch is to render the NV family YUV formats by OpenGL shader. V3: refine the fragment shader for better pixel color. Signed-off-by: Show Liu <show.liu@linaro.org> --- src/qcam/main.cpp | 2 + src/qcam/main_window.cpp | 19 ++- src/qcam/main_window.h | 3 +- src/qcam/meson.build | 2 + src/qcam/shader.h | 104 ++++++++++++ src/qcam/viewfinder.cpp | 18 +- src/qcam/viewfinder.h | 23 ++- src/qcam/viewfinderGL.cpp | 335 ++++++++++++++++++++++++++++++++++++++ src/qcam/viewfinderGL.h | 101 ++++++++++++ 9 files changed, 593 insertions(+), 14 deletions(-) create mode 100644 src/qcam/shader.h create mode 100644 src/qcam/viewfinderGL.cpp create mode 100644 src/qcam/viewfinderGL.h