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

[Jacopo Mondi]

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 ?

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 ?

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