[libcamera-devel] [PATCH v4 1/5] libcamera: camera: Make stop() idempotent

[Laurent Pinchart]

Hi Nícolas,

Thank you for the patch.

On Fri, May 21, 2021 at 10:30:50AM -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 v4:
> - Thanks to Jacopo:
>   - Added \todo in Camera::stop()
> 
> No changes in v3
> 
> 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 | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 1340c266cc5f..be0428ecbd46 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;

You could write this

	return state_.load(std::memory_order_acquire) == CameraRunning;

If that's the only change needed (and assuming you agree) I can handle
this when applying the series.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +}
> +
>  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,13 @@ int Camera::stop()
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>  
> +	/*
> +	 * \todo Make calling stop() when not in 'Running' part of the state
> +	 * machine rather than take this shortcut
> +	 */
> +	if (!d->isRunning())
> +		return 0;
> +
>  	int ret = d->isAccessAllowed(Private::CameraRunning);
>  	if (ret < 0)
>  		return ret;

-- 
Regards,

Laurent Pinchart