[libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make stop() idempotent

[Kieran Bingham]

Hi Nicolas, Jacopo,

On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:
> Hi Jacopo,
> 
> Em 2021-05-04 04:41, Jacopo Mondi escreveu:
> 
>> Hi Nicolas,
>>
>> On Mon, May 03, 2021 at 04:33:05PM -0300, Nícolas F. R. A. Prado wrote:
>>> Make Camera::stop() idempotent so that it can be called in any state and
>>> consecutive times. When called in any state other than CameraRunning, it
>>> is a no-op. This simplifies the cleanup path for applications.
>>>
>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
>>> ---
>>> Changes in v2:
>>> - Suggested by Kieran:
>>>   - Add new isRunning() function instead of relying on isAccessAllowed()
>>> - Suggested by Laurent:
>>>   - Make stop()'s description clearer
>>>
>>>  src/libcamera/camera.cpp | 20 +++++++++++++++++---
>>>  1 file changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>> index 1340c266cc5f..1c6c76c7c01e 100644
>>> --- a/src/libcamera/camera.cpp
>>> +++ b/src/libcamera/camera.cpp
>>> @@ -347,6 +347,7 @@ public:
>>>  		const std::set<Stream *> &streams);
>>>  	~Private();
>>>
>>> +	bool isRunning() const;
>>>  	int isAccessAllowed(State state, bool allowDisconnected = false,
>>>  			    const char *from = __builtin_FUNCTION()) const;
>>>  	int isAccessAllowed(State low, State high,
>>> @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = {
>>>  	"Running",
>>>  };
>>>
>>> +bool Camera::Private::isRunning() const
>>> +{
>>> +	State currentState = state_.load(std::memory_order_acquire);
>>> +	if (currentState == CameraRunning)
>>> +		return true;
>>> +
>>> +	return false;
>>> +}
>>> +
>>>  int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,
>>>  				     const char *from) const
>>>  {
>>> @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls)
>>>   * This method stops capturing and processing requests immediately. All pending
>>>   * requests are cancelled and complete synchronously in an error state.
>>>   *
>>> - * \context This function may only be called when the camera is in the Running
>>> - * state as defined in \ref camera_operation, and shall be synchronized by the
>>> - * caller with other functions that affect the camera state.
>>> + * \context This function may be called in any camera state as defined in \ref
>>> + * camera_operation, and shall be synchronized by the caller with other
>>> + * functions that affect the camera state. If called when the camera isn't
>>> + * running, it is a no-op.
>>>   *
>>>   * \return 0 on success or a negative error code otherwise
>>>   * \retval -ENODEV The camera has been disconnected from the system
>>> @@ -1073,6 +1084,9 @@ int Camera::stop()
>>>  {
>>>  	Private *const d = LIBCAMERA_D_PTR();
>>>
>>> +	if (!d->isRunning())
>>> +		return 0;
>>> +
>>
>> This kind of defeats the purpose of the isAccessAllowed() here below.
>>
>> Please note that isAccessAllowed(Private::CameraRunning) will return
>> true only if the camera is running, which is the same thing your new
>> function checks for. Let me guess, you wanted to get rid of the error
>> message ?
> 
> Indeed, I missed that, sorry. In v1 [1] I added other parameters to
> isAccessAllowed() just to be able to exit silently, but Kieran pointed out that
> it would be cleaner with an isRunning() function. So I did that, but forgot to
> remove the isAccessAllowed() below as well.

Yes, the extra checks should be removed.


> 
> [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019735.html
> 
>>
>> Personally, I would be in favour of calling stop() only when it's
>> appropriate instead of shortcutting the camera state machine. What is
>> the use case that makes it hard to call stop() only when actually
>> required ?
> 
> The use case is always calling stop() in the destructor of the capture in
> lc-compliance, which makes the cleanup path simpler as we can safely fail an
> assert and still have the camera stopped.
> 
> That said, it wouldn't be hard to call stop() only when required. We could track
> whether the camera was started internally in lc-compliance. But it seemed like
> this would be a common pattern and handling it by making Camera::stop() itself
> be idempotent would be beneficial.
> 
> I'll wait for others to comment on this as well.

I think being able to clean up easily on shutdowns is beneficial.

I would consider this use case to be that of being able to call

	p = NULL;
	...
	kfree(p);

rather than
	if (p)
		kfree(p);

It's a convenience feature, so that extra state doesn't have to be
repeatedly maintained elsewhere.

--
Kieran


> 
> Thanks,
> Nícolas
> 
>>
>> Thanks
>> j
>>>  	int ret = d->isAccessAllowed(Private::CameraRunning);
>>>  	if (ret < 0)
>>>  		return ret;
>>> --
>>> 2.31.1
>>>
>>> _______________________________________________
>>> libcamera-devel mailing list
>>> libcamera-devel at lists.libcamera.org
>>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran