[libcamera-devel] [PATCH v3 1/1] qcam: Render YUV formats frame by OpenGL shader
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 3 12:14:05 CEST 2020
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 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.:-)
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__
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list