[libcamera-devel] [PATCH] libcamera: camera: Make stop() idempotent

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

Em 2021-04-23 12:10, Kieran Bingham escreveu:

> Hi Nicolas,
>
> On 23/04/2021 15:24, 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>
> > ---
> > This will be used to simplify the cleanup path in lc-compliance without having
> > error messages [1].
> > 
> > Also, I'm not sure if I should add the silent parameter to the other
> > isAccessAllowed variant as well. It would make sense for consistency's sake, but
> > it's not needed currently.
> > 
> > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019703.html
> > 
> >  src/libcamera/camera.cpp | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 763f3b9926fd..baffdafc8146 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -350,7 +350,7 @@ public:
> >  	int isAccessAllowed(State state, bool allowDisconnected = false,
> >  			    const char *from = __builtin_FUNCTION()) const;
> >  	int isAccessAllowed(State low, State high,
> > -			    bool allowDisconnected = false,
> > +			    bool allowDisconnected = false, bool silent = false,
> >  			    const char *from = __builtin_FUNCTION()) const;
> >  
> >  	void disconnect();
> > @@ -408,7 +408,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,
> >  }
> >  
> >  int Camera::Private::isAccessAllowed(State low, State high,
> > -				     bool allowDisconnected,
> > +				     bool allowDisconnected, bool silent,
> >  				     const char *from) const
> >  {
> >  	if (!allowDisconnected && disconnected_)
> > @@ -421,11 +421,12 @@ int Camera::Private::isAccessAllowed(State low, State high,
> >  	ASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) &&
> >  	       static_cast<unsigned int>(high) < std::size(camera_state_names));
> >  
> > -	LOG(Camera, Error) << "Camera in " << camera_state_names[currentState]
> > -			   << " state trying " << from
> > -			   << "() requiring state between "
> > -			   << camera_state_names[low] << " and "
> > -			   << camera_state_names[high];
> > +	if (!silent)
> > +		LOG(Camera, Error) << "Camera in " << camera_state_names[currentState]
> > +				   << " state trying " << from
> > +				   << "() requiring state between "
> > +				   << camera_state_names[low] << " and "
> > +				   << camera_state_names[high];
> >  
> >  	return -EACCES;
> >  }
> > @@ -1061,9 +1062,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 even if the camera isn't in the Running
> > + * state as defined in \ref camera_operation, in which cases it is a no-op. It
> > + * shall be synchronized by the caller with other functions that affect the
> > + * camera state.
> >   *
> >   * \return 0 on success or a negative error code otherwise
> >   * \retval -ENODEV The camera has been disconnected from the system
> > @@ -1073,7 +1075,12 @@ int Camera::stop()
> >  {
> >  	Private *const d = LIBCAMERA_D_PTR();
> >  
> > -	int ret = d->isAccessAllowed(Private::CameraRunning);
> > +	int ret = d->isAccessAllowed(Private::CameraAvailable,
> > +			Private::CameraStopping, false, true);
> > +	if (!ret)
> > +		return 0;
>
> This seems like a lot of complexity to add to be able to return out of
> this function silently if the state is not running.
>
> Effectively we're saying that this function 'Is allowed' in any state,
> but it simply returns early if state < CameraRunning. That makes me
> think using a function called 'isAccessAllowed()' isn't quite right -
> It's always allowed.
>
> Would it be better to provide an accessor on the private state to better
> represent that?
>
> Like isRunning()?
>
> Then this simply becomes
>
> if (!d->isRunning())
> return 0;

Yep, that sounds a lot better. I'll do that instead for v2.

Thanks,
Nícolas