[libcamera-devel] [PATCH 5/5] android: camera_device: Assert descriptors_ is empty

[Jacopo Mondi]

Hi Hiro

On Mon, Sep 06, 2021 at 11:56:50PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thank you for the patch.
>
> On Mon, Sep 6, 2021 at 11:01 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > The CameraDevice::descriptors_ class member holds one item per each
> > in-flight request. A new descriptor is added to the map each time a new
> > request is queued to the camera and it gets then popped out when the
> > request completes or when an error happens before queueing it.
> >
> > As stopping the camera guarantees that all in-flight requests are
> > completed synchronously to the Camera::stop() call, there is no need to
> > manually clear the descriptors_ map, on the contrary, it might hides
> > issues in the handling of in-flight requests.
> >
> > Remove it and replace it with an assertion to make sure the underlying
> > camera behaves as intended and all Request are completed when the camera
> > is stopped.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 0ce9b699bfae..1591ad98c7d5 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -442,6 +442,9 @@ void CameraDevice::flush()
> >         worker_.stop();
> >         camera_->stop();
> >
> > +       /* Make sure all in-flight requests have been completed. */
>
> Shall we say descriptors are flushed in worker.stop()?

descriptors, in my understanding, as removed from the descriptors_ map
one by one as requests get completed, as a consequence of the regular
flow of operations or because of a force completion caused by
Camera::stop().

Have I missed your comment ?

Thanks
  j

>
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
>
> -Hiro
> > +       ASSERT(descriptors_.empty());
> > +
> >         MutexLocker stateLock(stateMutex_);
> >         state_ = State::Stopped;
> >  }
> > @@ -454,9 +457,7 @@ void CameraDevice::stop()
> >                  * We get here if the list of streams requested by the camera
> >                  * service has to be updated but the camera has been stopped
> >                  * already, which happens if configureStreams() gets called
> > -                * after a flush(). Clear the list of stream configurations,
> > -                * no need to clear descriptors as stopping the camera completes
> > -                * all the pending requests.
> > +                * after a flush().
> >                  */
> >                 streams_.clear();
> >                 return;
> > @@ -465,7 +466,9 @@ void CameraDevice::stop()
> >         worker_.stop();
> >         camera_->stop();
> >
> > -       descriptors_.clear();
> > +       /* Make sure all in-flight requests have been completed. */
> > +       ASSERT(descriptors_.empty());
> > +
> >         streams_.clear();
> >
> >         state_ = State::Stopped;
> > --
> > 2.32.0
> >