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

[Laurent Pinchart]

Hello,

On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote:
> Em 2021-05-04 12:29, Jacopo Mondi escreveu:
> > On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:
> > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:
> > > > Hi Jacopo,
> > > >
> > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu:
> > > >
> >
> > [snip]
> >
> > > >> 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.
> > >
> > > I think being able to clean up easily on shutdowns is beneficial.
> > >
> > > I would consider this use case to be that of being able to call
> > >
> > > 	p = NULL;
> > > 	...
> > > 	kfree(p);
> > >
> > > rather than
> > > 	if (p)
> > > 		kfree(p);
> > >
> > > It's a convenience feature, so that extra state doesn't have to be
> > > repeatedly maintained elsewhere.
> >
> > Might be a personal opinion, but I would prefer, as a general idea,
> > a stricter API. I concur there's not much value in the error message
> > when stop() is called from the wrong state, but sometime it helped me
> > finding a wrong patter when working on the camera HAL. I guess it
> > might be beneficial for other use cases too...
> 
> I think adding a log at level DEBUG or INFO stating that "stop() called for
> non-running camera" would help debug those cases while still allowing the
> convenience of calling it in any state.

Sorry about the conflicting directions :-S

There are two options. Calling Camera::stop() when stopped is either
valid, in which case the function shouldn't log any message with a level
higher than debug, or it's invalid, in which case we should log an
error. In the latter case, the caller needs to keep track of the camera
state, which is a bit of a hassle. Taking Niklas' example, it would be
similar to requiring the caller to have a bool is_p_null variable, as
the Camera class doesn't expose its state, so a caller can do

	if (cam->state() == CameraRunning)
		cam->stop();

We could expose the camera state, but that increases the surface of the
public API, for little gain I believe.

Jacopo, what do you think ?

> > Anyway, I won't push, whatever's best for the majority is fine with
> > me.
> >
> > > >>>  	int ret = d->isAccessAllowed(Private::CameraRunning);
> > > >>>  	if (ret < 0)
> > > >>>  		return ret;

-- 
Regards,

Laurent Pinchart