| Message ID | 20200320085029.17875-4-show.liu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Show, Thanks for your work. On 2020-03-20 16:50:29 +0800, Show Liu wrote: > qcam: added an option to enable rendering via OpenGL shader > > Signed-off-by: Show Liu <show.liu@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"); > > 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" > > 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); > + > + if (options_.isSet(OptOpengl)) { > + setCentralWidget(glwidget_); > + } else { > + setCentralWidget(viewfinder_); > + } > + Yes this makes it a bit clearer that a single base class would be nicer :-) > adjustSize(); > > ret = openCamera(); > @@ -232,6 +240,7 @@ int MainWindow::startCapture() > } > > Stream *stream = cfg.stream(); > + > 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', > }; > > class MainWindow : public QMainWindow > @@ -79,6 +81,7 @@ private: > > QToolBar *toolbar_; > ViewFinder *viewfinder_; > + GLWidget *glwidget_; Then this would just be ViewFinder *viewfinder_ and one of the two implementations would be created and a pointer to it stored in viewfinder_. > std::map<int, std::pair<void *, unsigned int>> mappedBuffers_; > }; > > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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_; > }; >
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@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@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
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"); 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" 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); + + if (options_.isSet(OptOpengl)) { + setCentralWidget(glwidget_); + } else { + setCentralWidget(viewfinder_); + } + adjustSize(); ret = openCamera(); @@ -232,6 +240,7 @@ int MainWindow::startCapture() } Stream *stream = cfg.stream(); + 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', }; class MainWindow : public QMainWindow @@ -79,6 +81,7 @@ private: QToolBar *toolbar_; ViewFinder *viewfinder_; + GLWidget *glwidget_; std::map<int, std::pair<void *, unsigned int>> mappedBuffers_; };
qcam: added an option to enable rendering via OpenGL shader Signed-off-by: Show Liu <show.liu@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(-)