[libcamera-devel] [PATCH 4/5] libcamera: stream: Add basic stream roles definitions

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wed, Apr 03, 2019 at 03:12:20AM +0200, Niklas Söderlund wrote:
> In preparation of reworking how a default configuration is retrieved
> from a camera add stream roles. The roles will be used by applications
> to describe how it intends to use a camera and replace the Stream IDs

s/it intends/they intend/

> role when retrieving default configuration from the camera using
> streamConfiguration().
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/stream.h | 41 ++++++++++++++++++
>  src/libcamera/stream.cpp   | 88 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 129 insertions(+)
> 
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 970c479627fab064..adcf20d336347dad 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -21,9 +21,50 @@ struct StreamConfiguration {
>  	unsigned int bufferCount;
>  };
>  
> +class StreamRole
> +{
> +public:
> +	enum Role {
> +		StillCapture,
> +		VideoRecording,
> +		Viewfinder,
> +	};
> +
> +	Role role() const { return role_; }
> +	int width() const { return width_; }
> +	int height() const { return height_; }
> +
> +protected:
> +	StreamRole(Role role);
> +	StreamRole(Role role, int width, int height);
> +
> +private:
> +	Role role_;
> +	int width_;
> +	int height_;

How about using the new Size structure ?

> +};
> +
>  class Stream final
>  {
>  public:
> +	class StillCapture : public StreamRole
> +	{
> +	public:
> +		StillCapture();
> +	};
> +
> +	class VideoRecording : public StreamRole
> +	{
> +	public:
> +		VideoRecording();
> +	};
> +
> +	class Viewfinder : public StreamRole
> +	{
> +	public:
> +		Viewfinder(int width, int height);
> +	};
> +
>  	Stream();
>  	BufferPool &bufferPool() { return bufferPool_; }
>  	const StreamConfiguration &configuration() const { return configuration_; }
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index c4943c91b2e6ce13..f4be5d265e872842 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -60,6 +60,61 @@ namespace libcamera {
>   * \brief Requested number of buffers to allocate for the stream
>   */
>  
> +/**
> + * \class StreamRole
> + * \brief Stream role information
> + *
> + * The StreamRole class carries information about stream usage hints from the
> + * application to the camera. The camera shall take the usage hints into account
> + * when select which stream to use for the desired operation.

I'd like to drop the word "hint" from most of the documentation. How
about

The StreamRole class describes how a stream is intended to be used.
Stream roles are specified by applications and passed to cameras, that
then select the most appropriate streams and their default
configurations.

> + */
> +
> +/**
> + * \enum StreamRole::Role
> + * \brief List of different stream roles

Using the word role to name both the class and the enum makes
documentation a bit more difficult to write. You also need to document
the enum values by the way. How about something like the following ?

/**
 * \enum StreamRole::Role
 * Identify the role a stream is intended to play
 * \var StillCapture
 * The stream is intended to capture high-resolution, high-quality still
 * images with low frame rate. The captured frames may be exposed with
 * flash.
 * \var VideoRecording
 * The stream is intended to capture video for the purpose of recording
 * or streaming. The video stream may produce a high frame rate and may
 * be enhanced with video stabilization.
 * \var Viewfinder
 * The stream is intended to capture video for the purpose of display on
 * the local screen. The StreamRole includes the desired resolution.
 * Trade-offs between quality and usage of system resources are
 * acceptable.
 */

> + */
> +
> +/**
> + * \fn StreamRole::role()
> + * \brief Retrieve the stream role
> + *
> + * \return The stream role hint

s/ hint//

> + */
> +
> +/**
> + * \fn StreamRole::width()
> + * \brief Retrieve desired width
> + *
> + * \return The desired width if defined, -1 otherwise
> + */
> +
> +/**
> + * \fn StreamRole::height()
> + * \brief Retrieve desired height
> + *
> + * \return The desired height if defined, -1 otherwise
> + */

After merging Jacopo's ImgU patch series I think we could use the Size
class, and modify it to use -1 as a marker of invalid dimensions.

> +
> +/**
> + * \brief Create a stream role
> + * \param[in] role Stream role hint

I would like to drop the word hint, but "Stream role" sounds weird. I
wonder if we should rename the StreamRole class to StreamUsage in order
to have two different words, usage and role. Feel free to propose an
alternative for "hint" that wouldn't require such a rename :-)

> + */
> +StreamRole::StreamRole(Role role)
> +	: role_(role), width_(-1), height_(-1)
> +{
> +}
> +
> +/**
> + * \brief Create a stream role with dimension hints
> + * \param[in] role Stream role hint
> + * \param[in] width Desired width
> + * \param[in] height Desired height
> + */
> +StreamRole::StreamRole(Role role, int width, int height)
> +	: role_(role), width_(width), height_(height)
> +{
> +}
> +
>  /**
>   * \class Stream
>   * \brief Video stream for a camera
> @@ -78,6 +133,39 @@ namespace libcamera {
>   * optimal stream for the task.
>   */
>  
> +/**
> + * \class Stream::StillCapture
> + * \brief Describes a still capture role
> + */
> +Stream::StillCapture::StillCapture()
> +	: StreamRole(Role::StillCapture)
> +{
> +}
> +
> +/**
> + * \class Stream::VideoRecording
> + * \brief Describes a video recording role
> + */
> +Stream::VideoRecording::VideoRecording()
> +	: StreamRole(Role::VideoRecording)
> +{
> +}
> +
> +/**
> + * \class Stream::Viewfinder
> + * \brief Describes a viewfinder role
> + */
> +
> +/**
> + * \brief Create a viewfinder role with dimension hints
> + * \param[in] width Desired viewfinder width
> + * \param[in] height Desired viewfinder height
> + */
> +Stream::Viewfinder::Viewfinder(int width, int height)
> +	: StreamRole(Role::Viewfinder, width, height)
> +{
> +}
> +
>  /**
>   * \brief Construct a stream with default parameters
>   */

-- 
Regards,

Laurent Pinchart