[libcamera-devel] [PATCH v3 2/2] android: CameraDevice: Add stop()

[Hirokazu Honda]

Hi, Jacopo, thanks for reviewing.

On Mon, Mar 29, 2021 at 6:13 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Mon, Mar 29, 2021 at 03:11:19PM +0900, Hirokazu Honda wrote:
> > This adds CameraDevice::stop(), which cleans up the member
> > variables of CameraDevice. It is called in CameraDevice::close()
> > and CameraDevice::configureStreams().
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/android/camera_device.cpp | 17 +++++++++--------
> >  src/android/camera_device.h   |  2 ++
> >  2 files changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 02d3bfb2..d5447d8e 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -659,12 +659,17 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
> >
> >  void CameraDevice::close()
> >  {
> > -     streams_.clear();
> > +     stop();
> > +
> > +     camera_->release();
> > +}
> >
> > +void CameraDevice::stop()
> > +{
> >       worker_.stop();
> >       camera_->stop();
>
> I think the camera shall be stopped only if (running_). Otherwise
> you'll get an error message from the Camera state maching if I'm not
> mistaken.
>
> > -     camera_->release();
> >
> > +     descriptors_.clear();
>
> Ideally, this should be done as:
> 1) Introduce stop() in CameraDevice
> 2) Introduce descriptors_ and clear it in stop()
>
> As this bundle two changes in one patch.
>
> A minor though.
>

Okay, I reorder the patches to first one of adding stop() and the
second one of fixing leakage.


> The patch is fine, but I think the discussion with Laurent on what has
> triggered this issue in first place is still a bit on-going, right ?
>

We agree, regardless of issues, this patch should go as avoiding the leakage.
The issue is trivial, and we are discussing to select the most proper
way of fixing it.
That is, we shall merge this patch without minding the discussion.

Regards,
-Hiro
> >       running_ = false;
> >  }
> >
> > @@ -1547,12 +1552,8 @@ PixelFormat CameraDevice::toPixelFormat(int format) const
> >   */
> >  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  {
> > -     /* Before any configuration attempt, stop the camera if it's running. */
> > -     if (running_) {
> > -             worker_.stop();
> > -             camera_->stop();
> > -             running_ = false;
> > -     }
> > +     /* Before any configuration attempt, stop the camera. */
> > +     stop();
> >
> >       /*
> >        * Generate an empty configuration, and construct a StreamConfiguration
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 5a2d22d6..cc12fe20 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -87,6 +87,8 @@ private:
> >               int androidFormat;
> >       };
> >
> > +     void stop();
> > +
> >       int initializeStreamConfigurations();
> >       std::vector<libcamera::Size>
> >       getYUVResolutions(libcamera::CameraConfiguration *cameraConfig,
> > --
> > 2.31.0.291.g576ba9dcdaf-goog
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel