<div dir="ltr"><div dir="ltr">Hi Laurent,</div><div dir="ltr"><br></div><div dir="ltr">Thanks for your review comments.</div><div dir="ltr">I will have the next version soon accordingly.</div><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 20, 2020 at 10:54 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">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>
Thank you for the patch.<br>
<br>
On Fri, Mar 20, 2020 at 04:50:29PM +0800, Show Liu wrote:<br>
> qcam: added an option to enable rendering via OpenGL shader<br>
<br>
s/added/Add/<br>
<br>
You don't need to repeat the subject line in the commit message, but it<br>
could be useful to add a bit more detail in the commit message (although<br>
in this case the change is fairly trivial).<br>
<br>
> Signed-off-by: Show Liu <<a href="mailto:show.liu@linaro.org" target="_blank">show.liu@linaro.org</a>><br>
> ---<br>
> src/qcam/main.cpp | 2 ++<br>
> src/qcam/main_window.cpp | 31 ++++++++++++++++++++++++++++---<br>
> src/qcam/main_window.h | 3 +++<br>
> 3 files changed, 33 insertions(+), 3 deletions(-)<br>
> <br>
> diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp<br>
> index a7ff5c5..7727a09 100644<br>
> --- a/src/qcam/main.cpp<br>
> +++ b/src/qcam/main.cpp<br>
> @@ -39,6 +39,8 @@ OptionsParser::Options parseOptions(int argc, char *argv[])<br>
> "help");<br>
> parser.addOption(OptSize, &sizeParser, "Set the stream size",<br>
> "size", true);<br>
> + parser.addOption(OptOpengl, OptionNone, "Enable YUV format via OpenGL shader",<br>
> + "opengl");<br>
<br>
How about a renderer option instead, that would take a string value ? It<br>
would be useful to support more renderer in the future if we need to. Or<br>
maybe that will never be needed ?<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 98e55ba..5a5bc88 100644<br>
> --- a/src/qcam/main_window.cpp<br>
> +++ b/src/qcam/main_window.cpp<br>
> @@ -28,6 +28,7 @@<br>
> <br>
> #include "main_window.h"<br>
> #include "viewfinder.h"<br>
> +#include "glwidget.h"<br>
<br>
Alphabetical order please.<br>
<br>
> <br>
> using namespace libcamera;<br>
> <br>
> @@ -43,7 +44,14 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)<br>
> connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));<br>
> <br>
> viewfinder_ = new ViewFinder(this);<br>
> - setCentralWidget(viewfinder_);<br>
> + glwidget_ = new GLWidget(this);<br>
<br>
I think we should create a base ViewFinder class (renaming the existing<br>
ViewFinder to ViewFinderQImage or ViewFinderQPainter), and make the two<br>
implementations inherit from the base. This code would then become<br>
<br>
if (options_.isSet(OptOpengl))<br>
viewfinder_ = new ViewFinderGL(this);<br>
else<br>
viewfinder_ = new ViewFinderQPainter(this);<br>
<br>
setCentralWidget(viewfinder_);<br>
<br>
and you won't have to test for options_.isSet(OptOpengl) anywere below.<br>
<br>
> +<br>
> + if (options_.isSet(OptOpengl)) {<br>
> + setCentralWidget(glwidget_);<br>
> + } else {<br>
> + setCentralWidget(viewfinder_);<br>
> + }<br>
> +<br>
> adjustSize();<br>
> <br>
> ret = openCamera();<br>
> @@ -232,6 +240,7 @@ int MainWindow::startCapture()<br>
> }<br>
> <br>
> Stream *stream = cfg.stream();<br>
> +<br>
<br>
I don't think this is needed.<br>
<br>
> ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width,<br>
> cfg.size.height);<br>
> if (ret < 0) {<br>
> @@ -239,6 +248,11 @@ int MainWindow::startCapture()<br>
> return ret;<br>
> }<br>
> <br>
> + if (options_.isSet(OptOpengl)) {<br>
> + glwidget_->setFrameSize(cfg.size.width, cfg.size.height);<br>
> + glwidget_->setFixedSize(cfg.size.width, cfg.size.height);<br>
> + }<br>
> +<br>
> statusBar()->showMessage(QString(cfg.toString().c_str()));<br>
> <br>
> adjustSize();<br>
> @@ -353,7 +367,13 @@ void MainWindow::stopCapture()<br>
> <br>
> void MainWindow::saveImageAs()<br>
> {<br>
> - QImage image = viewfinder_->getCurrentImage();<br>
> + QImage image;<br>
> + if (options_.isSet(OptOpengl)) {<br>
> + image = glwidget_->grabFramebuffer();<br>
> + } else {<br>
> + image = viewfinder_->getCurrentImage();<br>
> + }<br>
> +<br>
> QString defaultPath = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation);<br>
> <br>
> QString filename = QFileDialog::getSaveFileName(this, "Save Image", defaultPath,<br>
> @@ -416,7 +436,12 @@ int MainWindow::display(FrameBuffer *buffer)<br>
> const FrameBuffer::Plane &plane = buffer->planes().front();<br>
> void *memory = mappedBuffers_[plane.fd.fd()].first;<br>
> unsigned char *raw = static_cast<unsigned char *>(memory);<br>
> - viewfinder_->display(raw, buffer->metadata().planes[0].bytesused);<br>
> +<br>
> + if (options_.isSet(OptOpengl)) {<br>
> + glwidget_->updateFrame(raw);<br>
> + } else {<br>
> + viewfinder_->display(raw, buffer->metadata().planes[0].bytesused);<br>
> + }<br>
> <br>
> return 0;<br>
> }<br>
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h<br>
> index 40aa10a..5501dd1 100644<br>
> --- a/src/qcam/main_window.h<br>
> +++ b/src/qcam/main_window.h<br>
> @@ -21,6 +21,7 @@<br>
> #include <libcamera/stream.h><br>
> <br>
> #include "../cam/options.h"<br>
> +#include "glwidget.h"<br>
> <br>
> using namespace libcamera;<br>
> <br>
> @@ -30,6 +31,7 @@ enum {<br>
> OptCamera = 'c',<br>
> OptHelp = 'h',<br>
> OptSize = 's',<br>
> + OptOpengl = 'o',<br>
<br>
Maybe OptOpenGL ?<br>
<br>
> };<br>
> <br>
> class MainWindow : public QMainWindow<br>
> @@ -79,6 +81,7 @@ private:<br>
> <br>
> QToolBar *toolbar_;<br>
> ViewFinder *viewfinder_;<br>
> + GLWidget *glwidget_;<br>
> std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;<br>
> };<br>
> <br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br></blockquote><div><br></div><br clear="all"><div><div dir="ltr"><div dir="ltr">Best Regards,</div></div></div><div>Show Liu </div></div></div>