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

[Niklas Söderlund]

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 at ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at 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 at jmondi.org>

Thanks.

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



-- 
Regards,
Niklas Söderlund