[libcamera-devel] [PATCH v3 1/1] qcam: Render YUV formats frame by OpenGL shader
Show Liu
show.liu at linaro.org
Mon Jul 6 10:06:04 CEST 2020
Hi Laurent,
On Fri, Jul 3, 2020 at 6:14 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Show,
>
> On Fri, Jul 03, 2020 at 03:46:32PM +0800, Show Liu wrote:
> > On Wed, Jul 1, 2020 at 12:26 AM Laurent Pinchart wrote:
> > > On Tue, Jun 30, 2020 at 05:03:43PM +0200, Niklas Söderlund wrote:
> > > > Hi Show,
> > > >
> > > > Thanks for your work.
> > >
> > > Likewise :-)
> > >
> > > > I really like this version! The structure is almost there and much
> > > > better then previous versions. As Kieran points out there are style
> > > > errors that checkstyle.py will help you point out so I will ignore
> them
> > > > in this review.
> >
> > Thank you.:-)
> >
> > > > On 2020-06-24 15:37:05 +0800, Show Liu wrote:
> > > > > The patch is to render the NV family YUV formats by OpenGL shader.
> > > > > V3: refine the fragment shader for better pixel color.
> > > > >
> > > > > Signed-off-by: Show Liu <show.liu 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.
>
OK. Understood. I will try this in the next version.
>
> > > > >
> > > > > OptionsParser::Options options = parser.parse(argc, argv);
> > > > > if (options.isSet(OptHelp))
> > > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > > > index 7bc1360..6edf370 100644
> > > > > --- a/src/qcam/main_window.cpp
> > > > > +++ b/src/qcam/main_window.cpp
> > > > > @@ -28,6 +28,9 @@
> > > > > #include <libcamera/version.h>
> > > > >
> > > > > #include "dng_writer.h"
> > > > > +#include "main_window.h"
> > > > > +#include "viewfinder.h"
> > > > > +#include "viewfinderGL.h"
> > >
> > > According to the name scheme we use, I think this should be
> > > viewfinder_gl.h.
> >
> > OK. will fix it.
> >
> > > > > using namespace libcamera;
> > > > >
> > > > > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm,
> const OptionsParser::Options &options)
> > > > > setWindowTitle(title_);
> > > > > connect(&titleTimer_, SIGNAL(timeout()), this,
> SLOT(updateTitle()));
> > > > >
> > > > > - viewfinder_ = new ViewFinder(this);
> > > > > - connect(viewfinder_, &ViewFinder::renderComplete,
> > > > > - this, &MainWindow::queueRequest);
> > > > > - setCentralWidget(viewfinder_);
> > > > > + if (options_.isSet(OptOpenGL)) {
> > > > > + viewfinder_ = new ViewFinderGL(this);
> > > > > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_),
> &ViewFinderGL::renderComplete,
> > > > > + this, &MainWindow::queueRequest);
> > > > > + setCentralWidget(dynamic_cast<ViewFinderGL
> *>(viewfinder_));
> > > > > + } else {
> > > > > + viewfinder_ = new ViewFinder(this);
> > > > > + connect(dynamic_cast<ViewFinder *>(viewfinder_),
> &ViewFinder::renderComplete,
> > > > > + this, &MainWindow::queueRequest);
> > > > > + setCentralWidget(dynamic_cast<ViewFinder
> *>(viewfinder_));
> > > > > + }
> > > >
> > > > I understand that one can't inherit from QObject twice, but this
> looks
> > > > odd. Is there someway the base class could inherit from QObject or
> > > > possibly QWidget and the derived classes hide their specilization
> > > > somehow? I'm no Qt export so maybe I'm asking for something
> impossible
> > > > or really stupid.
> >
> > No, I really appreciate your opinions, it'll help me to make this
> better. :)
> >
> > > If we assume all subclasses of Viewfinder will be QWidget, we could do
> > >
> > > if (options_.isSet(OptOpenGL))
> > > viewfinder_ = new ViewFinderGL(this);
> > > else
> > > viewfinder_ = new ViewFinder(this);
> > >
> > > QWidget *vf>idget = dynamic_cast<QWidget>(viewfinder_);
> > > connect(vfWidget, &ViewFinderHandler::renderComplete,
> > > this, &MainWindow::queueRequest);
> > > setCentralWidget(vfWidget);
> > >
> > > With ViewFinderHandler::renderComplete() being a signal in the base
> > > class.
> >
> > I will try this way in the next version.
> >
> > > > > +
> > > > > adjustSize();
> > > > >
> > > > > /* Hotplug/unplug support */
> > > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > > > index 4606fe4..a852ef4 100644
> > > > > --- a/src/qcam/main_window.h
> > > > > +++ b/src/qcam/main_window.h
> > > > > @@ -38,6 +38,7 @@ enum {
> > > > > OptCamera = 'c',
> > > > > OptHelp = 'h',
> > > > > OptStream = 's',
> > > > > + OptOpenGL = 'o',
> > > > > };
> > > > >
> > > > > class CaptureRequest
> > > > > @@ -102,7 +103,7 @@ private:
> > > > > QAction *startStopAction_;
> > > > > QComboBox *cameraCombo_;
> > > > > QAction *saveRaw_;
> > > > > - ViewFinder *viewfinder_;
> > > > > + ViewFinderHandler *viewfinder_;
> > >
> > > I'd split this patch in two, with one patch that renames the existing
> > > ViewFinder to ViewFinderQt and creates a ViewFinder base class (which
> > > you call ViewFinderHandler here), and a second patch that adds
> > > ViewFinderGL.
> >
> > OK. will do.
> >
> > > > > QIcon iconPlay_;
> > > > > QIcon iconStop_;
> > > > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> > > > > index 045db52..6a58947 100644
> > > > > --- a/src/qcam/meson.build
> > > > > +++ b/src/qcam/meson.build
> > > > > @@ -7,11 +7,13 @@ qcam_sources = files([
> > > > > 'main.cpp',
> > > > > 'main_window.cpp',
> > > > > 'viewfinder.cpp',
> > > > > + 'viewfinderGL.cpp'
> > > > > ])
> > > > >
> > > > > qcam_moc_headers = files([
> > > > > 'main_window.h',
> > > > > 'viewfinder.h',
> > > > > + 'viewfinderGL.h'
> > > > > ])
> > > > >
> > > > > qcam_resources = files([
> > > > > diff --git a/src/qcam/shader.h b/src/qcam/shader.h
> > > > > new file mode 100644
> > > > > index 0000000..f54c264
> > > > > --- /dev/null
> > > > > +++ b/src/qcam/shader.h
> > > > > @@ -0,0 +1,104 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2020, Linaro
> > > > > + *
> > > > > + * shader.h - YUV format converter shader source code
> > > > > + */
> > > > > +#ifndef __SHADER_H__
> > > > > +#define __SHADER_H__
> > > >
> > > > I think the content of this file should be moved inside
> viewfinderGL.cpp
> > >
> > > Or maybe to viewfinder_gl_shader.cpp if we want to keep it separate.
> > > Header files should only contain the declarations in any case, not the
> > > actual variable contents.
> > >
> > > Ideally we should store the shaders in files of their own, not in a C
> > > array, and have them pulled in as Qt resources
> > > (
> https://doc.qt.io/qt-5/resources.html#using-resources-in-the-application
> > > ).
> > > They can be read through a QFile. It's something we can do on top of
> > > this patch.
> >
> > Actually, QOpenGLShader/QOpenGLShaderProgram are provide some methods
> > to build/compile the shader source code from file directly but if I load
> it
> > from file which means
> > additional shader installation steps are required and also there are some
> > potential risks
> > if the shader is not in the right position. That's why I push them into
> one
> > header file.
> > Forgive my lazy ...:-P
> > Anyway refine this part in the next version.
>
> I agree with you that storing shaders in files that need to be installed
> isn't very nice, for the reasons you described. However, if you store
> them in files in the source tree that are then compiled as Qt resources,
> the data will be in the same binary, and always accessible. I think that
> would be a good middle-ground, making the shader sources nicer to read,
> modify and review, and still easy to use in the code through QFile. If
> QFile::map() works on resources, that will be very simple, otherwise
> QFile::readAll() will give you a QByteArray with the data.
>
OK. sounds good. I will try this in the next version too.
>
> > > > > +
> > > > > +/* Vertex shader for NV formats */
> > > > > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n"
> > > >
> > > > could this (and bellow) be made static const ?
> > > >
> > > > > + "attribute vec2 textureIn;\n"
> > > > > + "varying vec2 textureOut;\n"
> > > > > + "void main(void)\n"
> > > > > + "{\n"
> > > > > + "gl_Position = vertexIn;\n"
> > > > > + "textureOut = textureIn;\n"
> > > > > + "}\n";
> > > > > +
> > > > > +/* Fragment shader for NV12, NV16 and NV24 */
> > > > > +char NV_2_planes_UV[] = "#ifdef GL_ES\n"
> > > > > + "precision highp float;\n"
> > > > > + "#endif\n"
> > > > > + "varying vec2 textureOut;\n"
> > > > > + "uniform sampler2D tex_y;\n"
> > > > > + "uniform sampler2D tex_u;\n"
> > > > > + "void main(void)\n"
> > > > > + "{\n"
> > > > > + "vec3 yuv;\n"
> > > > > + "vec3 rgb;\n"
> > > > > + "mat3 convert_mat = mat3(vec3(1.1640625,
> 1.1640625, 1.1640625),\n"
> > > > > + " vec3(0.0, -0.390625,
> 2.015625),\n"
> > > > > + " vec3(1.5975625, -0.8125,
> 0.0));\n"
> > > > > + "yuv.x = texture2D(tex_y, textureOut).r -
> 0.0625;\n"
> > > > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n"
> > > > > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n"
> > > > > + "rgb = convert_mat * yuv;\n"
> > > > > + "gl_FragColor = vec4(rgb, 1.0);\n"
> > > > > + "}\n";
> > > > > +
> > > > > +/* Fragment shader for NV21, NV61 and NV42 */
> > > > > +char NV_2_planes_VU[] = "#ifdef GL_ES\n"
> > > > > + "precision highp float;\n"
> > > > > + "#endif\n"
> > > > > + "varying vec2 textureOut;\n"
> > > > > + "uniform sampler2D tex_y;\n"
> > > > > + "uniform sampler2D tex_u;\n"
> > > > > + "void main(void)\n"
> > > > > + "{\n"
> > > > > + "vec3 yuv;\n"
> > > > > + "vec3 rgb;\n"
> > > > > + "mat3 convert_mat = mat3(vec3(1.1640625,
> 1.1640625, 1.1640625),\n"
> > > > > + " vec3(0.0, -0.390625,
> 2.015625),\n"
> > > > > + " vec3(1.5975625, -0.8125,
> 0.0));\n"
> > > > > + "yuv.x = texture2D(tex_y, textureOut).r -
> 0.0625;\n"
> > > > > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n"
> > > > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n"
> > > > > + "rgb = convert_mat * yuv;\n"
> > > > > + "gl_FragColor = vec4(rgb, 1.0);\n"
> > > > > + "}\n";
> > > > > +
> > > > > +/* Fragment shader for YUV420 */
> > > > > +char NV_3_planes_UV[] = "#ifdef GL_ES\n"
> > > > > + "precision highp float;\n"
> > > > > + "#endif\n"
> > > > > + "varying vec2 textureOut;\n"
> > > > > + "uniform sampler2D tex_y;\n"
> > > > > + "uniform sampler2D tex_u;\n"
> > > > > + "uniform sampler2D tex_v;\n"
> > > > > + "void main(void)\n"
> > > > > + "{\n"
> > > > > + "vec3 yuv;\n"
> > > > > + "vec3 rgb;\n"
> > > > > + "mat3 convert_mat = mat3(vec3(1.1640625,
> 1.1640625, 1.1640625),\n"
> > > > > + " vec3(0.0, -0.390625,
> 2.015625),\n"
> > > > > + " vec3(1.5975625, -0.8125,
> 0.0));\n"
> > > > > + "yuv.x = texture2D(tex_y, textureOut).r -
> 0.0625;\n"
> > > > > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n"
> > > > > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n"
> > > > > + "rgb = convert_mat * yuv;\n"
> > > > > + "gl_FragColor = vec4(rgb, 1);\n"
> > > > > + "}\n";
> > > > > +
> > > > > +/* Fragment shader for YVU420 */
> > > > > +char NV_3_planes_VU[] = "precision highp float;\n"
> > > > > + "#endif\n"
> > > > > + "varying vec2 textureOut;\n"
> > > > > + "uniform sampler2D tex_y;\n"
> > > > > + "uniform sampler2D tex_u;\n"
> > > > > + "uniform sampler2D tex_v;\n"
> > > > > + "void main(void)\n"
> > > > > + "{\n"
> > > > > + "vec3 yuv;\n"
> > > > > + "vec3 rgb;\n"
> > > > > + "mat3 convert_mat = mat3(vec3(1.1640625,
> 1.1640625, 1.1640625),\n"
> > > > > + " vec3(0.0, -0.390625,
> 2.015625),\n"
> > > > > + " vec3(1.5975625, -0.8125,
> 0.0));\n"
> > > > > + "yuv.x = texture2D(tex_y, textureOut).r -
> 0.0625;\n"
> > > > > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n"
> > > > > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n"
> > > > > + "rgb = convert_mat * yuv;\n"
> > > > > + "gl_FragColor = vec4(rgb, 1);\n"
> > > > > + "}\n";
> > > > > +#endif // __SHADER_H__
> > > > > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> > > > > index dcf8a15..d3a2422 100644
> > > > > --- a/src/qcam/viewfinder.cpp
> > > > > +++ b/src/qcam/viewfinder.cpp
> > > > > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat,
> QImage::Format> nativeFormats
> > > > > { libcamera::formats::BGR888, QImage::Format_RGB888 },
> > > > > };
> > > > >
> > > > > -ViewFinder::ViewFinder(QWidget *parent)
> > > > > - : QWidget(parent), buffer_(nullptr)
> > > > > +ViewFinderHandler::ViewFinderHandler()
> > > > > {
> > > > > - icon_ = QIcon(":camera-off.svg");
> > > > > }
> > >
> > > You don't need an empty constructor in the base class, you can just
> drop
> > > it.
> > >
> > > > >
> > > > > -ViewFinder::~ViewFinder()
> > > > > +ViewFinderHandler::~ViewFinderHandler()
> > > > > {
> > > > > }
> > > > >
> > > > > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats()
> const
> > > > > +const QList<libcamera::PixelFormat>
> &ViewFinderHandler::nativeFormats() const
> > > > > {
> > > > > static const QList<libcamera::PixelFormat> formats =
> ::nativeFormats.keys();
> > > > > return formats;
> > > > > }
> > >
> > > I expect native formats to be different for ViewFinderQt and
> > > ViewFinderGL, so I'd make this a pure virtual function with
> > > implementations in the derived classes. ViewFinderGL should report all
> > > the formats for which you have conversion shaders.
> >
> > ok, I will rewrite this.
> >
> > > > >
> > > > > +ViewFinder::ViewFinder(QWidget *parent)
> > > > > + : QWidget(parent), buffer_(nullptr)
> > > > > +{
> > > > > + icon_ = QIcon(":camera-off.svg");
> > > > > +}
> > > >
> > > > I agree with Kieran here I think the base class should be named
> > > > ViewFinder and the two derived classes ViewFinderQt and ViewFinderGL
> or
> > > > something similar.
> > >
> > > Seems we all agree then :-)
> > >
> > > > I also think you should move the base class to its own .h file (and
> .cpp
> > > > file if needed).
> > >
> > > Agreed.
> >
> > I will modify it accordingly.
> >
> > > > > +
> > > > > +ViewFinder::~ViewFinder()
> > > > > +{
> > > > > +}
> > > > > +
> > > > > int ViewFinder::setFormat(const libcamera::PixelFormat &format,
> > > > > const QSize &size)
> > > > > {
> > > > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> > > > > index 26a1320..741d694 100644
> > > > > --- a/src/qcam/viewfinder.h
> > > > > +++ b/src/qcam/viewfinder.h
> > > > > @@ -13,6 +13,8 @@
> > > > > #include <QList>
> > > > > #include <QImage>
> > > > > #include <QMutex>
> > > > > +#include <QOpenGLWidget>
> > > > > +#include <QOpenGLFunctions>
> > > > > #include <QSize>
> > > > > #include <QWidget>
> > > > >
> > > > > @@ -28,7 +30,23 @@ struct MappedBuffer {
> > > > > size_t size;
> > > > > };
> > > > >
> > > > > -class ViewFinder : public QWidget
> > > > > +class ViewFinderHandler
> > > > > +{
> > > > > +public:
> > > > > + ViewFinderHandler();
> > > > > + virtual ~ViewFinderHandler();
> > > > > +
> > > > > + const QList<libcamera::PixelFormat> &nativeFormats() const;
> > > > > +
> > > > > + virtual int setFormat(const libcamera::PixelFormat &format,
> const QSize &size) =0;
> > > > > + virtual void render(libcamera::FrameBuffer *buffer,
> MappedBuffer *map) =0;
> > > > > + virtual void stop() =0;
> > > > > +
> > > > > + virtual QImage getCurrentImage() =0;
> > > > > +
> > > > > +};
> > > > > +
> > > > > +class ViewFinder : public QWidget, public ViewFinderHandler
> > > > > {
> > > > > Q_OBJECT
> > > > >
> > > > > @@ -36,8 +54,6 @@ public:
> > > > > ViewFinder(QWidget *parent);
> > > > > ~ViewFinder();
> > > > >
> > > > > - const QList<libcamera::PixelFormat> &nativeFormats() const;
> > > > > -
> > > > > int setFormat(const libcamera::PixelFormat &format, const
> QSize &size);
> > > > > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map);
> > > > > void stop();
> > > > > @@ -67,5 +83,4 @@ private:
> > > > > QImage image_;
> > > > > QMutex mutex_; /* Prevent concurrent access to image_ */
> > > > > };
> > > > > -
> > >
> > > This blank line should be kept.
> >
> > sure, will fix it.
> >
> > > > > #endif /* __QCAM_VIEWFINDER__ */
> > > > > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp
> > > > > new file mode 100644
> > > > > index 0000000..7b47beb
> > > > > --- /dev/null
> > > > > +++ b/src/qcam/viewfinderGL.cpp
> > > > > @@ -0,0 +1,335 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2020, Linaro
> > > > > + *
> > > > > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader
> > > > > + */
> > > > > +
> > > > > +#include "shader.h"
> > > > > +#include "viewfinderGL.h"
> > > > > +
> > > > > +#include <QImage>
> > > > > +
> > > > > +#include <libcamera/formats.h>
> > > > > +
> > > > > +#define ATTRIB_VERTEX 0
> > > > > +#define ATTRIB_TEXTURE 1
> > > > > +
> > > > > +ViewFinderGL::ViewFinderGL(QWidget *parent)
> > > > > + : QOpenGLWidget(parent),
> > > > > + glBuffer(QOpenGLBuffer::VertexBuffer),
> > > > > + pFShader(nullptr),
> > > > > + pVShader(nullptr),
> > > > > + textureU(QOpenGLTexture::Target2D),
> > > > > + textureV(QOpenGLTexture::Target2D),
> > > > > + textureY(QOpenGLTexture::Target2D),
> > > > > + yuvDataPtr(nullptr)
> > > > > +
> > > > > +{
> > > > > +}
> > > > > +
> > > > > +ViewFinderGL::~ViewFinderGL()
> > > > > +{
> > > > > + removeShader();
> > > > > +
> > > > > + if(textureY.isCreated())
> > > > > + textureY.destroy();
> > > > > +
> > > > > + if(textureU.isCreated())
> > > > > + textureU.destroy();
> > > > > +
> > > > > + if(textureV.isCreated())
> > > > > + textureV.destroy();
> > > > > +
> > > > > + glBuffer.destroy();
> > > > > +}
> > > > > +
> > > > > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,
> > > > > + const QSize &size)
> > > > > +{
> > > > > + format_ = format;
> > > > > + size_ = size;
> > > > > +
> > > > > + updateGeometry();
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::stop()
> > > > > +{
> > > > > + if (buffer_) {
> > > > > + renderComplete(buffer_);
> > > > > + buffer_ = nullptr;
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +QImage ViewFinderGL::getCurrentImage()
> > > > > +{
> > > > > + QMutexLocker locker(&mutex_);
> > > > > +
> > > > > + return(grabFramebuffer());
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer,
> MappedBuffer *map)
> > > > > +{
> > > > > + if (buffer->planes().size() != 1) {
> > > > > + qWarning() << "Multi-planar buffers are not supported";
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + unsigned char *memory = static_cast<unsigned char
> *>(map->memory);
> > > > > + if(memory) {
> > >
> > > Can memory be null here ?
> >
> > I will rewrite this.
> >
> > > > > + yuvDataPtr = memory;
> > > > > + update();
> > > > > + buffer_ = buffer;
> > > > > + }
> > > > > +
> > > > > + if (buffer)
> > > > > + renderComplete(buffer);
> > >
> > > Here's you're supposed to signal completion of the previous buffer, not
> > > the current buffer. That's why there's a std::swap() in the existing
> > > ViewFinder::render().
> >
> > Is That means I should
> > emit renderComplete(buffer_); after update();
> > then
> > buffer_ = buffer;
> > ...
> > I don't quite understand what you mean, would you please explain it in
> > detail?
> > Forgive my stupid..:-P
>
> A stupid person couldn't have sent this patch in the first place :-)
>
> The ViewFinder component performs rendering asynchronously. It receives
> a libcamera buffer in render(), and is tasked to display it on the
> screen. The render() function returns before the rendering is complete.
> We thus need to tell the caller when the rendering is complete, to let
> it reuse the buffer once ViewFinder doesn't need it anymore. That's what
> renderComplete() is for.
>
> With the code above, you signal that the buffer has been rendered right
> after calling update(). That's not true, rendering is still in progress
> at that point. You can only give the buffer back to the caller with
> renderComplete() once a new buffer has been received to be rendered.
> That's why the current ViewFinder implementation has
>
> std::swap(buffer_, buffer);
>
> After this call, buffer contains the old buffer, and we then call
>
> if (buffer)
> renderComplete(buffer);
>
> to give the old buffer back. We need to check here if buffer is null, as
> when the first buffer is rendered, there's no old buffer to be given
> back.
>
Thanks for the detailed explanation. I totally misunderstood this portion
before.
BR,
Show Liu
>
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::updateFrame(unsigned char *buffer)
> > > > > +{
> > > > > + if(buffer) {
> > > > > + yuvDataPtr = buffer;
> > > > > + update();
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::setFrameSize(int width, int height)
> > > > > +{
> > > > > + if(width > 0 && height > 0) {
> > > > > + width_ = width;
> > > > > + height_ = height;
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +char *ViewFinderGL::selectFragmentShader(unsigned int format)
> > > > > +{
> > > > > + char *fsrc = nullptr;
> > > > > +
> > > > > + switch(format) {
> > > > > + case libcamera::formats::NV12:
> > > > > + horzSubSample_ = 2;
> > > > > + vertSubSample_ = 2;
> > > > > + fsrc = NV_2_planes_UV;
> > > > > + break;
> > > > > + case libcamera::formats::NV21:
> > > > > + horzSubSample_ = 2;
> > > > > + vertSubSample_ = 2;
> > > > > + fsrc = NV_2_planes_VU;
> > > > > + break;
> > > > > + case libcamera::formats::NV16:
> > > > > + horzSubSample_ = 2;
> > > > > + vertSubSample_ = 1;
> > > > > + fsrc = NV_2_planes_UV;
> > > > > + break;
> > > > > + case libcamera::formats::NV61:
> > > > > + horzSubSample_ = 2;
> > > > > + vertSubSample_ = 1;
> > > > > + fsrc = NV_2_planes_VU;
> > > > > + break;
> > > > > + case libcamera::formats::NV24:
> > > > > + horzSubSample_ = 1;
> > > > > + vertSubSample_ = 1;
> > > > > + fsrc = NV_2_planes_UV;
> > > > > + break;
> > > > > + case libcamera::formats::NV42:
> > > > > + horzSubSample_ = 1;
> > > > > + vertSubSample_ = 1;
> > > > > + fsrc = NV_2_planes_VU;
> > > > > + break;
> > > > > + case libcamera::formats::YUV420:
> > > > > + horzSubSample_ = 2;
> > > > > + vertSubSample_ = 2;
> > > > > + fsrc = NV_3_planes_UV;
> > > > > + break;
> > > > > + default:
> > > > > + break;
> > > > > + };
> > > > > +
> > > > > + if(fsrc == nullptr) {
> > > > > + qDebug() << __func__ << "[ERROR]:" <<" No suitable
> fragment shader can be select.";
> > > > > + }
> > > > > + return fsrc;
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::createFragmentShader()
> > > > > +{
> > > > > + bool bCompile;
> > > > > +
> > > > > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this);
> > > > > + char *fsrc = selectFragmentShader(format_);
> > > > > +
> > > > > + bCompile = pFShader->compileSourceCode(fsrc);
> > > > > + if(!bCompile)
> > > > > + {
> > > > > + qDebug() << __func__ << ":" << pFShader->log();
> > > > > + }
> > > > > +
> > > > > + shaderProgram.addShader(pFShader);
> > > > > +
> > > > > + // Link shader pipeline
> > > > > + if (!shaderProgram.link()) {
> > > > > + qDebug() << __func__ << ": shader program link
> failed.\n" << shaderProgram.log();
> > > > > + close();
> > > > > + }
> > > > > +
> > > > > + // Bind shader pipeline for use
> > > > > + if (!shaderProgram.bind()) {
> > > > > + qDebug() << __func__ << ": shader program binding
> failed.\n" << shaderProgram.log();
> > > > > + close();
> > > > > + }
> > > > > +
> > > > > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX);
> > > > > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE);
> > > > > +
> > > > > +
> shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat));
> > > > > +
> shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat));
> > > > > +
> > > > > + textureUniformY = shaderProgram.uniformLocation("tex_y");
> > > > > + textureUniformU = shaderProgram.uniformLocation("tex_u");
> > > > > + textureUniformV = shaderProgram.uniformLocation("tex_v");
> > > > > +
> > > > > + if(!textureY.isCreated())
> > > > > + textureY.create();
> > > > > +
> > > > > + if(!textureU.isCreated())
> > > > > + textureU.create();
> > > > > +
> > > > > + if(!textureV.isCreated())
> > > > > + textureV.create();
> > > > > +
> > > > > + id_y = textureY.textureId();
> > > > > + id_u = textureU.textureId();
> > > > > + id_v = textureV.textureId();
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::configureTexture(unsigned int id)
> > > > > +{
> > > > > + glBindTexture(GL_TEXTURE_2D, id);
> > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
> GL_LINEAR);
> > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
> GL_LINEAR);
> > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S,
> GL_CLAMP_TO_EDGE);
> > > > > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T,
> GL_CLAMP_TO_EDGE);
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::removeShader()
> > > > > +{
> > > > > + if (shaderProgram.isLinked()) {
> > > > > + shaderProgram.release();
> > > > > + shaderProgram.removeAllShaders();
> > > > > + }
> > > > > +
> > > > > + if(pFShader)
> > > > > + delete pFShader;
> > > > > +
> > > > > + if(pVShader)
> > > > > + delete pVShader;
> > >
> > > If we stop and restart the stream with a different format, the previous
> > > shader will be used, as removeShader() isn't called. I don't think
> > > that's right. I believe you need to call removeShader() at the
> beginning
> > > of setFormat().
> >
> > Yes. you're right. I will refine the setFormat() function.
> >
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::initializeGL()
> > > > > +{
> > > > > + bool bCompile;
> > > > > +
> > > > > + initializeOpenGLFunctions();
> > > > > + glEnable(GL_TEXTURE_2D);
> > > > > + glDisable(GL_DEPTH_TEST);
> > > > > +
> > > > > + static const GLfloat vertices[] {
> > > > > + -1.0f,-1.0f,
> > > > > + -1.0f,+1.0f,
> > > > > + +1.0f,+1.0f,
> > > > > + +1.0f,-1.0f,
> > > > > + 0.0f,1.0f,
> > > > > + 0.0f,0.0f,
> > > > > + 1.0f,0.0f,
> > > > > + 1.0f,1.0f,
> > > > > + };
> > > > > +
> > > > > + glBuffer.create();
> > > > > + glBuffer.bind();
> > > > > + glBuffer.allocate(vertices,sizeof(vertices));
> > > > > +
> > > > > + /* Create Vertex Shader */
> > > > > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this);
> > > > > + char *vsrc = NV_Vertex_shader;
> > > > > +
> > > > > + bCompile = pVShader->compileSourceCode(vsrc);
> > > > > + if(!bCompile) {
> > > > > + qDebug() << __func__<< ":" << pVShader->log();
> > > > > + }
> > > > > +
> > > > > + shaderProgram.addShader(pVShader);
> > > > > +
> > > > > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f);
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::render()
> > > > > +{
> > > > > + switch(format_) {
> > > > > + case libcamera::formats::NV12:
> > > > > + case libcamera::formats::NV21:
> > > > > + case libcamera::formats::NV16:
> > > > > + case libcamera::formats::NV61:
> > > > > + case libcamera::formats::NV24:
> > > > > + case libcamera::formats::NV42:
> > > > > + /* activate texture 0 */
> > > > > + glActiveTexture(GL_TEXTURE0);
> > > > > + configureTexture(id_y);
> > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(),
> size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr);
> > > > > + glUniform1i(textureUniformY, 0);
> > > > > +
> > > > > + /* activate texture 1 */
> > > > > + glActiveTexture(GL_TEXTURE1);
> > > > > + configureTexture(id_u);
> > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG,
> size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG,
> GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height());
> > > > > + glUniform1i(textureUniformU, 1);
> > > > > + break;
> > > > > + case libcamera::formats::YUV420:
> > > > > + /* activate texture 0 */
> > > > > + glActiveTexture(GL_TEXTURE0);
> > > > > + configureTexture(id_y);
> > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, size_.width(),
> size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr);
> > > > > + glUniform1i(textureUniformY, 0);
> > > > > +
> > > > > + /* activate texture 1 */
> > > > > + glActiveTexture(GL_TEXTURE1);
> > > > > + configureTexture(id_u);
> > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG,
> size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG,
> GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height());
> > > > > + glUniform1i(textureUniformU, 1);
> > > > > +
> > > > > + /* activate texture 2 */
> > > > > + glActiveTexture(GL_TEXTURE2);
> > > > > + configureTexture(id_v);
> > > > > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG,
> size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0, GL_RG,
> GL_UNSIGNED_BYTE, (char*)yuvDataPtr+size_.width()*size_.height()*5/4);
> > > > > + glUniform1i(textureUniformV, 2);
> > > > > + break;
> > > > > + default:
> > > > > + break;
> > > > > + };
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::paintGL()
> > > > > +{
> > > > > + if(pFShader == nullptr)
> > >
> > > We tend to write
> > >
> > > if (!pFShader)
> >
> > ok will fix it.
> >
> > > > > + createFragmentShader();
> > >
> > > I wonder if we could do this in setFormat().
> >
> > That should be ok, I will try to move this when setting Format.
> >
> > > > > +
> > > > > + if(yuvDataPtr)
> > > > > + {
> > > > > + glClearColor(0.0, 0.0, 0.0, 1.0);
> > > > > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT );
> > > > > +
> > > > > + render();
> > > > > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +void ViewFinderGL::resizeGL(int w, int h)
> > > > > +{
> > > > > + glViewport(0,0,w,h);
> > > > > +}
> > > > > +
> > > > > +QSize ViewFinderGL::sizeHint() const
> > > > > +{
> > > > > + return size_.isValid() ? size_ : QSize(640, 480);
> > > > > +}
> > > > > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h
> > > > > new file mode 100644
> > > > > index 0000000..08662ca
> > > > > --- /dev/null
> > > > > +++ b/src/qcam/viewfinderGL.h
> > > > > @@ -0,0 +1,101 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2020, Linaro
> > > > > + *
> > > > > + * viewfinderGL.h - Render YUV format frame by OpenGL shader
> > > > > + */
> > > > > +#ifndef __VIEWFINDERGL_H__
> > > > > +#define __VIEWFINDERGL_H__
> > > > > +
> > > > > +#include <fcntl.h>
> > > > > +#include <string.h>
> > > > > +#include <unistd.h>
> > > > > +
> > > > > +#include <iomanip>
> > > > > +#include <iostream>
> > > > > +#include <sstream>
> > > > > +
> > >
> > > Do you need all these headers in the .h file ? They seem to belong to
> > > the .cpp file.
> >
> > OK I will check or just remove it.
> >
> > > > > +#include <QImage>
> > > > > +#include <QOpenGLBuffer>
> > > > > +#include <QOpenGLFunctions>
> > > > > +#include <QOpenGLShader>
> > > > > +#include <QOpenGLShaderProgram>
> > > > > +#include <QSize>
> > > > > +#include <QOpenGLTexture>
> > > > > +#include <QOpenGLWidget>
> > > > > +
> > > > > +#include <libcamera/buffer.h>
> > > > > +#include <libcamera/pixel_format.h>
> > >
> > > Blank line missing here.
> >
> > sure, will do .
> >
> > > > > +#include "viewfinder.h"
> > > > > +
> > > > > +class ViewFinderGL : public QOpenGLWidget,
> > > > > + public ViewFinderHandler,
> > > > > + protected QOpenGLFunctions
> > > > > +{
> > > > > + Q_OBJECT
> > > > > +
> > > > > +public:
> > > > > + ViewFinderGL(QWidget *parent = 0);
> > > > > + ~ViewFinderGL();
> > > > > +
> > > > > + int setFormat(const libcamera::PixelFormat &format, const
> QSize &size);
> > >
> > > You should add "override" to qualify all overridden functions.
> > >
> > > int setFormat(const libcamera::PixelFormat &format,
> > > const QSize &size) override;
> >
> > ok will do.
> >
> > > > > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map);
> > > > > + void stop();
> > > > > +
> > > > > + QImage getCurrentImage();
> > > > > +
> > > > > + void setFrameSize(int width, int height);
> > > > > + void updateFrame(unsigned char *buffer);
> > >
> > > Those two functions seem unused.
> >
> > sure, I will remove it.
> >
> > > > > +
> > > > > + char *selectFragmentShader(unsigned int format);
> > > > > + void createFragmentShader();
> > >
> > > And these two functions can be private.
> >
> > will move to private.
> >
> > > > > + void render();
> > > > > +
> > > > > +Q_SIGNALS:
> > > > > + void renderComplete(libcamera::FrameBuffer *buffer);
> > > > > +
> > > > > +protected:
> > > > > + void initializeGL() Q_DECL_OVERRIDE;
> > >
> > > You can use "override" directly, we know the compiler supports it.
> >
> > ok, I will fix it up.
> >
> > > > > + void paintGL() Q_DECL_OVERRIDE;
> > > > > + void resizeGL(int w, int h) Q_DECL_OVERRIDE;
> > > > > + QSize sizeHint() const override;
> > > > > +
> > > > > +private:
> > > > > +
> > > > > + void configureTexture(unsigned int id);
> > > > > + void removeShader();
> > > > > +
> > > > > + /* Captured image size, format and buffer */
> > > > > + libcamera::FrameBuffer *buffer_;
> > > > > + libcamera::PixelFormat format_;
> > > > > + QOpenGLBuffer glBuffer;
> > > > > + QSize size_;
> > > > > +
> > > > > + GLuint id_u;
> > > > > + GLuint id_v;
> > > > > + GLuint id_y;
> > > > > +
> > > > > + QMutex mutex_; /* Prevent concurrent access to image_ */
> > > > > +
> > > > > + QOpenGLShader *pFShader;
> > > > > + QOpenGLShader *pVShader;
> > > > > + QOpenGLShaderProgram shaderProgram;
> > > > > +
> > > > > + GLuint textureUniformU;
> > > > > + GLuint textureUniformV;
> > > > > + GLuint textureUniformY;
> > > > > +
> > > > > + QOpenGLTexture textureU;
> > > > > + QOpenGLTexture textureV;
> > > > > + QOpenGLTexture textureY;
> > > > > +
> > > > > + unsigned int width_;
> > > > > + unsigned int height_;
> > > > > +
> > > > > + unsigned char* yuvDataPtr;
> > > > > +
> > > > > + /* NV parameters */
> > > > > + unsigned int horzSubSample_ ;
> > > > > + unsigned int vertSubSample_;
> > > > > +};
> > > > > +#endif // __VIEWFINDERGL_H__
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200706/664ef55c/attachment-0001.htm>
More information about the libcamera-devel
mailing list