[libcamera-devel] [PATCH] libcamera: camera: Fix the isAcquired test

[Laurent Pinchart]

Hi David,

On Mon, Nov 07, 2022 at 10:09:36AM +0000, David Plowman wrote:
> Hi Laurent
> 
> Thanks for the quick confirmation on this one!
> 
> On Sat, 5 Nov 2022 at 12:44, Laurent Pinchart wrote:
> >
> > Hi David,
> >
> > Thank you for the patch.
> >
> > On Fri, Nov 04, 2022 at 05:45:09PM +0000, David Plowman via libcamera-devel wrote:
> > > All states count as "acquired" except for "CameraAvailable".
> >
> > It should make no difference in practice given the current usage pattern
> > of isAcquired(), but it's certainly right.
> 
> Actually I was having trouble with this in the Python world, where I
> found that I couldn't properly "release" a camera (for another
> process) without dropping all my handles to the camera manager. Shows
> how dangerous Python is, users can just sit there and type in any old
> stuff!!

You're absolutely right. isAcquired() is called from Camera::release()
only, and the function returns before the call if the

        int ret = d->isAccessAllowed(Private::CameraAvailable,
                                     Private::CameraConfigured, true);

check fails. If the check succeeds, the state can't be CameraRunning, so
the isAcquired() test then fails, and the camera is never released.

The problem was introduced by a bad copy&paste in the commit that added
isAcquired().

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  src/libcamera/camera.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 9fe29ca9..f0575c13 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -501,7 +501,7 @@ static const char *const camera_state_names[] = {
> > >
> > >  bool Camera::Private::isAcquired() const
> > >  {
> > > -     return state_.load(std::memory_order_acquire) == CameraRunning;
> > > +     return state_.load(std::memory_order_acquire) != CameraAvailable;
> > >  }
> > >
> > >  bool Camera::Private::isRunning() const

-- 
Regards,

Laurent Pinchart