[libcamera-devel] [PATCH 2/2] cam: Add CamApp class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu May 23 17:13:46 CEST 2019
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 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
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;
> > > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list