[libcamera-devel,v10,1/5] libcamera: camera: Make stop() idempotent
diff mbox series

Message ID 20210629151218.248650-2-nfraprado@collabora.com
State Superseded
Headers show
Series
  • lc-compliance: Refactor using Googletest
Related show

Commit Message

Nícolas F. R. A. Prado June 29, 2021, 3:12 p.m. UTC 
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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
No changes in v10

No changes in v9

No changes in v8

No changes in v7

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(-)

Comments

Paul Elder June 30, 2021, 4:27 a.m. UTC | #1 
Hello Nicolas,

On Tue, Jun 29, 2021 at 12:12:14PM -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>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> No changes in v10
> 
> No changes in v9
> 
> No changes in v8
> 
> No changes in v7
> 
> 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 de0123aeafca..29f2d91d05d3 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -348,6 +348,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,
> @@ -389,6 +390,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
>  {
> @@ -1062,9 +1068,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.

The camera statemachine test is failing with this patch. I see that it
is intended behavior, in which case the test needs to be updated
accordingly.


Paul

>   *
>   * \return 0 on success or a negative error code otherwise
>   * \retval -ENODEV The camera has been disconnected from the system
> @@ -1074,6 +1081,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.32.0
>
Nícolas F. R. A. Prado June 30, 2021, 2:25 p.m. UTC | #2 
Hi Paul,

On Wed, Jun 30, 2021 at 01:27:20PM +0900, paul.elder@ideasonboard.com wrote:
> Hello Nicolas,
> 
> On Tue, Jun 29, 2021 at 12:12:14PM -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>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > No changes in v10
> > 
> > No changes in v9
> > 
> > No changes in v8
> > 
> > No changes in v7
> > 
> > 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 de0123aeafca..29f2d91d05d3 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -348,6 +348,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,
> > @@ -389,6 +390,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
> >  {
> > @@ -1062,9 +1068,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.
> 
> The camera statemachine test is failing with this patch. I see that it
> is intended behavior, in which case the test needs to be updated
> accordingly.

Thanks for the feedback. I'll have that fixed for v11.

Nícolas

> 
> 
> Paul
> 
> >   *
> >   * \return 0 on success or a negative error code otherwise
> >   * \retval -ENODEV The camera has been disconnected from the system
> > @@ -1074,6 +1081,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.32.0
> >

Patch
diff mbox series 

diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index de0123aeafca..29f2d91d05d3 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -348,6 +348,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,
@@ -389,6 +390,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
 {
@@ -1062,9 +1068,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
@@ -1074,6 +1081,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;