[libcamera-devel] [PATCH v3 1/1] qcam: Render YUV formats frame by OpenGL shader
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 30 18:26:24 CEST 2020
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 at 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__
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list