[libcamera-devel] checking camera state for shutdown

[Kieran Bingham]

Hi Christian,

Expanding with a bit more reference/context...

Quoting Jacopo Mondi via libcamera-devel (2022-09-23 08:30:41)
> Hi Christian
> 
> On Thu, Sep 22, 2022 at 09:47:23PM +0200, Christian Rauch via libcamera-devel wrote:
> > Hi Kieran,
> >
> > Am 22.09.22 um 10:35 schrieb Kieran Bingham:
> > > Quoting Christian Rauch via libcamera-devel (2022-09-21 23:01:47)
> > >> Hi,
> > >>
> > >> I am trying to shut down the camera and camera manager properly, but I
> > >> am getting messages such as "Camera in Stopping state trying
> > >> queueRequest() requiring state Running"

So indeed the message "Camera in Stopping state trying queueRequest()
requiring state Running" means you are trying to queue requests after
you have called Camera::stop()

> > >>  and "Removing media device /dev/media1 while still in use".

This one may be something else that still needs to be looked at.

> > >>
> > >> The general approach I am using is to
> > >> 1. disconnect callbacks:
> > >>      camera->requestCompleted.disconnect();
> > >> 2. stop camera:
> > >>     camera->stop();
> > >>     camera->release();
> > >> 3. stop camera manager:
> > >>     camera_manager.stop();
> > >>
> > >> But the messages above tell me that my callbacks, which re-queue the
> > >> requests, are not finished before I call stop, and the camera has not
> > >> been stopped when I stop the camera manager. I can work around this by a
> > >> "sleep(1)" after the disconnect, but this is not nice.
> > >
> > > Hrm... I think we normally disconnect after stopping the camera.
> > > Though that should still work I would expect, but ...
> >
> > I think the callback has to be disconnected before stopping the camera.
> > Otherwise, the callback might still process buffers etc. while the
> > camera is stopped.
> 
> Camera::stop() will forcefully complete all pending Request in error
> state, you're handler should check the Request completion state and
> not process buffers.

In simple-cam we see this at 
https://git.libcamera.org/libcamera/simple-cam.git/tree/simple-cam.cpp#n39

static void requestComplete(Request *request)
{
	if (request->status() == Request::RequestCancelled)
		return;

	loop.callLater(std::bind(&processRequest, request));
}

The request is not processed (or requeued) when the request->status() is
Cancelled.

I do think it might be useful to expand the states here so we can be
more explicit. For instance the buffer might be cancelled due to an
error but the application might still want to requeue...

So perhaps if we were to update this to a more explicit:

	if (request->status() == Request::EndOfStream)
		return;

it might be clearer to applications.


Even if an application did not check this - the camera would not allow a
request to be queued if stop() has been called due to the check at :
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1117

	int ret = d->isAccessAllowed(Private::CameraRunning);
	if (ret < 0)
		return ret;

Which would fail as the call to Camera::stop() puts the state into:

https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1225
	d->setState(Private::CameraStopping);



> > I assume this is happening here: The signal gets disconnected and the
> > camera is stopped immediately afterwards. But since the (now
> > disconnected) callback is still running from the previous "emit", it
> > will try to re-queue the request, which will fail since the camera has
> > already been stopped.
> >
> 
> Camera::stop() should be blocking the application thread until all
> pending requests are processed, and I presume the
> Camera::requestCompelte slot is run in the CameraManager thread, so
> that's a plausible explanation
> 
> > Since there is no way to tell that the camera has stopped, it is not
> > possible to prevent these invalid operations inside the callback.
> >
> > In any case, there should be a way to either:
> > A) wait for all callbacks to be finished (like a thread join), or

Camera::stop() already asserts this:

https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1230:
	ASSERT(!d->pipe_->hasPendingRequests(this));

So upon return of Camera::stop() all pending requests have been
returned, and no more are permitted to be queued.

--
Kieran


> > B) tell if the camera stopped (or generally the camera's state).
> >
> 
> Won't checking the request completion state help, and if you really
> want to maintain the state, you can do so with an atomic variable in
> your application ?
> 
> Exposing the camera state as it is now won't be enough as if
> Camera::stop() has been called but it has not yet been completed we
> should report something like "stopping in progress". Checking the
> Request completion state seems safer ?
> 
> 
> > >
> > >> Since the camera state is only accessible via internal/private API, how
> > >> is a user supposed to control the ordered shutdown of cameras etc.? Do
> > >> we need public API that lets a user check the camera state? Or should
> > >> the disconnect, stop and release calls make sure that the objects reach
> > >> these states before returning?
> > >
> > > Calling camera->stop() should indeed already block until the camera is
> > > stopped.
> > >
> > > At first I thought the lack of a requestCompleted handler might mean
> > > libcamera is still holding on to the requests and hasn't been able to
> > > get them back to the application - but I think they would just leak, and
> > > be lost. That could be the problem.
> > >
> > >
> > >> Best,
> > >> Christian
> > >>