[libcamera-devel] [PATCH v2 1/7] libcamera: stream: add basic Stream class

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback. I'm forever grateful for your grammar lessons.

On 2019-01-26 01:18:04 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Jan 25, 2019 at 04:33:34PM +0100, Niklas Söderlund wrote:
> > Add a extremely simple Stream implementation. The idea is that once
> 
> How about "an initial" or "an initial stub" instead of "a extremely
> simple" ? :-)
> 
> > capability support is added to the library each stream would describe
> 
> s/would/will/
> 
> > it's capabilities using this class. A application would then select one
> 
> s/it's/its/
> s/A/An/
> s/would/will/
> 
> > or more streams based on these capabilities and using them to configure
> 
> s/using/use/
> s/configure/configure the camera/
> 
> > and capture.
> > 
> > At this stage all the Stream class provides is a way for a Camera object
> > to communicate which stream ID it to operates on. This basically
> 
> "it to" ?
> 
> > limits the usefulness of the object to cameras which only provides one
> > stream per camera.
> 
> Why so ? Isn't the object still useful when you have multiple streams,
> to convey stream IDs ?

I replace this paragraph with

  At this stage all the Stream class provides is a way for a Camera 
  object to communicate which stream IDs it provides.

All other comments in this series are addressed according to your 
suggestions, thanks!

> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  include/libcamera/libcamera.h |  1 +
> >  include/libcamera/meson.build |  1 +
> >  include/libcamera/stream.h    | 25 +++++++++++++
> >  src/libcamera/meson.build     |  1 +
> >  src/libcamera/stream.cpp      | 66 +++++++++++++++++++++++++++++++++++
> >  5 files changed, 94 insertions(+)
> >  create mode 100644 include/libcamera/stream.h
> >  create mode 100644 src/libcamera/stream.cpp
> > 
> > diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> > index c0511cf6d662b63f..272dfd5e4a67d5de 100644
> > --- a/include/libcamera/libcamera.h
> > +++ b/include/libcamera/libcamera.h
> > @@ -12,6 +12,7 @@
> >  #include <libcamera/event_dispatcher.h>
> >  #include <libcamera/event_notifier.h>
> >  #include <libcamera/signal.h>
> > +#include <libcamera/stream.h>
> >  #include <libcamera/timer.h>
> >  
> >  #endif /* __LIBCAMERA_LIBCAMERA_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index d7cb55ba4a49e1e8..54a680787e5c17aa 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -5,6 +5,7 @@ libcamera_api = files([
> >      'event_notifier.h',
> >      'libcamera.h',
> >      'signal.h',
> > +    'stream.h',
> >      'timer.h',
> >  ])
> >  
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > new file mode 100644
> > index 0000000000000000..415815ba12c65e47
> > --- /dev/null
> > +++ b/include/libcamera/stream.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * stream.h - Stream object interface
> 
> "Video stream for a Camera" ?
> 
> > + */
> > +#ifndef __LIBCAMERA_STREAM_H__
> > +#define __LIBCAMERA_STREAM_H__
> > +
> > +namespace libcamera {
> > +
> > +class Stream final
> 
> I wonder if we should let pipeline handlers subclass the Stream. I
> suppose allowing Stream subclasses and not Camera subclasses wouldn't be
> a good idea. Maybe it wasn't a good idea to forbid subclasses of Camera
> :-S I suppose the future will tell.
> 
> > +{
> > +public:
> > +	Stream(unsigned int id);
> > +
> > +	unsigned int id() const { return id_; };
> > +
> > +private:
> > +	unsigned int id_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_STREAM_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index f9f25c0ecf1564cc..9f6ff99eebe2f5bc 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -10,6 +10,7 @@ libcamera_sources = files([
> >      'media_object.cpp',
> >      'pipeline_handler.cpp',
> >      'signal.cpp',
> > +    'stream.cpp',
> >      'timer.cpp',
> >      'v4l2_device.cpp',
> >  ])
> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > new file mode 100644
> > index 0000000000000000..3b44e834ee02b35a
> > --- /dev/null
> > +++ b/src/libcamera/stream.cpp
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * stream.cpp - Stream information handeling
> 
> I would use the same description as in stream.cpp
> 
> > + */
> > +
> > +#include <libcamera/stream.h>
> > +
> > +/**
> > + * \file stream.h
> > + * \brief Stream information
> 
> And here too.
> 
> > + *
> > + * A camera device can provide frames in different resolutions and formats
> > + * concurrently from a single image source. The Stream class represents
> > + * one of the multiple concurrent streams format.
> 
> Maybe s/ format// ?
> 
> > + * All streams exposed by a camera device share the same image source and are
> > + * thus not fully independent. Parameters related to the image source, such as
> > + * the exposure time or flash control, are common to all streams. Other
> > + * parameters, such as format or resolution, may be specified per-stream,
> > + * depending on the capabilities of the camera device.
> > + *
> > + * Camera devices expose at least one stream, and may expose additional streams
> > + * based on the device capabilities. This can be used, for instance, to
> > + * implement concurrent viewfinder and video capture, or concurrent viewfinder,
> > + * video capture and still image capture.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class Stream
> > + * \brief Stream information carrier
> 
> "Video stream for a camera" ?
> 
> > + *
> > + * The Stream class is a model of all static information which are associated
> 
> s/is a model/models/
> 
> > + * with a single video stream. A application should acquire Stream objects from
> > + * the camera.
> 
> I'd replace the second sentence with "Stream are exposed by the Camera
> object they belong to." 
> 
> > + *
> > + * Some cameras are capable of supplying more then one stream from the same
> > + * video source. In such cases a application will receive a array of streams to
> > + * inspect and select from to best fit its use-case.
> 
> "In such cases an application can inspect all available streams and
> select the ones that best fit its use case.
> 
> > + *
> > + * \todo Add capabilities to the Stream API. Without this the Stream class
> > + *	 only serves to reveal how many streams of unknown capabilities a camera
> > + *	 supports. This in it self is productive as it allows applications to
> 
> s/it self/itself/
> 
> > + *	 configure and capture from one or more streams even if it won't be able
> 
> s/it/they/
> 
> > + *	 to select the optimal stream for the task.
> 
> No need for indentation I think.
> 
> > + */
> > +
> > +/**
> > + * \brief Create a new stream with a ID
> > + * \param[in] id Numerical ID which should be unique for the camera device the stream belongs to
> 
> Line wrap ?
> 
> > + */
> > +Stream::Stream(unsigned int id)
> > +	: id_(id)
> > +{
> > +}
> > +
> > +/**
> > + * \fn Stream::id()
> > + * \brief Retrieve the streams ID
> > + * \return The stream ID
> > + */
> > +
> > +} /* namespace libcamera */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund