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

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2019-01-29 10:16:43 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 29/01/2019 02:00, 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 at 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 3ebb8e96cc63b98a..7a0357f3a2919752 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();
> >  
> >  	std::vector<Stream *> streams() const;
> > +	std::map<Stream*, StreamConfiguration> streamConfiguration(std::vector<Stream*> &streams);
> > +	int configureStreams(std::map<Stream*, StreamConfiguration> &config);
> >  
> >  private:
> >  	Camera(PipelineHandler *pipe, const std::string &name);
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index c8604ec8a8e6eaf4..69d28a10f1f73d0d 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -190,4 +190,65 @@ 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
> > + * 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.
> > + */
> > +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.
> > + *
> > + * \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
> > + * \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;
> > +
> 
> Perhaps not necessary in this patch - but it looks like perhaps we
> should wrap the access control into a function.
> 
> It's going to be required for almost every call through the Camera
> object, and it might need flags to determine what control is allowed.
> 
> int CameraAccessAllowed(AccessFlags)
> {
>   if (disconnected_)
> 	return -ENODEV;
> 
>   if ( (AccessFlags & MUST_ACQUIRE) && !acquired )
> 	return -EACCES;
> 
>   /* Other checks / flags here ? */
> 
>   return true;
> }
> 
> Camera::configureStreams(std::map<Stream*, StreamConfiguration> &config)
> {
> 
>   int ret = CameraAccessAllowed(MUST_ACQUIRE | MUST_EXIST)
>   if (ret)
> 	return ret;
> 
>   if (!config.size())
> 	return -EINVAL;
>   ...
> }

I like this idea but would first like to see some more operations added 
so that we can break this out into the most useful helper. For this 
patch I would like to keep it as is as to not prematurely optimize 
things.

> 
> Then this :
> > \retval -ENODEV The camera is not connected to any hardware
> > \retval -EACCES The user have not acquired exclusive access to the camera
> > \retval -EINVAL The configuration is not valid
> 
> can be documented just once, instead of on every API call.
> 
> 
> > +	return pipe_->configureStreams(this, config);
> > +}
> > +
> >  } /* namespace libcamera */
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund