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

Tue Jan 29 14:09:47 CET 2019

Hi Kieran,

Thanks for your feedback.

On 2019-01-29 09:45:22 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 29/01/2019 02:00, 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 the Stream class is empty as capabilities are yet to be
> > added. The class is still useful as it will be used to communicate how
> > many streams a Camera object provides.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> Some queries about capitalisation in the Doxygen - but I'm you can
> handle that however you see fit.

I have tried to no capitalise them as I think the text flows nicer. I 
have no strong feelings in this area and are open to change if that is 
the consensus. I have left them as is for now.

> 
> --
> Kieran
> 
> 
> > ---
> >  include/libcamera/libcamera.h |  1 +
> >  include/libcamera/meson.build |  1 +
> >  include/libcamera/stream.h    | 18 +++++++++++++
> >  src/libcamera/meson.build     |  1 +
> >  src/libcamera/stream.cpp      | 51 +++++++++++++++++++++++++++++++++++
> >  5 files changed, 72 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..4f47d85ed6382b36
> > --- /dev/null
> > +++ b/include/libcamera/stream.h
> > @@ -0,0 +1,18 @@
> > +/* 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
> > +{
> > +};
> > +
> > +} /* 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..01f4e5008af8ac46
> > --- /dev/null
> > +++ b/src/libcamera/stream.cpp
> > @@ -0,0 +1,51 @@
> > +/* 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
> 
> Should we capitalise 'camera' as it's the name of one of our classes?
> 
> 
> 
> > + * 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
> 
> Same here ? 'Camera'?
> 
> > + * 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
> 
> Stream? on the same basis?
> 
> > + * 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
> > + * with a single video stream. Stream are exposed by the Camera object they
> 
> s/video stream/video Stream/ ?
> 
> s/Stream are exposed by/Streams are exposed by/

Thanks!

> 
> > + * belong to.
> > + *
> > + * Some cameras are capable of supplying more then one stream from the same
> 
> Some Cameras, ... one Stream ?

s/Some//.

> 
> > + * 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.
> 
> I agree - even just for abstraction - it's better to get this in soon to
> model the layouts.
> 
> 
> > + */
> > +
> > +} /* namespace libcamera */
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund