[libcamera-devel] [PATCH 2/2] cam: Add CamApp class

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, May 23, 2019 at 02:55:34AM +0200, Niklas Söderlund wrote:
> Add more structure to main.cpp by breaking up the logic into a CamApp
> class. This makes the code easier to read and removes all but one of the
> organically grown global variables.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/main.cpp | 171 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 112 insertions(+), 59 deletions(-)
> 
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index fe7d4f90dbf14ffd..5ca8356025a0c9f9 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -18,17 +18,101 @@
>  
>  using namespace libcamera;
>  
> -OptionsParser::Options options;
> -std::shared_ptr<Camera> camera;
> -EventLoop *loop;
> +class CamApp
> +{
> +public:
> +	CamApp();
> +
> +	int init(int argc, char **argv);

I would pass the argc and argv to the constructor, following the
https://doc.qt.io/qt-5/qapplication.html API. You can either store them
internally and parse the options in init(), or do part of the
initialisation in the constructor and stored a valid state that would
make init() return an error immediately. The global app variable would
then become a pointer, or possibly even better, you could add a static
method to CamApp() named instance() that would return the instance
(stored in a static member variable in the constructor), and remove the
last global variable :-) The signal handler would call
CamApp::instance()->quit().

> +	void cleanup();
> +
> +	int run();

I would call this exec() to mimic Qt to.

> +
> +	EventLoop *loop;

Missing blank line ?

> +private:
> +	int parseOptions(int argc, char *argv[]);
> +
> +	OptionsParser::Options options_;
> +	CameraManager *cm_;
> +	std::shared_ptr<Camera> camera_;
> +};
> +
> +CamApp::CamApp()
> +	: cm_(nullptr), camera_(nullptr)
> +{
> +}
> +
> +int CamApp::init(int argc, char **argv)
> +{
> +	int ret;
> +
> +	ret = parseOptions(argc, argv);
> +	if (ret < 0)
> +		return ret == -EINTR ? 0 : ret;
> +
> +	cm_ = CameraManager::instance();
> +
> +	ret = cm_->start();
> +	if (ret) {
> +		std::cout << "Failed to start camera manager: "
> +			  << strerror(-ret) << std::endl;
> +		return ret;
> +	}
>  
> -void signalHandler(int signal)
> +	if (options_.isSet(OptCamera)) {
> +		camera_ = cm_->get(options_[OptCamera]);
> +		if (!camera_) {
> +			std::cout << "Camera "
> +				  << std::string(options_[OptCamera])
> +				  << " not found" << std::endl;
> +			cm_->stop();
> +			return -ENODEV;
> +		}
> +
> +		if (camera_->acquire()) {
> +			std::cout << "Failed to acquire camera" << std::endl;
> +			camera_.reset();
> +			cm_->stop();
> +			return -EINVAL;
> +		}
> +
> +		std::cout << "Using camera " << camera_->name() << std::endl;
> +	}
> +
> +	loop = new EventLoop(cm_->eventDispatcher());
> +
> +	return 0;
> +}
> +
> +void CamApp::cleanup()
>  {
> -	std::cout << "Exiting" << std::endl;
> -	loop->exit();
> +	delete loop;
> +
> +	if (camera_) {
> +		camera_->release();
> +		camera_.reset();
> +	}
> +
> +	cm_->stop();
> +}
> +
> +int CamApp::run()
> +{
> +	if (options_.isSet(OptList)) {
> +		std::cout << "Available cameras:" << std::endl;
> +		for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> +			std::cout << "- " << cam->name() << std::endl;
> +	}
> +
> +	if (options_.isSet(OptCapture)) {
> +		Capture capture;
> +		return capture.run(camera_.get(), loop, options_);
> +	}
> +
> +	return 0;
>  }
>  
> -static int parseOptions(int argc, char *argv[])
> +int CamApp::parseOptions(int argc, char *argv[])
>  {
>  	KeyValueParser streamKeyValue;
>  	streamKeyValue.addOption("role", OptionString,
> @@ -58,77 +142,46 @@ static int parseOptions(int argc, char *argv[])
>  			 "help");
>  	parser.addOption(OptList, OptionNone, "List all cameras", "list");
>  
> -	options = parser.parse(argc, argv);
> -	if (!options.valid())
> +	options_ = parser.parse(argc, argv);
> +	if (!options_.valid())
>  		return -EINVAL;
>  
> -	if (options.empty() || options.isSet(OptHelp)) {
> +	if (options_.empty() || options_.isSet(OptHelp)) {
>  		parser.usage();
> -		return options.empty() ? -EINVAL : -EINTR;
> +		return options_.empty() ? -EINVAL : -EINTR;
>  	}
>  
>  	return 0;
>  }
>  
> +CamApp app;
> +
> +void signalHandler(int signal)
> +{
> +	std::cout << "Exiting" << std::endl;
> +
> +	if (app.loop)
> +		app.loop->exit();

Instead of exposing the loop member, how about adding a public quit()
method that would call loop->exit() ? loop should then become loop_.

> +}
> +
>  int main(int argc, char **argv)
>  {
>  	int ret;
>  
> -	ret = parseOptions(argc, argv);
> -	if (ret < 0)
> -		return ret == -EINTR ? 0 : EXIT_FAILURE;
> -
> -	CameraManager *cm = CameraManager::instance();
> -
> -	ret = cm->start();
> -	if (ret) {
> -		std::cout << "Failed to start camera manager: "
> -			  << strerror(-ret) << std::endl;
> +	ret = app.init(argc, argv);
> +	if (ret)
>  		return EXIT_FAILURE;
> -	}
> -
> -	loop = new EventLoop(cm->eventDispatcher());
>  
>  	struct sigaction sa = {};
>  	sa.sa_handler = &signalHandler;
>  	sigaction(SIGINT, &sa, nullptr);
>  
> -	if (options.isSet(OptList)) {
> -		std::cout << "Available cameras:" << std::endl;
> -		for (const std::shared_ptr<Camera> &cam : cm->cameras())
> -			std::cout << "- " << cam->name() << std::endl;
> -	}
> +	ret = app.run();
>  
> -	if (options.isSet(OptCamera)) {
> -		camera = cm->get(options[OptCamera]);
> -		if (!camera) {
> -			std::cout << "Camera "
> -				  << std::string(options[OptCamera])
> -				  << " not found" << std::endl;
> -			goto out;
> -		}
> +	app.cleanup();

Should cleanup() be called internally at the end of run() ?

>  
> -		if (camera->acquire()) {
> -			std::cout << "Failed to acquire camera" << std::endl;
> -			goto out;
> -		}
> +	if (ret)
> +		return EXIT_FAILURE;
>  
> -		std::cout << "Using camera " << camera->name() << std::endl;
> -	}
> -
> -	if (options.isSet(OptCapture)) {
> -		Capture capture;
> -		ret = capture.run(camera.get(), loop, options);
> -	}
> -
> -	if (camera) {
> -		camera->release();
> -		camera.reset();
> -	}
> -out:
> -	delete loop;
> -
> -	cm->stop();
> -
> -	return ret;
> +	return 0;
>  }

-- 
Regards,

Laurent Pinchart