[libcamera-devel] [PATCH v2 2/7] libcamera: stream: add basic StreamConfiguration class

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jan 25, 2019 at 04:33:35PM +0100, Niklas Söderlund wrote:
> Add a simple StreamConfiguration class to hold configuration data for a
> single stream of a Camera. In its current form not many configuration
> parameters are supported but it's expected the number of options will
> grow over time.
> 
> At this stage the pixel format is represented as a unsigned int to allow

s/as a/as an/

> for a easy mapping to the V4L2 API. This might be subject to change in

s/a easy/an easy/ (or just s/a easy/easy/)

> the future as we finalize how libcamera shall represent pixelformats.

s/pixelformats/pixel formats/

> A StreamConfiguration objected needs to be created from the Stream
> object it should configure. As the two objects are so closely related I
> have at this stage opted to implement them in the same stream.{h,cpp} as
> the Stream class.

Why does the StreamConfiguration object need to be created with the
Stream passed as an argument to the constructor ?

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/stream.h | 20 ++++++++++++
>  src/libcamera/stream.cpp   | 64 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 415815ba12c65e47..8750797c36dd9b42 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -20,6 +20,26 @@ private:
>  	unsigned int id_;
>  };
>  
> +class StreamConfiguration final
> +{
> +public:
> +	StreamConfiguration(class Stream &stream);

s/class //

> +
> +	unsigned int id() const { return id_; };

Given that the stream is given to the constructor, should we store the
stream reference instead of the id ?

> +	unsigned int width() const { return width_; };
> +	unsigned int height() const { return height_; };
> +	unsigned int pixelformat() const { return pixelformat_; };

Open question, pixelformat or pixelFormat ?

> +
> +	void setDimension(unsigned int width, unsigned int height);

I'd name this setSize(). I think we'll need a class to represent sizes
and rectangles at some point, but that can be done later.

> +	void setPixelFormat(unsigned int pixelformat);
> +
> +private:
> +	unsigned int id_;
> +	unsigned int width_;
> +	unsigned int height_;
> +	unsigned int pixelformat_;

With accessors for width_, height_ and pixelformat_ that expose the
fields directly, how about just making those three fields public ? The
StreamConfiguration is more of a data structure that groups stream
parameters without much associated processing than a real object with
methods.

> +};
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_STREAM_H__ */
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index 3b44e834ee02b35a..e40756260c5768f3 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -63,4 +63,68 @@ Stream::Stream(unsigned int id)
>   * \return The stream ID
>   */
>  
> +/**
> + * \class StreamConfiguration
> + * \brief Stream configuration object

Maybe "Configuration parameters for a stream" ?

> + *
> + * The StreamConfiguration class is a model of all information which can be
> + * configured for a single video stream. A application should acquire a all
> + * Stream object from a camera, select the ones it wish to use, create a
> + * StreamConfiguration for each of those streams, set the desired parameter on
> + * the objects and feed them back to the camera object to configure the camera.

I think we should just describe here the StreamConfiguration class, and
explain how it interacts with Stream and Camera in the Camera
documentation.

> + */
> +
> +/**
> + * \fn StreamConfiguration::id()
> + * \brief Retrieve the streams ID

s/streams/stream/

> + * \return The stream ID
> + */
> +
> +/**
> + * \fn StreamConfiguration::width()
> + * \brief Retrieve the Stream width

s/Stream/stream/

We should describe what the stream width (and height and pixel format
below) is.

> + * \return The stream width
> + */
> +
> +/**
> + * \fn StreamConfiguration::height()
> + * \brief Retrieve the Stream height

Same here.

> + * \return The stream height
> + */
> +
> +/**
> + * \fn StreamConfiguration::pixelformat()
> + * \brief Retrieve the Stream pixelformat

And here too.

> + * \return The stream pixelformat
> + */
> +
> +/**
> + * \brief Set desired width and height for the stream
> + * \param[in] width The desired width of the stream
> + * \param[in] height The desired height of the stream
> + */
> +void StreamConfiguration::setDimension(unsigned int width, unsigned int height)
> +{
> +	width_ = width;
> +	height_ = height;
> +}
> +
> +/**
> + * \brief Set desired pixelformat for the stream
> + * \param[in] pixelformat The desired pixelformat of the stream
> + */
> +void StreamConfiguration::setPixelFormat(unsigned int pixelformat)
> +{
> +	pixelformat_ = pixelformat;
> +}
> +
> +/**
> + * \brief Create a new configuration container for a stream
> + * \param[in] stream The stream object the configuration object should act on
> + */
> +StreamConfiguration::StreamConfiguration(class Stream &stream)
> +	: id_(stream.id()), width_(0), height_(0), pixelformat_(0)
> +{
> +}

Please order functions as in the class definition.

> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart