[libcamera-devel,v2,01/12] libcamera: Add Camera class

96 diff mbox series

Provide a Camera class which represents our main interface to handling
camera devices. This is a rework of Kieran's initial proposal and
Laurent's documentation of the file changed to fit the device
enumerators needs.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
* Changes v1
- Fix missing /* namespace libcamera */ comment, thanks Jacopo.
- Removed Debug messages from Camera class constructor and destructor.
- Reworded documentation for Camera::get() and Camera::put(), thanks
  Laurent.
---
 include/libcamera/camera.h    | 31 ++++++++++++
 include/libcamera/libcamera.h |  2 +
 include/libcamera/meson.build |  1 +
 src/libcamera/camera.cpp      | 89 +++++++++++++++++++++++++++++++++++
 src/libcamera/meson.build     |  1 +
 5 files changed, 124 insertions(+)
 create mode 100644 include/libcamera/camera.h
 create mode 100644 src/libcamera/camera.cpp

Hi Niklas,
   thanks for the patch

On Sat, Dec 29, 2018 at 04:28:44AM +0100, Niklas Söderlund wrote:
> Provide a Camera class which represents our main interface to handling
> camera devices. This is a rework of Kieran's initial proposal and
> Laurent's documentation of the file changed to fit the device
> enumerators needs.
>

