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

[Laurent Pinchart]

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 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 ?

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

-- 
Regards,

Laurent Pinchart