[libcamera-devel] [PATCH v3 7/8] libcamera: camera: Add CameraConfiguration

Niklas Söderlund niklas.soderlund at ragnatech.se
Mon Apr 8 15:35:47 CEST 2019 

Hi Laurent,

On 2019-04-08 15:23:25 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Mon, Apr 08, 2019 at 01:34:31PM +0200, Niklas Söderlund wrote:
> > On 2019-04-06 20:10:32 +0300, Laurent Pinchart wrote:
> > > On Sat, Apr 06, 2019 at 08:01:00PM +0300, Laurent Pinchart wrote:
> > >> On Sat, Apr 06, 2019 at 01:58:41AM +0200, Niklas Söderlund wrote:
> > >>> To properly support both multiple streams and stream usages the library
> > >>> must provide a method to map the stream usages to the returned streams
> > >>> configurations. Add a camera configuration object to handle this
> > >>> mapping.
> > >>> 
> > >>> Applications can iterate over the returned camera configuration to
> > >>> retrieve the streams selected by the library in the same order as the
> > >>> usages it provided to the library.
> > >>> 
> > >>> Application can use the operator[] to retrieve the stream pointer and
> > >>> the stream configuration. Using a numerical index retrieves the stream
> > >>> pointer, the numerical indexes corresponds to the insertion order of
> > >>> usages by the application, using the stream pointer retrieves the
> > >>> stream's configuration.
> > >>> 
> > >>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > >>> ---
> > >>>  include/libcamera/camera.h |  28 ++++++
> > >>>  src/libcamera/camera.cpp   | 173 +++++++++++++++++++++++++++++++++++++
> > >>>  2 files changed, 201 insertions(+)
> > >>> 
> > >>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > >>> index 0386671c902e55e8..8455049151d5c5a2 100644
> > >>> --- a/include/libcamera/camera.h
> > >>> +++ b/include/libcamera/camera.h
> > >>> @@ -24,6 +24,34 @@ class Stream;
> > >>>  class StreamConfiguration;
> > >>>  class StreamUsage;
> > >>>  
> > >>> +class CameraConfiguration
> > >>> +{
> > >>> +public:
> > >>> +	using iterator = std::vector<Stream *>::iterator;
> > >>> +	using const_iterator = std::vector<Stream *>::const_iterator;
> > >>> +
> > >>> +	CameraConfiguration();
> > >>> +
> > >>> +	iterator begin();
> > >>> +	iterator end();
> > >>> +	const_iterator begin() const;
> > >>> +	const_iterator end() const;
> > >>> +
> > >>> +	bool valid() const;
> > >>> +	bool empty() const;
> > > 
> > > valid() and empty() vs. isValid() and isEmpty() ? I have a preference
> > > for the latter due to my Qt bias, but I'm open to discuss this.
> > 
> > I have no strong opinion on valid() vs isValid() so will bend to popular 
> > opinion. For empty() I think we should keep it as is as this class to 
> > some degree tries to mimic the std::{vector,map} interface where empty() 
> > is used.
> 
> I think we should remain consistent and use either empty(), valid() or
> isEmpty(), isValid(). STL doesn't use an "is" prefix, Qt does. We have a
> few functions with the "is" prefix already, and that's my preference,
> but as I aid I'm open to discuss this.

I agree consistency is key, I have added the is prefix to both 
functions.

> 
> > >>> +	std::size_t size() const;
> > >>> +
> > >>> +	Stream *front();
> > >> 
> > >> For completeness, should you have a const Stream *front() const ?
> > >> 
> > >>> +
> > >>> +	Stream *operator[](unsigned int index) const;
> > >>> +	StreamConfiguration &operator[](Stream *stream);
> > >>> +	const StreamConfiguration &operator[](Stream *stream) const;
> > >>> +
> > >>> +private:
> > >>> +	std::vector<Stream *> order_;
> > >>> +	std::map<Stream *, StreamConfiguration> config_;
> > >>> +};
> > >>> +
> > >>>  class Camera final
> > >>>  {
> > >>>  public:
> > >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > >>> index 63fde0ffc3d02d6c..98145edea1ac9c91 100644
> > >>> --- a/src/libcamera/camera.cpp
> > >>> +++ b/src/libcamera/camera.cpp
> > >>> @@ -39,6 +39,179 @@ namespace libcamera {
> > >>>  
> > >>>  LOG_DECLARE_CATEGORY(Camera)
> > >>>  
> > >>> +/**
> > >>> + * \class CameraConfiguration
> > >>> + * \brief Hold configuration for streams of the camera
> > >>> + *
> > >>> + * The CameraConfiguration is filled with information by the application either
> > >>> + * manually or with the streamConfiguration() helper. The helper takes a list
> > >> 
> > >> s/streamConfiguration()/Camera::streamConfigure()/ (to get doxygen to
> > >> generate a link)
> > >> 
> > >>> + * of usages describing how the application intends to use streams of the
> > >>> + * camera, the application in return are provided with a default camera
> > >> 
> > >> s/are provided/is provided/
> > >> 
> > >>> + * configuration it can tune.
> > >> 
> > >> Maybe s/tune/fine-tune/ ?
> > >> 
> > >>> + *
> > >>> + * Applications iterate over the CameraConfiguration to discover which streams
> > >>> + * the camera has associated to the usages, and can inspect the configuration
> > >>> + * of individual streams using the operator[].
> > >> 
> > >> This all explains how to use CameraConfiguration with the
> > >> streamConfiguration() helper, which I think belongs more to the helper's
> > >> documentation. Here I would explain what the CameraConfiguration is and
> > >> how it is used. How about the following ?
> > >> 
> > >>  * The CameraConfiguration holds an ordered list of streams and their associated
> > >>  * StreamConfiguration. From a data storage point of view, the class operates as
> > >>  * a map of Stream pointers to StreamConfiguration, with entries accessed with
> > >>  * operator[](Stream *). Accessing an entry for a Stream pointer not yet stored
> > >>  * in the configuration inserts a new empty entry.
> > >>  *
> > >>  * The class also suppors iterators, and from that point of view operates as a
> > >>  * vector of Stream pointers. The streams are iterated in insertion order, and
> > >>  * the operator[](int) returns the Stream pointer based on its insertion index.
> > >>  * Accessing a stream with an invalid index returns a null pointer.
> > >> 
> > >> The text you wrote should be reused to document Camera::streamConfiguration().
> > >> 
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \typedef CameraConfiguration::iterator
> > >>> + * \brief Iterator for the streams in the configuration
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \typedef CameraConfiguration::const_iterator
> > >>> + * \brief Const iterator for the streams in the configuration
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \brief Create an empty camera configuration
> > >>> + */
> > >>> +CameraConfiguration::CameraConfiguration()
> > >>> +	: order_({}), config_({})
> > >>> +{
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Retrieve an iterator to the first stream in the sequence
> > >>> + *
> > >>> + * \return An iterator to the first stream
> > >>> + */
> > >>> +std::vector<Stream *>::iterator CameraConfiguration::begin()
> > >>> +{
> > >>> +	return order_.begin();
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Retrieve an iterator pointing to the past-the-end stream in the
> > >>> + * sequence
> > >>> + *
> > >>> + * \return An iterator to the element following the last stream
> > >>> + */
> > >>> +std::vector<Stream *>::iterator CameraConfiguration::end()
> > >>> +{
> > >>> +	return order_.end();
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Retrieve an iterator to the first element of the streams
> > >> 
> > >> Maybe "a const iterator" ?
> > >> 
> > >>> + *
> > >>> + * \return An iterator to the first stream
> > >> 
> > >> And here too, and for the const version of end() as well.
> > >> 
> > >>> + */
> > >>> +std::vector<Stream *>::const_iterator CameraConfiguration::begin() const
> > >>> +{
> > >>> +	return order_.begin();
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Retrieve an iterator to the end of the streams
> > >> 
> > >> The non-const version mentions past-the-end.
> > >> 
> > >>> + *
> > >>> + * \return An iterator to the element following the last stream
> > >>> + */
> > >>> +std::vector<Stream *>::const_iterator CameraConfiguration::end() const
> > >>> +{
> > >>> +	return order_.end();
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Check if the camera configuration is valid
> > >> 
> > >> I think you should explain what an invalid configuration is.
> > >> 
> > >>> + *
> > >>> + * \return True if the configuration is valid
> > >>> + */
> > >>> +bool CameraConfiguration::valid() const
> > >>> +{
> > >>> +	if (empty())
> > >>> +		return false;
> > >>> +
> > >>> +	for (auto const &it : config_) {
> > >>> +		const StreamConfiguration &conf = it.second;
> > >>> +
> > >>> +		if (conf.width == 0 || conf.height == 0 ||
> > >>> +		    conf.pixelFormat == 0 || conf.bufferCount == 0)
> > >>> +			return false;
> > >> 
> > >> Do we have use cases for a non-empty CameraConfiguration with invalid
> > >> StreamConfiguration ? Or is it just for completeness ?
> > >> 
> > >>> +	}
> > >>> +
> > >>> +	return true;
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Check if the camera configuration is empty
> > >>> + *
> > >>> + * \return True if the configuration is empty
> > >>> + */
> > >>> +bool CameraConfiguration::empty() const
> > >>> +{
> > >>> +	return order_.empty();
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Retrieve the number of stream configurations
> > >>> + *
> > >>> + * \return Number of stream configurations
> > >>> + */
> > >>> +std::size_t CameraConfiguration::size() const
> > >>> +{
> > >>> +	return order_.size();
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Access the first stream in the configuration
> > >>> + *
> > >>> + * \return The first stream in the configuration
> > >>> + */
> > >>> +Stream *CameraConfiguration::front()
> > >>> +{
> > >>> +	return order_.front();
> > >>> +}
> > >>> +/**
> > >>> + * \brief Retrieve a stream pointer from index
> > >>> + * \param[in] index Numerical index
> > >>> + *
> > >>> + * The \a index represents the zero bases insertion order of stream and stream
> > >> 
> > >> s/bases/based/
> > >> 
> > >>> + * configuration into the camera configuration.
> > >>> + *
> > >>> + * \return The stream pointer at index, or a nullptr if the index is out of
> > >>> + * bounds
> > >>> + */
> > >>> +Stream *CameraConfiguration::operator[](unsigned int index) const
> > >>> +{
> > >>> +	if (index >= order_.size())
> > >>> +		return nullptr;
> > >>> +
> > >>> +	return order_.at(index);
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Retrieve a reference to a stream configuration
> > >>> + * \param[in] stream Stream to retrieve configuration for
> > >>> + *
> > >>> + * If the camera configuration does not yet contain a configuration for
> > >>> + * the requested stream, create and return an empty stream configuration.
> > >>> + *
> > >>> + * \return The configuration for the stream
> > >>> + */
> > >>> +StreamConfiguration &CameraConfiguration::operator[](Stream *stream)
> > >>> +{
> > >>> +	if (config_.find(stream) == config_.end())
> > >>> +		order_.push_back(stream);
> > >>> +
> > >>> +	return config_[stream];
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Retrieve a const reference to a stream configuration
> > >>> + * \param[in] stream Stream to retrieve configuration for
> > >>> + *
> > >>> + * No new stream configuration is created if called with \a stream that is not
> > >>> + * already part of the camera configuration, doing so is an illegal operation.
> > >> 
> > >> s/illegal/invalid/ ?
> > >> 
> > >> I would also say "and results in undefined behaviour" as the std::map
> > >> will throw an exception.
> > >> 
> > >>> + *
> > >>> + * \return The configuration for the stream
> > >>> + */
> > >>> +const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const
> > >>> +{
> > >>> +	return config_.at(stream);
> > >>> +}
> > >>> +
> > >>>  /**
> > >>>   * \class Camera
> > >>>   * \brief Camera device
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund

More information about the libcamera-devel mailing list