My comments on v1 should be discarded. This class WILL grow, and we
can keep methods implementation in the cpp file. However, I have two
questions,

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> ---
> * Changes v1
> - Fix missing /* namespace libcamera */ comment, thanks Jacopo.
> - Removed Debug messages from Camera class constructor and destructor.
> - Reworded documentation for Camera::get() and Camera::put(), thanks
>   Laurent.
> ---
>  include/libcamera/camera.h    | 31 ++++++++++++
>  include/libcamera/libcamera.h |  2 +
>  include/libcamera/meson.build |  1 +
>  src/libcamera/camera.cpp      | 89 +++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |  1 +
>  5 files changed, 124 insertions(+)
>  create mode 100644 include/libcamera/camera.h
>  create mode 100644 src/libcamera/camera.cpp
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> new file mode 100644
> index 0000000000000000..9a7579d61fa331ee
> --- /dev/null
> +++ b/include/libcamera/camera.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * camera.h - Camera object interface
> + */
> +#ifndef __LIBCAMERA_CAMERA_H__
> +#define __LIBCAMERA_CAMERA_H__
> +
> +#include <string>
> +
> +namespace libcamera {
> +
> +class Camera
> +{
> +public:
> +	Camera(const std::string &name);
> +
> +	const std::string &name() const;
> +	void get();
> +	void put();
> +
> +private:
> +	virtual ~Camera() { };
> +	int ref_;
> +	std::string name_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CAMERA_H__ */
> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> index 790771b61e41e123..44c094d92feed5ba 100644
> --- a/include/libcamera/libcamera.h
> +++ b/include/libcamera/libcamera.h
> @@ -7,6 +7,8 @@
>  #ifndef __LIBCAMERA_LIBCAMERA_H__
>  #define __LIBCAMERA_LIBCAMERA_H__
>
> +#include <libcamera/camera.h>
> +
>  namespace libcamera {
>
>  class libcamera
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 8c82675a25d29913..9b266ad926681db9 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -1,4 +1,5 @@
>  libcamera_api = files([
> +    'camera.h',
>      'libcamera.h',
>  ])
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> new file mode 100644
> index 0000000000000000..6da2b20137d45da2
> --- /dev/null
> +++ b/src/libcamera/camera.cpp
> @@ -0,0 +1,89 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * camera.cpp - Camera device
> + */
> +
> +#include <libcamera/camera.h>
> +
> +#include "log.h"
> +
> +/**
> + * \file camera.h
> + * \brief Camera device handling
> + *
> + * At the core of libcamera is the camera device, combining one image source
> + * with processing hardware able to provide one or multiple image streams. The
> + * Camera class represents a camera device.
> + *
> + * A camera device contains a single image source, and separate camera device
> + * instances relate to different image sources. For instance, a phone containing
> + * front and back image sensors will be modelled with two camera devices, one
> + * for each sensor. When multiple streams can be produced from the same image
> + * source, all those streams are guaranteed to be part of the same camera
> + * device.
> + *
> + * While not sharing image sources, separate camera devices can share other
> + * system resources, such as an ISP. For this reason camera device instances may
> + * not be fully independent, in which case usage restrictions may apply. For
> + * instance, a phone with a front and a back camera device may not allow usage
> + * of the two devices simultaneously.
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Camera
> + * \brief Camera device
> + *
> + * The Camera class models a camera capable of producing one or more image
> + * streams from a single image source. It provides the main interface to
> + * configuring and controlling the device, and capturing image streams. It is
> + * the central object exposed by libcamera.
> + */
> +
> +/**
> + * \brief Construct a named camera device
> + *
> + * \param[in] name The name to set on the camera device
> + *
> + * The caller is responsible for guaranteeing unicity of the camera
> + * device name.
> + */
> +Camera::Camera(const std::string &name)
> +	: ref_(1), name_(name)
> +{
> +}
> +
> +/**
> + * \brief Retrieve the name of the camera
> + *
> + * \return Name of the camera device
> + */
> +const std::string &Camera::name() const
> +{
> +	return name_;
> +}
> +
> +/**
> + * \brief Acquire a reference to the camera
> + */
> +void Camera::get()
> +{
> +	ref_++;
> +}
> +
> +/**
> + * \brief Release a reference to the camera
> + *
> + * When the last reference is released the camera device is deleted. Callers
> + * shall not access the camera device after calling this function.
> + */
> +void Camera::put()
> +{
> +	if (--ref_ == 0)

Should this be protected to make sure ref_ does not become negative or
is this fine?


> +		delete this;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index f632eb5dd7791ad2..46591069aa5f8beb 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,4 +1,5 @@
>  libcamera_sources = files([
> +    'camera.cpp',
>      'log.cpp',
>      'main.cpp',
>  ])
> --

Should we had headers to the 'libcamera_headers' vector? I see it is
used for generating documentation.

With or without these two addressed
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thanks for your feedback.

On 2018-12-30 10:56:44 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>    thanks for the patch
> 
> On Sat, Dec 29, 2018 at 04:28:44AM +0100, Niklas Söderlund wrote:
> > Provide a Camera class which represents our main interface to handling
> > camera devices. This is a rework of Kieran's initial proposal and
> > Laurent's documentation of the file changed to fit the device
> > enumerators needs.
> >
> 
> My comments on v1 should be discarded. This class WILL grow, and we
> can keep methods implementation in the cpp file. However, I have two
> questions,
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > ---
> > * Changes v1
> > - Fix missing /* namespace libcamera */ comment, thanks Jacopo.
> > - Removed Debug messages from Camera class constructor and destructor.
> > - Reworded documentation for Camera::get() and Camera::put(), thanks
> >   Laurent.
> > ---
> >  include/libcamera/camera.h    | 31 ++++++++++++
> >  include/libcamera/libcamera.h |  2 +
> >  include/libcamera/meson.build |  1 +
> >  src/libcamera/camera.cpp      | 89 +++++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build     |  1 +
> >  5 files changed, 124 insertions(+)
> >  create mode 100644 include/libcamera/camera.h
> >  create mode 100644 src/libcamera/camera.cpp
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > new file mode 100644
> > index 0000000000000000..9a7579d61fa331ee
> > --- /dev/null
> > +++ b/include/libcamera/camera.h
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * camera.h - Camera object interface
> > + */
> > +#ifndef __LIBCAMERA_CAMERA_H__
> > +#define __LIBCAMERA_CAMERA_H__
> > +
> > +#include <string>
> > +
> > +namespace libcamera {
> > +
> > +class Camera
> > +{
> > +public:
> > +	Camera(const std::string &name);
> > +
> > +	const std::string &name() const;
> > +	void get();
> > +	void put();
> > +
> > +private:
> > +	virtual ~Camera() { };
> > +	int ref_;
> > +	std::string name_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_CAMERA_H__ */
> > diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> > index 790771b61e41e123..44c094d92feed5ba 100644
> > --- a/include/libcamera/libcamera.h
> > +++ b/include/libcamera/libcamera.h
> > @@ -7,6 +7,8 @@
> >  #ifndef __LIBCAMERA_LIBCAMERA_H__
> >  #define __LIBCAMERA_LIBCAMERA_H__
> >
> > +#include <libcamera/camera.h>
> > +
> >  namespace libcamera {
> >
> >  class libcamera
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 8c82675a25d29913..9b266ad926681db9 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -1,4 +1,5 @@
> >  libcamera_api = files([
> > +    'camera.h',
> >      'libcamera.h',
> >  ])
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > new file mode 100644
> > index 0000000000000000..6da2b20137d45da2
> > --- /dev/null
> > +++ b/src/libcamera/camera.cpp
> > @@ -0,0 +1,89 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * camera.cpp - Camera device
> > + */
> > +
> > +#include <libcamera/camera.h>
> > +
> > +#include "log.h"
> > +
> > +/**
> > + * \file camera.h
> > + * \brief Camera device handling
> > + *
> > + * At the core of libcamera is the camera device, combining one image source
> > + * with processing hardware able to provide one or multiple image streams. The
> > + * Camera class represents a camera device.
> > + *
> > + * A camera device contains a single image source, and separate camera device
> > + * instances relate to different image sources. For instance, a phone containing
> > + * front and back image sensors will be modelled with two camera devices, one
> > + * for each sensor. When multiple streams can be produced from the same image
> > + * source, all those streams are guaranteed to be part of the same camera
> > + * device.
> > + *
> > + * While not sharing image sources, separate camera devices can share other
> > + * system resources, such as an ISP. For this reason camera device instances may
> > + * not be fully independent, in which case usage restrictions may apply. For
> > + * instance, a phone with a front and a back camera device may not allow usage
> > + * of the two devices simultaneously.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class Camera
> > + * \brief Camera device
> > + *
> > + * The Camera class models a camera capable of producing one or more image
> > + * streams from a single image source. It provides the main interface to
> > + * configuring and controlling the device, and capturing image streams. It is
> > + * the central object exposed by libcamera.
> > + */
> > +
> > +/**
> > + * \brief Construct a named camera device
> > + *
> > + * \param[in] name The name to set on the camera device
> > + *
> > + * The caller is responsible for guaranteeing unicity of the camera
> > + * device name.
> > + */
> > +Camera::Camera(const std::string &name)
> > +	: ref_(1), name_(name)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Retrieve the name of the camera
> > + *
> > + * \return Name of the camera device
> > + */
> > +const std::string &Camera::name() const
> > +{
> > +	return name_;
> > +}
> > +
> > +/**
> > + * \brief Acquire a reference to the camera
> > + */
> > +void Camera::get()
> > +{
> > +	ref_++;
> > +}
> > +
> > +/**
> > + * \brief Release a reference to the camera
> > + *
> > + * When the last reference is released the camera device is deleted. Callers
> > + * shall not access the camera device after calling this function.
> > + */
> > +void Camera::put()
> > +{
> > +	if (--ref_ == 0)
> 
> Should this be protected to make sure ref_ does not become negative or
> is this fine?

No. The user of the library must be able to call cam->put() once it's 
done using a camera.

> 
> 
> > +		delete this;
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index f632eb5dd7791ad2..46591069aa5f8beb 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -1,4 +1,5 @@
> >  libcamera_sources = files([
> > +    'camera.cpp',
> >      'log.cpp',
> >      'main.cpp',
> >  ])
> > --
> 
> Should we had headers to the 'libcamera_headers' vector? I see it is
> used for generating documentation.

Public headers (like camera.h) are added to 'libcamera_api' which like 
'libcamera_headers' for private headers are used to generate the Doxygen 
documentation.

> 
> With or without these two addressed
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks.

> 
> Thanks
>   j
> 
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel