[EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture
Hui Fang
hui.fang at nxp.com
Tue Mar 26 09:51:28 CET 2024
Hi, Jacopo
We (Anle and me) are ok with your proposition, please help refine and apply, thanks!
BRs,
Fang Hui
________________________________
From: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Sent: Tuesday, March 26, 2024 4:22 PM
To: Hui Fang <hui.fang at nxp.com>
Cc: Jacopo Mondi <jacopo.mondi at ideasonboard.com>; Anle Pan <anle.pan at nxp.com>; Umang Jain <umang.jain at ideasonboard.com>; libcamera-devel at lists.libcamera.org <libcamera-devel at lists.libcamera.org>
Subject: Re: [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
No worries, no need to re-send. Small things can be fixed (if you're
fine with what I proposed) when I'll apply the patch.
On Tue, Mar 26, 2024 at 08:20:31AM +0000, Hui Fang wrote:
> Hi, Jacopo
> For refined patch, always use "git send-email", Even the subject changed?
>
> BRs,
> Fang Hui
> ________________________________
> From: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Sent: Tuesday, March 26, 2024 4:08 PM
> To: Hui Fang <hui.fang at nxp.com>
> Cc: Anle Pan <anle.pan at nxp.com>; Umang Jain <umang.jain at ideasonboard.com>; Jacopo Mondi <jacopo.mondi at ideasonboard.com>; libcamera-devel at lists.libcamera.org <libcamera-devel at lists.libcamera.org>
> Subject: Re: [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,
>
> On Tue, Mar 26, 2024 at 03:13:05AM +0000, Hui Fang wrote:
> > @Anle Pan<mailto:anle.pan at nxp.com> No need "git send-email" again, will new another topic.
> >
> > Just reply the refined patch.
>
> not a problem for this patch, but as a general rule, do not reply to a
> patch with a new version of the same patch, just send a new one. It's
> easier to track and I think it is also required for patchwork to
> identify it correctly.
>
> >
> > Author: Anle Pan <anle.pan at nxp.com>
> > Date: Fri Mar 1 17:32:54 2024 +0000
> >
> > android: camera_device: Fix the stuck issue in sendCaptureResults.
>
> No '.' at the end of the commit message.
>
> The actual subject should be something like
>
> android: camera_device: Always clear descriptors_ in stop()
>
> >
> > When flush() is being called and then a new stream configuration is set,
> > descriptor_ may has a chance to be not cleared, which was skipped due to
>
> descriptors_
>
> > the Stopped State. This will cause a stuck in sendCaptureResults.
>
> due to the Camera being in Stopped state.
> >
> > To fix the issue, ensure the descriptors_ is always empty when the camera
> > State is Stopped.
> >
> > Signed-off-by: Anle Pan <anle.pan at nxp.com>
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> With the few fixes above, I'll merge.
>
> Also, as you have sent this new version, I'll add
>
> Signed-off-by: Fang Hui <hui.fang at nxp.com>
>
> Thanks
> j
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index d2679a97..7fda6ce6 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();
> > ________________________________
> > From: Anle Pan <anle.pan at nxp.com>
> > Sent: Monday, March 25, 2024 5:20 PM
> > To: Umang Jain <umang.jain at ideasonboard.com>; Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > Cc: libcamera-devel at lists.libcamera.org <libcamera-devel at lists.libcamera.org>; Hui Fang <hui.fang at nxp.com>
> > Subject: RE: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture
> >
> > Hi Umang, Jacopo.
> >
> > Yes, when issue happened, stuck in sendCaptureResults. The reproduce chance on my side was also not high for the cts (1/10)
> >
> > void CameraDevice::sendCaptureResults()
> > {
> > while (!descriptors_.empty() && !descriptors_.front()->isPending()) {
> >
> > And Jacopo, your description is accurate, the descriptors_ has a chance to be not cleared due to the State was already 'Stopped' after flush() .
> > will ask hui to help update the commit message since the "git send" configuration has issue on my side.
> >
> > Best Regards
> > Anle Pan
> >
> > -----Original Message-----
> > From: Umang Jain <umang.jain at ideasonboard.com>
> > Sent: 2024年3月25日 15:44
> > To: Jacopo Mondi <jacopo.mondi at ideasonboard.com>; Anle Pan <anle.pan at nxp.com>
> > Cc: libcamera-devel at lists.libcamera.org; Hui Fang <hui.fang at nxp.com>
> > Subject: Re: [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 Jacopo,
> >
> > Thanks for testing,
> >
> > On 22/03/24 4:06 pm, Jacopo Mondi wrote:
> > > 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()) {
> > >
> > > ?
> >
> > We should to formalise this as an proper issue, after getting more information.
> > >>> 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;
> > >>> + }
> >
> > I am not comfortable with this patch. Consider my points below
> >
> > - First we don't have a exact description of the issue it is fixing
> >
> > - Second, if the camera state is ::Stopped already. Clears descriptors_ doesn't look a good idea to me. The function which has set the camera state::Stopped already, should be ideally be emptying the descriptors_...
> >
> > - If we are accepting this, why not just let the Camera::stop() fall through and remove
> >
> > if (state_ == State::Stopped)
> > return;
> >
> > as well. Then, CameraDevice::stop() will behave same as Camera::stop() - a no-op when called even when State::Stopped and responsible to clean up queues(and stuff) for the HAL layer.
> >
> > I should ideally back up my thoughts with some testing, but I don't have any kind of CTS testing available to me right now :(
> > >>> camera_->stop();
> > >>>
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240326/6f66f00e/attachment.htm>
More information about the libcamera-devel
mailing list