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