[libcamera-devel,0/2] Support resource acquisition at 'acquire()'

17806 mbox series

Pipeline handlers may benefit from being able to postpone resource
acquisition until the acquire() operation, or even release any temporary
resources used during initialisation and only use them when acquired.

This series hopes to balance the updates to the
PipelineHandler->release(Camera) call, with a corresponding
PipelineHandler->acquire(camera) and facilitate pipeline handlers
managing resources only when the camera is in use.

While the pipeline patch could make some progress, the uvcvideo patch in
this series is not working. I fear this may be a symptom of a bug in
either our thread or fdNotifier / event mechanisms, but haven't been
able to track it down yet.

Kieran Bingham (2):
  libcamera: pipeline: Add an acquireDevice method
  [DNI/RFC] pipeline: uvcvideo: Only open devices upon acquire

 include/libcamera/internal/pipeline_handler.h |  3 +-
 src/libcamera/camera.cpp                      |  2 +-
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  | 55 +++++++++++++++----
 src/libcamera/pipeline_handler.cpp            | 23 +++++++-
 4 files changed, 69 insertions(+), 14 deletions(-)

Hi Kieran,

On Wed, Nov 16, 2022 at 12:17:22AM +0000, Kieran Bingham via libcamera-devel wrote:
> Pipeline handlers may benefit from being able to postpone resource
> acquisition until the acquire() operation, or even release any temporary
> resources used during initialisation and only use them when acquired.
> 
> This series hopes to balance the updates to the
> PipelineHandler->release(Camera) call, with a corresponding
> PipelineHandler->acquire(camera) and facilitate pipeline handlers
> managing resources only when the camera is in use.
> 
> While the pipeline patch could make some progress, the uvcvideo patch in
> this series is not working. I fear this may be a symptom of a bug in
> either our thread or fdNotifier / event mechanisms, but haven't been
> able to track it down yet.

Did you read my reply to the patch that introduces releaseDevice() ? One
of my concerns is that it ties resource lifetime to ownership lifetime,
which makes it impossible to keep a device acquired for exclusive access
without also holding onto resources.

Could you please expand on the design rationale for this series ? The
rationale should also be reflected in high-level documentation.

> Kieran Bingham (2):
>   libcamera: pipeline: Add an acquireDevice method
>   [DNI/RFC] pipeline: uvcvideo: Only open devices upon acquire
> 
>  include/libcamera/internal/pipeline_handler.h |  3 +-
>  src/libcamera/camera.cpp                      |  2 +-
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  | 55 +++++++++++++++----
>  src/libcamera/pipeline_handler.cpp            | 23 +++++++-

Missing update to the pipeline handler writer's guide (releaseDevice()
also needs to be documented).

>  4 files changed, 69 insertions(+), 14 deletions(-)

Quoting Laurent Pinchart (2022-11-16 07:42:12)
> Hi Kieran,
> 
> On Wed, Nov 16, 2022 at 12:17:22AM +0000, Kieran Bingham via libcamera-devel wrote:
> > Pipeline handlers may benefit from being able to postpone resource
> > acquisition until the acquire() operation, or even release any temporary
> > resources used during initialisation and only use them when acquired.
> > 
> > This series hopes to balance the updates to the
> > PipelineHandler->release(Camera) call, with a corresponding
> > PipelineHandler->acquire(camera) and facilitate pipeline handlers
> > managing resources only when the camera is in use.
> > 
> > While the pipeline patch could make some progress, the uvcvideo patch in
> > this series is not working. I fear this may be a symptom of a bug in
> > either our thread or fdNotifier / event mechanisms, but haven't been
> > able to track it down yet.
> 
> Did you read my reply to the patch that introduces releaseDevice() ? One
> of my concerns is that it ties resource lifetime to ownership lifetime,
> which makes it impossible to keep a device acquired for exclusive access
> without also holding onto resources.

Isn't it the opposite right now though? We currently have
implementations (uvcvideo) where the resources are held at construction
of the Camera class, and *kept open*.

This proposal is to reduce that resource acquisition until an
application actually states it indeeds to acquire the camera.

See Bug #168: https://bugs.libcamera.org/show_bug.cgi?id=168

What do you see is different between an application acquiring a camera
exclusively by keeping it acquired, with resources obtained at that
point, and the current situation where the camera resources are
consumed, even though the camera is not acquired?

Perhaps would you rather see a clear resource acquisition only at ...
configure time? We would then have the unbalance that we don't have an
unconfigure() call to explicitly release those resources, which is why
this is expected to match and be symetrical with the patch provided by
David.

--
Kieran


> Could you please expand on the design rationale for this series ? The
> rationale should also be reflected in high-level documentation.

Sure - As I said in the replies to Davids series, this initial posting
was really to ask for help on the DNI/RFC patch ... which I suspect /
fear highlights a potential threading/event bug (or me just doing
something wrong).



> 
> > Kieran Bingham (2):
> >   libcamera: pipeline: Add an acquireDevice method
> >   [DNI/RFC] pipeline: uvcvideo: Only open devices upon acquire
> > 
> >  include/libcamera/internal/pipeline_handler.h |  3 +-
> >  src/libcamera/camera.cpp                      |  2 +-
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  | 55 +++++++++++++++----
> >  src/libcamera/pipeline_handler.cpp            | 23 +++++++-
> 
> Missing update to the pipeline handler writer's guide (releaseDevice()
> also needs to be documented).
> 
> >  4 files changed, 69 insertions(+), 14 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart