[{"id":617,"web_url":"https://patchwork.libcamera.org/comment/617/","msgid":"<20190125231804.GB15524@pendragon.ideasonboard.com>","date":"2019-01-25T23:18:04","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: stream: add basic\n\tStream 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 Fri, Jan 25, 2019 at 04:33:34PM +0100, Niklas Söderlund wrote:\n> Add a extremely simple Stream implementation. The idea is that once\n\nHow about \"an initial\" or \"an initial stub\" instead of \"a extremely\nsimple\" ? :-)\n\n> capability support is added to the library each stream would describe\n\ns/would/will/\n\n> it's capabilities using this class. A application would then select one\n\ns/it's/its/\ns/A/An/\ns/would/will/\n\n> or more streams based on these capabilities and using them to configure\n\ns/using/use/\ns/configure/configure the camera/\n\n> and capture.\n> \n> At this stage all the Stream class provides is a way for a Camera object\n> to communicate which stream ID it to operates on. This basically\n\n\"it to\" ?\n\n> limits the usefulness of the object to cameras which only provides one\n> stream per camera.\n\nWhy so ? Isn't the object still useful when you have multiple streams,\nto convey stream IDs ?\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/libcamera.h | 1 +\n> include/libcamera/meson.build | 1 +\n> include/libcamera/stream.h | 25 +++++++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/stream.cpp | 66 +++++++++++++++++++++++++++++++++++\n> 5 files changed, 94 insertions(+)\n> create mode 100644 include/libcamera/stream.h\n> create mode 100644 src/libcamera/stream.cpp\n> \n> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> index c0511cf6d662b63f..272dfd5e4a67d5de 100644\n> --- a/include/libcamera/libcamera.h\n> +++ b/include/libcamera/libcamera.h\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #endif /* __LIBCAMERA_LIBCAMERA_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index d7cb55ba4a49e1e8..54a680787e5c17aa 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -5,6 +5,7 @@ libcamera_api = files([\n> 'event_notifier.h',\n> 'libcamera.h',\n> 'signal.h',\n> + 'stream.h',\n> 'timer.h',\n> ])\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> new file mode 100644\n> index 0000000000000000..415815ba12c65e47\n> --- /dev/null\n> +++ b/include/libcamera/stream.h\n> @@ -0,0 +1,25 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * stream.h - Stream object interface\n\n\"Video stream for a Camera\" ?\n\n> + */\n> +#ifndef __LIBCAMERA_STREAM_H__\n> +#define __LIBCAMERA_STREAM_H__\n> +\n> +namespace libcamera {\n> +\n> +class Stream final\n\nI wonder if we should let pipeline handlers subclass the Stream. I\nsuppose allowing Stream subclasses and not Camera subclasses wouldn't be\na good idea. Maybe it wasn't a good idea to forbid subclasses of Camera\n:-S I suppose the future will tell.\n\n> +{\n> +public:\n> +\tStream(unsigned int id);\n> +\n> +\tunsigned int id() const { return id_; };\n> +\n> +private:\n> +\tunsigned int id_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_STREAM_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index f9f25c0ecf1564cc..9f6ff99eebe2f5bc 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -10,6 +10,7 @@ libcamera_sources = files([\n> 'media_object.cpp',\n> 'pipeline_handler.cpp',\n> 'signal.cpp',\n> + 'stream.cpp',\n> 'timer.cpp',\n> 'v4l2_device.cpp',\n> ])\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> new file mode 100644\n> index 0000000000000000..3b44e834ee02b35a\n> --- /dev/null\n> +++ b/src/libcamera/stream.cpp\n> @@ -0,0 +1,66 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * stream.cpp - Stream information handeling\n\nI would use the same description as in stream.cpp\n\n> + */\n> +\n> +#include \n> +\n> +/**\n> + * \\file stream.h\n> + * \\brief Stream information\n\nAnd here too.\n\n> + *\n> + * A camera device can provide frames in different resolutions and formats\n> + * concurrently from a single image source. The Stream class represents\n> + * one of the multiple concurrent streams format.\n\nMaybe s/ format// ?\n\n> + * All streams exposed by a camera device share the same image source and are\n> + * thus not fully independent. Parameters related to the image source, such as\n> + * the exposure time or flash control, are common to all streams. Other\n> + * parameters, such as format or resolution, may be specified per-stream,\n> + * depending on the capabilities of the camera device.\n> + *\n> + * Camera devices expose at least one stream, and may expose additional streams\n> + * based on the device capabilities. This can be used, for instance, to\n> + * implement concurrent viewfinder and video capture, or concurrent viewfinder,\n> + * video capture and still image capture.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class Stream\n> + * \\brief Stream information carrier\n\n\"Video stream for a camera\" ?\n\n> + *\n> + * The Stream class is a model of all static information which are associated\n\ns/is a model/models/\n\n> + * with a single video stream. A application should acquire Stream objects from\n> + * the camera.\n\nI'd replace the second sentence with \"Stream are exposed by the Camera\nobject they belong to.\" \n\n> + *\n> + * Some cameras are capable of supplying more then one stream from the same\n> + * video source. In such cases a application will receive a array of streams to\n> + * inspect and select from to best fit its use-case.\n\n\"In such cases an application can inspect all available streams and\nselect the ones that best fit its use case.\n\n> + *\n> + * \\todo Add capabilities to the Stream API. Without this the Stream class\n> + *\t only serves to reveal how many streams of unknown capabilities a camera\n> + *\t supports. This in it self is productive as it allows applications to\n\ns/it self/itself/\n\n> + *\t configure and capture from one or more streams even if it won't be able\n\ns/it/they/\n\n> + *\t to select the optimal stream for the task.\n\nNo need for indentation I think.\n\n> + */\n> +\n> +/**\n> + * \\brief Create a new stream with a ID\n> + * \\param[in] id Numerical ID which should be unique for the camera device the stream belongs to\n\nLine wrap ?\n\n> + */\n> +Stream::Stream(unsigned int id)\n> +\t: id_(id)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn Stream::id()\n> + * \\brief Retrieve the streams ID\n> + * \\return The stream ID\n> + */\n> +\n> +} /* namespace libcamera */","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 18A7460B1B\n\tfor ;\n\tSat, 26 Jan 2019 00:20:14 +0100 (CET)","from pendragon.ideasonboard.com (unknown [109.129.169.54])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9499A325;\n\tSat, 26 Jan 2019 00:20:13 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548458413;\n\tbh=EJaMSjF8JZL5Y6XLUAa1ys9Zk0BnLJ+2KC4YLvjw2so=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O4ZfPiGRFynQry3rOnxidoa7ZVjM6D7FvdmSGLhnbxmDDpmeJO6g2w1leKMS2EBI3\n\tdrx2x5/4hdaRoMtTW+hizHzW82nxsHET1Kc/pGlxwdNv4D8IUmUI/tksxtBMcP8l4H\n\tnaylafARd6N9GaSI9Ux9qYDrr9lvZu+6SrAHjShY=","Date":"Sat, 26 Jan 2019 01:18:04 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190125231804.GB15524@pendragon.ideasonboard.com>","References":"<20190125153340.2744-1-niklas.soderlund@ragnatech.se>\n\t<20190125153340.2744-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190125153340.2744-2-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: stream: add basic\n\tStream 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, 25 Jan 2019 23:20:14 -0000"}},{"id":625,"web_url":"https://patchwork.libcamera.org/comment/625/","msgid":"<20190126102632.GT4127@bigcity.dyn.berto.se>","date":"2019-01-26T10:26:32","subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: stream: add basic\n\tStream 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. I'm forever grateful for your grammar lessons.\n\nOn 2019-01-26 01:18:04 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Fri, Jan 25, 2019 at 04:33:34PM +0100, Niklas Söderlund wrote:\n> > Add a extremely simple Stream implementation. The idea is that once\n> \n> How about \"an initial\" or \"an initial stub\" instead of \"a extremely\n> simple\" ? :-)\n> \n> > capability support is added to the library each stream would describe\n> \n> s/would/will/\n> \n> > it's capabilities using this class. A application would then select one\n> \n> s/it's/its/\n> s/A/An/\n> s/would/will/\n> \n> > or more streams based on these capabilities and using them to configure\n> \n> s/using/use/\n> s/configure/configure the camera/\n> \n> > and capture.\n> > \n> > At this stage all the Stream class provides is a way for a Camera object\n> > to communicate which stream ID it to operates on. This basically\n> \n> \"it to\" ?\n> \n> > limits the usefulness of the object to cameras which only provides one\n> > stream per camera.\n> \n> Why so ? Isn't the object still useful when you have multiple streams,\n> to convey stream IDs ?\n\nI replace this paragraph with\n\n At this stage all the Stream class provides is a way for a Camera \n object to communicate which stream IDs it provides.\n\nAll other comments in this series are addressed according to your \nsuggestions, thanks!\n\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/libcamera.h | 1 +\n> > include/libcamera/meson.build | 1 +\n> > include/libcamera/stream.h | 25 +++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/stream.cpp | 66 +++++++++++++++++++++++++++++++++++\n> > 5 files changed, 94 insertions(+)\n> > create mode 100644 include/libcamera/stream.h\n> > create mode 100644 src/libcamera/stream.cpp\n> > \n> > diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> > index c0511cf6d662b63f..272dfd5e4a67d5de 100644\n> > --- a/include/libcamera/libcamera.h\n> > +++ b/include/libcamera/libcamera.h\n> > @@ -12,6 +12,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > #endif /* __LIBCAMERA_LIBCAMERA_H__ */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index d7cb55ba4a49e1e8..54a680787e5c17aa 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -5,6 +5,7 @@ libcamera_api = files([\n> > 'event_notifier.h',\n> > 'libcamera.h',\n> > 'signal.h',\n> > + 'stream.h',\n> > 'timer.h',\n> > ])\n> > \n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > new file mode 100644\n> > index 0000000000000000..415815ba12c65e47\n> > --- /dev/null\n> > +++ b/include/libcamera/stream.h\n> > @@ -0,0 +1,25 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * stream.h - Stream object interface\n> \n> \"Video stream for a Camera\" ?\n> \n> > + */\n> > +#ifndef __LIBCAMERA_STREAM_H__\n> > +#define __LIBCAMERA_STREAM_H__\n> > +\n> > +namespace libcamera {\n> > +\n> > +class Stream final\n> \n> I wonder if we should let pipeline handlers subclass the Stream. I\n> suppose allowing Stream subclasses and not Camera subclasses wouldn't be\n> a good idea. Maybe it wasn't a good idea to forbid subclasses of Camera\n> :-S I suppose the future will tell.\n> \n> > +{\n> > +public:\n> > +\tStream(unsigned int id);\n> > +\n> > +\tunsigned int id() const { return id_; };\n> > +\n> > +private:\n> > +\tunsigned int id_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_STREAM_H__ */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index f9f25c0ecf1564cc..9f6ff99eebe2f5bc 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -10,6 +10,7 @@ libcamera_sources = files([\n> > 'media_object.cpp',\n> > 'pipeline_handler.cpp',\n> > 'signal.cpp',\n> > + 'stream.cpp',\n> > 'timer.cpp',\n> > 'v4l2_device.cpp',\n> > ])\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > new file mode 100644\n> > index 0000000000000000..3b44e834ee02b35a\n> > --- /dev/null\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -0,0 +1,66 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * stream.cpp - Stream information handeling\n> \n> I would use the same description as in stream.cpp\n> \n> > + */\n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file stream.h\n> > + * \\brief Stream information\n> \n> And here too.\n> \n> > + *\n> > + * A camera device can provide frames in different resolutions and formats\n> > + * concurrently from a single image source. The Stream class represents\n> > + * one of the multiple concurrent streams format.\n> \n> Maybe s/ format// ?\n> \n> > + * All streams exposed by a camera device share the same image source and are\n> > + * thus not fully independent. Parameters related to the image source, such as\n> > + * the exposure time or flash control, are common to all streams. Other\n> > + * parameters, such as format or resolution, may be specified per-stream,\n> > + * depending on the capabilities of the camera device.\n> > + *\n> > + * Camera devices expose at least one stream, and may expose additional streams\n> > + * based on the device capabilities. This can be used, for instance, to\n> > + * implement concurrent viewfinder and video capture, or concurrent viewfinder,\n> > + * video capture and still image capture.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class Stream\n> > + * \\brief Stream information carrier\n> \n> \"Video stream for a camera\" ?\n> \n> > + *\n> > + * The Stream class is a model of all static information which are associated\n> \n> s/is a model/models/\n> \n> > + * with a single video stream. A application should acquire Stream objects from\n> > + * the camera.\n> \n> I'd replace the second sentence with \"Stream are exposed by the Camera\n> object they belong to.\" \n> \n> > + *\n> > + * Some cameras are capable of supplying more then one stream from the same\n> > + * video source. In such cases a application will receive a array of streams to\n> > + * inspect and select from to best fit its use-case.\n> \n> \"In such cases an application can inspect all available streams and\n> select the ones that best fit its use case.\n> \n> > + *\n> > + * \\todo Add capabilities to the Stream API. Without this the Stream class\n> > + *\t only serves to reveal how many streams of unknown capabilities a camera\n> > + *\t supports. This in it self is productive as it allows applications to\n> \n> s/it self/itself/\n> \n> > + *\t configure and capture from one or more streams even if it won't be able\n> \n> s/it/they/\n> \n> > + *\t to select the optimal stream for the task.\n> \n> No need for indentation I think.\n> \n> > + */\n> > +\n> > +/**\n> > + * \\brief Create a new stream with a ID\n> > + * \\param[in] id Numerical ID which should be unique for the camera device the stream belongs to\n> \n> Line wrap ?\n> \n> > + */\n> > +Stream::Stream(unsigned int id)\n> > +\t: id_(id)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\fn Stream::id()\n> > + * \\brief Retrieve the streams ID\n> > + * \\return The stream ID\n> > + */\n> > +\n> > +} /* namespace libcamera */\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 752F460C65\n\tfor ;\n\tSat, 26 Jan 2019 11:26:35 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id y11so8673298lfj.4\n\tfor ;\n\tSat, 26 Jan 2019 02:26:35 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tc203sm2024644lfe.95.2019.01.26.02.26.33\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSat, 26 Jan 2019 02:26:33 -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=BxErigTtnYSUhtx8JLqbAeoNsGIRLVT/JFjv2XQ+MV0=;\n\tb=ShniZN8XdvyDHnqZUxpbDKvk3yBWWogJizWupd5vGdk5jCXS4rKzszoScMqpDFYL0d\n\tXaUDy55CcEgCtOFBnXZHGeG6I3m9kIhnf7BfVY+9Yv0zpMYjVuN+QLHwKyozMzDNAdiO\n\ts3Om2+7OtRVy5dvwhVD9lpQp6IjzW1Yn241Z9/fTxeCbkeG5i4huixJ8pzeo78TGeZ/8\n\tot+ssnmjtsDswmtpxFp/9InA6LeSatvBOZygXX58JyLXnLx2p01zasSqhF7bfGlT31yW\n\t3CmVSo5kaR32ty85o2DMmjEv9+pz/xiELFGY0mPKj+XBjO42UxWZQTz1SshYlDwVOxom\n\teLmQ==","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=BxErigTtnYSUhtx8JLqbAeoNsGIRLVT/JFjv2XQ+MV0=;\n\tb=BGohGZAVdAfZjP4fZJM4m8asSmyF/7VTANLgQMILIeN18tdaajweVKu5l88diTrBGq\n\t5lObXwITQDLuh7qK75jFaof5Lsj9ycl8I7Zu/3MdEUhq/M0vhlJuH8pgs5qPt5rSM0xR\n\ty+EzBgvi9oednrrP51HBy2CzkCCZzB7Bn0Oza/1oRe04QumLpRtZFek30AxqP/uYvdmm\n\tLrSUVO5QugXDFrsfKlnMpFlLJNjuhX0DWsE00ysYhfJsWkJ5iCTK+uOZvCThcKfGpOXc\n\tJKHFSzEJOgpxBJkOlDGFzcbtmrgkHAZ7IPI8suoKEk5LgVCIRd3yggdGgJDzH/e7jkzN\n\tK8FQ==","X-Gm-Message-State":"AJcUukfZ+4JFNQ1ItTP5j0nxpoV/L9ACQ6RIT8cPOR0uP0KcYopKb9St\n\tOhN04wzyaqQv/JoY8b7KRS4+PcTGvNU=","X-Google-Smtp-Source":"ALg8bN7u9u0khW1rBihsdVjzROZeTgeR6XZfu+UBMol9dxoGHHkLzZFrBqmWgC+O5QUzS67nJD9qZg==","X-Received":"by 2002:a19:d486:: with SMTP id\n\tl128mr11174817lfg.114.1548498394456; \n\tSat, 26 Jan 2019 02:26:34 -0800 (PST)","Date":"Sat, 26 Jan 2019 11:26:32 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190126102632.GT4127@bigcity.dyn.berto.se>","References":"<20190125153340.2744-1-niklas.soderlund@ragnatech.se>\n\t<20190125153340.2744-2-niklas.soderlund@ragnatech.se>\n\t<20190125231804.GB15524@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190125231804.GB15524@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] libcamera: stream: add basic\n\tStream 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, 26 Jan 2019 10:26:35 -0000"}}]