[{"id":78,"web_url":"https://patchwork.libcamera.org/comment/78/","msgid":"<20181224101948.GB2364@archlinux>","date":"2018-12-24T10:19:48","subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n I'm going patch-by-patch for minor things\n\nOn Sun, Dec 23, 2018 at 12:00:30AM +0100, Niklas S??derlund wrote:\n> Provide a Camera class which represents our main interface to handling\n> camera devices. This is a rework of Kieran's initial proposal and\n> Laurent's documentation of the file changed to fit the device\n> enumerators needs.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera.h | 31 +++++++++++\n> include/libcamera/libcamera.h | 2 +\n> include/libcamera/meson.build | 1 +\n> src/libcamera/camera.cpp | 97 +++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 5 files changed, 132 insertions(+)\n> create mode 100644 include/libcamera/camera.h\n> create mode 100644 src/libcamera/camera.cpp\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> new file mode 100644\n> index 0000000000000000..7622385cc94c11cd\n> --- /dev/null\n> +++ b/include/libcamera/camera.h\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * camera.h - Camera object interface\n> + */\n> +#ifndef __LIBCAMERA_CAMERA_H__\n> +#define __LIBCAMERA_CAMERA_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Camera\n> +{\n> +public:\n> +\tCamera(const std::string &name);\n> +\t~Camera();\n> +\n> +\tconst std::string &name() const;\n> +\tvoid get();\n> +\tvoid put();\n> +\n> +private:\n> +\tint ref_;\n> +\tstd::string name_;\n> +};\n> +\n> +}\n\nnit: } /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CAMERA_H__ */\n> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> index 790771b61e41e123..44c094d92feed5ba 100644\n> --- a/include/libcamera/libcamera.h\n> +++ b/include/libcamera/libcamera.h\n> @@ -7,6 +7,8 @@\n> #ifndef __LIBCAMERA_LIBCAMERA_H__\n> #define __LIBCAMERA_LIBCAMERA_H__\n>\n> +#include \n> +\n\nWhy do you need this here?\n\n> namespace libcamera {\n>\n> class libcamera\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 8c82675a25d29913..9b266ad926681db9 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -1,4 +1,5 @@\n> libcamera_api = files([\n> + 'camera.h',\n> 'libcamera.h',\n> ])\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> new file mode 100644\n> index 0000000000000000..a85516876ce79ba4\n> --- /dev/null\n> +++ b/src/libcamera/camera.cpp\n> @@ -0,0 +1,97 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * camera.cpp - Camera device\n> + */\n> +\n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file camera.h\n> + * \\brief Camera device handling\n> + *\n> + * At the core of libcamera is the camera device, combining one image source\n> + * with processing hardware able to provide one or multiple image streams. The\n> + * Camera class represents a camera device.\n> + *\n> + * A camera device contains a single image source, and separate camera device\n> + * instances relate to different image sources. For instance, a phone containing\n> + * front and back image sensors will be modelled with two camera devices, one\n> + * for each sensor. When multiple streams can be produced from the same image\n> + * source, all those streams are guaranteed to be part of the same camera\n> + * device.\n> + *\n> + * While not sharing image sources, separate camera devices can share other\n> + * system resources, such as an ISP. For this reason camera device instances may\n> + * not be fully independent, in which case usage restrictions may apply. For\n> + * instance, a phone with a front and a back camera device may not allow usage\n> + * of the two devices simultaneously.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class Camera\n> + * \\brief Camera device\n> + *\n> + * The Camera class models a camera capable of producing one or more image\n> + * streams from a single image source. It provides the main interface to\n> + * configuring and controlling the device, and capturing image streams. It is\n> + * the central object exposed by libcamera.\n> + */\n> +\n> +/**\n> + * \\brief Construct a named camera device\n> + *\n> + * \\param[in] name The name to set on the camera device\n> + *\n> + * The caller is responsible for guaranteeing unicity of the camera\n> + * device name.\n> + */\n> +Camera::Camera(const std::string &name)\n> +\t: ref_(1), name_(name)\n> +{\n> +\tLOG(Debug) << \"Camera Constructed for \" << name_;\n> +}\n> +\n> +Camera::~Camera()\n> +{\n> +\tif (ref_)\n> +\t\tLOG(Error) << \"Camera Destroyed while still in use!\";\n> +\telse\n> +\t\tLOG(Debug) << \"Camera Destroyed\";\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the name of the camera\n> + *\n> + * \\return Name of the camera device\n> + */\n> +const std::string &Camera::name() const\n> +{\n> +\treturn name_;\n> +}\n> +\n> +/**\n> + * \\brief Increase the use count of the camera\n> + */\n> +void Camera::get()\n> +{\n> +\tref_++;\n> +}\n> +\n> +/**\n> + * \\brief Decreases the use count of the camera.\n> + *\n> + * When the use count of the camera reaches zero the camera device is deleted.\n> + */\n> +void Camera::put()\n> +{\n> +\tif (--ref_ == 0)\n> +\t\tdelete this;\n> +}\n\nDo you expect this class to grow? Otherwise, all these methods could\nbe inlined imo\n\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index f632eb5dd7791ad2..46591069aa5f8beb 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,4 +1,5 @@\n> libcamera_sources = files([\n> + 'camera.cpp',\n> 'log.cpp',\n> 'main.cpp',\n> ])\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ADB6A60B0B\n\tfor ;\n\tMon, 24 Dec 2018 11:19:50 +0100 (CET)","from archlinux (host54-51-dynamic.16-87-r.retail.telecomitalia.it\n\t[87.16.51.54]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id B12A124000F;\n\tMon, 24 Dec 2018 10:19:49 +0000 (UTC)"],"Date":"Mon, 24 Dec 2018 11:19:48 +0100","From":"Jacopo Mondi ","To":"Niklas S??derlund ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181224101948.GB2364@archlinux>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"LpQ9ahxlCli8rRTG\"","Content-Disposition":"inline","In-Reply-To":"<20181222230041.29999-2-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Dec 2018 10:19:50 -0000"}},{"id":85,"web_url":"https://patchwork.libcamera.org/comment/85/","msgid":"<20181228025624.GO19796@bigcity.dyn.berto.se>","date":"2018-12-28T02:56:24","subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your comments.\n\nOn 2018-12-24 11:19:48 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> I'm going patch-by-patch for minor things\n> \n> On Sun, Dec 23, 2018 at 12:00:30AM +0100, Niklas S??derlund wrote:\n> > Provide a Camera class which represents our main interface to handling\n> > camera devices. This is a rework of Kieran's initial proposal and\n> > Laurent's documentation of the file changed to fit the device\n> > enumerators needs.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/camera.h | 31 +++++++++++\n> > include/libcamera/libcamera.h | 2 +\n> > include/libcamera/meson.build | 1 +\n> > src/libcamera/camera.cpp | 97 +++++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 5 files changed, 132 insertions(+)\n> > create mode 100644 include/libcamera/camera.h\n> > create mode 100644 src/libcamera/camera.cpp\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > new file mode 100644\n> > index 0000000000000000..7622385cc94c11cd\n> > --- /dev/null\n> > +++ b/include/libcamera/camera.h\n> > @@ -0,0 +1,31 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * camera.h - Camera object interface\n> > + */\n> > +#ifndef __LIBCAMERA_CAMERA_H__\n> > +#define __LIBCAMERA_CAMERA_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class Camera\n> > +{\n> > +public:\n> > +\tCamera(const std::string &name);\n> > +\t~Camera();\n> > +\n> > +\tconst std::string &name() const;\n> > +\tvoid get();\n> > +\tvoid put();\n> > +\n> > +private:\n> > +\tint ref_;\n> > +\tstd::string name_;\n> > +};\n> > +\n> > +}\n> \n> nit: } /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_CAMERA_H__ */\n> > diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> > index 790771b61e41e123..44c094d92feed5ba 100644\n> > --- a/include/libcamera/libcamera.h\n> > +++ b/include/libcamera/libcamera.h\n> > @@ -7,6 +7,8 @@\n> > #ifndef __LIBCAMERA_LIBCAMERA_H__\n> > #define __LIBCAMERA_LIBCAMERA_H__\n> >\n> > +#include \n> > +\n> \n> Why do you need this here?\n\nThe Camera object will be part of the public API and any application \nshould only have to include the top header. Am I'm missing something?\n\n> \n> > namespace libcamera {\n> >\n> > class libcamera\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 8c82675a25d29913..9b266ad926681db9 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -1,4 +1,5 @@\n> > libcamera_api = files([\n> > + 'camera.h',\n> > 'libcamera.h',\n> > ])\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > new file mode 100644\n> > index 0000000000000000..a85516876ce79ba4\n> > --- /dev/null\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -0,0 +1,97 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * camera.cpp - Camera device\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +\n> > +/**\n> > + * \\file camera.h\n> > + * \\brief Camera device handling\n> > + *\n> > + * At the core of libcamera is the camera device, combining one image source\n> > + * with processing hardware able to provide one or multiple image streams. The\n> > + * Camera class represents a camera device.\n> > + *\n> > + * A camera device contains a single image source, and separate camera device\n> > + * instances relate to different image sources. For instance, a phone containing\n> > + * front and back image sensors will be modelled with two camera devices, one\n> > + * for each sensor. When multiple streams can be produced from the same image\n> > + * source, all those streams are guaranteed to be part of the same camera\n> > + * device.\n> > + *\n> > + * While not sharing image sources, separate camera devices can share other\n> > + * system resources, such as an ISP. For this reason camera device instances may\n> > + * not be fully independent, in which case usage restrictions may apply. For\n> > + * instance, a phone with a front and a back camera device may not allow usage\n> > + * of the two devices simultaneously.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class Camera\n> > + * \\brief Camera device\n> > + *\n> > + * The Camera class models a camera capable of producing one or more image\n> > + * streams from a single image source. It provides the main interface to\n> > + * configuring and controlling the device, and capturing image streams. It is\n> > + * the central object exposed by libcamera.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a named camera device\n> > + *\n> > + * \\param[in] name The name to set on the camera device\n> > + *\n> > + * The caller is responsible for guaranteeing unicity of the camera\n> > + * device name.\n> > + */\n> > +Camera::Camera(const std::string &name)\n> > +\t: ref_(1), name_(name)\n> > +{\n> > +\tLOG(Debug) << \"Camera Constructed for \" << name_;\n> > +}\n> > +\n> > +Camera::~Camera()\n> > +{\n> > +\tif (ref_)\n> > +\t\tLOG(Error) << \"Camera Destroyed while still in use!\";\n> > +\telse\n> > +\t\tLOG(Debug) << \"Camera Destroyed\";\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the name of the camera\n> > + *\n> > + * \\return Name of the camera device\n> > + */\n> > +const std::string &Camera::name() const\n> > +{\n> > +\treturn name_;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Increase the use count of the camera\n> > + */\n> > +void Camera::get()\n> > +{\n> > +\tref_++;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Decreases the use count of the camera.\n> > + *\n> > + * When the use count of the camera reaches zero the camera device is deleted.\n> > + */\n> > +void Camera::put()\n> > +{\n> > +\tif (--ref_ == 0)\n> > +\t\tdelete this;\n> > +}\n> \n> Do you expect this class to grow? Otherwise, all these methods could\n> be inlined imo\n\nI'm sure it will grow as it will be the main object for a application to \ninteract with. The Camera object will likely be the interface to control \na camera with, or maybe I'm missing something obvious?\n\n> \n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index f632eb5dd7791ad2..46591069aa5f8beb 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -1,4 +1,5 @@\n> > libcamera_sources = files([\n> > + 'camera.cpp',\n> > 'log.cpp',\n> > 'main.cpp',\n> > ])\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F548600CC\n\tfor ;\n\tFri, 28 Dec 2018 03:56:26 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id t18-v6so17660618ljd.4\n\tfor ;\n\tThu, 27 Dec 2018 18:56:26 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tr26-v6sm8432441lji.25.2018.12.27.18.56.25\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 27 Dec 2018 18:56:25 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=wIN7dOmDvFOa3RmAAh0uBLBGGwH3nBZGZZxSaHq5cKQ=;\n\tb=F3Z8l9y6CRFD0GsVrCbW606lOrul4AmEXG0nF5LKEy9o0LdEJfo1Xerp2X4jEElmbM\n\tioBk6HXR0hkVfFn0hwMbXqu1FByipOJjZEwzMNhU2BFkSQKrP7qBd6HSPKNza0+fiMMD\n\tPa5qeKsZE1zoa7KBBilfcDBeoPdJgAwJuUusI8wkN7SA8Pzy1yhtl6PhcN1o6uvWqppS\n\tSpGa2WhvjkCzUwDLCVWO/OdwN/ac/7W3LGyEPgovKK77WF6+HbwKM9w/pvkgcuYBycHL\n\tIGGVXc9n0h0RKr4suHIhYOD8noosrjqrFwloD3RotOfnFeHXPMGtaWXs5GYJFbW9wL21\n\tLyzw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=wIN7dOmDvFOa3RmAAh0uBLBGGwH3nBZGZZxSaHq5cKQ=;\n\tb=n5VByEPk4akt6oYJURPFI2wTYhB+fjnFaYdo3hAlhTAyTzgD2axobQyfz/ylgY5XdN\n\tEVbd1lLAsSZDJzE7MOk7XMIinFF3zhVrF6HTja6lMyYmy3xgIlb2kGkuF1nVNCABKKKC\n\tyDGOqCGfEOn7NGHkDRBS3/rkuE4ZRTjQxDVKWpX7iAcmcT40dAfbjRfV9BjUj8SYkIvv\n\tsZIqqYlCrjB7Rs6Q1XdCkoK3ddKRwo6Ng5A2okSm/y8siPe6SCNq6DGB9iwrFEkEqgvZ\n\tRzLu2vN5zusqYLNJjmzGKHBbn2C3PdTu9YNvBYrZb7eg8EgSIiIisJZuerFkeGz1B0VX\n\tAEIg==","X-Gm-Message-State":"AJcUukfotEiP9ox1iQAqJBO8hhmkYEqjXHL0/6IjrKuTcG8yf7vAue4i\n\tTfZExEX0nzLA359QYxOVlXjuZuMOXKY=","X-Google-Smtp-Source":"ALg8bN42fGdZgy30+Nw0dYvyaae3ZYo+peojx5URP4izCR6Ys2HkkdKaDp3pmhAC4CN1AWwWrEHprw==","X-Received":"by 2002:a2e:9181:: with SMTP id\n\tf1-v6mr14674292ljg.64.1545965785836; \n\tThu, 27 Dec 2018 18:56:25 -0800 (PST)","Date":"Fri, 28 Dec 2018 03:56:24 +0100","From":"Niklas S??derlund ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181228025624.GO19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-2-niklas.soderlund@ragnatech.se>\n\t<20181224101948.GB2364@archlinux>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20181224101948.GB2364@archlinux>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 02:56:26 -0000"}},{"id":100,"web_url":"https://patchwork.libcamera.org/comment/100/","msgid":"<1556289.MHxoJoZQBd@avalon>","date":"2018-12-28T16:46:59","subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Friday, 28 December 2018 04:56:24 EET Niklas S??derlund wrote:\n> On 2018-12-24 11:19:48 +0100, Jacopo Mondi wrote:\n> > On Sun, Dec 23, 2018 at 12:00:30AM +0100, Niklas S??derlund wrote:\n> > > Provide a Camera class which represents our main interface to handling\n> > > camera devices. This is a rework of Kieran's initial proposal and\n> > > Laurent's documentation of the file changed to fit the device\n> > > enumerators needs.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > \n> > > include/libcamera/camera.h | 31 +++++++++++\n> > > include/libcamera/libcamera.h | 2 +\n> > > include/libcamera/meson.build | 1 +\n> > > src/libcamera/camera.cpp | 97 +++++++++++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 5 files changed, 132 insertions(+)\n> > > create mode 100644 include/libcamera/camera.h\n> > > create mode 100644 src/libcamera/camera.cpp\n\n[snip]\n\n> > > diff --git a/include/libcamera/libcamera.h\n> > > b/include/libcamera/libcamera.h\n> > > index 790771b61e41e123..44c094d92feed5ba 100644\n> > > --- a/include/libcamera/libcamera.h\n> > > +++ b/include/libcamera/libcamera.h\n> > > @@ -7,6 +7,8 @@\n> > > \n> > > #ifndef __LIBCAMERA_LIBCAMERA_H__\n> > > #define __LIBCAMERA_LIBCAMERA_H__\n> > > \n> > > +#include \n> > > +\n> > \n> > Why do you need this here?\n> \n> The Camera object will be part of the public API and any application\n> should only have to include the top header. Am I'm missing something?\n\nlibcamera.h should include all public headers, and serve as a shortcut for \napplications that want to pull in the whole public API. Applications can \nhowever include the other files separately as needed.\n\n> > > namespace libcamera {\n> > > \n> > > class libcamera\n\n[snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC61E60B2E\n\tfor ;\n\tFri, 28 Dec 2018 17:46:02 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 43BB5DF;\n\tFri, 28 Dec 2018 17:46:02 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546015562;\n\tbh=oSMTQinA2PDKI0IC2XbS/ZhIS0zoQZfoBOXuf83ID/o=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=XWYg+1MIp7aDh0wGNx9XGUCNf1X9fE6mByUqY3Z5rka1el6NcxkKAgPwZHSZzODsi\n\t/cQQlOmgK3sTEDFhMBPcaB8yDM+xKSF4SknH1SDhkzl6v26FuIwgvtxgoBCpaMfRJ2\n\thO+o+n8+itJIFcNp824CnvlsmB5hum87JZhUp7VI=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Fri, 28 Dec 2018 18:46:59 +0200","Message-ID":"<1556289.MHxoJoZQBd@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181228025624.GO19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181224101948.GB2364@archlinux>\n\t<20181228025624.GO19796@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 16:46:10 -0000"}},{"id":101,"web_url":"https://patchwork.libcamera.org/comment/101/","msgid":"<2110354.IMSRXZu0E8@avalon>","date":"2018-12-28T17:01:54","subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sunday, 23 December 2018 01:00:30 EET Niklas Söderlund wrote:\n> Provide a Camera class which represents our main interface to handling\n> camera devices. This is a rework of Kieran's initial proposal and\n> Laurent's documentation of the file changed to fit the device\n> enumerators needs.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera.h | 31 +++++++++++\n> include/libcamera/libcamera.h | 2 +\n> include/libcamera/meson.build | 1 +\n> src/libcamera/camera.cpp | 97 +++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 5 files changed, 132 insertions(+)\n> create mode 100644 include/libcamera/camera.h\n> create mode 100644 src/libcamera/camera.cpp\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> new file mode 100644\n> index 0000000000000000..7622385cc94c11cd\n> --- /dev/null\n> +++ b/include/libcamera/camera.h\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * camera.h - Camera object interface\n> + */\n> +#ifndef __LIBCAMERA_CAMERA_H__\n> +#define __LIBCAMERA_CAMERA_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Camera\n> +{\n> +public:\n> +\tCamera(const std::string &name);\n> +\t~Camera();\n> +\n> +\tconst std::string &name() const;\n> +\tvoid get();\n> +\tvoid put();\n> +\n> +private:\n> +\tint ref_;\n> +\tstd::string name_;\n> +};\n> +\n> +}\n> +\n> +#endif /* __LIBCAMERA_CAMERA_H__ */\n> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> index 790771b61e41e123..44c094d92feed5ba 100644\n> --- a/include/libcamera/libcamera.h\n> +++ b/include/libcamera/libcamera.h\n> @@ -7,6 +7,8 @@\n> #ifndef __LIBCAMERA_LIBCAMERA_H__\n> #define __LIBCAMERA_LIBCAMERA_H__\n> \n> +#include \n> +\n> namespace libcamera {\n> \n> class libcamera\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 8c82675a25d29913..9b266ad926681db9 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -1,4 +1,5 @@\n> libcamera_api = files([\n> + 'camera.h',\n> 'libcamera.h',\n> ])\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> new file mode 100644\n> index 0000000000000000..a85516876ce79ba4\n> --- /dev/null\n> +++ b/src/libcamera/camera.cpp\n> @@ -0,0 +1,97 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * camera.cpp - Camera device\n> + */\n> +\n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file camera.h\n> + * \\brief Camera device handling\n> + *\n> + * At the core of libcamera is the camera device, combining one image\n> source + * with processing hardware able to provide one or multiple image\n> streams. The + * Camera class represents a camera device.\n> + *\n> + * A camera device contains a single image source, and separate camera\n> device + * instances relate to different image sources. For instance, a\n> phone containing + * front and back image sensors will be modelled with two\n> camera devices, one + * for each sensor. When multiple streams can be\n> produced from the same image + * source, all those streams are guaranteed\n> to be part of the same camera + * device.\n> + *\n> + * While not sharing image sources, separate camera devices can share other\n> + * system resources, such as an ISP. For this reason camera device\n> instances may + * not be fully independent, in which case usage\n> restrictions may apply. For + * instance, a phone with a front and a back\n> camera device may not allow usage + * of the two devices simultaneously.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class Camera\n> + * \\brief Camera device\n> + *\n> + * The Camera class models a camera capable of producing one or more image\n> + * streams from a single image source. It provides the main interface to\n> + * configuring and controlling the device, and capturing image streams. It\n> is + * the central object exposed by libcamera.\n> + */\n> +\n> +/**\n> + * \\brief Construct a named camera device\n> + *\n> + * \\param[in] name The name to set on the camera device\n\nIs [in] a standard doxygen construct ? Should we use it through the \ndocumentation ?\n\n> + *\n> + * The caller is responsible for guaranteeing unicity of the camera\n> + * device name.\n> + */\n> +Camera::Camera(const std::string &name)\n> +\t: ref_(1), name_(name)\n> +{\n> +\tLOG(Debug) << \"Camera Constructed for \" << name_;\n\nI think logging creation of camera objects, and later their deletion when \nwe'll have hotplug support, is useful. I wouldn't put that in the Camera class \nconstructor, but in the code that instantiates the object, as it should have \naccess to more information. This is camera manager or device enumerator debug \ncode in my opinion, and should then be bumped to Log(Info).\n\n> +}\n> +\n> +Camera::~Camera()\n> +{\n> +\tif (ref_)\n> +\t\tLOG(Error) << \"Camera Destroyed while still in use!\";\n> +\telse\n> +\t\tLOG(Debug) << \"Camera Destroyed\";\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the name of the camera\n> + *\n> + * \\return Name of the camera device\n> + */\n> +const std::string &Camera::name() const\n> +{\n> +\treturn name_;\n> +}\n> +\n> +/**\n> + * \\brief Increase the use count of the camera\n> + */\n> +void Camera::get()\n> +{\n> +\tref_++;\n> +}\n> +\n> +/**\n> + * \\brief Decreases the use count of the camera.\n> + *\n> + * When the use count of the camera reaches zero the camera device is\n> deleted.\n\nHow about talking about references instead of use count ?\n\n\\brief Release a reference to the camera\n\nWhen the last reference is released the camera device is deleted. Callers \nshall not access the camera device after calling this function.\n\nAnd something similar for get().\n\n> + */\n> +void Camera::put()\n> +{\n> +\tif (--ref_ == 0)\n> +\t\tdelete this;\n\nYou can make the destructor private, and remove the error message there.\n\nOnce fixed,\n\nReviewed-by: Laurent Pinchart \n\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index f632eb5dd7791ad2..46591069aa5f8beb 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,4 +1,5 @@\n> libcamera_sources = files([\n> + 'camera.cpp',\n> 'log.cpp',\n> 'main.cpp',\n> ])","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8906660B2E\n\tfor ;\n\tFri, 28 Dec 2018 18:00:57 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 203F3DF;\n\tFri, 28 Dec 2018 18:00:57 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546016457;\n\tbh=Z17uVBh0sFpirocN+QCVRPQu/mYTODZxW1YrAwaPZjw=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=OTHjaI1Lb4SIzBnrXpEpisgDdKRTlijihUlCrUOsaTKaW1dadh7MJMCqDNmIgaubo\n\tgGZu8rVPUS6f3UtVAbOHOo5ND5qTYBegeIohFUDPzhPrMpFwxynSuCO3QDzWZbFkoV\n\ttLacERFqrxsZegTjRXnh98TssKtmmI2P5UOfwZfg=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Fri, 28 Dec 2018 19:01:54 +0200","Message-ID":"<2110354.IMSRXZu0E8@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181222230041.29999-2-niklas.soderlund@ragnatech.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 17:00:57 -0000"}},{"id":110,"web_url":"https://patchwork.libcamera.org/comment/110/","msgid":"<20181229002948.GY19796@bigcity.dyn.berto.se>","date":"2018-12-29T00:29:48","subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2018-12-28 19:01:54 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sunday, 23 December 2018 01:00:30 EET Niklas Söderlund wrote:\n> > Provide a Camera class which represents our main interface to handling\n> > camera devices. This is a rework of Kieran's initial proposal and\n> > Laurent's documentation of the file changed to fit the device\n> > enumerators needs.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/camera.h | 31 +++++++++++\n> > include/libcamera/libcamera.h | 2 +\n> > include/libcamera/meson.build | 1 +\n> > src/libcamera/camera.cpp | 97 +++++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 5 files changed, 132 insertions(+)\n> > create mode 100644 include/libcamera/camera.h\n> > create mode 100644 src/libcamera/camera.cpp\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > new file mode 100644\n> > index 0000000000000000..7622385cc94c11cd\n> > --- /dev/null\n> > +++ b/include/libcamera/camera.h\n> > @@ -0,0 +1,31 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * camera.h - Camera object interface\n> > + */\n> > +#ifndef __LIBCAMERA_CAMERA_H__\n> > +#define __LIBCAMERA_CAMERA_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class Camera\n> > +{\n> > +public:\n> > +\tCamera(const std::string &name);\n> > +\t~Camera();\n> > +\n> > +\tconst std::string &name() const;\n> > +\tvoid get();\n> > +\tvoid put();\n> > +\n> > +private:\n> > +\tint ref_;\n> > +\tstd::string name_;\n> > +};\n> > +\n> > +}\n> > +\n> > +#endif /* __LIBCAMERA_CAMERA_H__ */\n> > diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> > index 790771b61e41e123..44c094d92feed5ba 100644\n> > --- a/include/libcamera/libcamera.h\n> > +++ b/include/libcamera/libcamera.h\n> > @@ -7,6 +7,8 @@\n> > #ifndef __LIBCAMERA_LIBCAMERA_H__\n> > #define __LIBCAMERA_LIBCAMERA_H__\n> > \n> > +#include \n> > +\n> > namespace libcamera {\n> > \n> > class libcamera\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 8c82675a25d29913..9b266ad926681db9 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -1,4 +1,5 @@\n> > libcamera_api = files([\n> > + 'camera.h',\n> > 'libcamera.h',\n> > ])\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > new file mode 100644\n> > index 0000000000000000..a85516876ce79ba4\n> > --- /dev/null\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -0,0 +1,97 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * camera.cpp - Camera device\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +\n> > +/**\n> > + * \\file camera.h\n> > + * \\brief Camera device handling\n> > + *\n> > + * At the core of libcamera is the camera device, combining one image\n> > source + * with processing hardware able to provide one or multiple image\n> > streams. The + * Camera class represents a camera device.\n> > + *\n> > + * A camera device contains a single image source, and separate camera\n> > device + * instances relate to different image sources. For instance, a\n> > phone containing + * front and back image sensors will be modelled with two\n> > camera devices, one + * for each sensor. When multiple streams can be\n> > produced from the same image + * source, all those streams are guaranteed\n> > to be part of the same camera + * device.\n> > + *\n> > + * While not sharing image sources, separate camera devices can share other\n> > + * system resources, such as an ISP. For this reason camera device\n> > instances may + * not be fully independent, in which case usage\n> > restrictions may apply. For + * instance, a phone with a front and a back\n> > camera device may not allow usage + * of the two devices simultaneously.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class Camera\n> > + * \\brief Camera device\n> > + *\n> > + * The Camera class models a camera capable of producing one or more image\n> > + * streams from a single image source. It provides the main interface to\n> > + * configuring and controlling the device, and capturing image streams. It\n> > is + * the central object exposed by libcamera.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a named camera device\n> > + *\n> > + * \\param[in] name The name to set on the camera device\n> \n> Is [in] a standard doxygen construct ? Should we use it through the \n> documentation ?\n\nI think it's a standard construct. I find it useful to mark the \ndirection of the parameters but I'm open to dropping them if not all \nfind them useful. I leave them in for v2.\n\n> \n> > + *\n> > + * The caller is responsible for guaranteeing unicity of the camera\n> > + * device name.\n> > + */\n> > +Camera::Camera(const std::string &name)\n> > +\t: ref_(1), name_(name)\n> > +{\n> > +\tLOG(Debug) << \"Camera Constructed for \" << name_;\n> \n> I think logging creation of camera objects, and later their deletion when \n> we'll have hotplug support, is useful. I wouldn't put that in the Camera class \n> constructor, but in the code that instantiates the object, as it should have \n> access to more information. This is camera manager or device enumerator debug \n> code in my opinion, and should then be bumped to Log(Info).\n\nAgreed.\n\n> \n> > +}\n> > +\n> > +Camera::~Camera()\n> > +{\n> > +\tif (ref_)\n> > +\t\tLOG(Error) << \"Camera Destroyed while still in use!\";\n> > +\telse\n> > +\t\tLOG(Debug) << \"Camera Destroyed\";\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the name of the camera\n> > + *\n> > + * \\return Name of the camera device\n> > + */\n> > +const std::string &Camera::name() const\n> > +{\n> > +\treturn name_;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Increase the use count of the camera\n> > + */\n> > +void Camera::get()\n> > +{\n> > +\tref_++;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Decreases the use count of the camera.\n> > + *\n> > + * When the use count of the camera reaches zero the camera device is\n> > deleted.\n> \n> How about talking about references instead of use count ?\n> \n> \\brief Release a reference to the camera\n> \n> When the last reference is released the camera device is deleted. Callers \n> shall not access the camera device after calling this function.\n> \n> And something similar for get().\n\nAgreed.\n\n> \n> > + */\n> > +void Camera::put()\n> > +{\n> > +\tif (--ref_ == 0)\n> > +\t\tdelete this;\n> \n> You can make the destructor private, and remove the error message there.\n\nWith all error messages moved or removed from the destructor I can \ndelete it instead of making it private :-)\n\n> \n> Once fixed,\n> \n> Reviewed-by: Laurent Pinchart \n\nThanks!\n\n> \n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index f632eb5dd7791ad2..46591069aa5f8beb 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -1,4 +1,5 @@\n> > libcamera_sources = files([\n> > + 'camera.cpp',\n> > 'log.cpp',\n> > 'main.cpp',\n> > ])\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> \n>","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA4E160B2E\n\tfor ;\n\tSat, 29 Dec 2018 01:29:50 +0100 (CET)","by mail-lf1-x141.google.com with SMTP id l10so15432091lfh.9\n\tfor ;\n\tFri, 28 Dec 2018 16:29:50 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tc5-v6sm8685473lja.62.2018.12.28.16.29.48\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 28 Dec 2018 16:29:48 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=pFHRZKOnxNWMPzB/Kg+in2t+B1UfclzXUz74g5Cb8A0=;\n\tb=BEEpJhpBWO2lvaZtGAZDgx51I1iv5yMBN3UUtxB/iH3FbdntVJKG2/q+LkWRMLg7cf\n\tjGd9AG5XNYe/cJNHAbGFY7RSjRQzQM2uGgEEx7COoXp17DUjSlz5Lv/2RbzbCS98mLzP\n\taBW0cpQqDo/Zv8eTEhHtHth+eiEMwaW1TQQqyiP1SYZv4kD44PFxmwLz2G6a3U+4l9As\n\tG7jxH3D5RcZhnKaaTDCYbRKQOQ9AXuG1KQabQeFH7oRb5QKBmdhoQs6bwvV+tzCuY9+3\n\tInrCSbTZ8gCoyydoyY9Lf928JW3fGAFJ2XXppFu1gVynA007NvmJ7CYpYms5eQSnV6eJ\n\tIKYw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=pFHRZKOnxNWMPzB/Kg+in2t+B1UfclzXUz74g5Cb8A0=;\n\tb=MuZ+7SH/3OFO1v9lF96pVWVUxpjoutGyyhB8Ron1KcWQgd5XNlHKVmebsIgVFc6Y0k\n\t+t5RhkuSF9AX53e5D05BdFQYhKZ8HGkdslr8vjsT9ZYCoF5TXFmNDFDhyoldhlZ4S/ck\n\tRkR8bNbmxIkEBbzkoH2SaIVKeWdYIv26m44FWOXAPBFPmipf5pFxxvAAVsW6RMawbiow\n\tKNPRAYrb99F5pu3Kr4baBmrgU0X1B1Fzw2g1pepFPWBh1+rzBqZP1Es0j9xIs/RdAq6E\n\t6DW0EXAKCxRwmhPZNFt8aLRE61gVJ3HDznt4S0n3/KqmsyC1UmfpP5y0v0WlWyw+ZXNw\n\td3xQ==","X-Gm-Message-State":"AA+aEWZ3DdoNqxVckDtEGv6WBchqlcmM8S0cqjTlgoXkZirsrIiDfG/e\n\tqVHWJ/LCA8D8Y8XEeKQDSeEGlw==","X-Google-Smtp-Source":"AFSGD/UGljmc3QnwmcHNoSLikmnTo6IbJJQd9WP3WoYhKuyeoxVKcTg4/C5y93+59F34lLSuOPwBWQ==","X-Received":"by 2002:a19:cd50:: with SMTP id\n\td77mr14400790lfg.125.1546043389682; \n\tFri, 28 Dec 2018 16:29:49 -0800 (PST)","Date":"Sat, 29 Dec 2018 01:29:48 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181229002948.GY19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-2-niklas.soderlund@ragnatech.se>\n\t<2110354.IMSRXZu0E8@avalon>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<2110354.IMSRXZu0E8@avalon>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 29 Dec 2018 00:29:51 -0000"}},{"id":115,"web_url":"https://patchwork.libcamera.org/comment/115/","msgid":"<2059978.Nz4LCz158D@avalon>","date":"2018-12-29T01:24:30","subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Saturday, 29 December 2018 02:29:48 EET Niklas Söderlund wrote:\n> On 2018-12-28 19:01:54 +0200, Laurent Pinchart wrote:\n> > On Sunday, 23 December 2018 01:00:30 EET Niklas Söderlund wrote:\n> > > Provide a Camera class which represents our main interface to handling\n> > > camera devices. This is a rework of Kieran's initial proposal and\n> > > Laurent's documentation of the file changed to fit the device\n> > > enumerators needs.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > \n> > > include/libcamera/camera.h | 31 +++++++++++\n> > > include/libcamera/libcamera.h | 2 +\n> > > include/libcamera/meson.build | 1 +\n> > > src/libcamera/camera.cpp | 97 +++++++++++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 5 files changed, 132 insertions(+)\n> > > create mode 100644 include/libcamera/camera.h\n> > > create mode 100644 src/libcamera/camera.cpp\n> > > \n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > new file mode 100644\n> > > index 0000000000000000..7622385cc94c11cd\n> > > --- /dev/null\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -0,0 +1,31 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\n> > > + * camera.h - Camera object interface\n> > > + */\n> > > +#ifndef __LIBCAMERA_CAMERA_H__\n> > > +#define __LIBCAMERA_CAMERA_H__\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class Camera\n> > > +{\n> > > +public:\n> > > +\tCamera(const std::string &name);\n> > > +\t~Camera();\n> > > +\n> > > +\tconst std::string &name() const;\n> > > +\tvoid get();\n> > > +\tvoid put();\n> > > +\n> > > +private:\n> > > +\tint ref_;\n> > > +\tstd::string name_;\n> > > +};\n> > > +\n> > > +}\n> > > +\n> > > +#endif /* __LIBCAMERA_CAMERA_H__ */\n> > > diff --git a/include/libcamera/libcamera.h\n> > > b/include/libcamera/libcamera.h\n> > > index 790771b61e41e123..44c094d92feed5ba 100644\n> > > --- a/include/libcamera/libcamera.h\n> > > +++ b/include/libcamera/libcamera.h\n> > > @@ -7,6 +7,8 @@\n> > > \n> > > #ifndef __LIBCAMERA_LIBCAMERA_H__\n> > > #define __LIBCAMERA_LIBCAMERA_H__\n> > > \n> > > +#include \n> > > +\n> > > \n> > > namespace libcamera {\n> > > \n> > > class libcamera\n> > > \n> > > diff --git a/include/libcamera/meson.build\n> > > b/include/libcamera/meson.build\n> > > index 8c82675a25d29913..9b266ad926681db9 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -1,4 +1,5 @@\n> > > \n> > > libcamera_api = files([\n> > > \n> > > + 'camera.h',\n> > > \n> > > 'libcamera.h',\n> > > \n> > > ])\n> > > \n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > new file mode 100644\n> > > index 0000000000000000..a85516876ce79ba4\n> > > --- /dev/null\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -0,0 +1,97 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\n> > > + * camera.cpp - Camera device\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"log.h\"\n> > > +\n> > > +/**\n> > > + * \\file camera.h\n> > > + * \\brief Camera device handling\n> > > + *\n> > > + * At the core of libcamera is the camera device, combining one image\n> > > source + * with processing hardware able to provide one or multiple\n> > > image\n> > > streams. The + * Camera class represents a camera device.\n> > > + *\n> > > + * A camera device contains a single image source, and separate camera\n> > > device + * instances relate to different image sources. For instance, a\n> > > phone containing + * front and back image sensors will be modelled with\n> > > two\n> > > camera devices, one + * for each sensor. When multiple streams can be\n> > > produced from the same image + * source, all those streams are\n> > > guaranteed\n> > > to be part of the same camera + * device.\n> > > + *\n> > > + * While not sharing image sources, separate camera devices can share\n> > > other + * system resources, such as an ISP. For this reason camera\n> > > device instances may + * not be fully independent, in which case usage\n> > > restrictions may apply. For + * instance, a phone with a front and a\n> > > back\n> > > camera device may not allow usage + * of the two devices simultaneously.\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\class Camera\n> > > + * \\brief Camera device\n> > > + *\n> > > + * The Camera class models a camera capable of producing one or more\n> > > image\n> > > + * streams from a single image source. It provides the main interface\n> > > to\n> > > + * configuring and controlling the device, and capturing image streams.\n> > > It\n> > > is + * the central object exposed by libcamera.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct a named camera device\n> > > + *\n> > > + * \\param[in] name The name to set on the camera device\n> > \n> > Is [in] a standard doxygen construct ? Should we use it through the\n> > documentation ?\n> \n> I think it's a standard construct. I find it useful to mark the\n> direction of the parameters but I'm open to dropping them if not all\n> find them useful. I leave them in for v2.\n\nI wasn't asking to drop it, but whether we should generalize its usage.\n\n> > > + *\n> > > + * The caller is responsible for guaranteeing unicity of the camera\n> > > + * device name.\n> > > + */\n> > > +Camera::Camera(const std::string &name)\n> > > +\t: ref_(1), name_(name)\n> > > +{\n> > > +\tLOG(Debug) << \"Camera Constructed for \" << name_;\n> > \n> > I think logging creation of camera objects, and later their deletion when\n> > we'll have hotplug support, is useful. I wouldn't put that in the Camera\n> > class constructor, but in the code that instantiates the object, as it\n> > should have access to more information. This is camera manager or device\n> > enumerator debug code in my opinion, and should then be bumped to\n> > Log(Info).\n> \n> Agreed.\n> \n> > > +}\n> > > +\n> > > +Camera::~Camera()\n> > > +{\n> > > +\tif (ref_)\n> > > +\t\tLOG(Error) << \"Camera Destroyed while still in use!\";\n> > > +\telse\n> > > +\t\tLOG(Debug) << \"Camera Destroyed\";\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Retrieve the name of the camera\n> > > + *\n> > > + * \\return Name of the camera device\n> > > + */\n> > > +const std::string &Camera::name() const\n> > > +{\n> > > +\treturn name_;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Increase the use count of the camera\n> > > + */\n> > > +void Camera::get()\n> > > +{\n> > > +\tref_++;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Decreases the use count of the camera.\n> > > + *\n> > > + * When the use count of the camera reaches zero the camera device is\n> > > deleted.\n> > \n> > How about talking about references instead of use count ?\n> > \n> > \\brief Release a reference to the camera\n> > \n> > When the last reference is released the camera device is deleted. Callers\n> > shall not access the camera device after calling this function.\n> > \n> > And something similar for get().\n> \n> Agreed.\n> \n> > > + */\n> > > +void Camera::put()\n> > > +{\n> > > +\tif (--ref_ == 0)\n> > > +\t\tdelete this;\n> > \n> > You can make the destructor private, and remove the error message there.\n> \n> With all error messages moved or removed from the destructor I can\n> delete it instead of making it private :-)\n\nYou can't as it should be virtual (which I forgot to point out in my review).\n\n> > Once fixed,\n> > \n> > Reviewed-by: Laurent Pinchart \n> \n> Thanks!\n> \n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index f632eb5dd7791ad2..46591069aa5f8beb 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -1,4 +1,5 @@\n> > > \n> > > libcamera_sources = files([\n> > > \n> > > + 'camera.cpp',\n> > > \n> > > 'log.cpp',\n> > > 'main.cpp',\n> > > \n> > > ])","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1EC860B30\n\tfor ;\n\tSat, 29 Dec 2018 02:23:34 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35A02DF;\n\tSat, 29 Dec 2018 02:23:34 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546046614;\n\tbh=7yIhpg62+/Hde8vb8H6BzomkOJUOoW23WPk6EdVpeSw=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=pU5onjs0tlAKYu1DmIWvxqmUrMH/EvU2uoEMCtPzBXDjEop0iB1MqTv5ZgDQboT2/\n\t4J51Wi2Bnsegkbi5lpmY4mmZ2LGxkWPXR29JSfdeAO1Fl7x+z6u2FoVsPwAiJyzhcT\n\tyIHK6KEtjSXATRceFqQ1bEih/Yqj9SK5wY/MBdTA=","From":"Laurent Pinchart ","To":"Niklas =?iso-8859-1?q?S=F6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Sat, 29 Dec 2018 03:24:30 +0200","Message-ID":"<2059978.Nz4LCz158D@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181229002948.GY19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<2110354.IMSRXZu0E8@avalon>\n\t<20181229002948.GY19796@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 01/12] libcamera: Add Camera class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 29 Dec 2018 01:23:34 -0000"}}]