[libcamera-devel] [PATCH v3 1/6] libcamera: stream: add initial Stream class

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-01-28 00:19:40 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sun, Jan 27, 2019 at 01:22:03AM +0100, Niklas Söderlund wrote:
> > Add an initial Stream implementation. The idea is that once capability
> > support is added to the library each stream will describe its
> > capabilities using this class. An application will then select one or
> > more streams based on these capabilities and use them to configure the
> > camera and capture.
> > 
> > At this stage all the Stream class provides is a way for a Camera object
> > to communicate which streams it provides.
> > 
> > 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      | 67 +++++++++++++++++++++++++++++++++++
> >  5 files changed, 95 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..580f2cc8d3a3ad59
> > --- /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 - Video stream for a Camera
> > + */
> > +#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..307de3710d0ac6b1
> > --- /dev/null
> > +++ b/src/libcamera/stream.cpp
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * stream.cpp - Video stream for a Camera
> > + */
> > +
> > +#include <libcamera/stream.h>
> > +
> > +/**
> > + * \file stream.h
> > + * \brief Video stream for a Camera
> > + *
> > + * 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.
> > + *
> > + * 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 Video stream for a camera
> > + *
> > + * The Stream class models of all static information which are associated
> 
> s/of all/all/ ?

Yes, thanks.

> 
> > + * with a single video stream. 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 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 itself is productive as it allows applications to configure and
> > + * capture from one or more streams even if they won't be able to select the
> > + * optimal stream for the task.
> > + */
> > +
> > +/**
> > + * \brief Create a new stream with a ID
> 
> s/a ID/an ID/

I thought I found all of them after your good grammar lesson in v2 :-)

> 
> > + * \param[in] id Numerical ID which should be unique for the camera device the
> > + * stream belongs to
> 
> I think we should document the constraints on stream IDs (for instance
> contiguous IDs starting at 0, but that might not be the best option).
> This can be done in a subsequent patch as the Stream API will still
> evolve a lot.

So far the only constraint is that they need to be unique within a 
camera. I agree it might be a good idea that they should be contiguous 
starting from 0, but any integer works with the current implementation.  
Lets discuss this as the stream API evolves.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > + */
> > +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