[libcamera-devel,v5,6/6] libcamera: camera: extend camera object to support configuration of streams

452 diff mbox series

Extend the camera to support reading and configuring formats for
groups of streams. The implementation in the Camera are minimalistic as
the heavy lifting are done by the pipeline handler implementations.

The most important functionality the camera provides in this context is
validation of data structures passed to it from the application and
access control to the pipeline handler.

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

Hi Niklas,

Thank you for the patch.

On Wed, Jan 30, 2019 at 12:56:15PM +0100, Niklas Söderlund wrote:
> Extend the camera to support reading and configuring formats for
> groups of streams. The implementation in the Camera are minimalistic as
> the heavy lifting are done by the pipeline handler implementations.
> 
> The most important functionality the camera provides in this context is
> validation of data structures passed to it from the application and
> access control to the pipeline handler.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/camera.h |  4 +++
>  src/libcamera/camera.cpp   | 61 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 01498935ae4aab58..b21d116edc0f3a57 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_CAMERA_H__
>  #define __LIBCAMERA_CAMERA_H__
>  
> +#include <map>
>  #include <memory>
>  #include <string>
>  
> @@ -16,6 +17,7 @@ namespace libcamera {
>  
>  class PipelineHandler;
>  class Stream;
> +class StreamConfiguration;
>  
>  class Camera final
>  {
> @@ -35,6 +37,8 @@ public:
>  	void release();
>  
>  	const std::vector<Stream *> streams() const;
> +	std::map<Stream*, StreamConfiguration> streamConfiguration(std::vector<Stream*> &streams);

s/;/ const;/

> +	int configureStreams(std::map<Stream*, StreamConfiguration> &config);

s/(/(const /

>  
>  private:
>  	Camera(PipelineHandler *pipe, const std::string &name);
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 986b74407aed6bd2..a293350f731e18df 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -190,4 +190,65 @@ const std::vector<Stream *> Camera::streams() const
>  	return streams_;
>  }
>  
> +/**
> + * \brief Retrieve a group of stream configurations
> + * \param[in] streams A map of stream IDs and configurations to setup
> + *
> + * Retrieve the camera's configuration for a specified group of streams. The
> + * caller can specifies which of the camera's streams to retrieve configuration
> + * from by populating \a streams.
> + *
> + * The easiest way to populate the array of streams to fetch configuration from
> + * is to first retrieve the camera's full array of stream by with streams() and

"by with" ?

> + * then potentially trim it down to only contain the streams the caller
> + * are interested in.
> + *
> + * \return A map of successfully retrieved stream IDs and configurations or an
> + * empty list on error.

That doesn't allow returning an error code to discriminate between
errors :-/ I wonder if we should pass the map as an argument by pointer
and return an integer. Or maybe there's no need to discriminate between
errors ?

> + */
> +std::map<Stream*, StreamConfiguration>
> +Camera::streamConfiguration(std::vector<Stream*> &streams)
> +{
> +	if (disconnected_ || !streams.size())
> +		std::map<unsigned int, StreamConfiguration>{};
> +
> +	return pipe_->streamConfiguration(this, streams);
> +}
> +
> +/**
> + * \brief Configure the camera's streams prior to capture
> + * \param[in] config A map of stream IDs and configurations to setup
> + *
> + * Prior to starting capture, the camera must be configured to select a
> + * group of streams to be involved in the capture and their configuration.
> + * The caller specifies which streams are to be involved and their configuration
> + * by populating \a config.
> + *
> + * The easiest way to populate the array of config is to fetch an initial
> + * configuration from the camera with streamConfiguration() and then change the
> + * parameters to fit the caller's need and once all the streams parameters are
> + * configured hand that over to configureStreams() to actually setup the camera.
> + *
> + * Exclusive access to the camera shall be ensured by a call to acquire() prior
> + * to calling this function, otherwise an -EACCES error is be returned.

s/is be/will be/

> + *
> + * \return 0 on success or a negative error code on error.
> + * \retval -ENODEV The camera is not connected to any hardware
> + * \retval -EACCES The user have not acquired exclusive access to the camera

s/have not/has not/

> + * \retval -EINVAL The configuration is not valid
> + */
> +int Camera::configureStreams(std::map<Stream*, StreamConfiguration> &config)
> +{
> +	if (disconnected_)
> +		return -ENODEV;
> +
> +	if (!acquired_)
> +		return -EACCES;
> +
> +	if (!config.size())
> +		return -EINVAL;
> +
> +	return pipe_->configureStreams(this, config);
> +}
> +
>  } /* namespace libcamera */

Hi Laurent,

Thanks for your feedback.

On 2019-01-31 02:30:32 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Wed, Jan 30, 2019 at 12:56:15PM +0100, Niklas Söderlund wrote:
> > Extend the camera to support reading and configuring formats for
> > groups of streams. The implementation in the Camera are minimalistic as
> > the heavy lifting are done by the pipeline handler implementations.
> > 
> > The most important functionality the camera provides in this context is
> > validation of data structures passed to it from the application and
> > access control to the pipeline handler.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/camera.h |  4 +++
> >  src/libcamera/camera.cpp   | 61 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> > 
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 01498935ae4aab58..b21d116edc0f3a57 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __LIBCAMERA_CAMERA_H__
> >  #define __LIBCAMERA_CAMERA_H__
> >  
> > +#include <map>
> >  #include <memory>
> >  #include <string>
> >  
> > @@ -16,6 +17,7 @@ namespace libcamera {
> >  
> >  class PipelineHandler;
> >  class Stream;
> > +class StreamConfiguration;
> >  
> >  class Camera final
> >  {
> > @@ -35,6 +37,8 @@ public:
> >  	void release();
> >  
> >  	const std::vector<Stream *> streams() const;
> > +	std::map<Stream*, StreamConfiguration> streamConfiguration(std::vector<Stream*> &streams);
> 
> s/;/ const;/
> 
> > +	int configureStreams(std::map<Stream*, StreamConfiguration> &config);
> 
> s/(/(const /
> 
> >  
> >  private:
> >  	Camera(PipelineHandler *pipe, const std::string &name);
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 986b74407aed6bd2..a293350f731e18df 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -190,4 +190,65 @@ const std::vector<Stream *> Camera::streams() const
> >  	return streams_;
> >  }
> >  
> > +/**
> > + * \brief Retrieve a group of stream configurations
> > + * \param[in] streams A map of stream IDs and configurations to setup
> > + *
> > + * Retrieve the camera's configuration for a specified group of streams. The
> > + * caller can specifies which of the camera's streams to retrieve configuration
> > + * from by populating \a streams.
> > + *
> > + * The easiest way to populate the array of streams to fetch configuration from
> > + * is to first retrieve the camera's full array of stream by with streams() and
> 
> "by with" ?
> 
> > + * then potentially trim it down to only contain the streams the caller
> > + * are interested in.
> > + *
> > + * \return A map of successfully retrieved stream IDs and configurations or an
> > + * empty list on error.
> 
> That doesn't allow returning an error code to discriminate between
> errors :-/ I wonder if we should pass the map as an argument by pointer
> and return an integer. Or maybe there's no need to discriminate between
> errors ?

Good question, I'm not sure if we need to discriminate between errors 
here? If we do how would an application act on the different errors?

> 
> > + */
> > +std::map<Stream*, StreamConfiguration>
> > +Camera::streamConfiguration(std::vector<Stream*> &streams)
> > +{
> > +	if (disconnected_ || !streams.size())
> > +		std::map<unsigned int, StreamConfiguration>{};
> > +
> > +	return pipe_->streamConfiguration(this, streams);
> > +}
> > +
> > +/**
> > + * \brief Configure the camera's streams prior to capture
> > + * \param[in] config A map of stream IDs and configurations to setup
> > + *
> > + * Prior to starting capture, the camera must be configured to select a
> > + * group of streams to be involved in the capture and their configuration.
> > + * The caller specifies which streams are to be involved and their configuration
> > + * by populating \a config.
> > + *
> > + * The easiest way to populate the array of config is to fetch an initial
> > + * configuration from the camera with streamConfiguration() and then change the
> > + * parameters to fit the caller's need and once all the streams parameters are
> > + * configured hand that over to configureStreams() to actually setup the camera.
> > + *
> > + * Exclusive access to the camera shall be ensured by a call to acquire() prior
> > + * to calling this function, otherwise an -EACCES error is be returned.
> 
> s/is be/will be/
> 
> > + *
> > + * \return 0 on success or a negative error code on error.
> > + * \retval -ENODEV The camera is not connected to any hardware
> > + * \retval -EACCES The user have not acquired exclusive access to the camera
> 
> s/have not/has not/
> 
> > + * \retval -EINVAL The configuration is not valid
> > + */
> > +int Camera::configureStreams(std::map<Stream*, StreamConfiguration> &config)
> > +{
> > +	if (disconnected_)
> > +		return -ENODEV;
> > +
> > +	if (!acquired_)
> > +		return -EACCES;
> > +
> > +	if (!config.size())
> > +		return -EINVAL;
> > +
> > +	return pipe_->configureStreams(this, config);
> > +}
> > +
> >  } /* namespace libcamera */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hi Niklas,

On Thu, Jan 31, 2019 at 07:01:13PM +0100, Niklas Söderlund wrote:
> On 2019-01-31 02:30:32 +0200, Laurent Pinchart wrote:
> > On Wed, Jan 30, 2019 at 12:56:15PM +0100, Niklas Söderlund wrote:
> >> Extend the camera to support reading and configuring formats for
> >> groups of streams. The implementation in the Camera are minimalistic as
> >> the heavy lifting are done by the pipeline handler implementations.
> >> 
> >> The most important functionality the camera provides in this context is
> >> validation of data structures passed to it from the application and
> >> access control to the pipeline handler.
> >> 
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >> ---
> >>  include/libcamera/camera.h |  4 +++
> >>  src/libcamera/camera.cpp   | 61 ++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 65 insertions(+)
> >> 
> >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> >> index 01498935ae4aab58..b21d116edc0f3a57 100644
> >> --- a/include/libcamera/camera.h
> >> +++ b/include/libcamera/camera.h
> >> @@ -7,6 +7,7 @@
> >>  #ifndef __LIBCAMERA_CAMERA_H__
> >>  #define __LIBCAMERA_CAMERA_H__
> >>  
> >> +#include <map>
> >>  #include <memory>
> >>  #include <string>
> >>  
> >> @@ -16,6 +17,7 @@ namespace libcamera {
> >>  
> >>  class PipelineHandler;
> >>  class Stream;
> >> +class StreamConfiguration;
> >>  
> >>  class Camera final
> >>  {
> >> @@ -35,6 +37,8 @@ public:
> >>  	void release();
> >>  
> >>  	const std::vector<Stream *> streams() const;
> >> +	std::map<Stream*, StreamConfiguration> streamConfiguration(std::vector<Stream*> &streams);
> > 
> > s/;/ const;/
> > 
> >> +	int configureStreams(std::map<Stream*, StreamConfiguration> &config);
> > 
> > s/(/(const /
> > 
> >>  private:
> >>  	Camera(PipelineHandler *pipe, const std::string &name);
> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> >> index 986b74407aed6bd2..a293350f731e18df 100644
> >> --- a/src/libcamera/camera.cpp
> >> +++ b/src/libcamera/camera.cpp
> >> @@ -190,4 +190,65 @@ const std::vector<Stream *> Camera::streams() const
> >>  	return streams_;
> >>  }
> >>  
> >> +/**
> >> + * \brief Retrieve a group of stream configurations
> >> + * \param[in] streams A map of stream IDs and configurations to setup
> >> + *
> >> + * Retrieve the camera's configuration for a specified group of streams. The
> >> + * caller can specifies which of the camera's streams to retrieve configuration
> >> + * from by populating \a streams.
> >> + *
> >> + * The easiest way to populate the array of streams to fetch configuration from
> >> + * is to first retrieve the camera's full array of stream by with streams() and
> > 
> > "by with" ?
> > 
> >> + * then potentially trim it down to only contain the streams the caller
> >> + * are interested in.
> >> + *
> >> + * \return A map of successfully retrieved stream IDs and configurations or an
> >> + * empty list on error.
> > 
> > That doesn't allow returning an error code to discriminate between
> > errors :-/ I wonder if we should pass the map as an argument by pointer
> > and return an integer. Or maybe there's no need to discriminate between
> > errors ?
> 
> Good question, I'm not sure if we need to discriminate between errors 
> here? If we do how would an application act on the different errors?

I'm not sure either, although distinguishing invalid arguments from
disconnection may be useful. Maybe we can leave it as-is for now.

> >> + */
> >> +std::map<Stream*, StreamConfiguration>
> >> +Camera::streamConfiguration(std::vector<Stream*> &streams)
> >> +{
> >> +	if (disconnected_ || !streams.size())
> >> +		std::map<unsigned int, StreamConfiguration>{};
> >> +
> >> +	return pipe_->streamConfiguration(this, streams);
> >> +}
> >> +
> >> +/**
> >> + * \brief Configure the camera's streams prior to capture
> >> + * \param[in] config A map of stream IDs and configurations to setup
> >> + *
> >> + * Prior to starting capture, the camera must be configured to select a
> >> + * group of streams to be involved in the capture and their configuration.
> >> + * The caller specifies which streams are to be involved and their configuration
> >> + * by populating \a config.
> >> + *
> >> + * The easiest way to populate the array of config is to fetch an initial
> >> + * configuration from the camera with streamConfiguration() and then change the
> >> + * parameters to fit the caller's need and once all the streams parameters are
> >> + * configured hand that over to configureStreams() to actually setup the camera.
> >> + *
> >> + * Exclusive access to the camera shall be ensured by a call to acquire() prior
> >> + * to calling this function, otherwise an -EACCES error is be returned.
> > 
> > s/is be/will be/
> > 
> >> + *
> >> + * \return 0 on success or a negative error code on error.
> >> + * \retval -ENODEV The camera is not connected to any hardware
> >> + * \retval -EACCES The user have not acquired exclusive access to the camera
> > 
> > s/have not/has not/
> > 
> >> + * \retval -EINVAL The configuration is not valid
> >> + */
> >> +int Camera::configureStreams(std::map<Stream*, StreamConfiguration> &config)
> >> +{
> >> +	if (disconnected_)
> >> +		return -ENODEV;
> >> +
> >> +	if (!acquired_)
> >> +		return -EACCES;
> >> +
> >> +	if (!config.size())
> >> +		return -EINVAL;
> >> +
> >> +	return pipe_->configureStreams(this, config);
> >> +}
> >> +
> >>  } /* namespace libcamera */