[EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture

[Jacopo Mondi]

Hello

On Tue, Mar 05, 2024 at 09:28:00AM +0000, Anle Pan wrote:
> Hi Umang Jain,
>
> this issue was random(around 1/10), and can be reproduced by single run below cts test:
> 	./cts-tradefed run commandAndExit cts --abi arm64-v8a -m CtsMediaPlayerTestCases -t android.media.player.cts.MediaPlayerTest#testRecordedVideoPlayback90
>
> •	3 times configuring streams in this Test:
> 1)  2 streams:   (0) 320x240-NV12 (1) 1280x720-YUYV:   capture several frames on stream1
> 2)  3 streams:   (0) 320x240-NV12 (1) 320x240-NV12 (2) 1280x720-YUYV: will capture on stream0 and stream2 at the same time
> 3)  2 streams:   (0) 320x240-NV12 (1) 1280x720-YUYV:   capture several frames on stream1
>
> •	when single run the test several times and met fail. When digging the log, can find that the current failed is caused by the abnormal complete of the previous PASS test in step3, which is the situation described in my patch.
>

The only code path I see that could lead to descriptor_ not being
cleared is flush() being called and then stop() called just after
possibily as the result of a new configuration

void CameraDevice::flush()
{
	{
		MutexLocker stateLock(stateMutex_);
		if (state_ != State::Running)
			return;

		state_ = State::Flushing;
	}

	camera_->stop();

	MutexLocker stateLock(stateMutex_);
	state_ = State::Stopped;
}

void CameraDevice::stop()
{
	MutexLocker stateLock(stateMutex_);
	if (state_ == State::Stopped)
		return;

	camera_->stop();

	{
		MutexLocker descriptorsLock(descriptorsMutex_);
		descriptors_ = {};
	}

	streams_.clear();

	state_ = State::Stopped;
}

int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
{
	/* Before any configuration attempt, stop the camera. */
	stop();

        ...

}

Looking at the flush() implementation right now I wonder why we stop
the camera without the stateMutex_ held (the only reason I see is
because it can be concurrent with processCaptureRequest() which keeps
the mutex held), but I certainly don't want to touch that code right
now without a good reason.

All in all, I think there's indeed a path that could lead to the
descriptor not being cleaned up, and we can't do it in flush() because
a concurrent call to processCaptureRequest() might be using the
descriptors to complete the in-flight requests with error state.

Anle, does this match your understanding ? If so, this is what should
be recorded in the commit message (the cause of an issue, not the
symptoms)


> Best Regards
> Anle Pan
>
> -----Original Message-----
> From: Umang Jain <umang.jain at ideasonboard.com>
> Sent: 2024年3月5日 16:27
> To: Anle Pan <anle.pan at nxp.com>; libcamera-devel at lists.libcamera.org
> Cc: Hui Fang <hui.fang at nxp.com>
> Subject: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture
>
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
>
>
> Hi Anle Pan, Hui Fang
>
> On 04/03/24 3:05 pm, Anle Pan wrote:
> > The issue occurs when stopping capture soon after starting capture.
>
> Can you describe the issue/use case in more detail? It's really hard to infer from one line.
> >
> > In this case, no frame get from the device, the related capture
> > request has been pushed to the queue descriptors_, but the
> > queuedRequests_ was still empty due to no requests will be queue to
> > the device since the stream will be stopped soon,
>
> I don't get it how is this possible? Since processCaptureRequest() will push the request in the descriptors_ queue along with queuing the request to the camera.
>
> How will the call path be interleaved /before/ processCaptureRequest() returns? Possibly we need more details on what you are trying to do, before reviewing the code here. Is this related to CTS ?
> > so there will be no camera->requestComplete called later, then the
> > descriptors_ can not pop normally, this will cause the pending if we
> > want to start capture next time.
> >
> > To fix the issue, ensure the descriptors_ is empty after the camera
> > device is stopped.

Do you know precisely why a non-empty descriptors_ stalls the camera ?
Does it get stuck on

void CameraDevice::sendCaptureResults()
{
	while (!descriptors_.empty() && !descriptors_.front()->isPending()) {

?

> >
> > Signed-off-by: Anle Pan <anle.pan at nxp.com>

Tested with CTS, no regressions detected

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

> > ---
> >   src/android/camera_device.cpp | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/android/camera_device.cpp
> > b/src/android/camera_device.cpp index 25cedd44..d452992d 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -433,8 +433,11 @@ void CameraDevice::flush()
> >   void CameraDevice::stop()
> >   {
> >       MutexLocker stateLock(stateMutex_);
> > -     if (state_ == State::Stopped)
> > +     if (state_ == State::Stopped) {
> > +             MutexLocker descriptorsLock(descriptorsMutex_);
> > +             descriptors_ = {};
> >               return;
> > +     }
> >
> >       camera_->stop();
> >
>