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

[Nícolas F. R. A. Prado]

Hi Jacopo,

Em 2021-05-04 04:41, Jacopo Mondi escreveu:

> 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 ?

Indeed, I missed that, sorry. In v1 [1] I added other parameters to
isAccessAllowed() just to be able to exit silently, but Kieran pointed out that
it would be cleaner with an isRunning() function. So I did that, but forgot to
remove the isAccessAllowed() below as well.

[1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019735.html

>
> 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 ?

The use case is always calling stop() in the destructor of the capture in
lc-compliance, which makes the cleanup path simpler as we can safely fail an
assert and still have the camera stopped.

That said, it wouldn't be hard to call stop() only when required. We could track
whether the camera was started internally in lc-compliance. But it seemed like
this would be a common pattern and handling it by making Camera::stop() itself
be idempotent would be beneficial.

I'll wait for others to comment on this as well.

Thanks,
Nícolas

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