[libcamera-devel] [PATCH] libcamera: camera: Make stop() idempotent

[Kieran Bingham]

Hi Nicolas,

On 23/04/2021 15:24, 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>
> ---
> This will be used to simplify the cleanup path in lc-compliance without having
> error messages [1].
> 
> Also, I'm not sure if I should add the silent parameter to the other
> isAccessAllowed variant as well. It would make sense for consistency's sake, but
> it's not needed currently.
> 
> [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019703.html
> 
>  src/libcamera/camera.cpp | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 763f3b9926fd..baffdafc8146 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -350,7 +350,7 @@ public:
>  	int isAccessAllowed(State state, bool allowDisconnected = false,
>  			    const char *from = __builtin_FUNCTION()) const;
>  	int isAccessAllowed(State low, State high,
> -			    bool allowDisconnected = false,
> +			    bool allowDisconnected = false, bool silent = false,
>  			    const char *from = __builtin_FUNCTION()) const;
>  
>  	void disconnect();
> @@ -408,7 +408,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,
>  }
>  
>  int Camera::Private::isAccessAllowed(State low, State high,
> -				     bool allowDisconnected,
> +				     bool allowDisconnected, bool silent,
>  				     const char *from) const
>  {
>  	if (!allowDisconnected && disconnected_)
> @@ -421,11 +421,12 @@ int Camera::Private::isAccessAllowed(State low, State high,
>  	ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) &&
>  	       static_cast<unsigned int>(high) < std::size(camera_state_names));
>  
> -	LOG(Camera, Error) << "Camera in " << camera_state_names[currentState]
> -			   << " state trying " << from
> -			   << "() requiring state between "
> -			   << camera_state_names[low] << " and "
> -			   << camera_state_names[high];
> +	if (!silent)
> +		LOG(Camera, Error) << "Camera in " << camera_state_names[currentState]
> +				   << " state trying " << from
> +				   << "() requiring state between "
> +				   << camera_state_names[low] << " and "
> +				   << camera_state_names[high];
>  
>  	return -EACCES;
>  }
> @@ -1061,9 +1062,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 even if the camera isn't in the Running
> + * state as defined in \ref camera_operation, in which cases it is a no-op. It
> + * shall be synchronized by the caller with other functions that affect the
> + * camera state.
>   *
>   * \return 0 on success or a negative error code otherwise
>   * \retval -ENODEV The camera has been disconnected from the system
> @@ -1073,7 +1075,12 @@ int Camera::stop()
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> -	int ret = d->isAccessAllowed(Private::CameraRunning);
> +	int ret = d->isAccessAllowed(Private::CameraAvailable,
> +			Private::CameraStopping, false, true);
> +	if (!ret)
> +		return 0;

This seems like a lot of complexity to add to be able to return out of
this function silently if the state is not running.

Effectively we're saying that this function 'Is allowed' in any state,
but it simply returns early if state < CameraRunning. That makes me
think using a function called 'isAccessAllowed()' isn't quite right -
It's always allowed.

Would it be better to provide an accessor on the private state to better
represent that?

Like isRunning()?

Then this simply becomes

	if (!d->isRunning())
		return 0;




> +
> +	ret = d->isAccessAllowed(Private::CameraRunning);
>  	if (ret < 0)
>  		return ret;
>  
> 

-- 
Regards
--
Kieran