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

[Jacopo Mondi]

HI Niklas,

On Wed, Apr 03, 2019 at 09:56:52AM +0300, Laurent Pinchart wrote:
> 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

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.

> > +	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"

> 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.

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 ?
>  * 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 ?

> > +
> > +/**
> > + * \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

Thanks
   j
>
> > + */
> > +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
> _______________________________________________
> 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/20190403/2b55ec05/attachment.sig>