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

[Jacopo Mondi]

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?


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

With or without these two addressed
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181230/0a160f46/attachment.sig>