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

1275 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 of the
organically grown global variables.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/cam/main.cpp | 184 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 125 insertions(+), 59 deletions(-)

Hi Niklas,

Thank you for the patch.

On Thu, May 23, 2019 at 05:13:06PM +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 of the
> organically grown global variables.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/cam/main.cpp | 184 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 125 insertions(+), 59 deletions(-)
> 
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index fe7d4f90dbf14ffd..f7fe83aafd81b633 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -18,17 +18,117 @@
>  
>  using namespace libcamera;
>  
> -OptionsParser::Options options;
> -std::shared_ptr<Camera> camera;
> -EventLoop *loop;
> +class CamApp
> +{
> +public:
> +	CamApp();
> +
> +	static CamApp *instance();
> +
> +	int init(int argc, char **argv);
> +	void cleanup();
> +
> +	int exec();
> +	void quit();
> +
> +private:
> +	int parseOptions(int argc, char *argv[]);
> +
> +	OptionsParser::Options options_;
> +	CameraManager *cm_;
> +	std::shared_ptr<Camera> camera_;
> +	EventLoop *loop_;
> +};
> +
> +CamApp::CamApp()
> +	: cm_(nullptr), camera_(nullptr), loop_(nullptr)
> +{
> +}
> +
> +CamApp *CamApp::instance()
> +{
> +	static CamApp app;
> +	return &app;

I think it's best to make someone responsible for constructing the
instance, as that's where init() will need to be called. For that
reason, I think you should add a static CamApp *app_ = nullptr member to
CamApp, set it to this in the constructor, and return it here. The
main() function would then create the CamApp instance (either on the
stack, or through new) and initialise it.

> +}
> +
> +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;
> +	}
> +
> +	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()
> +{
> +	delete loop_;
> +	loop_ = nullptr;
> +
> +	if (camera_) {
> +		camera_->release();
> +		camera_.reset();
> +	}
> +
> +	cm_->stop();
> +}
> +
> +int CamApp::exec()
> +{
> +	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;
> +}

How about renaming this to run(), and implementing exec() as

int CamApp::exec()
{
	int ret;

	ret = run();
	cleanup();

	return ret;
}

With these two comments addressed,

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

>  
> -void signalHandler(int signal)
> +void CamApp::quit()
>  {
> -	std::cout << "Exiting" << std::endl;
> -	loop->exit();
> +	if (loop_)
> +		loop_->exit();
>  }
>  
> -static int parseOptions(int argc, char *argv[])
> +int CamApp::parseOptions(int argc, char *argv[])
>  {
>  	KeyValueParser streamKeyValue;
>  	streamKeyValue.addOption("role", OptionString,
> @@ -58,77 +158,43 @@ 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;
>  }
>  
> +void signalHandler(int signal)
> +{
> +	std::cout << "Exiting" << std::endl;
> +	CamApp::instance()->quit();
> +}
> +
>  int main(int argc, char **argv)
>  {
> +	CamApp *app = CamApp::instance();
>  	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->exec();
>  
> -	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;
>  }