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

77 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>
---
 include/libcamera/camera.h    | 31 +++++++++++
 include/libcamera/libcamera.h |  2 +
 include/libcamera/meson.build |  1 +
 src/libcamera/camera.cpp      | 97 +++++++++++++++++++++++++++++++++++
 src/libcamera/meson.build     |  1 +
 5 files changed, 132 insertions(+)
 create mode 100644 include/libcamera/camera.h
 create mode 100644 src/libcamera/camera.cpp

Hi Niklas,
   I'm going patch-by-patch for minor things

On Sun, Dec 23, 2018 at 12:00:30AM +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.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/camera.h    | 31 +++++++++++
>  include/libcamera/libcamera.h |  2 +
>  include/libcamera/meson.build |  1 +
>  src/libcamera/camera.cpp      | 97 +++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |  1 +
>  5 files changed, 132 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..7622385cc94c11cd
> --- /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);
> +	~Camera();
> +
> +	const std::string &name() const;
> +	void get();
> +	void put();
> +
> +private:
> +	int ref_;
> +	std::string name_;
> +};
> +
> +}

nit: } /* 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>
> +

Why do you need this here?

>  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..a85516876ce79ba4
> --- /dev/null
> +++ b/src/libcamera/camera.cpp
> @@ -0,0 +1,97 @@
> +/* 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)
> +{
> +	LOG(Debug) << "Camera Constructed for " << name_;
> +}
> +
> +Camera::~Camera()
> +{
> +	if (ref_)
> +		LOG(Error) << "Camera Destroyed while still in use!";
> +	else
> +		LOG(Debug) << "Camera Destroyed";
> +}
> +
> +/**
> + * \brief Retrieve the name of the camera
> + *
> + * \return Name of the camera device
> + */
> +const std::string &Camera::name() const
> +{
> +	return name_;
> +}
> +
> +/**
> + * \brief Increase the use count of the camera
> + */
> +void Camera::get()
> +{
> +	ref_++;
> +}
> +
> +/**
> + * \brief Decreases the use count of the camera.
> + *
> + * When the use count of the camera reaches zero the camera device is deleted.
> + */
> +void Camera::put()
> +{
> +	if (--ref_ == 0)
> +		delete this;
> +}

Do you expect this class to grow? Otherwise, all these methods could
be inlined imo

> +
> +} /* 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',
>  ])
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thanks for your comments.

On 2018-12-24 11:19:48 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>    I'm going patch-by-patch for minor things
> 
> On Sun, Dec 23, 2018 at 12:00:30AM +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.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/camera.h    | 31 +++++++++++
> >  include/libcamera/libcamera.h |  2 +
> >  include/libcamera/meson.build |  1 +
> >  src/libcamera/camera.cpp      | 97 +++++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build     |  1 +
> >  5 files changed, 132 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..7622385cc94c11cd
> > --- /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);
> > +	~Camera();
> > +
> > +	const std::string &name() const;
> > +	void get();
> > +	void put();
> > +
> > +private:
> > +	int ref_;
> > +	std::string name_;
> > +};
> > +
> > +}
> 
> nit: } /* 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>
> > +
> 
> Why do you need this here?

The Camera object will be part of the public API and any application 
should only have to include the top header. Am I'm missing something?

> 
> >  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..a85516876ce79ba4
> > --- /dev/null
> > +++ b/src/libcamera/camera.cpp
> > @@ -0,0 +1,97 @@
> > +/* 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)
> > +{
> > +	LOG(Debug) << "Camera Constructed for " << name_;
> > +}
> > +
> > +Camera::~Camera()
> > +{
> > +	if (ref_)
> > +		LOG(Error) << "Camera Destroyed while still in use!";
> > +	else
> > +		LOG(Debug) << "Camera Destroyed";
> > +}
> > +
> > +/**
> > + * \brief Retrieve the name of the camera
> > + *
> > + * \return Name of the camera device
> > + */
> > +const std::string &Camera::name() const
> > +{
> > +	return name_;
> > +}
> > +
> > +/**
> > + * \brief Increase the use count of the camera
> > + */
> > +void Camera::get()
> > +{
> > +	ref_++;
> > +}
> > +
> > +/**
> > + * \brief Decreases the use count of the camera.
> > + *
> > + * When the use count of the camera reaches zero the camera device is deleted.
> > + */
> > +void Camera::put()
> > +{
> > +	if (--ref_ == 0)
> > +		delete this;
> > +}
> 
> Do you expect this class to grow? Otherwise, all these methods could
> be inlined imo

I'm sure it will grow as it will be the main object for a application to 
interact with. The Camera object will likely be the interface to control 
a camera with, or maybe I'm missing something obvious?

