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

1268 diff mbox series

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@ragnatech.se>
---
 src/cam/main.cpp | 171 +++++++++++++++++++++++++++++++----------------
 1 file changed, 112 insertions(+), 59 deletions(-)

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@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;
>  }

Hi Niklas,

On 23/05/2019 01:55, 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.

Which one is left :-) (Perhaps I'll find it in the code below, but it
might be nice to reference it here separately for clarity)

In the diff I see -options, -camera, -loop, and +app.
Is it the +app you are referring to?

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

I'm struggling to find comments for this patch... (so I'm sure that's a
good thing) I could bikeshed the CamApp class name, but I won't :-D

The rest is mostly code move which should stay as minimal change as
possible IMO for now so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> ---
>  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);
> +	void cleanup();
> +
> +	int run();
> +
> +	EventLoop *loop;

I see we're accessing the loop directly from the signal handler.

We could add an inline exitLoop() to make this private as the other
variables:

	void exitLoop() { if (loop_) loop_->exit(); }

Or it could be CamApp::exit() too ...
But I won't object loudly if you just want to keep it public for ease?


Otherwise could do with an empty line here to separate public/private
sections?

> +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;

Not for this patch, but it would be really nice to have some logging
helpers, that can easily be shared across our apps and tests...

do we have a task created for that anywhere I wonder ?

> +		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();
> +}
> +
>  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();
>  
> -		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;
>  }
>

Hi Laurent,

Thanks for your feedback.

On 2019-05-23 12:48:09 +0300, Laurent Pinchart wrote:
> 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@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().

I like quit() and instance() and will do so in v2.

I do not particularly like combining instance() with moving arg{v,c} to 
to the constructor and caching them, so I will try without this in v2 
;-)

> 
> > +	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() ?

No, the reason for having it separate is to make error handling in run() 
simpler

> 
> >  
> > -		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

Hi Niklas,

On Thu, May 23, 2019 at 05:07:14PM +0200, Niklas Söderlund wrote:
> On 2019-05-23 12:48:09 +0300, Laurent Pinchart wrote:
> > 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@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().
> 
> I like quit() and instance() and will do so in v2.
> 
> I do not particularly like combining instance() with moving arg{v,c} to 
> to the constructor and caching them, so I will try without this in v2 
> ;-)
> 
> > > +	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() ?
> 
> No, the reason for having it separate is to make error handling in run() 
> simpler

Let's make the caller simple too, and add an exec() that calls run() and
cleanup() :-)

> > >  
> > > -		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;
> > >  }