[libcamera-devel] [PATCH v3 1/1] qcam: Render YUV formats frame by OpenGL shader
Show Liu
show.liu at linaro.org
Fri Jul 3 09:46:32 CEST 2020
Hi Niklas, Laurent,
Thanks for the review.
On Wed, Jul 1, 2020 at 12:26 AM Laurent Pinchart <
laurent.pinchart at 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 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}
>
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200703/7b283493/attachment-0001.htm>
More information about the libcamera-devel
mailing list