> 
> > +
> > +} /* 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',
> >  ])
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hello,

On Friday, 28 December 2018 04:56:24 EET Niklas S??derlund wrote:
> On 2018-12-24 11:19:48 +0100, Jacopo Mondi wrote:
> > On Sun, Dec 23, 2018 at 12:00:30AM +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.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > > 
> > >  include/libcamera/camera.h    | 31 +++++++++++
> > >  include/libcamera/libcamera.h |  2 +
> > >  include/libcamera/meson.build |  1 +
> > >  src/libcamera/camera.cpp      | 97 +++++++++++++++++++++++++++++++++++
> > >  src/libcamera/meson.build     |  1 +
> > >  5 files changed, 132 insertions(+)
> > >  create mode 100644 include/libcamera/camera.h
> > >  create mode 100644 src/libcamera/camera.cpp

[snip]

> > > 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>
> > > +
> > 
> > Why do you need this here?
> 
> The Camera object will be part of the public API and any application
> should only have to include the top header. Am I'm missing something?

libcamera.h should include all public headers, and serve as a shortcut for 
applications that want to pull in the whole public API. Applications can 
however include the other files separately as needed.

> > >  namespace libcamera {
> > >  
> > >  class libcamera

[snip]

Hi Niklas,

Thank you for the patch.

On Sunday, 23 December 2018 01:00:30 EET 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.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/camera.h    | 31 +++++++++++
>  include/libcamera/libcamera.h |  2 +
>  include/libcamera/meson.build |  1 +
>  src/libcamera/camera.cpp      | 97 +++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |  1 +
>  5 files changed, 132 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..7622385cc94c11cd
> --- /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);
> +	~Camera();
> +
> +	const std::string &name() const;
> +	void get();
> +	void put();
> +
> +private:
> +	int ref_;
> +	std::string name_;
> +};
> +
> +}
> +
> +#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..a85516876ce79ba4
> --- /dev/null
> +++ b/src/libcamera/camera.cpp
> @@ -0,0 +1,97 @@
> +/* 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

Is [in] a standard doxygen construct ? Should we use it through the 
documentation ?

> + *
> + * The caller is responsible for guaranteeing unicity of the camera
> + * device name.
> + */
> +Camera::Camera(const std::string &name)
> +	: ref_(1), name_(name)
> +{
> +	LOG(Debug) << "Camera Constructed for " << name_;

I think logging creation of camera objects, and later their deletion when 
we'll have hotplug support, is useful. I wouldn't put that in the Camera class 
constructor, but in the code that instantiates the object, as it should have 
access to more information. This is camera manager or device enumerator debug 
code in my opinion, and should then be bumped to Log(Info).

> +}
> +
> +Camera::~Camera()
> +{
> +	if (ref_)
> +		LOG(Error) << "Camera Destroyed while still in use!";
> +	else
> +		LOG(Debug) << "Camera Destroyed";
> +}
> +
> +/**
> + * \brief Retrieve the name of the camera
> + *
> + * \return Name of the camera device
> + */
> +const std::string &Camera::name() const
> +{
> +	return name_;
> +}
> +
> +/**
> + * \brief Increase the use count of the camera
> + */
> +void Camera::get()
> +{
> +	ref_++;
> +}
> +
> +/**
> + * \brief Decreases the use count of the camera.
> + *
> + * When the use count of the camera reaches zero the camera device is
> deleted.

How about talking about references instead of use count ?

\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.

And something similar for get().

> + */
> +void Camera::put()
> +{
> +	if (--ref_ == 0)
> +		delete this;

You can make the destructor private, and remove the error message there.

Once fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +}
> +
> +} /* 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',
>  ])

Hi Laurent,

Thanks for your feedback.

On 2018-12-28 19:01:54 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sunday, 23 December 2018 01:00:30 EET 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.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/camera.h    | 31 +++++++++++
> >  include/libcamera/libcamera.h |  2 +
> >  include/libcamera/meson.build |  1 +
> >  src/libcamera/camera.cpp      | 97 +++++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build     |  1 +
> >  5 files changed, 132 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..7622385cc94c11cd
> > --- /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);
> > +	~Camera();
> > +
> > +	const std::string &name() const;
> > +	void get();
> > +	void put();
> > +
> > +private:
> > +	int ref_;
> > +	std::string name_;
> > +};
> > +
> > +}
> > +
> > +#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..a85516876ce79ba4
> > --- /dev/null
> > +++ b/src/libcamera/camera.cpp
> > @@ -0,0 +1,97 @@
> > +/* 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
> 
> Is [in] a standard doxygen construct ? Should we use it through the 
> documentation ?

I think it's a standard construct. I find it useful to mark the 
direction of the parameters but I'm open to dropping them if not all 
find them useful. I leave them in for v2.

> 
> > + *
> > + * The caller is responsible for guaranteeing unicity of the camera
> > + * device name.
> > + */
> > +Camera::Camera(const std::string &name)
> > +	: ref_(1), name_(name)
> > +{
> > +	LOG(Debug) << "Camera Constructed for " << name_;
> 
> I think logging creation of camera objects, and later their deletion when 
> we'll have hotplug support, is useful. I wouldn't put that in the Camera class 
> constructor, but in the code that instantiates the object, as it should have 
> access to more information. This is camera manager or device enumerator debug 
> code in my opinion, and should then be bumped to Log(Info).

Agreed.

> 
> > +}
> > +
> > +Camera::~Camera()
> > +{
> > +	if (ref_)
> > +		LOG(Error) << "Camera Destroyed while still in use!";
> > +	else
> > +		LOG(Debug) << "Camera Destroyed";
> > +}
> > +
> > +/**
> > + * \brief Retrieve the name of the camera
> > + *
> > + * \return Name of the camera device
> > + */
> > +const std::string &Camera::name() const
> > +{
> > +	return name_;
> > +}
> > +
> > +/**
> > + * \brief Increase the use count of the camera
> > + */
> > +void Camera::get()
> > +{
> > +	ref_++;
> > +}
> > +
> > +/**
> > + * \brief Decreases the use count of the camera.
> > + *
> > + * When the use count of the camera reaches zero the camera device is
> > deleted.
> 
> How about talking about references instead of use count ?
> 
> \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.
> 
> And something similar for get().

Agreed.

> 
> > + */
> > +void Camera::put()
> > +{
> > +	if (--ref_ == 0)
> > +		delete this;
> 
> You can make the destructor private, and remove the error message there.

With all error messages moved or removed from the destructor I can 
delete it instead of making it private :-)

