| Message ID | 20190125153340.2744-2-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
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@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 */
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@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
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 + */ +#ifndef __LIBCAMERA_STREAM_H__ +#define __LIBCAMERA_STREAM_H__ + +namespace libcamera { + +class Stream final +{ +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 + */ + +#include <libcamera/stream.h> + +/** + * \file stream.h + * \brief Stream information + * + * 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. + * + * 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 + * + * The Stream class is a model of all static information which are associated + * with a single video stream. A application should acquire Stream objects from + * the camera. + * + * 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. + * + * \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 + * configure and capture from one or more streams even if it won't be able + * to select the optimal stream for the task. + */ + +/** + * \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 + */ +Stream::Stream(unsigned int id) + : id_(id) +{ +} + +/** + * \fn Stream::id() + * \brief Retrieve the streams ID + * \return The stream ID + */ + +} /* namespace libcamera */
Add a extremely simple Stream implementation. The idea is that once capability support is added to the library each stream would describe it's capabilities using this class. A application would then select one or more streams based on these capabilities and using them to configure 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 limits the usefulness of the object to cameras which only provides one stream per camera. Signed-off-by: Niklas Söderlund <niklas.soderlund@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