[libcamera-devel] [PATCH v5 4/4] qcam: add additional command line option to select the render type

[Laurent Pinchart]

Hi Show,

Thank you for the patch.

On Fri, Sep 04, 2020 at 04:43:16PM +0800, Show Liu wrote:
> Add new option "--render=qt|gles" to select the render type,
> "--render=gles" to accelerate format conversion and rendering
> "--render=qt" is the original Qt rendering
> 
> Signed-off-by: Show Liu <show.liu at linaro.org>
> ---
>  src/qcam/main.cpp        |  3 +++
>  src/qcam/main_window.cpp | 29 ++++++++++++++++++++++++-----
>  src/qcam/main_window.h   |  6 ++++++
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
> index bae358d..19d0559 100644
> --- a/src/qcam/main.cpp
> +++ b/src/qcam/main.cpp
> @@ -35,6 +35,9 @@ OptionsParser::Options parseOptions(int argc, char *argv[])
>  			 "help");
>  	parser.addOption(OptStream, &streamKeyValue,
>  			 "Set configuration of a camera stream", "stream", true);
> +	parser.addOption(OptRender, OptionString,

Should this be named OptRendered as we're selecting a renderer ?

> +			 "Choose the render type use qt|gles", "render",

"Choose the renderer type {qt,gles} (default: qt)", "renderer"

> +			 ArgumentRequired, "render");

Let's keep options sorted alphabetically.

>  
>  	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 612d978..467cc74 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -91,7 +91,7 @@ private:
>  
>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>  	: saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr),
> -	  isCapturing_(false), captureRaw_(false)
> +	  isCapturing_(false), captureRaw_(false), renderType_("qt")
>  {
>  	int ret;
>  
> @@ -105,10 +105,29 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>  	setWindowTitle(title_);
>  	connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));
>  
> -	viewfinder_ = new ViewFinder(this);
> -	connect(viewfinder_, &ViewFinder::renderComplete,
> -		this, &MainWindow::queueRequest);
> -	setCentralWidget(viewfinder_);
> +	if (options_.isSet(OptRender))
> +		renderType_ = options_[OptRender].toString().c_str();

No need to convert the option to a C string to then convert it back to a
std::string :-)

A note mostly for myself, it would be nice to add support for default
values to the OptionParser class, so that we could specify the default
as "qt" when creating the parser, and just do

	std::string renderType = options_[OptRender].toString();

here. And adding support for pre-defined choices would be good too, so
that the parser would catch invalid values.

> +
> +	if (renderType_ == "qt") {
> +		viewfinder_ = new ViewFinderQt(this);
> +		ViewFinderQt *vf = dynamic_cast<ViewFinderQt *>(viewfinder_);

I'd do it the other way around,

		ViewFinderQt *vf = new ViewFinderQt(this);
		viewfinder_ = vf;

so you can avoid the dynamic_cast.

> +		connect(vf, &ViewFinderQt::renderComplete,
> +			this, &MainWindow::queueRequest);
> +		setCentralWidget(vf);
> +	} else if (renderType_ == "gles") {
> +		viewfinder_ = new ViewFinderGL(this);
> +		ViewFinderGL *vf = dynamic_cast<ViewFinderGL *>(viewfinder_);
> +		connect(vf, &ViewFinderGL::renderComplete,
> +			this, &MainWindow::queueRequest);
> +		setCentralWidget(vf);
> +	} else {
> +		qWarning() << "Render Type:"

s/Type:/type/

> +			   << QString::fromStdString(renderType_)
> +			   << " is not support.";

s/ is/is/ (as Qt inserts spaces automatically)
s/support/supported/

Or maybe

		qWarning() << "Invalid render type"
			   << QString::fromStdString(renderType_);

?

> +		quit();
> +		return;
> +	}
> +
>  	adjustSize();
>  
>  	/* Hotplug/unplug support */
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 3d21779..a69b399 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -26,6 +26,8 @@
>  
>  #include "../cam/stream_options.h"
>  #include "viewfinder.h"
> +#include "viewfinder_gl.h"
> +#include "viewfinder_qt.h"

This is not needed in main_window.h, you can move it to main_window.cpp.

>  
>  using namespace libcamera;
>  
> @@ -38,6 +40,7 @@ enum {
>  	OptCamera = 'c',
>  	OptHelp = 'h',
>  	OptStream = 's',
> +	OptRender = 'r',

Let's keep options sorted alphabetically here too.

>  };
>  
>  class CaptureRequest
> @@ -134,6 +137,9 @@ private:
>  	QElapsedTimer frameRateInterval_;
>  	uint32_t previousFrames_;
>  	uint32_t framesCaptured_;
> +
> +	/* Render Type Qt or GLES */
> +	std::string renderType_;

This is only used in MainWindow::MainWindow(), it can be a local
variable.

With these small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  };
>  
>  #endif /* __QCAM_MAIN_WINDOW__ */

-- 
Regards,

Laurent Pinchart