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

[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 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_;
> > > +};
> > > +
> > > +}
> > > +
> > > +#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 at 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