[libcamera-devel] [PATCH v2 5/8] libcamera: stream: Add basic stream usages

[Jacopo Mondi]

Hi Niklas,

On Fri, Apr 05, 2019 at 11:43:20AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2019-04-05 10:26:48 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >    thank you for the patch
> >
> > On Fri, Apr 05, 2019 at 04:02:53AM +0200, Niklas Söderlund wrote:
> > > In preparation of reworking how a default configuration is retrieved
> > > from a camera add stream usages. The usages will be used by applications
> > > to describe how they intend to use a camera and replace the Stream IDs
> > > when retrieving default configuration from the camera using
> > > streamConfiguration().
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  include/libcamera/stream.h | 40 +++++++++++++++++
> > >  src/libcamera/stream.cpp   | 92 ++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 132 insertions(+)
> > >
> > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > > index 970c479627fab064..1dea696f2e5ca95d 100644
> > > --- a/include/libcamera/stream.h
> > > +++ b/include/libcamera/stream.h
> > > @@ -8,6 +8,7 @@
> > >  #define __LIBCAMERA_STREAM_H__
> > >
> > >  #include <libcamera/buffer.h>
> > > +#include <libcamera/geometry.h>
> > >
> > >  namespace libcamera {
> > >
> > > @@ -21,9 +22,48 @@ struct StreamConfiguration {
> > >  	unsigned int bufferCount;
> > >  };
> > >
> > > +class StreamUsage
> > > +{
> > > +public:
> > > +	enum Role {
> > > +		StillCapture,
> > > +		VideoRecording,
> > > +		Viewfinder,
> > > +	};
> > > +
> > > +	Role role() const { return role_; }
> > > +	Size size() const { return size_; }
> >
> > Maybe not that relevant as Size is just two integers, but
> >         Size &size() const
> > or maybe even
> >         const Size &size() const
>
> I have no strong opinion, I will make it so in next version.
>
> >
> > Also
> >         const Role role() const
>
> As this returns a value and not a reference this can not be const :-)

Indeed, sorry for the stupid comment.

Thanks
  j

>
> > > +
> > > +protected:
> > > +	StreamUsage(Role role);
> >
> > explicit
> >
> > > +	StreamUsage(Role role, int width, int height);
> > > +
> > > +private:
> > > +	Role role_;
> > > +	Size size_;
> > > +};
> > > +
> > >  class Stream final
> > >  {
> > >  public:
> > > +	class StillCapture : public StreamUsage
> > > +	{
> > > +	public:
> > > +		StillCapture();
> > > +	};
> > > +
> > > +	class VideoRecording : public StreamUsage
> > > +	{
> > > +	public:
> > > +		VideoRecording();
> > > +	};
> > > +
> > > +	class Viewfinder : public StreamUsage
> > > +	{
> > > +	public:
> > > +		explicit Viewfinder(int width, int height);
> >
> > No need for explicit, it takes two arguments.
>
> Wops, my intention was for this to be on StreamRole, somthing went very
> wrong :-) Thanks for spotting it!
>
> >
> > > +	};
> > > +
> > >  	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..816dff2d12acd923 100644
> > > --- a/src/libcamera/stream.cpp
> > > +++ b/src/libcamera/stream.cpp
> > > @@ -60,6 +60,65 @@ namespace libcamera {
> > >   * \brief Requested number of buffers to allocate for the stream
> > >   */
> > >
> > > +/**
> > > + * \class StreamUsage
> > > + * \brief Stream usage information
> > > + *
> > > + * The StreamUsage class describes how an application intends to use a stream.
> > > + * Usages are specified by applications and passed to cameras, that then select
> > > + * the most appropriate streams and their default configurations.
> > > + */
> > > +
> > > +/**
> > > + * \enum StreamUsage::Role
> > > + * \brief Identify the role a stream is intended to play
> > > + * \var StreamUsage::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 StreamUsage::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 StreamUsage::Viewfinder
> > > + * The stream is intended to capture video for the purpose of display on the
> > > + * local screen. The StreamUsage includes the desired resolution. Trade-offs
> > > + * between quality and usage of system resources are acceptable.
> > > + */
> > > +
> > > +/**
> > > + * \fn StreamUsage::role()
> > > + * \brief Retrieve the stream role
> > > + *
> > > + * \return The stream role
> > > + */
> > > +
> > > +/**
> > > + * \fn StreamUsage::size()
> > > + * \brief Retrieve desired size
> > > + *
> > > + * \return The desired size
> > > + */
> > > +
> > > +/**
> > > + * \brief Create a stream usage
> >
> > should you somehow mention "a stream usage with sizes = 0" ?
>
> No, this should IMHO be explained in the documentation for Size. As
> similar behavior would be true for all instances of Size.
>
> >
> > > + * \param[in] role Stream role
> > > + */
> > > +StreamUsage::StreamUsage(Role role)
> > > +	: role_(role), size_(Size(0, 0))
> >
> > Size default contructor initialize both fields to 0, so you can drop
> > the assignement.
>
> Cool then I will drop it for v3.
>
> >
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Create a stream usage with size hint
> > > + * \param[in] role Stream role
> > > + * \param[in] width Desired width
> > > + * \param[in] height Desired height
> >
> > "The desired" ?
>
> You are correct this seems to be the convention in other places, will do
> so for v3.
>
> >
> > > + */
> > > +StreamUsage::StreamUsage(Role role, int width, int height)
> > > +	: role_(role), size_(Size(width, height))
> > > +{
> > > +}
> > > +
> > >  /**
> > >   * \class Stream
> > >   * \brief Video stream for a camera
> > > @@ -78,6 +137,39 @@ namespace libcamera {
> > >   * optimal stream for the task.
> > >   */
> > >
> > > +/**
> > > + * \class Stream::StillCapture
> > > + * \brief Describe a still capture usage
> > > + */
> > > +Stream::StillCapture::StillCapture()
> > > +	: StreamUsage(Role::StillCapture)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \class Stream::VideoRecording
> > > + * \brief Describe a video recording usage
> > > + */
> > > +Stream::VideoRecording::VideoRecording()
> > > +	: StreamUsage(Role::VideoRecording)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \class Stream::Viewfinder
> > > + * \brief Describe a viewfinder usage
> > > + */
> > > +
> > > +/**
> > > + * \brief Create a viewfinder usage with dimension hints
> >
> > Nit: you have used "size" everywhere in place of dimension if I'm not
> > wrong. I like dimension too though.
>
> I like dimension, no strong feeling will bend to popular opinion else
> keep it as is.
>
> >
> > Thanks
> >    j
> >
> > > + * \param[in] width Desired viewfinder width
> > > + * \param[in] height Desired viewfinder height
> > > + */
> > > +Stream::Viewfinder::Viewfinder(int width, int height)
> > > +	: StreamUsage(Role::Viewfinder, width, height)
> > > +{
> > > +}
> > > +
> > >  /**
> > >   * \brief Construct a stream with default parameters
> > >   */
> > > --
> > > 2.21.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
>
>
> --
> Regards,
> Niklas Söderlund
-------------- 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/20190405/f16aba8f/attachment.sig>