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

[Laurent Pinchart]

Hi Jacopo,

On Wed, Apr 03, 2019 at 03:06:14PM +0200, Jacopo Mondi wrote:
> On Wed, Apr 03, 2019 at 09:56:52AM +0300, Laurent Pinchart wrote:
> > 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
> 
> s/Stream IDs role/Stream IDs/ ?
> 
> >> 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);
> 
> This is protected so it might not matter, but constructors with 1
> parameteres should be marked explicit.

Good point.

> >> +	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.
> 
> or "how an application intends to use a stream"

That's better indeed. The active form usually feels lighter to read.

> > 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
> 
> We are using both third and first person verbs in the documentation.
> Ie. "Identify" here, "Describes" below.

I think the library prefers the first person, so let's fix it below.
> 
> Also, \brief ?
> 
> >  * \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.
> 
> s/The/This ?

I mean that for the Viewfinder role, the StreamRole instance 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
> 
> the desired?
> I would s/desired/requested
> 
> >> + *
> >> + * \return The desired width if defined, -1 otherwise
> 
> ", -1 otherwise" seems like an error condition, while it is the
> default value of an uninitialized width.
> 
> >> + */
> >> +
> >> +/**
> >> + * \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.
> 
> Also, yes. Anway, 0 should not be a valid size, and should be used as
> default, isn't it ?

I would go for -1 to mark an invalid size in general. For an image 0
isn't valid either, but for some other usages it may. 0x0 marks an empty
sizes, while -1x-1 would mark an invalid size.

> >> +
> >> +/**
> >> + * \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 :-)
> 
> +1 for StreamUsage
> 
> >> + */
> >> +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