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

[Kieran Bingham]

Hi Jacopo,

On 08/05/2021 08:24, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Sat, May 08, 2021 at 03:03:54AM +0300, Laurent Pinchart wrote:
>> Hello,
>>
>> On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote:
>>> Em 2021-05-04 12:29, Jacopo Mondi escreveu:
>>>> On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:
>>>>> On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:
>>>>>> Hi Jacopo,
>>>>>>
>>>>>> Em 2021-05-04 04:41, Jacopo Mondi escreveu:
>>>>>>
>>>>
>>>> [snip]
>>>>
>>>>>>> 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.
>>>>
>>>> Might be a personal opinion, but I would prefer, as a general idea,
>>>> a stricter API. I concur there's not much value in the error message
>>>> when stop() is called from the wrong state, but sometime it helped me
>>>> finding a wrong patter when working on the camera HAL. I guess it
>>>> might be beneficial for other use cases too...
>>>
>>> I think adding a log at level DEBUG or INFO stating that "stop() called for
>>> non-running camera" would help debug those cases while still allowing the
>>> convenience of calling it in any state.
>>
>> Sorry about the conflicting directions :-S
>>
>> There are two options. Calling Camera::stop() when stopped is either
>> valid, in which case the function shouldn't log any message with a level
>> higher than debug, or it's invalid, in which case we should log an
>> error. In the latter case, the caller needs to keep track of the camera
>> state, which is a bit of a hassle. Taking Niklas' example, it would be
>> similar to requiring the caller to have a bool is_p_null variable, as
>> the Camera class doesn't expose its state, so a caller can do
>>
>> 	if (cam->state() == CameraRunning)
>> 		cam->stop();
>>
>> We could expose the camera state, but that increases the surface of the
>> public API, for little gain I believe.
>>
>> Jacopo, what do you think ?
>>
> 
> Either we change the camera state machine and allow Stop->Stop by
> adding a state
>         Stopping -> Stopping [label = "stop()"];

Hrm - Are we missing some updates to our state diagram already?
Shouldn't this be 'Stopped'?

(I bet that's my fault)


> and change
>         isAccessAllowed(Private::CameraStopped,
>                         Private::CameraRunning);
> 
> not to throw any error
> 
> Or we mandate applications to track the state themeselves.
> 
> Exposing the state has a little gain, as application will
> unconditionally repeat the pattern
> 
>  	if (cam->state() == CameraRunning)
>  		cam->stop();
> 
> hence they could similarly be allowed to just call stop() without
> bothering about the state check.
> 
> I would like application to be in charge of tracking the state and
> know what they're doing precisely, but I'm afraid they will just
> call stop() and ignore the error, and I'm not 100% sure it is fair to
> mandate the state tracking burden to apps.
> 
> So yes, allowing stop->stop seems fair, but it should be made part of
> the Camera state machine and not worked around by not checking if the
> access is actually allowed or not imho.

I think making it clear in the state machine diagrams is a very good point.

But doesn't it then mean allowing stop() from any of the other states?

What happens if we call stop in {Available, Acquired, Configured} for
example.

In fact, indeed we shouldn't allow calling stop() on a camera which has
not been acquired, as that would imply that the caller could stop() a
camera which was in a running state, belonging to someone else.

We may still need to have the instrumentation in the stop function for
isRunning() so that it doesn't try to stop, when already stopped though,
but I agree the isAccessAllowed() could in fact be a bit more restrictive.


> 
> Thanks
>    j
> 
>>>> Anyway, I won't push, whatever's best for the majority is fine with
>>>> me.
>>>>
>>>>>>>>  	int ret = d->isAccessAllowed(Private::CameraRunning);
>>>>>>>>  	if (ret < 0)
>>>>>>>>  		return ret;
>>
>> --
>> Regards,
>>
>> Laurent Pinchart

-- 
Regards
--
Kieran