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

[Kieran Bingham]

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

-- 
Regards
--
Kieran