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

[Laurent Pinchart]

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 ?

> 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