[libcamera-devel,v2,10/11] libcamera: pipeline_handler: Add functions to lock a whole pipeline

1175 diff mbox series

Add lock() and unlock() which backed by the MediaDevice implementation
can will lock all media devices claimed by a pipeline handler instance.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/include/pipeline_handler.h |  3 ++
 src/libcamera/pipeline_handler.cpp       | 38 ++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

Hi Niklas,

On 08/05/2019 16:18, Niklas Söderlund wrote:
> Add lock() and unlock() which backed by the MediaDevice implementation

'which are backed'

> can will lock all media devices claimed by a pipeline handler instance.

and will lock

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/include/pipeline_handler.h |  3 ++
>  src/libcamera/pipeline_handler.cpp       | 38 ++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 4c13496246c2251c..019c8f4751a8c2b2 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -57,6 +57,9 @@ public:
>  	MediaDevice *tryAcquire(DeviceEnumerator *enumerator,
>  				const DeviceMatch &dm);
>  
> +	bool lock();
> +	void unlock();
> +
>  	virtual CameraConfiguration
>  	streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0;
>  	virtual int configureStreams(Camera *camera, const CameraConfiguration &config) = 0;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 8f50ef51f0c23301..6aa6c6ede49beb12 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -182,6 +182,44 @@ out:
>  	return media.get();
>  }
>  
> +/**
> + * \brief Lock all media devices acquired by the pipeline
> + *
> + * This method shall not be called from a pipeline handler implementation
> + * directly, as the Camera handles this on the behalf of the specified
> + * implementation.
> + *
> + * \return True if the devices could be locked, false otherwise
> + * \sa unlock()
> + * \sa MediaDevice::lock()
> + */
> +bool PipelineHandler::lock()
> +{
> +	for (std::shared_ptr<MediaDevice> media : mediaDevices_) {

for (auto media : mediaDevices_) { ?


> +		if (!media->lock()) {
> +			unlock();
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +/**
> + * \brief Unlock all media devices acquired by the pipeline
> + *
> + * This method shall not be called from a pipeline handler implementation
> + * directly, as the Camera handles this on the behalf of the specified
> + * implementation.
> + *
> + * \sa lock()
> + */
> +void PipelineHandler::unlock()
> +{
> +	for (std::shared_ptr<MediaDevice> media : mediaDevices_)

auto ?

> +		media->unlock();
> +}
> +
>  /**
>   * \fn PipelineHandler::streamConfiguration()
>   * \brief Retrieve a group of stream configurations for a specified camera
>

Hi Kieran,

Thanks for your feedback.

On 2019-05-09 11:00:24 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 08/05/2019 16:18, Niklas Söderlund wrote:
> > Add lock() and unlock() which backed by the MediaDevice implementation
> 
> 'which are backed'
> 
> > can will lock all media devices claimed by a pipeline handler instance.
> 
> and will lock
> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/include/pipeline_handler.h |  3 ++
> >  src/libcamera/pipeline_handler.cpp       | 38 ++++++++++++++++++++++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index 4c13496246c2251c..019c8f4751a8c2b2 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -57,6 +57,9 @@ public:
> >  	MediaDevice *tryAcquire(DeviceEnumerator *enumerator,
> >  				const DeviceMatch &dm);
> >  
> > +	bool lock();
> > +	void unlock();
> > +
> >  	virtual CameraConfiguration
> >  	streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0;
> >  	virtual int configureStreams(Camera *camera, const CameraConfiguration &config) = 0;
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 8f50ef51f0c23301..6aa6c6ede49beb12 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -182,6 +182,44 @@ out:
> >  	return media.get();
> >  }
> >  
> > +/**
> > + * \brief Lock all media devices acquired by the pipeline
> > + *
> > + * This method shall not be called from a pipeline handler implementation
> > + * directly, as the Camera handles this on the behalf of the specified
> > + * implementation.
> > + *
> > + * \return True if the devices could be locked, false otherwise
> > + * \sa unlock()
> > + * \sa MediaDevice::lock()
> > + */
> > +bool PipelineHandler::lock()
> > +{
> > +	for (std::shared_ptr<MediaDevice> media : mediaDevices_) {
> 
> for (auto media : mediaDevices_) { ?

I think we should try to only use auto when the loop variable is an 
iterator, so I would prefer to keep it as is. Looking at it again it 
should probably be a reference, will make it as such for next version.

    for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {

> 
> 
> > +		if (!media->lock()) {
> > +			unlock();
> > +			return false;
> > +		}
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/**
> > + * \brief Unlock all media devices acquired by the pipeline
> > + *
> > + * This method shall not be called from a pipeline handler implementation
> > + * directly, as the Camera handles this on the behalf of the specified
> > + * implementation.
> > + *
> > + * \sa lock()
> > + */
> > +void PipelineHandler::unlock()
> > +{
> > +	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> 
> auto ?
> 
> > +		media->unlock();
> > +}
> > +
> >  /**
> >   * \fn PipelineHandler::streamConfiguration()
> >   * \brief Retrieve a group of stream configurations for a specified camera
> > 
> 
> -- 
> Regards
> --
> Kieran