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

[Kieran Bingham]

Hi Umang, Laurent,

On 01/08/2020 13:53, Laurent Pinchart wrote:
> 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;
>>>>   	}

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.

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.


--
Kieran



>>>> +
>>>>   	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
--
Kieran