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

[Jacopo Mondi]

Hi Niklas,
   thanks for the patch.

On Fri, Apr 05, 2019 at 04:02:55AM +0200, Niklas Söderlund wrote:
> To properly support both multiple streams and usages the library must
> provide a method to map the stream usages to the returned stream
> configuration. 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. Further more the application can use

"in the same order as it provided usages to the library"

furthermore

> the streams to retrieve there configuration using operator[] of the

"a stream pointer to retrieve its configuration" or
"the streams to retrieve their configuration"

> camera configuration.
>

This is a nice and clean way to replace the cumbersome map<Stream *,
StreamConfiguration> used by both streamConfiguration() and
configureStream() and makes it easy to access a configuration from a
Stream *, so it's nice. Though, I don't see how this would help in
handling the "what stream is what" problem at requestComplete() time.
What's missing is the association with a name/id to a configuration,
is this something planned for later or am I missing how this should be
used?

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/camera.h |  26 ++++++++
>  src/libcamera/camera.cpp   | 121 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 147 insertions(+)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 0386671c902e55e8..311a51e4070d135c 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -14,6 +14,7 @@
>
>  #include <libcamera/request.h>
>  #include <libcamera/signal.h>
> +#include <libcamera/stream.h>
>
>  namespace libcamera {
>
> @@ -24,6 +25,31 @@ class Stream;
>  class StreamConfiguration;
>  class StreamUsage;
>
> +class CameraConfiguration
> +{
> +public:
> +	using iterator = std::vector<Stream *>::iterator;
> +	using const_iterator = std::vector<Stream *>::const_iterator;

Have you considered giving this class iterator traits and implement
methods required for at least, say, ForwardIterator?
http://www.cplusplus.com/reference/iterator/ForwardIterator/

That would make possible for applications to use STL library functions
that operates on iterators on the class. In example, can you use
range-based for loops on this class?

> +
> +	CameraConfiguration();
> +
> +	iterator begin();
> +	iterator end();
> +	const_iterator begin() const;
> +	const_iterator end() const;
> +
> +	bool empty() const;
> +	std::size_t size() const;
> +
> +	Stream *front();
> +
> +	StreamConfiguration &operator[](Stream *stream);
> +
> +private:
> +	std::vector<Stream *> order_;
> +	std::map<Stream *, StreamConfiguration> config_;

I wonder if keeping the Stream * in two separate places might lead to
issues in keeping the two in sync. But these are privates, so it
should be fine...

> +};
> +
>  class Camera final
>  {
>  public:
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 63fde0ffc3d02d6c..16162c524297012f 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -39,6 +39,127 @@ namespace libcamera {
>
>  LOG_DECLARE_CATEGORY(Camera)
>
> +/**
> + * \class CameraConfiguration
> + * \brief Hold configuration for streams of the camera that applications
I would drop what's after the "of the camera".
> + * wish to modify and apply.
> + *
> + * The CameraConfiguration is filled with information by the application either
> + * manually or with the streamConfiguration() helper. The helper takes a list
> + * list of usages describing how the application intents to use the camera, the
s/list list/list/
s/intents/intends/
s/use the camera/use the stream/

> + * application in returns is provided with a default camera configuration it
s/in returns/in return/
> + * can tweak.
> + *
> + * Applications iterates over the CameraConfiguration to discover which streams
s/iterates/iterate/
> + * the camera have selected for its usages and can inspect the configuration
s/the camera have selected for its usage/the camera has associated to a usage/
s/can inspect/can access/
> + * using the operator[].
> + */
> +
> +/**
> + * \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 element of the streams
> + *

The standard iterator documentation for begin reports:
"Returns an iterator pointing to the first element in the sequence"

Could we match this?

> + * \return An iterator to the first stream
> + */
> +std::vector<Stream *>::iterator CameraConfiguration::begin()
> +{
> +	return order_.begin();
> +}
> +
> +/**
> + * \brief Retrieve an iterator to the end of the streams

Same as per 'begin()':

"Returns an iterator pointing to the past-the-end element 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
> + *
> + * \return An iterator to the first stream
> + */
> +std::vector<Stream *>::const_iterator CameraConfiguration::begin() const
> +{
> +	return order_.begin();
> +}
> +
> +/**
> + * \brief Retrieve an iterator to the end of the streams
> + *
> + * \return An iterator to the element following the last stream
> + */
> +std::vector<Stream *>::const_iterator CameraConfiguration::end() const
> +{
> +	return order_.end();
> +}
> +
> +/**
> + * \brief Checks whether the camera configuration is empty
> + *
> + * \return True if the configuration is empty
> + */
> +bool CameraConfiguration::empty() const
> +{
> +	return order_.empty();
> +}
> +
> +/**
> + * \brief Check 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 reference to a stream configuration
> + * \param[in] stream Stream to retrieve configuration for
> + *
> + * If the camera configuration do not yet contain configuration for the

contain a configuration

> + * requested stream an empty stream configuration is created and returned.

empty one

> + *
> + * \return Configuration for the stream

The configuration

> + */
> +StreamConfiguration &CameraConfiguration::operator[](Stream *stream)
> +{
> +	if (config_.find(stream) == config_.end())
> +		order_.push_back(stream);
> +
> +	return config_[stream];
> +}
> +

I don't know how welcome would the idea of making of this class a
full-fledged iterator, but I think increment, decrement and access by
integer index should be implemented to make it more useful. Have you
considered that?

Thanks
  j

>  /**
>   * \class Camera
>   * \brief Camera device
> --
> 2.21.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190405/f58a3301/attachment-0001.sig>