<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Sep 16, 2020 at 10:33 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Show,<br>
<br>
On Tue, Sep 15, 2020 at 05:00:54PM +0800, Show Liu wrote:<br>
> On Tue, Sep 15, 2020 at 10:46 AM Laurent Pinchart wrote:<br>
> > Hello,<br>
> ><br>
> > This patch series is an updated version of Show's v6. I've updated the<br>
> > patches during review as I wanted to test my review comments, and it<br>
> > would be pointless for Show to do the same independently to post a v7.<br>
> ><br>
> > The series only incorporates review comments. Show, could you please let<br>
> > me know if you're fine with the result ? To make the comparison easier,<br>
> > here's the diff between v6 and v7.<br>
> ><br>
> > diff --git a/src/qcam/assets/shader/NV_2_planes_UV_f.glsl<br>
> > b/src/qcam/assets/shader/NV_2_planes_UV_f.glsl<br>
> > index 80478c5d0dd4..67633a11ee0f 100644<br>
> > --- a/src/qcam/assets/shader/NV_2_planes_UV_f.glsl<br>
> > +++ b/src/qcam/assets/shader/NV_2_planes_UV_f.glsl<br>
> > @@ -18,10 +18,10 @@ void main(void)<br>
> > vec3 yuv;<br>
> > vec3 rgb;<br>
> > mat3 yuv2rgb_bt601_mat = mat3(<br>
> > - vec3(1.164, 1.164, 1.164),<br>
> > - vec3(0.000, -0.392, 2.017),<br>
> > - vec3(1.596, -0.813, 0.000)<br>
> > - );<br>
> > + vec3(1.164, 1.164, 1.164),<br>
> > + vec3(0.000, -0.392, 2.017),<br>
> > + vec3(1.596, -0.813, 0.000)<br>
> > + );<br>
> ><br>
> > yuv.x = texture2D(tex_y, textureOut).r - 0.063;<br>
> > yuv.y = texture2D(tex_u, textureOut).r - 0.500;<br>
> > diff --git a/src/qcam/assets/shader/NV_2_planes_VU_f.glsl<br>
> > b/src/qcam/assets/shader/NV_2_planes_VU_f.glsl<br>
> > index 3794be843590..086c5b6d11bd 100644<br>
> > --- a/src/qcam/assets/shader/NV_2_planes_VU_f.glsl<br>
> > +++ b/src/qcam/assets/shader/NV_2_planes_VU_f.glsl<br>
> > @@ -18,10 +18,10 @@ void main(void)<br>
> > vec3 yuv;<br>
> > vec3 rgb;<br>
> > mat3 yuv2rgb_bt601_mat = mat3(<br>
> > - vec3(1.164, 1.164, 1.164),<br>
> > - vec3(0.000, -0.392, 2.017),<br>
> > - vec3(1.596, -0.813, 0.000)<br>
> > - );<br>
> > + vec3(1.164, 1.164, 1.164),<br>
> > + vec3(0.000, -0.392, 2.017),<br>
> > + vec3(1.596, -0.813, 0.000)<br>
> > + );<br>
> ><br>
> > yuv.x = texture2D(tex_y, textureOut).r - 0.063;<br>
> > yuv.y = texture2D(tex_u, textureOut).g - 0.500;<br>
> > diff --git a/src/qcam/assets/shader/NV_3_planes_f.glsl<br>
> > b/src/qcam/assets/shader/NV_3_planes_f.glsl<br>
> > index fca9b659ca05..4bc941842710 100644<br>
> > --- a/src/qcam/assets/shader/NV_3_planes_f.glsl<br>
> > +++ b/src/qcam/assets/shader/NV_3_planes_f.glsl<br>
> > @@ -19,10 +19,10 @@ void main(void)<br>
> > vec3 yuv;<br>
> > vec3 rgb;<br>
> > mat3 yuv2rgb_bt601_mat = mat3(<br>
> > - vec3(1.164, 1.164, 1.164),<br>
> > - vec3(0.000, -0.392, 2.017),<br>
> > - vec3(1.596, -0.813, 0.000)<br>
> > - );<br>
> > + vec3(1.164, 1.164, 1.164),<br>
> > + vec3(0.000, -0.392, 2.017),<br>
> > + vec3(1.596, -0.813, 0.000)<br>
> > + );<br>
> ><br>
> > yuv.x = texture2D(tex_y, textureOut).r - 0.063;<br>
> > yuv.y = texture2D(tex_u, textureOut).r - 0.500;<br>
> > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp<br>
> > index 4b7d04100c08..f60d3cef0ecb 100644<br>
> > --- a/src/qcam/main.cpp<br>
> > +++ b/src/qcam/main.cpp<br>
> > @@ -33,9 +33,9 @@ OptionsParser::Options parseOptions(int argc, char<br>
> > *argv[])<br>
> > ArgumentRequired, "camera");<br>
> > parser.addOption(OptHelp, OptionNone, "Display this help message",<br>
> > "help");<br>
> > - parser.addOption(OptRendered, OptionString,<br>
> > - "Choose the renderer type {qt,gles} (default:<br>
> > qt)", "renderer",<br>
> > - ArgumentRequired, "render");<br>
> > + parser.addOption(OptRenderer, OptionString,<br>
> > + "Choose the renderer type {qt,gles} (default:<br>
> > qt)",<br>
> > + "renderer", ArgumentRequired, "renderer");<br>
> > parser.addOption(OptStream, &streamKeyValue,<br>
> > "Set configuration of a camera stream", "stream",<br>
> > true);<br>
> ><br>
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp<br>
> > index 315102c39526..e5233f4fb706 100644<br>
> > --- a/src/qcam/main_window.cpp<br>
> > +++ b/src/qcam/main_window.cpp<br>
> > @@ -28,6 +28,8 @@<br>
> > #include <libcamera/version.h><br>
> ><br>
> > #include "dng_writer.h"<br>
> > +#include "viewfinder_gl.h"<br>
> > +#include "viewfinder_qt.h"<br>
> ><br>
> > using namespace libcamera;<br>
> ><br>
> > @@ -95,9 +97,6 @@ MainWindow::MainWindow(CameraManager *cm, const<br>
> > OptionsParser::Options &options)<br>
> > {<br>
> > int ret;<br>
> ><br>
> > - /* Render Type Qt or GLES, and set Qt by default */<br>
> > - std::string renderType_ = "qt";<br>
> > -<br>
> > /*<br>
> > * Initialize the UI: Create the toolbar, set the window title and<br>
> > * create the viewfinder widget.<br>
> > @@ -108,17 +107,19 @@ MainWindow::MainWindow(CameraManager *cm, const<br>
> > OptionsParser::Options &options)<br>
> > setWindowTitle(title_);<br>
> > connect(&titleTimer_, SIGNAL(timeout()), this,<br>
> > SLOT(updateTitle()));<br>
> ><br>
> > - if (options_.isSet(OptRendered))<br>
> > - renderType_ = options_[OptRendered].toString();<br>
> > + /* Renderer type Qt or GLES, select Qt by default. */<br>
> > + std::string renderType = "qt";<br>
> > + if (options_.isSet(OptRenderer))<br>
> > + renderType = options_[OptRenderer].toString();<br>
> ><br>
> > - if (renderType_ == "qt") {<br>
> > + if (renderType == "qt") {<br>
> > ViewFinderQt *viewfinder = new ViewFinderQt(this);<br>
> > connect(viewfinder, &ViewFinderQt::renderComplete,<br>
> > this, &MainWindow::queueRequest);<br>
> > viewfinder_ = viewfinder;<br>
> > setCentralWidget(viewfinder);<br>
> > #ifndef QT_NO_OPENGL<br>
> > - } else if (renderType_ == "gles") {<br>
> > + } else if (renderType == "gles") {<br>
> > ViewFinderGL *viewfinder = new ViewFinderGL(this);<br>
> > connect(viewfinder, &ViewFinderGL::renderComplete,<br>
> > this, &MainWindow::queueRequest);<br>
> > @@ -127,7 +128,7 @@ MainWindow::MainWindow(CameraManager *cm, const<br>
> > OptionsParser::Options &options)<br>
> > #endif<br>
> > } else {<br>
> > qWarning() << "Invalid render type"<br>
> > - << QString::fromStdString(renderType_);<br>
> > + << QString::fromStdString(renderType);<br>
> > quit();<br>
> > return;<br>
> > }<br>
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h<br>
> > index 251f78bf6833..5c61a4dfce53 100644<br>
> > --- a/src/qcam/main_window.h<br>
> > +++ b/src/qcam/main_window.h<br>
> > @@ -26,8 +26,6 @@<br>
> ><br>
> > #include "../cam/stream_options.h"<br>
> > #include "viewfinder.h"<br>
> > -#include "viewfinder_gl.h"<br>
> > -#include "viewfinder_qt.h"<br>
> ><br>
> > using namespace libcamera;<br>
> ><br>
> > @@ -39,7 +37,7 @@ class HotplugEvent;<br>
> > enum {<br>
> > OptCamera = 'c',<br>
> > OptHelp = 'h',<br>
> > - OptRendered = 'r',<br>
> > + OptRenderer = 'r',<br>
> > OptStream = 's',<br>
> > };<br>
> ><br>
> > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp<br>
> > index 84f48666af5b..fbe21dcf1ad2 100644<br>
> > --- a/src/qcam/viewfinder_gl.cpp<br>
> > +++ b/src/qcam/viewfinder_gl.cpp<br>
> > @@ -19,7 +19,7 @@ static const QList<libcamera::PixelFormat><br>
> > supportedFormats{<br>
> > libcamera::formats::NV24,<br>
> > libcamera::formats::NV42,<br>
> > libcamera::formats::YUV420,<br>
> > - libcamera::formats::YVU420<br>
> > + libcamera::formats::YVU420,<br>
> > };<br>
> ><br>
> > ViewFinderGL::ViewFinderGL(QWidget *parent)<br>
> > @@ -35,9 +35,6 @@ ViewFinderGL::ViewFinderGL(QWidget *parent)<br>
> > ViewFinderGL::~ViewFinderGL()<br>
> > {<br>
> > removeShader();<br>
> > -<br>
> > - if (vertexBuffer_.isCreated())<br>
> > - vertexBuffer_.destroy();<br>
> > }<br>
> ><br>
> > const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const<br>
> > @@ -48,9 +45,7 @@ const QList<libcamera::PixelFormat><br>
> > &ViewFinderGL::nativeFormats() const<br>
> > int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,<br>
> > const QSize &size)<br>
> > {<br>
> > - int ret = 0;<br>
> > -<br>
> > - /* If the fragment is ceeated remove it and create a new one */<br>
> > + /* If the fragment is created remove it and create a new one. */<br>
> > if (fragmentShader_) {<br>
> > if (shaderProgram_.isLinked()) {<br>
> > shaderProgram_.release();<br>
> > @@ -59,14 +54,14 @@ int ViewFinderGL::setFormat(const<br>
> > libcamera::PixelFormat &format,<br>
> > }<br>
> > }<br>
> ><br>
> > - if (selectFormat(format)) {<br>
> > - format_ = format;<br>
> > - size_ = size;<br>
> > - } else {<br>
> > - ret = -1;<br>
> > - }<br>
> > + if (!selectFormat(format))<br>
> > + return -1;<br>
> > +<br>
> > + format_ = format;<br>
> > + size_ = size;<br>
> > +<br>
> > updateGeometry();<br>
> > - return ret;<br>
> > + return 0;<br>
> > }<br>
> ><br>
> > void ViewFinderGL::stop()<br>
> > @@ -279,10 +274,10 @@ void ViewFinderGL::initializeGL()<br>
> > },<br>
> > {<br>
> > /* Texture coordinates */<br>
> > - { 1.0f, 0.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>
> > };<br>
> ><br>
> > @@ -306,7 +301,7 @@ void ViewFinderGL::doRender()<br>
> > case libcamera::formats::NV61:<br>
> > case libcamera::formats::NV24:<br>
> > case libcamera::formats::NV42:<br>
> > - /* activate texture Y */<br>
> > + /* Activate texture Y */<br>
> > glActiveTexture(GL_TEXTURE0);<br>
> > configureTexture(id_y_);<br>
> > glTexImage2D(GL_TEXTURE_2D,<br>
> > @@ -320,7 +315,7 @@ void ViewFinderGL::doRender()<br>
> > yuvData_);<br>
> > shaderProgram_.setUniformValue(textureUniformY_, 0);<br>
> ><br>
> > - /* activate texture UV/VU */<br>
> > + /* Activate texture UV/VU */<br>
> > glActiveTexture(GL_TEXTURE1);<br>
> > configureTexture(id_u_);<br>
> > glTexImage2D(GL_TEXTURE_2D,<br>
> > @@ -336,7 +331,7 @@ void ViewFinderGL::doRender()<br>
> > break;<br>
> ><br>
> > case libcamera::formats::YUV420:<br>
> > - /* activate texture Y */<br>
> > + /* Activate texture Y */<br>
> > glActiveTexture(GL_TEXTURE0);<br>
> > configureTexture(id_y_);<br>
> > glTexImage2D(GL_TEXTURE_2D,<br>
> > @@ -350,7 +345,7 @@ void ViewFinderGL::doRender()<br>
> > yuvData_);<br>
> > shaderProgram_.setUniformValue(textureUniformY_, 0);<br>
> ><br>
> > - /* activate texture U */<br>
> > + /* Activate texture U */<br>
> > glActiveTexture(GL_TEXTURE1);<br>
> > configureTexture(id_u_);<br>
> > glTexImage2D(GL_TEXTURE_2D,<br>
> > @@ -364,7 +359,7 @@ void ViewFinderGL::doRender()<br>
> > (char *)yuvData_ + size_.width() *<br>
> > size_.height());<br>
> > shaderProgram_.setUniformValue(textureUniformU_, 1);<br>
> ><br>
> > - /* activate texture V */<br>
> > + /* Activate texture V */<br>
> > glActiveTexture(GL_TEXTURE2);<br>
> > configureTexture(id_v_);<br>
> > glTexImage2D(GL_TEXTURE_2D,<br>
> > @@ -380,7 +375,7 @@ void ViewFinderGL::doRender()<br>
> > break;<br>
> ><br>
> > case libcamera::formats::YVU420:<br>
> > - /* activate texture Y */<br>
> > + /* Activate texture Y */<br>
> > glActiveTexture(GL_TEXTURE0);<br>
> > configureTexture(id_y_);<br>
> > glTexImage2D(GL_TEXTURE_2D,<br>
> > @@ -394,7 +389,7 @@ void ViewFinderGL::doRender()<br>
> > yuvData_);<br>
> > shaderProgram_.setUniformValue(textureUniformY_, 0);<br>
> ><br>
> > - /* activate texture V */<br>
> > + /* Activate texture V */<br>
> > glActiveTexture(GL_TEXTURE2);<br>
> > configureTexture(id_v_);<br>
> > glTexImage2D(GL_TEXTURE_2D,<br>
> > @@ -406,9 +401,9 @@ void ViewFinderGL::doRender()<br>
> > GL_RED,<br>
> > GL_UNSIGNED_BYTE,<br>
> > (char *)yuvData_ + size_.width() *<br>
> > size_.height());<br>
> > - shaderProgram_.setUniformValue(textureUniformV_, 1);<br>
> > + shaderProgram_.setUniformValue(textureUniformV_, 2);<br>
> ><br>
> > - /* activate texture U */<br>
> > + /* Activate texture U */<br>
> > glActiveTexture(GL_TEXTURE1);<br>
> > configureTexture(id_u_);<br>
> > glTexImage2D(GL_TEXTURE_2D,<br>
> > @@ -420,7 +415,7 @@ void ViewFinderGL::doRender()<br>
> > GL_RED,<br>
> > GL_UNSIGNED_BYTE,<br>
> > (char *)yuvData_ + size_.width() *<br>
> > size_.height() * 5 / 4);<br>
> > - shaderProgram_.setUniformValue(textureUniformU_, 2);<br>
> > + shaderProgram_.setUniformValue(textureUniformU_, 1);<br>
> ><br>
> <br>
> I applied the above changes and tested on my rockpi4b with imx219. It's<br>
> running well.<br>
> But I have only the imx219 camera module to test with qcam, which means<br>
> qcam only verified NV12 format is correct.<br>
> So I use another small tool to test other formats and shader code.<br>
<br>
Would it be possible to share that tool ?<br>
<br>
> When the format YVU420 and YUV420 use the same shader code.<br>
> + shaderProgram_.setUniformValue(textureUniformV_, 2);<br>
> ...<br>
> + shaderProgram_.setUniformValue(textureUniformU_, 1);<br>
> above change cause the color became blue in format YVU420.<br>
> The original value is correct I think.<br>
<br>
There's something I don't get then. The common shader is written as<br>
<br>
yuv.x = texture2D(tex_y, textureOut).r - 0.063;<br>
yuv.y = texture2D(tex_u, textureOut).r - 0.500;<br>
yuv.z = texture2D(tex_v, textureOut).r - 0.500;<br>
<br>
so it expects Y in the tex_y uniform, U in the tex_u uniform and V in<br>
the tex_v uniform. We retrieve the uniforms with<br>
<br>
textureUniformY_ = shaderProgram_.uniformLocation("tex_y");<br>
textureUniformU_ = shaderProgram_.uniformLocation("tex_u");<br>
textureUniformV_ = shaderProgram_.uniformLocation("tex_v");<br>
<br>
For YUV420, we have<br>
<br>
case libcamera::formats::YUV420:<br>
/* Activate texture Y */<br>
glActiveTexture(GL_TEXTURE0);<br>
configureTexture(id_y_);<br>
glTexImage2D(...);<br>
shaderProgram_.setUniformValue(textureUniformY_, 0);<br>
<br>
/* Activate texture U */<br>
glActiveTexture(GL_TEXTURE1);<br>
configureTexture(id_u_);<br>
glTexImage2D(GL_TEXTURE_2D,<br>
0,<br>
GL_RED,<br>
size_.width() / horzSubSample_,<br>
size_.height() / vertSubSample_,<br>
0,<br>
GL_RED,<br>
GL_UNSIGNED_BYTE,<br>
(char *)yuvData_ + size_.width() * size_.height());<br>
shaderProgram_.setUniformValue(textureUniformU_, 1);<br>
<br>
/* Activate texture V */<br>
glActiveTexture(GL_TEXTURE2);<br>
configureTexture(id_v_);<br>
glTexImage2D(GL_TEXTURE_2D,<br>
0,<br>
GL_RED,<br>
size_.width() / horzSubSample_,<br>
size_.height() / vertSubSample_,<br>
0,<br>
GL_RED,<br>
GL_UNSIGNED_BYTE,<br>
(char *)yuvData_ + size_.width() * size_.height() * 5 / 4);<br>
shaderProgram_.setUniformValue(textureUniformV_, 2);<br>
<br>
We store 0, which I assume corresponds to GL_TEXTURE0, in<br>
textureUniformY_, 1 in textureUniformU_ and 2 in textureUniformV_.<br>
GL_TEXTURE[012] are filled with the Y, U and V data respectively. So<br>
far, so good.<br>
<br>
For YVU420, we have<br>
<br>
case libcamera::formats::YVU420:<br>
/* Activate texture Y */<br>
glActiveTexture(GL_TEXTURE0);<br>
configureTexture(id_y_);<br>
glTexImage2D(...);<br>
shaderProgram_.setUniformValue(textureUniformY_, 0);<br>
<br>
/* Activate texture V */<br>
glActiveTexture(GL_TEXTURE2);<br>
configureTexture(id_v_);<br>
glTexImage2D(GL_TEXTURE_2D,<br>
0,<br>
GL_RED,<br>
size_.width() / horzSubSample_,<br>
size_.height() / vertSubSample_,<br>
0,<br>
GL_RED,<br>
GL_UNSIGNED_BYTE,<br>
(char *)yuvData_ + size_.width() * size_.height());<br>
shaderProgram_.setUniformValue(textureUniformV_, 2);<br>
<br>
/* Activate texture U */<br>
glActiveTexture(GL_TEXTURE1);<br>
configureTexture(id_u_);<br>
glTexImage2D(GL_TEXTURE_2D,<br>
0,<br>
GL_RED,<br>
size_.width() / horzSubSample_,<br>
size_.height() / vertSubSample_,<br>
0,<br>
GL_RED,<br>
GL_UNSIGNED_BYTE,<br>
(char *)yuvData_ + size_.width() * size_.height() * 5 / 4);<br>
shaderProgram_.setUniformValue(textureUniformU_, 1);<br>
<br>
We handle Y the same way. We also store V in GL_TEXTURE2, and set<br>
textureUniformV_ to 2 accordingly. The difference is in the<br>
glTexImage2D() call, that's where we swap U and V. What am I missing ?<br></blockquote><div>Yeah. you are right.</div><div>Sorry, my fault. I activated the wrong texture(GL_TEXTURE1/2) in that tool. </div><div><br></div><div><br></div><div>Thanks,</div><div>Show</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > break;<br>
> ><br>
> > default:<br>
> ><br>
> > Show Liu (4):<br>
> > qcam: Add OpenGL shader code as Qt resource<br>
> > qcam: New viewfinder hierarchy<br>
> > qcam: Add ViewFinderGL class to accelerate the format conversion<br>
> > qcam: Add additional command line option to select the renderer type<br>
> ><br>
> > meson.build | 1 +<br>
> > src/qcam/assets/shader/NV_2_planes_UV_f.glsl | 32 ++<br>
> > src/qcam/assets/shader/NV_2_planes_VU_f.glsl | 32 ++<br>
> > src/qcam/assets/shader/NV_3_planes_f.glsl | 33 ++<br>
> > src/qcam/assets/shader/NV_vertex_shader.glsl | 16 +<br>
> > src/qcam/assets/shader/shaders.qrc | 9 +<br>
> > src/qcam/main.cpp | 3 +<br>
> > src/qcam/main_window.cpp | 32 +-<br>
> > src/qcam/main_window.h | 1 +<br>
> > src/qcam/meson.build | 20 +-<br>
> > src/qcam/viewfinder.h | 57 +--<br>
> > src/qcam/viewfinder_gl.cpp | 451 ++++++++++++++++++<br>
> > src/qcam/viewfinder_gl.h | 96 ++++<br>
> > .../{viewfinder.cpp => viewfinder_qt.cpp} | 24 +-<br>
> > src/qcam/viewfinder_qt.h | 64 +++<br>
> > 15 files changed, 806 insertions(+), 65 deletions(-)<br>
> > create mode 100644 src/qcam/assets/shader/NV_2_planes_UV_f.glsl<br>
> > create mode 100644 src/qcam/assets/shader/NV_2_planes_VU_f.glsl<br>
> > create mode 100644 src/qcam/assets/shader/NV_3_planes_f.glsl<br>
> > create mode 100644 src/qcam/assets/shader/NV_vertex_shader.glsl<br>
> > create mode 100644 src/qcam/assets/shader/shaders.qrc<br>
> > create mode 100644 src/qcam/viewfinder_gl.cpp<br>
> > create mode 100644 src/qcam/viewfinder_gl.h<br>
> > rename src/qcam/{viewfinder.cpp => viewfinder_qt.cpp} (86%)<br>
> > create mode 100644 src/qcam/viewfinder_qt.h<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>