[libcamera-devel] checking camera state for shutdown

[Christian Rauch]

Hi Kieran,

Am 23.09.22 um 14:15 schrieb 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...

This is exactly what is going on here. In my case, I encountered
conflicting auto- and manual-exposure configurations that caused
requests to be cancelled, but they would return to normal when I undo
these conflicting settings. Returning from the callback when a request
is cancelled this way would indefinitely end the request processing,
without the option to reuse cancelled requests. Therefore, I only print
a warning in case of "RequestCancelled" and reuse the request:

  if (request->status() == libcamera::Request::RequestComplete)
    // process request
  else if (request->status() == libcamera::Request::RequestCancelled)
    // show warning

  // queue the request again for the next frame
  request->reuse(libcamera::Request::ReuseBuffers);
  camera->queueRequest(request);

This allows me to recover from a temporary conflicting or malformed
configuration, but now prevents me to detect when a request was
cancelled due to the stopping of the camera. An additional request state
(e.g. RequestEnd) would be required to distinguish these two cases.

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