> 
> Once fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

> 
> > +}
> > +
> > +} /* 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',
> >  ])
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
>

Hi Niklas,

On Saturday, 29 December 2018 02:29:48 EET Niklas Söderlund wrote:
> On 2018-12-28 19:01:54 +0200, Laurent Pinchart wrote:
> > On Sunday, 23 December 2018 01:00:30 EET 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.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > > 
> > >  include/libcamera/camera.h    | 31 +++++++++++
> > >  include/libcamera/libcamera.h |  2 +
> > >  include/libcamera/meson.build |  1 +
> > >  src/libcamera/camera.cpp      | 97 +++++++++++++++++++++++++++++++++++
> > >  src/libcamera/meson.build     |  1 +
> > >  5 files changed, 132 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..7622385cc94c11cd
> > > --- /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);
> > > +	~Camera();
> > > +
> > > +	const std::string &name() const;
> > > +	void get();
> > > +	void put();
> > > +
> > > +private:
> > > +	int ref_;
> > > +	std::string name_;
> > > +};
> > > +
> > > +}
> > > +
> > > +#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..a85516876ce79ba4
> > > --- /dev/null
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -0,0 +1,97 @@
> > > +/* 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
> > 
> > Is [in] a standard doxygen construct ? Should we use it through the
> > documentation ?
> 
> I think it's a standard construct. I find it useful to mark the
> direction of the parameters but I'm open to dropping them if not all
> find them useful. I leave them in for v2.

I wasn't asking to drop it, but whether we should generalize its usage.

> > > + *
> > > + * The caller is responsible for guaranteeing unicity of the camera
> > > + * device name.
> > > + */
> > > +Camera::Camera(const std::string &name)
> > > +	: ref_(1), name_(name)
> > > +{
> > > +	LOG(Debug) << "Camera Constructed for " << name_;
> > 
> > I think logging creation of camera objects, and later their deletion when
> > we'll have hotplug support, is useful. I wouldn't put that in the Camera
> > class constructor, but in the code that instantiates the object, as it
> > should have access to more information. This is camera manager or device
> > enumerator debug code in my opinion, and should then be bumped to
> > Log(Info).
> 
> Agreed.
> 
> > > +}
> > > +
> > > +Camera::~Camera()
> > > +{
> > > +	if (ref_)
> > > +		LOG(Error) << "Camera Destroyed while still in use!";
> > > +	else
> > > +		LOG(Debug) << "Camera Destroyed";
> > > +}
> > > +
> > > +/**
> > > + * \brief Retrieve the name of the camera
> > > + *
> > > + * \return Name of the camera device
> > > + */
> > > +const std::string &Camera::name() const
> > > +{
> > > +	return name_;
> > > +}
> > > +
> > > +/**
> > > + * \brief Increase the use count of the camera
> > > + */
> > > +void Camera::get()
> > > +{
> > > +	ref_++;
> > > +}
> > > +
> > > +/**
> > > + * \brief Decreases the use count of the camera.
> > > + *
> > > + * When the use count of the camera reaches zero the camera device is
> > > deleted.
> > 
> > How about talking about references instead of use count ?
> > 
> > \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.
> > 
> > And something similar for get().
> 
> Agreed.
> 
> > > + */
> > > +void Camera::put()
> > > +{
> > > +	if (--ref_ == 0)
> > > +		delete this;
> > 
> > You can make the destructor private, and remove the error message there.
> 
> With all error messages moved or removed from the destructor I can
> delete it instead of making it private :-)

You can't as it should be virtual (which I forgot to point out in my review).

> > Once fixed,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks!
> 
> > > +}
> > > +
> > > +} /* 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',
> > >  
> > >  ])