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

Sat Dec 29 01:29:48 CET 2018

Hi Laurent,

Thanks for your feedback.

On 2018-12-28 19:01:54 +0200, Laurent Pinchart wrote:
> 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 ?

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.

> 
> > + *
> > + * 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 :-)

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

-- 
Regards,
Niklas Söderlund