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

[Laurent Pinchart]

Hi Kieran,

On Mon, Aug 03, 2020 at 10:01:09AM +0100, Kieran Bingham wrote:
> On 01/08/2020 13:53, Laurent Pinchart wrote:
> > 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;
> >>>>   	}
> 
> My comment previously was that I think we should always connect these
> signals in this function (without an 'if isSet(OptMonitor)') so that any
> time cam is running, if there is an event which makes it to the
> application the user is notified.
> 
> >>>>   
> >>>>   	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.
> 
> Thus, if the connections are done in CamApp::init() (always, regardless
> of the -m) then we don't need to reconnect here ...
> 
> >>>>   		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;
> >>>> +	}
> 
> But we do still need to provide a loop.
> 
> I'm not sure I'm keen on the idea of mixing --monitor and --capture as a
> requirement to obtain notifications during a capture. I think it should
> always notify. The '--monitor' is just a convenience to provide an event
> loop which will do nothing else except wait for events...
> 
> Any thoughts anyone?
> 
> I don't mind if the consensus is that to see notifications while doing a
> capture you should still specify the '-m', because the connection will
> already be done (conditionally in init), and we will be running in an
> event loop in 'return capture.run(options_);' which will return when it
> completes.
> 
> 
> So if consensus is "to get events, then the -m option should be
> provided", then v1 of this patch already does that.

The reason why I think -m should be mandatory to listen to hotplug
events is that the cam application is meant to be a low-level test
application that can exercise the whole libcamera API. Having the
ability to capture without listening to hotplug events could be useful.
It would be a different story in a more specialized application, but for
cam I think it's best to let the user decide what they need.

> Otherwise, if we should always report hotplug events, then v1 just needs
> to be adjusted to unconditionally connect the signal/slots during
> CamApp::init.
> 
> 
> Personally, I would choose to always display the events. They are cheap,
> and the monitor loop becomes just a convenience.
> 
> >>>> +
> >>>>   	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