[libcamera-devel] [PATCH v2] cam: Add --monitor option

[Laurent Pinchart]

Hi Umang,

CC'ing Kieran,

On Sat, Aug 01, 2020 at 10:19:11AM +0000, Umang Jain wrote:
> On 8/1/20 2:08 AM, Laurent Pinchart wrote:
> > On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:
> >> Add --monitor to monitor new hotplug and unplug camera events from
> >> the CameraManager.
> >>
> >> Signed-off-by: Umang Jain <email at uajain.com>
> >> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>   src/cam/main.cpp | 26 ++++++++++++++++++++++++++
> >>   src/cam/main.h   |  1 +
> >>   2 files changed, 27 insertions(+)
> >>
> >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> >> index f5aba04..b03437e 100644
> >> --- a/src/cam/main.cpp
> >> +++ b/src/cam/main.cpp
> >> @@ -36,6 +36,8 @@ public:
> >>   	void quit();
> >>   
> >>   private:
> >> +	void cameraAdded(std::shared_ptr<Camera> cam);
> >> +	void cameraRemoved(std::shared_ptr<Camera> cam);
> >>   	int parseOptions(int argc, char *argv[]);
> >>   	int prepareConfig();
> >>   	int listControls();
> >> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)
> >>   		ret = prepareConfig();
> >>   		if (ret)
> >>   			return ret;
> >> +	} else if (options_.isSet(OptMonitor)) {
> >> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> >> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> >> +		std::cout << "Monitoring new hotplug or unplug camera events…" << std::endl;
> >>   	}
> >>   
> >>   	loop_ = new EventLoop(cm_->eventDispatcher());
> >> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])
> >>   	parser.addOption(OptStrictFormats, OptionNone,
> >>   			 "Do not allow requested stream format(s) to be adjusted",
> >>   			 "strict-formats");
> >> +	parser.addOption(OptMonitor, OptionNone, "Monitor for hotplug and unplug camera events",
> >
> > I'd move the message to the next line to avoid too long lines.
> >
> >> +			 "monitor");
> >>   
> >>   	options_ = parser.parse(argc, argv);
> >>   	if (!options_.valid())
> >> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()
> >>   	return 0;
> >>   }
> >>   
> >> +void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
> >> +{
> >> +	std::cout << "Camera Added: " << cam->name() << std::endl;
> >> +}
> >> +
> >> +void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
> >> +{
> >> +	std::cout << "Camera Removed: " << cam->name() << std::endl;
> >> +}
> >> +
> >>   int CamApp::run()
> >>   {
> >>   	int ret;
> >> @@ -342,10 +360,18 @@ int CamApp::run()
> >>   	}
> >>   
> >>   	if (options_.isSet(OptCapture)) {
> >> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> >> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> >
> > This is done in CamApp::init() already, do we need it here ? One can
> > specific both --monitor and --capture if they want to monitor for
> > hotplug during capture, I wouldn't make it automatic.
>
> hmm, one of reviews from Kieran in v1 was to make it automatic, if we 
> run any
> kind of loop (like Capture), hence the patch got driven in that direction.
> 
> I think specifying --monitor (shorthand -m) explicitly shouldn't be that 
> big a deal.

I'll let Kieran comment on this before merging the patch.

> >>   		Capture capture(camera_, config_.get(), loop_);
> >>   		return capture.run(options_);
> >>   	}
> >>   
> >> +	if (options_.isSet(OptMonitor)) {
> >> +		ret = loop_->exec();
> >> +		if (ret)
> >> +			std::cout << "Failed to run monitor loop" << std::endl;
> >> +	}
> >> +
> >>   	return 0;
> >>   }
> >>   
> >> diff --git a/src/cam/main.h b/src/cam/main.h
> >> index 6f95add..ea8dfd3 100644
> >> --- a/src/cam/main.h
> >> +++ b/src/cam/main.h
> >> @@ -15,6 +15,7 @@ enum {
> >>   	OptInfo = 'I',
> >>   	OptList = 'l',
> >>   	OptListProperties = 'p',
> >> +	OptMonitor = 'm',
> >>   	OptStream = 's',
> >>   	OptListControls = 256,
> >>   	OptStrictFormats = 257,

-- 
Regards,

Laurent Pinchart