[libcamera-devel] [RFC] [PATCH 3/3] qcam: added an option to enable rendering via OpenGL shader

[Show Liu]

Hi Laurent,

Thanks for your review comments.
I will have the next version soon accordingly.


On Fri, Mar 20, 2020 at 10:54 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

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


Best Regards,
Show Liu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200323/031a8507/attachment-0001.htm>