[libcamera-devel] [RFC 4/5] libcamera: stream: Add basic stream usage hints definitions

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tue, Apr 02, 2019 at 02:53:31AM +0200, Niklas Söderlund wrote:
> In preparation of reworking how a default configuration is retrieved
> from a camera add stream usage hints. The hints will be used by
> applications to describe how it intends to use a camera and replace the
> Stream IDs 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 | 39 ++++++++++++++++++
>  src/libcamera/stream.cpp   | 84 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 123 insertions(+)
> 
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 3e8e83a2ff245879..445b80de66217934 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -35,7 +35,46 @@ private:
>  	StreamConfiguration configuration_;
>  };
>  
> +class StreamHint

Should we name this StreamUsage instead ? I'm not sure the word "hint"
is a really good fit for the API, as these are more than hints. In any
case it would be usage hints, so I think StreamUsage is better than
StreamHint, and StreamUsageHint seems a bit too long.

> +{
> +public:
> +	enum Type {

Maybe Role instead or Type ?

> +		Viewfinder,
> +		Video,
> +		Still,

You could go for VideoRecording and StillCapture, to be a bit more
descriptive.

> +	};
>  
> +	Type type() const { return type_; }
> +
> +protected:
> +	StreamHint(Type type);
> +	StreamHint(Type type, StreamConfiguration hints);

Doesn't this abuse the StreamConfiguration class a bit, shouldn't we use
a size (or possibly format) class ?

> +
> +private:
> +	Type type_;
> +	StreamConfiguration hints_;
> +};
> +
> +class Viewfinder : public StreamHint
> +{
> +public:
> +	Viewfinder(unsigned int width, unsigned int height);
> +
> +private:
> +	static StreamConfiguration initHints(unsigned int width, unsigned int height);

A bit overkill in my opinion, I don't think we need this helper.

> +};
> +
> +class Video : public StreamHint
> +{
> +public:
> +	Video();
> +};
> +
> +class Still : public StreamHint
> +{
> +public:
> +	Still();
> +};

Those are very generic names, and may lead to namespace clashes. You
could isolate them in a separate namespace (possibly moving them to the
Stream class), or rename them to StreamUsageViewfinder,
StreamUsageVideoRecording and StreamUsageStillCapture.

I like this overall, but I wonder if it's really worth subclassing
StreamHint.

>  
>  } /* namespace libcamera */
>  
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index c4943c91b2e6ce13..32f26a1f8e6adb2c 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -102,4 +102,88 @@ Stream::Stream()
>   * \return The active configuration of the stream
>   */
>  
> +/**
> + * \class StreamHint
> + * \brief Stream usage hint information
> + *
> + * The StreamHint class carries information about stream usage hints from the
> + * application to a specific pipeline handler implementation. The pipeline
> + * handler shall take the usage hints into account when select which stream
> + * to use for the desired operation.
> + */
> +
> +/**
> + * \enum StreamHint::Type
> + * \brief List of types of usage hints
> + */
> +
> +/**
> + * \fn StreamHint::type()
> + * \brief Retrieve the usage hint type
> + * \return The hint type
> + */
> +
> +/**
> + * \brief Create a stream hint
> + * \param[in] type Stream hint type
> + */
> +StreamHint::StreamHint(Type type)
> +	: type_(type), hints_()
> +{
> +}
> +
> +/**
> + * \brief Create a stream hint with parameters
> + * \param[in] type Stream hint type
> + * \param[in] hints Stream hint parameters
> + */
> +StreamHint::StreamHint(Type type, StreamConfiguration hints)
> +	: type_(type), hints_(hints)
> +{
> +}
> +
> +/**
> + * \class Viewfinder
> + * \brief Stream usage hint for viewfinder
> + */
> +
> +/**
> + * \brief Create a viewfinder stream hint
> + * \param[in] width Desired view finder width
> + * \param[in] height Desired view finder height
> + */
> +Viewfinder::Viewfinder(unsigned int width, unsigned int height)
> +	: StreamHint(Type::Viewfinder, initHints(width, height))
> +{
> +}
> +
> +StreamConfiguration Viewfinder::initHints(unsigned int width,
> +					  unsigned int height)
> +{
> +	StreamConfiguration hints = {};
> +
> +	hints.width = width;
> +	hints.height = height;
> +
> +	return hints;
> +}
> +
> +/**
> + * \class Video
> + * \brief Stream usage hint for video
> + */
> +Video::Video()
> +	: StreamHint(Type::Video)
> +{
> +}
> +
> +/**
> + * \class Still
> + * \brief Stream usage hint for still images
> + */
> +Still::Still()
> +	: StreamHint(Type::Still)
> +{
> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart