[libcamera-devel] [PATCH v3 1/1] qcam: Render YUV formats frame by OpenGL shader
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jun 24 11:34:39 CEST 2020
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 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");
(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__
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list