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

[Laurent Pinchart]

Hi Nícolas,

On Fri, Apr 23, 2021 at 01:29:47PM -0300, Nícolas F. R. A. Prado wrote:
> Em 2021-04-23 12:10, Kieran Bingham escreveu:
> > 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,

I know this will change in v2, but on a general note, bool parameters
are not great (this applies to the existing allowDisconnected parameter
too). When reading the caller

	int ret = d->isAccessAllowed(Private::CameraAvailable,
			Private::CameraStopping, false, true);

it's hard to tell what false and true refer to. Instead, the following
would be better:

	int ret = d->isAccessAllowed(Private::CameraAvailable,
				     Private::CameraStopping,
				     Private::AccessCheck::NoLogging);

with a new

	enum AccessCbeck {
		AllowDisconnected = 0x1,
		NoLogging = 0x2,
	};

in the Private class. An enum class would be even better, but that
wouldn't allow us to | multiple flags.
https://patchwork.libcamera.org/cover/8991/ could be a solution.

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

How about stating that the function may be called in any 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.

-- 
Regards,

Laurent Pinchart