[libcamera-devel,v6,1/5] libcamera: camera: Make stop() idempotent

12504 diff mbox series

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@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
No changes in v6

Changes in v5:
- Thanks to Laurent:
  - Simplified isRunning()

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 | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Hi Nicolas,

On Mon, Jun 07, 2021 at 03:15:02PM -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@collabora.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Still not in love with this one :)
I guess now that you're moving to use gtest it's even harder to stop
the camera at the right time, so

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j
> ---
> No changes in v6
>
> Changes in v5:
> - Thanks to Laurent:
>   - Simplified isRunning()
>
> 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 | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 1340c266cc5f..a13a15dd1fc9 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,11 @@ static const char *const camera_state_names[] = {
>  	"Running",
>  };
>
> +bool Camera::Private::isRunning() const
> +{
> +	return state_.load(std::memory_order_acquire) == CameraRunning;
> +}
> +
>  int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,
>  				     const char *from) const
>  {
> @@ -1061,9 +1067,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 +1080,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;
> --
> 2.31.1
>