[libcamera-devel] [PATCH v3 1/1] qcam: Render YUV formats frame by OpenGL shader
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jun 25 13:36:43 CEST 2020
Hi Show,
On 25/06/2020 09:43, Show Liu wrote:
> Hi Kieran,
>
> Thanks for your review.
> Your suggestions and opinions are really good for me.
>
> I will try to hook the checkstyle.py and check my patch first. I believe
> that will fix most of problems below. Sorry for the inconvenience.
No problem, or inconvenience, that's why we (well Laurent) made
checkstyle.py :-D
Looking forward to your update.
Kieran
>
> - Show
>
>
>
>
> Kieran Bingham <kieran.bingham at ideasonboard.com
> <mailto:kieran.bingham at ideasonboard.com>> 於 2020年6月24日 週三 下午5:34
> 寫道：
>
> Hi Show,
>
> Thank you, I'm quite excited to see this update, and I know Niklas is
> keen for this to get in too, as he uses a Rockchip target mostly so this
> will improve the performances for him quite a lot.
>
> Most of my comments below are stylistic, (but theres a couple of other
> topics too), so as suggested below, could you install the checkstyle.py
> hook as a git commit hook please?
>
>
> Th
>
>
> I personally prefer having it as a 'post-commit' hook, rather than a
> pre-commit so that it's easier to 'ignore' suggestions that I don't care
> for ;-)
>
> --
> Kieran
>
>
> On 24/06/2020 08:37, Show Liu wrote:
> > The patch is to render the NV family YUV formats by OpenGL shader.
> > V3: refine the fragment shader for better pixel color.
> >
> > Signed-off-by: Show Liu <show.liu at linaro.org
> <mailto:show.liu at linaro.org>>
> > ---
> > src/qcam/main.cpp | 2 +
> > src/qcam/main_window.cpp | 19 ++-
> > src/qcam/main_window.h | 3 +-
> > src/qcam/meson.build | 2 +
> > src/qcam/shader.h | 104 ++++++++++++
> > src/qcam/viewfinder.cpp | 18 +-
> > src/qcam/viewfinder.h | 23 ++-
> > src/qcam/viewfinderGL.cpp | 335
> ++++++++++++++++++++++++++++++++++++++
> > src/qcam/viewfinderGL.h | 101 ++++++++++++
> > 9 files changed, 593 insertions(+), 14 deletions(-)
> > create mode 100644 src/qcam/shader.h
> > create mode 100644 src/qcam/viewfinderGL.cpp
> > create mode 100644 src/qcam/viewfinderGL.h
> >
> > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
> > index b3468cb..a3ea471 100644
> > --- a/src/qcam/main.cpp
> > +++ b/src/qcam/main.cpp
> > @@ -35,6 +35,8 @@ OptionsParser::Options parseOptions(int argc,
> char *argv[])
> > "help");
> > parser.addOption(OptStream, &streamKeyValue,
> > "Set configuration of a camera stream",
> "stream", true);
> > + parser.addOption(OptOpenGL, OptionNone, "Render YUV formats
> frame via OpenGL shader",
> > + "opengl");
>
> (Just a question to everyone) - Should we default to the OpenGL
> viewfinder if open-gl is available, and fall back to the QWidget version
> otherwise? (We can always do such actions on top/later if we choose.)
>
>
> >
> > OptionsParser::Options options = parser.parse(argc, argv);
> > if (options.isSet(OptHelp))
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 7bc1360..6edf370 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -28,6 +28,9 @@
> > #include <libcamera/version.h>
> >
> > #include "dng_writer.h"
> > +#include "main_window.h"
> > +#include "viewfinder.h"
> > +#include "viewfinderGL.h"
> >
> > using namespace libcamera;
> >
> > @@ -105,10 +108,18 @@ MainWindow::MainWindow(CameraManager *cm,
> const OptionsParser::Options &options)
> > setWindowTitle(title_);
> > connect(&titleTimer_, SIGNAL(timeout()), this,
> SLOT(updateTitle()));
> >
> > - viewfinder_ = new ViewFinder(this);
> > - connect(viewfinder_, &ViewFinder::renderComplete,
> > - this, &MainWindow::queueRequest);
> > - setCentralWidget(viewfinder_);
> > + if (options_.isSet(OptOpenGL)) {
> > + viewfinder_ = new ViewFinderGL(this);
> > + connect(dynamic_cast<ViewFinderGL *>(viewfinder_),
> &ViewFinderGL::renderComplete,
> > + this, &MainWindow::queueRequest);
>
> checkstyle.py highlights that the indentation of the second line should
> be pulled back, and I agree:
> connect(dynamic_cast<ViewFinderGL *>(viewfinder_),
> &ViewFinderGL::renderComplete,
> - this, &MainWindow::queueRequest);
> + this, &MainWindow::queueRequest);
>
>
>
> > + setCentralWidget(dynamic_cast<ViewFinderGL
> *>(viewfinder_));
>
> Does the setCentralWidget need to have the dynamic_cast? Or can it just
> be a call to
>
> setCentralWidget(viewfinder_);
>
> I got build error if use above if no dynamic_cast.
>
>
> after this conditional block?
>
>
> Perhaps if the base viewfinder class had a method to call which handled
> the connect, that could remove casting requirements there - but it would
> probably end up needing to deal with complex template things for the
> signal/slot parsing ... so I suspect that part isn't worth it...
>
>
> I can trying to refine this parts.
>
>
>
> > + } else {
> > + viewfinder_ = new ViewFinder(this);
> > + connect(dynamic_cast<ViewFinder *>(viewfinder_),
> &ViewFinder::renderComplete,
> > + this, &MainWindow::queueRequest);
>
> Same indentation fault here.
>
> If you add the checkstyle.py hook to your libcamera sources, you'll get
> these notifications as you commit (you can then decide if checkstyle was
> right or not ;D):
>
> $ cp utils/hooks/post-commit .git/hooks/post-commit
>
> > + setCentralWidget(dynamic_cast<ViewFinder
> *>(viewfinder_));
> > + }
> > +
> > adjustSize();
> >
> > /* Hotplug/unplug support */
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 4606fe4..a852ef4 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -38,6 +38,7 @@ enum {
> > OptCamera = 'c',
> > OptHelp = 'h',
> > OptStream = 's',
> > + OptOpenGL = 'o',
> > };
> >
> > class CaptureRequest
> > @@ -102,7 +103,7 @@ private:
> > QAction *startStopAction_;
> > QComboBox *cameraCombo_;
> > QAction *saveRaw_;
> > - ViewFinder *viewfinder_;
> > + ViewFinderHandler *viewfinder_;
>
> Handler?
>
> I guess I'll see below, but I think I'd expect the base class interface
> to be 'ViewFinder', and then make specific derived implementations
> of that?
>
> Ok I will think about how to refine this too.
>
>
> >
> > QIcon iconPlay_;
> > QIcon iconStop_;
> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> > index 045db52..6a58947 100644
> > --- a/src/qcam/meson.build
> > +++ b/src/qcam/meson.build
> > @@ -7,11 +7,13 @@ qcam_sources = files([
> > 'main.cpp',
> > 'main_window.cpp',
> > 'viewfinder.cpp',
> > + 'viewfinderGL.cpp'
> > ])
> >
> > qcam_moc_headers = files([
> > 'main_window.h',
> > 'viewfinder.h',
> > + 'viewfinderGL.h'
> > ])
> >
> > qcam_resources = files([
> > diff --git a/src/qcam/shader.h b/src/qcam/shader.h
> > new file mode 100644
> > index 0000000..f54c264
> > --- /dev/null
> > +++ b/src/qcam/shader.h
> > @@ -0,0 +1,104 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Linaro
> > + *
> > + * shader.h - YUV format converter shader source code
> > + */
> > +#ifndef __SHADER_H__
> > +#define __SHADER_H__
> > +
> > +/* Vertex shader for NV formats */
> > +char NV_Vertex_shader[] = "attribute vec4 vertexIn;\n"
> > + "attribute vec2 textureIn;\n"
> > + "varying vec2 textureOut;\n"
> > + "void main(void)\n"
> > + "{\n"
> > + "gl_Position = vertexIn;\n"
> > + "textureOut = textureIn;\n"
> > + "}\n";
>
> I'd pull that indentation back so that they all match, including moving
> the first line to a new line to match...
>
> char NV_Vertex_shader[] =
> "attribute vec4 vertexIn;\n"
> "attribute vec2 textureIn;\n"
> "varying vec2 textureOut;\n"
> "void main(void)\n"
> "{\n"
> "gl_Position = vertexIn;\n"
> "textureOut = textureIn;\n"
> "}\n";
>
>
>
> > +
> > +/* Fragment shader for NV12, NV16 and NV24 */
> > +char NV_2_planes_UV[] = "#ifdef GL_ES\n"
>
> And ofcourse be consistent with the code block indentation style
> throughout.
>
> checkstyle might complain whatever you do here, so I would take
> checkstyle with a pinch of salt and just make sure the file is consitent
> and well laid out.
>
>
>
> > + "precision highp float;\n"
> > + "#endif\n"
> > + "varying vec2 textureOut;\n"
> > + "uniform sampler2D tex_y;\n"
> > + "uniform sampler2D tex_u;\n"
> > + "void main(void)\n"
> > + "{\n"
> > + "vec3 yuv;\n"
> > + "vec3 rgb;\n"
> > + "mat3 convert_mat = mat3(vec3(1.1640625,
> 1.1640625, 1.1640625),\n"
> > + " vec3(0.0, -0.390625,
> 2.015625),\n"
> > + " vec3(1.5975625, -0.8125,
> 0.0));\n"
> > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n"
> > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n"
> > + "yuv.z = texture2D(tex_u, textureOut).g - 0.5;\n"
> > + "rgb = convert_mat * yuv;\n"
> > + "gl_FragColor = vec4(rgb, 1.0);\n"
> > + "}\n";
> > +
> > +/* Fragment shader for NV21, NV61 and NV42 */
> > +char NV_2_planes_VU[] = "#ifdef GL_ES\n"
> > + "precision highp float;\n"
> > + "#endif\n"
> > + "varying vec2 textureOut;\n"
> > + "uniform sampler2D tex_y;\n"
> > + "uniform sampler2D tex_u;\n"
> > + "void main(void)\n"
> > + "{\n"
> > + "vec3 yuv;\n"
> > + "vec3 rgb;\n"
> > + "mat3 convert_mat = mat3(vec3(1.1640625,
> 1.1640625, 1.1640625),\n"
> > + " vec3(0.0, -0.390625,
> 2.015625),\n"
> > + " vec3(1.5975625, -0.8125,
> 0.0));\n"
> > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n"
> > + "yuv.y = texture2D(tex_u, textureOut).g - 0.5;\n"
> > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n"
> > + "rgb = convert_mat * yuv;\n"
> > + "gl_FragColor = vec4(rgb, 1.0);\n"
> > + "}\n";
> > +
> > +/* Fragment shader for YUV420 */
> > +char NV_3_planes_UV[] = "#ifdef GL_ES\n"
> > + "precision highp float;\n"
> > + "#endif\n"
> > + "varying vec2 textureOut;\n"
> > + "uniform sampler2D tex_y;\n"
> > + "uniform sampler2D tex_u;\n"
> > + "uniform sampler2D tex_v;\n"
> > + "void main(void)\n"
> > + "{\n"
> > + "vec3 yuv;\n"
> > + "vec3 rgb;\n"
> > + "mat3 convert_mat = mat3(vec3(1.1640625,
> 1.1640625, 1.1640625),\n"
> > + " vec3(0.0, -0.390625,
> 2.015625),\n"
> > + " vec3(1.5975625, -0.8125,
> 0.0));\n"
> > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n"
> > + "yuv.y = texture2D(tex_u, textureOut).r - 0.5;\n"
> > + "yuv.z = texture2D(tex_v, textureOut).g - 0.5;\n"
> > + "rgb = convert_mat * yuv;\n"
> > + "gl_FragColor = vec4(rgb, 1);\n"
> > + "}\n";
> > +
> > +/* Fragment shader for YVU420 */
> > +char NV_3_planes_VU[] = "precision highp float;\n"
> > + "#endif\n"
> > + "varying vec2 textureOut;\n"
> > + "uniform sampler2D tex_y;\n"
> > + "uniform sampler2D tex_u;\n"
> > + "uniform sampler2D tex_v;\n"
> > + "void main(void)\n"
> > + "{\n"
> > + "vec3 yuv;\n"
> > + "vec3 rgb;\n"
> > + "mat3 convert_mat = mat3(vec3(1.1640625,
> 1.1640625, 1.1640625),\n"
> > + " vec3(0.0, -0.390625,
> 2.015625),\n"
> > + " vec3(1.5975625, -0.8125,
> 0.0));\n"
> > + "yuv.x = texture2D(tex_y, textureOut).r - 0.0625;\n"
> > + "yuv.y = texture2D(tex_v, textureOut).g - 0.5;\n"
> > + "yuv.z = texture2D(tex_u, textureOut).r - 0.5;\n"
> > + "rgb = convert_mat * yuv;\n"
> > + "gl_FragColor = vec4(rgb, 1);\n"
> > + "}\n";
> > +#endif // __SHADER_H__
>
> I can't comment on the shaders without a lot of research into shaders...
> so I'm going to say "Yay, if this black magic works, then it works for
> me" ;-D
>
>
> > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> > index dcf8a15..d3a2422 100644
> > --- a/src/qcam/viewfinder.cpp
> > +++ b/src/qcam/viewfinder.cpp
> > @@ -33,22 +33,30 @@ static const QMap<libcamera::PixelFormat,
> QImage::Format> nativeFormats
> > { libcamera::formats::BGR888, QImage::Format_RGB888 },
> > };
> >
> > -ViewFinder::ViewFinder(QWidget *parent)
> > - : QWidget(parent), buffer_(nullptr)
> > +ViewFinderHandler::ViewFinderHandler()
> > {
> > - icon_ = QIcon(":camera-off.svg");
> > }
> >
> > -ViewFinder::~ViewFinder()
> > +ViewFinderHandler::~ViewFinderHandler()
> > {
> > }
> >
> > -const QList<libcamera::PixelFormat> &ViewFinder::nativeFormats()
> const
> > +const QList<libcamera::PixelFormat>
> &ViewFinderHandler::nativeFormats() const
> > {
> > static const QList<libcamera::PixelFormat> formats =
> ::nativeFormats.keys();
> > return formats;
> > }
> >
> > +ViewFinder::ViewFinder(QWidget *parent)
> > + : QWidget(parent), buffer_(nullptr)
> > +{
> > + icon_ = QIcon(":camera-off.svg");
> > +}
> > +
> > +ViewFinder::~ViewFinder()
> > +{
> > +}
> > +
> > int ViewFinder::setFormat(const libcamera::PixelFormat &format,
> > const QSize &size)
> > {
> > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> > index 26a1320..741d694 100644
> > --- a/src/qcam/viewfinder.h
> > +++ b/src/qcam/viewfinder.h
> > @@ -13,6 +13,8 @@
> > #include <QList>
> > #include <QImage>
> > #include <QMutex>
> > +#include <QOpenGLWidget>
> > +#include <QOpenGLFunctions>
>
> Shouldn't those includes be in
> src/qcam/viewfinderGL.cpp or src/qcam/viewfinderGL.h only?
>
> Also - they should be alphabetical order.
> (Checkstyle will highlight that for you).
>
> > #include <QSize>
> > #include <QWidget>
> >
> > @@ -28,7 +30,23 @@ struct MappedBuffer {
> > size_t size;
> > };
> >
> > -class ViewFinder : public QWidget
> > +class ViewFinderHandler
>
> I think it's just naming, but I would have called this ViewFinder,...
>
> > +{
> > +public:
> > + ViewFinderHandler();
> > + virtual ~ViewFinderHandler();
> > +
> > + const QList<libcamera::PixelFormat> &nativeFormats() const;
> > +
> > + virtual int setFormat(const libcamera::PixelFormat &format,
> const QSize &size) =0;
> > + virtual void render(libcamera::FrameBuffer *buffer,
> MappedBuffer *map) =0;
> > + virtual void stop() =0;
> > +
> > + virtual QImage getCurrentImage() =0;
>
> on each of those, a space after the = looks better:
> functionName() = 0;
>
>
> > +
> > +};
> > +
> > +class ViewFinder : public QWidget, public ViewFinderHandler
>
> And this, ViewFinderQT or ViewFinderQWidget? (probably the later..)
>
> To keep consistent with coding style, I think the viewfinder.h would
> then only contain the base interface, and create a new file+header for
> the QWidget version.
>
> Or maybe that's over kill, as the QWidget version would be the 'default'
> ViewFinder anyway ...
>
>
> > {
> > Q_OBJECT
> >
> > @@ -36,8 +54,6 @@ public:
> > ViewFinder(QWidget *parent);
> > ~ViewFinder();
> >
> > - const QList<libcamera::PixelFormat> &nativeFormats() const;
> > -
> > int setFormat(const libcamera::PixelFormat &format, const
> QSize &size);
> > void render(libcamera::FrameBuffer *buffer, MappedBuffer *map);
> > void stop();
> > @@ -67,5 +83,4 @@ private:
> > QImage image_;
> > QMutex mutex_; /* Prevent concurrent access to image_ */
> > };
> > -
> > #endif /* __QCAM_VIEWFINDER__ */
> > diff --git a/src/qcam/viewfinderGL.cpp b/src/qcam/viewfinderGL.cpp
> > new file mode 100644
> > index 0000000..7b47beb
> > --- /dev/null
> > +++ b/src/qcam/viewfinderGL.cpp
> > @@ -0,0 +1,335 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Linaro
> > + *
> > + * viewfinderGL.cpp - Render YUV format frame by OpenGL shader
> > + */
> > +
> > +#include "shader.h"
> > +#include "viewfinderGL.h"
> > +
> > +#include <QImage>
>
> QImage is provided in viewfinder.h ... And do you actually use it in
> here?
>
>
> > +
> > +#include <libcamera/formats.h>
> > +
> > +#define ATTRIB_VERTEX 0
> > +#define ATTRIB_TEXTURE 1
> > +
> > +ViewFinderGL::ViewFinderGL(QWidget *parent)
> > + : QOpenGLWidget(parent),
> > + glBuffer(QOpenGLBuffer::VertexBuffer),
> > + pFShader(nullptr),
> > + pVShader(nullptr),
> > + textureU(QOpenGLTexture::Target2D),
> > + textureV(QOpenGLTexture::Target2D),
> > + textureY(QOpenGLTexture::Target2D),
> > + yuvDataPtr(nullptr)
>
> Checkstyle will tell you to align those vertically with the first
> QOpenGLWidget(parent), rather than the ':'.
>
>
> > +
> > +{
> > +}
> > +
> > +ViewFinderGL::~ViewFinderGL()
> > +{
> > + removeShader();
> > +
> > + if(textureY.isCreated())
>
> Space after if: "if ()"
>
> > + textureY.destroy();
> > +
> > + if(textureU.isCreated())
> > + textureU.destroy();
> > +
> > + if(textureV.isCreated())
> > + textureV.destroy();
>
> For each of those ;-)
>
> There's plenty more too, but I'll let you find them with checkstyle.
>
>
> > +
> > + glBuffer.destroy();
> > +}
> > +
> > +int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,
> > + const QSize &size)
> > +{
> > + format_ = format;
> > + size_ = size;
> > +
> > + updateGeometry();
> > + return 0;
> > +}
> > +
> > +void ViewFinderGL::stop()
> > +{
> > + if (buffer_) {
> > + renderComplete(buffer_);
> > + buffer_ = nullptr;
> > + }
> > +}
> > +
> > +QImage ViewFinderGL::getCurrentImage()
> > +{
> > + QMutexLocker locker(&mutex_);
> > +
> > + return(grabFramebuffer());
>
> I think that could be :
>
> return grabFrameBuffer();
>
> > +}
> > +
> > +void ViewFinderGL::render(libcamera::FrameBuffer *buffer,
> MappedBuffer *map)
> > +{
> > + if (buffer->planes().size() != 1) {
> > + qWarning() << "Multi-planar buffers are not supported";
> > + return;
> > + }
> > +
> > + unsigned char *memory = static_cast<unsigned char
> *>(map->memory);
> > + if(memory) {
> > + yuvDataPtr = memory;
> > + update();
> > + buffer_ = buffer;
> > + }
> > +
> > + if (buffer)
> > + renderComplete(buffer);
> > +}
> > +
> > +void ViewFinderGL::updateFrame(unsigned char *buffer)
> > +{
> > + if(buffer) {
> > + yuvDataPtr = buffer;
> > + update();
> > + }
> > +}
> > +
> > +void ViewFinderGL::setFrameSize(int width, int height)
>
> Can width/height be negative? Maybe the should be unsigned int?
>
> What about using a QSize too rather than individually passing
> width/height?
>
> I can't see what calls setFrameSize(), is this an override from
>
> > +{
> > + if(width > 0 && height > 0) {
> > + width_ = width;
> > + height_ = height;
> > + }
>
> In fact, don't we already have a size_ in the class? (which could also
> be in the base class too most likely...
>
> Sure, I will use QSize instead.
>
>
>
> > +}
> > +
> > +char *ViewFinderGL::selectFragmentShader(unsigned int format)
> > +{
> > + char *fsrc = nullptr;
> > +
> > + switch(format) {
> > + case libcamera::formats::NV12:
> > + horzSubSample_ = 2;
> > + vertSubSample_ = 2;
> > + fsrc = NV_2_planes_UV;
> > + break;
> > + case libcamera::formats::NV21:
> > + horzSubSample_ = 2;
> > + vertSubSample_ = 2;
> > + fsrc = NV_2_planes_VU;
> > + break;
> > + case libcamera::formats::NV16:
> > + horzSubSample_ = 2;
> > + vertSubSample_ = 1;
> > + fsrc = NV_2_planes_UV;
> > + break;
> > + case libcamera::formats::NV61:
> > + horzSubSample_ = 2;
> > + vertSubSample_ = 1;
> > + fsrc = NV_2_planes_VU;
> > + break;
> > + case libcamera::formats::NV24:
> > + horzSubSample_ = 1;
> > + vertSubSample_ = 1;
> > + fsrc = NV_2_planes_UV;
> > + break;
> > + case libcamera::formats::NV42:
> > + horzSubSample_ = 1;
> > + vertSubSample_ = 1;
> > + fsrc = NV_2_planes_VU;
> > + break;
> > + case libcamera::formats::YUV420:
> > + horzSubSample_ = 2;
> > + vertSubSample_ = 2;
> > + fsrc = NV_3_planes_UV;
> > + break;
> > + default:
> > + break;
> > + };
> > +
> > + if(fsrc == nullptr) {
> > + qDebug() << __func__ << "[ERROR]:" <<" No suitable
> fragment shader can be select.";
>
> 'selected'
>
> Ok. Will fix it.
>
>
>
> > + }
> > + return fsrc;
> > +}
> > +
> > +void ViewFinderGL::createFragmentShader()
> > +{
> > + bool bCompile;
> > +
> > + pFShader = new QOpenGLShader(QOpenGLShader::Fragment, this);
> > + char *fsrc = selectFragmentShader(format_);
> > +
> > + bCompile = pFShader->compileSourceCode(fsrc);
> > + if(!bCompile)
> > + {
> > + qDebug() << __func__ << ":" << pFShader->log();
> > + }
> > +
> > + shaderProgram.addShader(pFShader);
> > +
> > + // Link shader pipeline
>
> Even though it's C++, we usually use C style comments...
>
>
> > + if (!shaderProgram.link()) {
> > + qDebug() << __func__ << ": shader program link
> failed.\n" << shaderProgram.log();
> > + close();
> > + }
> > +
> > + // Bind shader pipeline for use
> > + if (!shaderProgram.bind()) {
> > + qDebug() << __func__ << ": shader program binding
> failed.\n" << shaderProgram.log();
> > + close();
> > + }
> > +
> > + shaderProgram.enableAttributeArray(ATTRIB_VERTEX);
> > + shaderProgram.enableAttributeArray(ATTRIB_TEXTURE);
> > +
> > +
> shaderProgram.setAttributeBuffer(ATTRIB_VERTEX,GL_FLOAT,0,2,2*sizeof(GLfloat));
> > +
> shaderProgram.setAttributeBuffer(ATTRIB_TEXTURE,GL_FLOAT,8*sizeof(GLfloat),2,2*sizeof(GLfloat));
> > +
> > + textureUniformY = shaderProgram.uniformLocation("tex_y");
> > + textureUniformU = shaderProgram.uniformLocation("tex_u");
> > + textureUniformV = shaderProgram.uniformLocation("tex_v");
> > +
> > + if(!textureY.isCreated())
> > + textureY.create();
> > +
> > + if(!textureU.isCreated())
> > + textureU.create();
> > +
> > + if(!textureV.isCreated())
> > + textureV.create();
> > +
> > + id_y = textureY.textureId();
> > + id_u = textureU.textureId();
> > + id_v = textureV.textureId();
> > +}
> > +
> > +void ViewFinderGL::configureTexture(unsigned int id)
> > +{
> > + glBindTexture(GL_TEXTURE_2D, id);
> > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
> GL_LINEAR);
> > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
> GL_LINEAR);
> > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S,
> GL_CLAMP_TO_EDGE);
> > + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T,
> GL_CLAMP_TO_EDGE);
> > +}
> > +
> > +void ViewFinderGL::removeShader()
> > +{
> > + if (shaderProgram.isLinked()) {
> > + shaderProgram.release();
> > + shaderProgram.removeAllShaders();
> > + }
> > +
> > + if(pFShader)
> > + delete pFShader;
> > +
> > + if(pVShader)
> > + delete pVShader;
> > +}
> > +
> > +void ViewFinderGL::initializeGL()
> > +{
> > + bool bCompile;
> > +
> > + initializeOpenGLFunctions();
> > + glEnable(GL_TEXTURE_2D);
> > + glDisable(GL_DEPTH_TEST);
> > +
> > + static const GLfloat vertices[] {
> > + -1.0f,-1.0f,
> > + -1.0f,+1.0f,
> > + +1.0f,+1.0f,
> > + +1.0f,-1.0f,
> > + 0.0f,1.0f,
> > + 0.0f,0.0f,
> > + 1.0f,0.0f,
> > + 1.0f,1.0f,
>
> When you get here, checkstyle will suggest putting this all in one
> vertical column.
>
> Ignore it... (checkstyle is just advice, and in this case your layout is
> better).
>
> Though I would add a space at least after the first ',' on each line to
> make the columns clearer., Or go further and align the 'f's... ?
>
> > + };
> > +
> > + glBuffer.create();
> > + glBuffer.bind();
> > + glBuffer.allocate(vertices,sizeof(vertices));
> > +
> > + /* Create Vertex Shader */
> > + pVShader = new QOpenGLShader(QOpenGLShader::Vertex, this);
> > + char *vsrc = NV_Vertex_shader;
> > +
> > +�� bCompile = pVShader->compileSourceCode(vsrc);
> > + if(!bCompile) {
> > + qDebug() << __func__<< ":" << pVShader->log();
> > + }
> > +
> > + shaderProgram.addShader(pVShader);
> > +
> > + glClearColor(1.0f, 1.0f, 1.0f, 0.0f);
> > +}
> > +
> > +void ViewFinderGL::render()
> > +{
> > + switch(format_) {
> > + case libcamera::formats::NV12:
> > + case libcamera::formats::NV21:
> > + case libcamera::formats::NV16:
> > + case libcamera::formats::NV61:
> > + case libcamera::formats::NV24:
> > + case libcamera::formats::NV42:
> > + /* activate texture 0 */
> > + glActiveTexture(GL_TEXTURE0);
> > + configureTexture(id_y);
> > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED,
> size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr);
> > + glUniform1i(textureUniformY, 0);
> > +
> > + /* activate texture 1 */
> > + glActiveTexture(GL_TEXTURE1);
> > + configureTexture(id_u);
> > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG,
> size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0,
> GL_RG, GL_UNSIGNED_BYTE,
> (char*)yuvDataPtr+size_.width()*size_.height());
> > + glUniform1i(textureUniformU, 1);
> > + break;
> > + case libcamera::formats::YUV420:
> > + /* activate texture 0 */
> > + glActiveTexture(GL_TEXTURE0);
> > + configureTexture(id_y);
> > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RED,
> size_.width(), size_.height(), 0, GL_RED, GL_UNSIGNED_BYTE, yuvDataPtr);
> > + glUniform1i(textureUniformY, 0);
> > +
> > + /* activate texture 1 */
> > + glActiveTexture(GL_TEXTURE1);
> > + configureTexture(id_u);
> > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG,
> size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0,
> GL_RG, GL_UNSIGNED_BYTE,
> (char*)yuvDataPtr+size_.width()*size_.height());
> > + glUniform1i(textureUniformU, 1);
> > +
> > + /* activate texture 2 */
> > + glActiveTexture(GL_TEXTURE2);
> > + configureTexture(id_v);
> > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RG,
> size_.width()/horzSubSample_, size_.height()/vertSubSample_, 0,
> GL_RG, GL_UNSIGNED_BYTE,
> (char*)yuvDataPtr+size_.width()*size_.height()*5/4);
> > + glUniform1i(textureUniformV, 2);
> > + break;
> > + default:
> > + break;
> > + };
> > +}
> > +
> > +void ViewFinderGL::paintGL()
> > +{
> > + if(pFShader == nullptr)
> > + createFragmentShader();
> > +
> > + if(yuvDataPtr)
> > + {
> > + glClearColor(0.0, 0.0, 0.0, 1.0);
> > + glClear( GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT );
> > +
> > + render();
> > + glDrawArrays(GL_TRIANGLE_FAN, 0, 4);
> > + }
> > +}
> > +
> > +void ViewFinderGL::resizeGL(int w, int h)
> > +{
> > + glViewport(0,0,w,h);
> > +}
> > +
> > +QSize ViewFinderGL::sizeHint() const
> > +{
> > + return size_.isValid() ? size_ : QSize(640, 480);
> > +}
>
> I wonder if this sizeHint should go to the base class?
>
> Does ViewFinderGL use it? I think it's there to provide a required
> overload for the QWidget ... if it's common it could go to the base, and
> if it's not used, it could get dropped.
>
> Got it.
>
>
>
>
> > diff --git a/src/qcam/viewfinderGL.h b/src/qcam/viewfinderGL.h
> > new file mode 100644
> > index 0000000..08662ca
> > --- /dev/null
> > +++ b/src/qcam/viewfinderGL.h
> > @@ -0,0 +1,101 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Linaro
> > + *
> > + * viewfinderGL.h - Render YUV format frame by OpenGL shader
> > + */
> > +#ifndef __VIEWFINDERGL_H__
>
> Perhaps to make this clearer use this: __VIEWFINDER_GL_H__ ?
> An underscore between the ViewFinder and the GL would separate the base,
> and the derived names...
> Ok, I will fix this.
>
> > +#define __VIEWFINDERGL_H__
> > +
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +
> > +#include <iomanip>
> > +#include <iostream>
> > +#include <sstream>
> > +
> > +#include <QImage>
> > +#include <QOpenGLBuffer>
> > +#include <QOpenGLFunctions>
> > +#include <QOpenGLShader>
> > +#include <QOpenGLShaderProgram>
> > +#include <QSize>
> > +#include <QOpenGLTexture>
> > +#include <QOpenGLWidget>
> > +
> > +#include <libcamera/buffer.h>
> > +#include <libcamera/pixel_format.h>
> > +#include "viewfinder.h"
> > +
> > +class ViewFinderGL : public QOpenGLWidget,
> > + public ViewFinderHandler,
> > + protected QOpenGLFunctions
>
> That indentation looks off, I think they should be aligned vertically at
> least.
>
> Sure.
>
>
>
> > +{
> > + Q_OBJECT
> > +
> > +public:
> > + ViewFinderGL(QWidget *parent = 0);
> > + ~ViewFinderGL();
> > +
> > + int setFormat(const libcamera::PixelFormat &format, const
> QSize &size);
> > + void render(libcamera::FrameBuffer *buffer, MappedBuffer *map);
> > + void stop();
> > +
> > + QImage getCurrentImage();
>
> Aha - that's why we need QImage, I think that's part of the save routine
> perhaps isn't it ...
>
> > +
> > + void setFrameSize(int width, int height);
>
> I can't see what calls setFrameSize... is it redundant? Or is it an
> override used externally or such. If so I think it needs to be marked as
> accordingly as an override?
>
> > + void updateFrame(unsigned char *buffer);
>
> Same here... What calls updateFrame()?
>
> Actually, only NV12, I can verify with Libcamera on rock pi 4, I use an
> external application to verify other shaders and above functions are for
> that.
> I will remove that in next version.
>
>
>
> > +
> > + char *selectFragmentShader(unsigned int format);
> > + void createFragmentShader();
> > + void render();
> > +
> > +Q_SIGNALS:
> > + void renderComplete(libcamera::FrameBuffer *buffer);
> > +
> > +protected:
> > + void initializeGL() Q_DECL_OVERRIDE;
> > + void paintGL() Q_DECL_OVERRIDE;
> > + void resizeGL(int w, int h) Q_DECL_OVERRIDE;
> > + QSize sizeHint() const override;
>
> is sizeHint() used?
>
>
> > +
> > +private:
> > +
> > + void configureTexture(unsigned int id);
> > + void removeShader();
> > +
> > + /* Captured image size, format and buffer */
> > + libcamera::FrameBuffer *buffer_;
> > + libcamera::PixelFormat format_;
> > + QOpenGLBuffer glBuffer;
>
> glBuffer_;
>
> Will fix it.
>
>
>
>
> > + QSize size_;
> > +
> > + GLuint id_u;
> > + GLuint id_v;
> > + GLuint id_y;
> > +
> > + QMutex mutex_; /* Prevent concurrent access to image_ */
> > +
>
> All of the member variables below should have a '_' suffix...
>
> > + QOpenGLShader *pFShader;
> > + QOpenGLShader *pVShader;
> > + QOpenGLShaderProgram shaderProgram;
> > +
> > + GLuint textureUniformU;
> > + GLuint textureUniformV;
> > + GLuint textureUniformY;
> > +
> > + QOpenGLTexture textureU;
> > + QOpenGLTexture textureV;
> > + QOpenGLTexture textureY;
> > +
> > + unsigned int width_;
> > + unsigned int height_;
>
> There is already a QSize size_, do these duplicate that purpose?
>
> Yeah, I will use size_ instead.
>
>
> > +
> > + unsigned char* yuvDataPtr;
> > +
> > + /* NV parameters */
> > + unsigned int horzSubSample_ ;
>
> Extra space?
>
> Ok will fix it.
>
>
> > + unsigned int vertSubSample_;
> > +};
> > +#endif // __VIEWFINDERGL_H__
> >
>
>
>
> --
> Regards
> --
> Kieran
>
>
> Thank you for your comments again.
> I will post next version asap.
>
> --
> Regards
> --
> Show
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list