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

[Niklas S??derlund]

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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,
Niklas Söderlund