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

[Niklas Söderlund]

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 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().

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

-- 
Regards,
Niklas Söderlund