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

[Laurent Pinchart]

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