[libcamera-devel] [PATCH v2 1/7] libcamera: stream: add basic Stream class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat Jan 26 11:26:32 CET 2019
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
More information about the libcamera-devel
mailing list