[{"id":1241,"web_url":"https://patchwork.libcamera.org/comment/1241/","msgid":"<20190403065652.GD4813@pendragon.ideasonboard.com>","date":"2019-04-03T06:56:52","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: stream: Add basic\n\tstream roles definitions","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 Wed, Apr 03, 2019 at 03:12:20AM +0200, Niklas Söderlund wrote:\n> In preparation of reworking how a default configuration is retrieved\n> from a camera add stream roles. The roles will be used by applications\n> to describe how it intends to use a camera and replace the Stream IDs\n\ns/it intends/they intend/\n\n> role when retrieving default configuration from the camera using\n> streamConfiguration().\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/stream.h | 41 ++++++++++++++++++\n> src/libcamera/stream.cpp | 88 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 129 insertions(+)\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 970c479627fab064..adcf20d336347dad 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -21,9 +21,50 @@ struct StreamConfiguration {\n> \tunsigned int bufferCount;\n> };\n> \n> +class StreamRole\n> +{\n> +public:\n> +\tenum Role {\n> +\t\tStillCapture,\n> +\t\tVideoRecording,\n> +\t\tViewfinder,\n> +\t};\n> +\n> +\tRole role() const { return role_; }\n> +\tint width() const { return width_; }\n> +\tint height() const { return height_; }\n> +\n> +protected:\n> +\tStreamRole(Role role);\n> +\tStreamRole(Role role, int width, int height);\n> +\n> +private:\n> +\tRole role_;\n> +\tint width_;\n> +\tint height_;\n\nHow about using the new Size structure ?\n\n> +};\n> +\n> class Stream final\n> {\n> public:\n> +\tclass StillCapture : public StreamRole\n> +\t{\n> +\tpublic:\n> +\t\tStillCapture();\n> +\t};\n> +\n> +\tclass VideoRecording : public StreamRole\n> +\t{\n> +\tpublic:\n> +\t\tVideoRecording();\n> +\t};\n> +\n> +\tclass Viewfinder : public StreamRole\n> +\t{\n> +\tpublic:\n> +\t\tViewfinder(int width, int height);\n> +\t};\n> +\n> \tStream();\n> \tBufferPool &bufferPool() { return bufferPool_; }\n> \tconst StreamConfiguration &configuration() const { return configuration_; }\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index c4943c91b2e6ce13..f4be5d265e872842 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -60,6 +60,61 @@ namespace libcamera {\n> * \\brief Requested number of buffers to allocate for the stream\n> */\n> \n> +/**\n> + * \\class StreamRole\n> + * \\brief Stream role information\n> + *\n> + * The StreamRole class carries information about stream usage hints from the\n> + * application to the camera. The camera shall take the usage hints into account\n> + * when select which stream to use for the desired operation.\n\nI'd like to drop the word \"hint\" from most of the documentation. How\nabout\n\nThe StreamRole class describes how a stream is intended to be used.\nStream roles are specified by applications and passed to cameras, that\nthen select the most appropriate streams and their default\nconfigurations.\n\n> + */\n> +\n> +/**\n> + * \\enum StreamRole::Role\n> + * \\brief List of different stream roles\n\nUsing the word role to name both the class and the enum makes\ndocumentation a bit more difficult to write. You also need to document\nthe enum values by the way. How about something like the following ?\n\n/**\n * \\enum StreamRole::Role\n * Identify the role a stream is intended to play\n * \\var StillCapture\n * The stream is intended to capture high-resolution, high-quality still\n * images with low frame rate. The captured frames may be exposed with\n * flash.\n * \\var VideoRecording\n * The stream is intended to capture video for the purpose of recording\n * or streaming. The video stream may produce a high frame rate and may\n * be enhanced with video stabilization.\n * \\var Viewfinder\n * The stream is intended to capture video for the purpose of display on\n * the local screen. The StreamRole includes the desired resolution.\n * Trade-offs between quality and usage of system resources are\n * acceptable.\n */\n\n> + */\n> +\n> +/**\n> + * \\fn StreamRole::role()\n> + * \\brief Retrieve the stream role\n> + *\n> + * \\return The stream role hint\n\ns/ hint//\n\n> + */\n> +\n> +/**\n> + * \\fn StreamRole::width()\n> + * \\brief Retrieve desired width\n> + *\n> + * \\return The desired width if defined, -1 otherwise\n> + */\n> +\n> +/**\n> + * \\fn StreamRole::height()\n> + * \\brief Retrieve desired height\n> + *\n> + * \\return The desired height if defined, -1 otherwise\n> + */\n\nAfter merging Jacopo's ImgU patch series I think we could use the Size\nclass, and modify it to use -1 as a marker of invalid dimensions.\n\n> +\n> +/**\n> + * \\brief Create a stream role\n> + * \\param[in] role Stream role hint\n\nI would like to drop the word hint, but \"Stream role\" sounds weird. I\nwonder if we should rename the StreamRole class to StreamUsage in order\nto have two different words, usage and role. Feel free to propose an\nalternative for \"hint\" that wouldn't require such a rename :-)\n\n> + */\n> +StreamRole::StreamRole(Role role)\n> +\t: role_(role), width_(-1), height_(-1)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Create a stream role with dimension hints\n> + * \\param[in] role Stream role hint\n> + * \\param[in] width Desired width\n> + * \\param[in] height Desired height\n> + */\n> +StreamRole::StreamRole(Role role, int width, int height)\n> +\t: role_(role), width_(width), height_(height)\n> +{\n> +}\n> +\n> /**\n> * \\class Stream\n> * \\brief Video stream for a camera\n> @@ -78,6 +133,39 @@ namespace libcamera {\n> * optimal stream for the task.\n> */\n> \n> +/**\n> + * \\class Stream::StillCapture\n> + * \\brief Describes a still capture role\n> + */\n> +Stream::StillCapture::StillCapture()\n> +\t: StreamRole(Role::StillCapture)\n> +{\n> +}\n> +\n> +/**\n> + * \\class Stream::VideoRecording\n> + * \\brief Describes a video recording role\n> + */\n> +Stream::VideoRecording::VideoRecording()\n> +\t: StreamRole(Role::VideoRecording)\n> +{\n> +}\n> +\n> +/**\n> + * \\class Stream::Viewfinder\n> + * \\brief Describes a viewfinder role\n> + */\n> +\n> +/**\n> + * \\brief Create a viewfinder role with dimension hints\n> + * \\param[in] width Desired viewfinder width\n> + * \\param[in] height Desired viewfinder height\n> + */\n> +Stream::Viewfinder::Viewfinder(int width, int height)\n> +\t: StreamRole(Role::Viewfinder, width, height)\n> +{\n> +}\n> +\n> /**\n> * \\brief Construct a stream with default parameters\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 E251B610B3\n\tfor ;\n\tWed, 3 Apr 2019 08:57:03 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 513CD2F9;\n\tWed, 3 Apr 2019 08:57:03 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554274623;\n\tbh=HAVLB0SKNTpreQ2ftdKcQHnIlkcs2uSm07tCPTx0rXk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=coNgdspxuMUVlPlKkApc4QToZpEPyEMNffPiJbKsh7NBeMa0tiZow9iXCXSe5Goir\n\tfqN6W0l9AkGv6Kzt0BhvwriY12tBPJisBqkoo3gpkg+C6yc9nnodcyAd3gbtN9fzPd\n\tLOBL7+nhHyXp8N7xRJJ9Yg07eiNrfuBRGULXYwro=","Date":"Wed, 3 Apr 2019 09:56:52 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190403065652.GD4813@pendragon.ideasonboard.com>","References":"<20190403011221.12711-1-niklas.soderlund@ragnatech.se>\n\t<20190403011221.12711-5-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":"<20190403011221.12711-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: stream: Add basic\n\tstream roles definitions","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":"Wed, 03 Apr 2019 06:57:04 -0000"}},{"id":1249,"web_url":"https://patchwork.libcamera.org/comment/1249/","msgid":"<20190403130614.gwx5kvywyjyq4pgg@uno.localdomain>","date":"2019-04-03T13:06:14","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: stream: Add basic\n\tstream roles definitions","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"HI Niklas,\n\nOn Wed, Apr 03, 2019 at 09:56:52AM +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n>\n> Thank you for the patch.\n>\n> On Wed, Apr 03, 2019 at 03:12:20AM +0200, Niklas Söderlund wrote:\n> > In preparation of reworking how a default configuration is retrieved\n> > from a camera add stream roles. The roles will be used by applications\n> > to describe how it intends to use a camera and replace the Stream IDs\n>\n> s/it intends/they intend/\n>\n> > role when retrieving default configuration from the camera using\n\ns/Stream IDs role/Stream IDs/ ?\n\n> > streamConfiguration().\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/stream.h | 41 ++++++++++++++++++\n> > src/libcamera/stream.cpp | 88 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 129 insertions(+)\n> >\n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index 970c479627fab064..adcf20d336347dad 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -21,9 +21,50 @@ struct StreamConfiguration {\n> > \tunsigned int bufferCount;\n> > };\n> >\n> > +class StreamRole\n> > +{\n> > +public:\n> > +\tenum Role {\n> > +\t\tStillCapture,\n> > +\t\tVideoRecording,\n> > +\t\tViewfinder,\n> > +\t};\n> > +\n> > +\tRole role() const { return role_; }\n> > +\tint width() const { return width_; }\n> > +\tint height() const { return height_; }\n> > +\n> > +protected:\n> > +\tStreamRole(Role role);\n\nThis is protected so it might not matter, but constructors with 1\nparameteres should be marked explicit.\n\n> > +\tStreamRole(Role role, int width, int height);\n> > +\n> > +private:\n> > +\tRole role_;\n> > +\tint width_;\n> > +\tint height_;\n>\n> How about using the new Size structure ?\n>\n> > +};\n> > +\n> > class Stream final\n> > {\n> > public:\n> > +\tclass StillCapture : public StreamRole\n> > +\t{\n> > +\tpublic:\n> > +\t\tStillCapture();\n> > +\t};\n> > +\n> > +\tclass VideoRecording : public StreamRole\n> > +\t{\n> > +\tpublic:\n> > +\t\tVideoRecording();\n> > +\t};\n> > +\n> > +\tclass Viewfinder : public StreamRole\n> > +\t{\n> > +\tpublic:\n> > +\t\tViewfinder(int width, int height);\n> > +\t};\n> > +\n> > \tStream();\n> > \tBufferPool &bufferPool() { return bufferPool_; }\n> > \tconst StreamConfiguration &configuration() const { return configuration_; }\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index c4943c91b2e6ce13..f4be5d265e872842 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -60,6 +60,61 @@ namespace libcamera {\n> > * \\brief Requested number of buffers to allocate for the stream\n> > */\n> >\n> > +/**\n> > + * \\class StreamRole\n> > + * \\brief Stream role information\n> > + *\n> > + * The StreamRole class carries information about stream usage hints from the\n> > + * application to the camera. The camera shall take the usage hints into account\n> > + * when select which stream to use for the desired operation.\n>\n> I'd like to drop the word \"hint\" from most of the documentation. How\n> about\n>\n> The StreamRole class describes how a stream is intended to be used.\n\nor \"how an application intends to use a stream\"\n\n> Stream roles are specified by applications and passed to cameras, that\n> then select the most appropriate streams and their default\n> configurations.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\enum StreamRole::Role\n> > + * \\brief List of different stream roles\n>\n> Using the word role to name both the class and the enum makes\n> documentation a bit more difficult to write. You also need to document\n> the enum values by the way. How about something like the following ?\n>\n> /**\n> * \\enum StreamRole::Role\n> * Identify the role a stream is intended to play\n\nWe are using both third and first person verbs in the documentation.\nIe. \"Identify\" here, \"Describes\" below.\n\nAlso, \\brief ?\n\n> * \\var StillCapture\n> * The stream is intended to capture high-resolution, high-quality still\n> * images with low frame rate. The captured frames may be exposed with\n> * flash.\n> * \\var VideoRecording\n> * The stream is intended to capture video for the purpose of recording\n> * or streaming. The video stream may produce a high frame rate and may\n> * be enhanced with video stabilization.\n> * \\var Viewfinder\n> * The stream is intended to capture video for the purpose of display on\n> * the local screen. The StreamRole includes the desired resolution.\ns/The/This ?\n> * Trade-offs between quality and usage of system resources are\n> * acceptable.\n> */\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn StreamRole::role()\n> > + * \\brief Retrieve the stream role\n> > + *\n> > + * \\return The stream role hint\n>\n> s/ hint//\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn StreamRole::width()\n> > + * \\brief Retrieve desired width\n\nthe desired?\nI would s/desired/requested\n\n> > + *\n> > + * \\return The desired width if defined, -1 otherwise\n\n\", -1 otherwise\" seems like an error condition, while it is the\ndefault value of an uninitialized width.\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn StreamRole::height()\n> > + * \\brief Retrieve desired height\n> > + *\n> > + * \\return The desired height if defined, -1 otherwise\n> > + */\n>\n> After merging Jacopo's ImgU patch series I think we could use the Size\n> class, and modify it to use -1 as a marker of invalid dimensions.\n>\n\nAlso, yes. Anway, 0 should not be a valid size, and should be used as\ndefault, isn't it ?\n\n> > +\n> > +/**\n> > + * \\brief Create a stream role\n> > + * \\param[in] role Stream role hint\n>\n> I would like to drop the word hint, but \"Stream role\" sounds weird. I\n> wonder if we should rename the StreamRole class to StreamUsage in order\n> to have two different words, usage and role. Feel free to propose an\n> alternative for \"hint\" that wouldn't require such a rename :-)\n\n+1 for StreamUsage\n\nThanks\n j\n>\n> > + */\n> > +StreamRole::StreamRole(Role role)\n> > +\t: role_(role), width_(-1), height_(-1)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Create a stream role with dimension hints\n> > + * \\param[in] role Stream role hint\n> > + * \\param[in] width Desired width\n> > + * \\param[in] height Desired height\n> > + */\n> > +StreamRole::StreamRole(Role role, int width, int height)\n> > +\t: role_(role), width_(width), height_(height)\n> > +{\n> > +}\n> > +\n> > /**\n> > * \\class Stream\n> > * \\brief Video stream for a camera\n> > @@ -78,6 +133,39 @@ namespace libcamera {\n> > * optimal stream for the task.\n> > */\n> >\n> > +/**\n> > + * \\class Stream::StillCapture\n> > + * \\brief Describes a still capture role\n> > + */\n> > +Stream::StillCapture::StillCapture()\n> > +\t: StreamRole(Role::StillCapture)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\class Stream::VideoRecording\n> > + * \\brief Describes a video recording role\n> > + */\n> > +Stream::VideoRecording::VideoRecording()\n> > +\t: StreamRole(Role::VideoRecording)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\class Stream::Viewfinder\n> > + * \\brief Describes a viewfinder role\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create a viewfinder role with dimension hints\n> > + * \\param[in] width Desired viewfinder width\n> > + * \\param[in] height Desired viewfinder height\n> > + */\n> > +Stream::Viewfinder::Viewfinder(int width, int height)\n> > +\t: StreamRole(Role::Viewfinder, width, height)\n> > +{\n> > +}\n> > +\n> > /**\n> > * \\brief Construct a stream with default parameters\n> > */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3D34600FB\n\tfor ;\n\tWed, 3 Apr 2019 15:05:28 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 2F84740007;\n\tWed, 3 Apr 2019 13:05:27 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 3 Apr 2019 15:06:14 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190403130614.gwx5kvywyjyq4pgg@uno.localdomain>","References":"<20190403011221.12711-1-niklas.soderlund@ragnatech.se>\n\t<20190403011221.12711-5-niklas.soderlund@ragnatech.se>\n\t<20190403065652.GD4813@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"otv4lltn67lhho26\"","Content-Disposition":"inline","In-Reply-To":"<20190403065652.GD4813@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: stream: Add basic\n\tstream roles definitions","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":"Wed, 03 Apr 2019 13:05:29 -0000"}},{"id":1250,"web_url":"https://patchwork.libcamera.org/comment/1250/","msgid":"<20190403131030.GH12646@pendragon.ideasonboard.com>","date":"2019-04-03T13:10:30","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: stream: Add basic\n\tstream roles definitions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Apr 03, 2019 at 03:06:14PM +0200, Jacopo Mondi wrote:\n> On Wed, Apr 03, 2019 at 09:56:52AM +0300, Laurent Pinchart wrote:\n> > On Wed, Apr 03, 2019 at 03:12:20AM +0200, Niklas Söderlund wrote:\n> >> In preparation of reworking how a default configuration is retrieved\n> >> from a camera add stream roles. The roles will be used by applications\n> >> to describe how it intends to use a camera and replace the Stream IDs\n> >\n> > s/it intends/they intend/\n> >\n> >> role when retrieving default configuration from the camera using\n> \n> s/Stream IDs role/Stream IDs/ ?\n> \n> >> streamConfiguration().\n> >>\n> >> Signed-off-by: Niklas Söderlund \n> >> ---\n> >> include/libcamera/stream.h | 41 ++++++++++++++++++\n> >> src/libcamera/stream.cpp | 88 ++++++++++++++++++++++++++++++++++++++\n> >> 2 files changed, 129 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> >> index 970c479627fab064..adcf20d336347dad 100644\n> >> --- a/include/libcamera/stream.h\n> >> +++ b/include/libcamera/stream.h\n> >> @@ -21,9 +21,50 @@ struct StreamConfiguration {\n> >> \tunsigned int bufferCount;\n> >> };\n> >>\n> >> +class StreamRole\n> >> +{\n> >> +public:\n> >> +\tenum Role {\n> >> +\t\tStillCapture,\n> >> +\t\tVideoRecording,\n> >> +\t\tViewfinder,\n> >> +\t};\n> >> +\n> >> +\tRole role() const { return role_; }\n> >> +\tint width() const { return width_; }\n> >> +\tint height() const { return height_; }\n> >> +\n> >> +protected:\n> >> +\tStreamRole(Role role);\n> \n> This is protected so it might not matter, but constructors with 1\n> parameteres should be marked explicit.\n\nGood point.\n\n> >> +\tStreamRole(Role role, int width, int height);\n> >> +\n> >> +private:\n> >> +\tRole role_;\n> >> +\tint width_;\n> >> +\tint height_;\n> >\n> > How about using the new Size structure ?\n> >\n> >> +};\n> >> +\n> >> class Stream final\n> >> {\n> >> public:\n> >> +\tclass StillCapture : public StreamRole\n> >> +\t{\n> >> +\tpublic:\n> >> +\t\tStillCapture();\n> >> +\t};\n> >> +\n> >> +\tclass VideoRecording : public StreamRole\n> >> +\t{\n> >> +\tpublic:\n> >> +\t\tVideoRecording();\n> >> +\t};\n> >> +\n> >> +\tclass Viewfinder : public StreamRole\n> >> +\t{\n> >> +\tpublic:\n> >> +\t\tViewfinder(int width, int height);\n> >> +\t};\n> >> +\n> >> \tStream();\n> >> \tBufferPool &bufferPool() { return bufferPool_; }\n> >> \tconst StreamConfiguration &configuration() const { return configuration_; }\n> >> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> >> index c4943c91b2e6ce13..f4be5d265e872842 100644\n> >> --- a/src/libcamera/stream.cpp\n> >> +++ b/src/libcamera/stream.cpp\n> >> @@ -60,6 +60,61 @@ namespace libcamera {\n> >> * \\brief Requested number of buffers to allocate for the stream\n> >> */\n> >>\n> >> +/**\n> >> + * \\class StreamRole\n> >> + * \\brief Stream role information\n> >> + *\n> >> + * The StreamRole class carries information about stream usage hints from the\n> >> + * application to the camera. The camera shall take the usage hints into account\n> >> + * when select which stream to use for the desired operation.\n> >\n> > I'd like to drop the word \"hint\" from most of the documentation. How\n> > about\n> >\n> > The StreamRole class describes how a stream is intended to be used.\n> \n> or \"how an application intends to use a stream\"\n\nThat's better indeed. The active form usually feels lighter to read.\n\n> > Stream roles are specified by applications and passed to cameras, that\n> > then select the most appropriate streams and their default\n> > configurations.\n> >\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\enum StreamRole::Role\n> >> + * \\brief List of different stream roles\n> >\n> > Using the word role to name both the class and the enum makes\n> > documentation a bit more difficult to write. You also need to document\n> > the enum values by the way. How about something like the following ?\n> >\n> > /**\n> > * \\enum StreamRole::Role\n> > * Identify the role a stream is intended to play\n> \n> We are using both third and first person verbs in the documentation.\n> Ie. \"Identify\" here, \"Describes\" below.\n\nI think the library prefers the first person, so let's fix it below.\n> \n> Also, \\brief ?\n> \n> > * \\var StillCapture\n> > * The stream is intended to capture high-resolution, high-quality still\n> > * images with low frame rate. The captured frames may be exposed with\n> > * flash.\n> > * \\var VideoRecording\n> > * The stream is intended to capture video for the purpose of recording\n> > * or streaming. The video stream may produce a high frame rate and may\n> > * be enhanced with video stabilization.\n> > * \\var Viewfinder\n> > * The stream is intended to capture video for the purpose of display on\n> > * the local screen. The StreamRole includes the desired resolution.\n> \n> s/The/This ?\n\nI mean that for the Viewfinder role, the StreamRole instance includes\nthe desired resolution.\n\n> > * Trade-offs between quality and usage of system resources are\n> > * acceptable.\n> > */\n> >\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn StreamRole::role()\n> >> + * \\brief Retrieve the stream role\n> >> + *\n> >> + * \\return The stream role hint\n> >\n> > s/ hint//\n> >\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn StreamRole::width()\n> >> + * \\brief Retrieve desired width\n> \n> the desired?\n> I would s/desired/requested\n> \n> >> + *\n> >> + * \\return The desired width if defined, -1 otherwise\n> \n> \", -1 otherwise\" seems like an error condition, while it is the\n> default value of an uninitialized width.\n> \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn StreamRole::height()\n> >> + * \\brief Retrieve desired height\n> >> + *\n> >> + * \\return The desired height if defined, -1 otherwise\n> >> + */\n> >\n> > After merging Jacopo's ImgU patch series I think we could use the Size\n> > class, and modify it to use -1 as a marker of invalid dimensions.\n> \n> Also, yes. Anway, 0 should not be a valid size, and should be used as\n> default, isn't it ?\n\nI would go for -1 to mark an invalid size in general. For an image 0\nisn't valid either, but for some other usages it may. 0x0 marks an empty\nsizes, while -1x-1 would mark an invalid size.\n\n> >> +\n> >> +/**\n> >> + * \\brief Create a stream role\n> >> + * \\param[in] role Stream role hint\n> >\n> > I would like to drop the word hint, but \"Stream role\" sounds weird. I\n> > wonder if we should rename the StreamRole class to StreamUsage in order\n> > to have two different words, usage and role. Feel free to propose an\n> > alternative for \"hint\" that wouldn't require such a rename :-)\n> \n> +1 for StreamUsage\n> \n> >> + */\n> >> +StreamRole::StreamRole(Role role)\n> >> +\t: role_(role), width_(-1), height_(-1)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Create a stream role with dimension hints\n> >> + * \\param[in] role Stream role hint\n> >> + * \\param[in] width Desired width\n> >> + * \\param[in] height Desired height\n> >> + */\n> >> +StreamRole::StreamRole(Role role, int width, int height)\n> >> +\t: role_(role), width_(width), height_(height)\n> >> +{\n> >> +}\n> >> +\n> >> /**\n> >> * \\class Stream\n> >> * \\brief Video stream for a camera\n> >> @@ -78,6 +133,39 @@ namespace libcamera {\n> >> * optimal stream for the task.\n> >> */\n> >>\n> >> +/**\n> >> + * \\class Stream::StillCapture\n> >> + * \\brief Describes a still capture role\n> >> + */\n> >> +Stream::StillCapture::StillCapture()\n> >> +\t: StreamRole(Role::StillCapture)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\class Stream::VideoRecording\n> >> + * \\brief Describes a video recording role\n> >> + */\n> >> +Stream::VideoRecording::VideoRecording()\n> >> +\t: StreamRole(Role::VideoRecording)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\class Stream::Viewfinder\n> >> + * \\brief Describes a viewfinder role\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Create a viewfinder role with dimension hints\n> >> + * \\param[in] width Desired viewfinder width\n> >> + * \\param[in] height Desired viewfinder height\n> >> + */\n> >> +Stream::Viewfinder::Viewfinder(int width, int height)\n> >> +\t: StreamRole(Role::Viewfinder, width, height)\n> >> +{\n> >> +}\n> >> +\n> >> /**\n> >> * \\brief Construct a stream with default parameters\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 1C629600FB\n\tfor ;\n\tWed, 3 Apr 2019 15:10:42 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A07519AA;\n\tWed, 3 Apr 2019 15:10:41 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554297041;\n\tbh=QYzHp4CD7nz/H8y50pbM95uvGVxyF7+l46sbNtL5WvA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LC/xGtBlFgeT86zmUtS6p8HAwXzdcnGPQSVUsOKwingS2VS9zYb5PaqUXShoKDPyx\n\tdYi1SBMfknywS3+s1cO5QXZ2BUVapLGLAHEC5F0s1xOrXIg3MWAXgz9cQSf8DeY5A1\n\tseMumWAFf445uJPdomrsLwXkmq3d31v/ko1y6/lk=","Date":"Wed, 3 Apr 2019 16:10:30 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190403131030.GH12646@pendragon.ideasonboard.com>","References":"<20190403011221.12711-1-niklas.soderlund@ragnatech.se>\n\t<20190403011221.12711-5-niklas.soderlund@ragnatech.se>\n\t<20190403065652.GD4813@pendragon.ideasonboard.com>\n\t<20190403130614.gwx5kvywyjyq4pgg@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190403130614.gwx5kvywyjyq4pgg@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: stream: Add basic\n\tstream roles definitions","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":"Wed, 03 Apr 2019 13:10:42 -0000"}},{"id":1260,"web_url":"https://patchwork.libcamera.org/comment/1260/","msgid":"<20190403232007.GK23466@bigcity.dyn.berto.se>","date":"2019-04-03T23:20:07","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: stream: Add basic\n\tstream roles definitions","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 2019-04-03 09:56:52 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Wed, Apr 03, 2019 at 03:12:20AM +0200, Niklas Söderlund wrote:\n> > In preparation of reworking how a default configuration is retrieved\n> > from a camera add stream roles. The roles will be used by applications\n> > to describe how it intends to use a camera and replace the Stream IDs\n> \n> s/it intends/they intend/\n> \n> > role when retrieving default configuration from the camera using\n> > streamConfiguration().\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/stream.h | 41 ++++++++++++++++++\n> > src/libcamera/stream.cpp | 88 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 129 insertions(+)\n> > \n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index 970c479627fab064..adcf20d336347dad 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -21,9 +21,50 @@ struct StreamConfiguration {\n> > \tunsigned int bufferCount;\n> > };\n> > \n> > +class StreamRole\n> > +{\n> > +public:\n> > +\tenum Role {\n> > +\t\tStillCapture,\n> > +\t\tVideoRecording,\n> > +\t\tViewfinder,\n> > +\t};\n> > +\n> > +\tRole role() const { return role_; }\n> > +\tint width() const { return width_; }\n> > +\tint height() const { return height_; }\n> > +\n> > +protected:\n> > +\tStreamRole(Role role);\n> > +\tStreamRole(Role role, int width, int height);\n> > +\n> > +private:\n> > +\tRole role_;\n> > +\tint width_;\n> > +\tint height_;\n> \n> How about using the new Size structure ?\n\nThat is a possibility. Unfortunately the new Size structure is a private \ndatatype not exposed to applications. This might change in the future \nbut for now I will keep it as is.\n\n> \n> > +};\n> > +\n> > class Stream final\n> > {\n> > public:\n> > +\tclass StillCapture : public StreamRole\n> > +\t{\n> > +\tpublic:\n> > +\t\tStillCapture();\n> > +\t};\n> > +\n> > +\tclass VideoRecording : public StreamRole\n> > +\t{\n> > +\tpublic:\n> > +\t\tVideoRecording();\n> > +\t};\n> > +\n> > +\tclass Viewfinder : public StreamRole\n> > +\t{\n> > +\tpublic:\n> > +\t\tViewfinder(int width, int height);\n> > +\t};\n> > +\n> > \tStream();\n> > \tBufferPool &bufferPool() { return bufferPool_; }\n> > \tconst StreamConfiguration &configuration() const { return configuration_; }\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index c4943c91b2e6ce13..f4be5d265e872842 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -60,6 +60,61 @@ namespace libcamera {\n> > * \\brief Requested number of buffers to allocate for the stream\n> > */\n> > \n> > +/**\n> > + * \\class StreamRole\n> > + * \\brief Stream role information\n> > + *\n> > + * The StreamRole class carries information about stream usage hints from the\n> > + * application to the camera. The camera shall take the usage hints into account\n> > + * when select which stream to use for the desired operation.\n> \n> I'd like to drop the word \"hint\" from most of the documentation. How\n> about\n> \n> The StreamRole class describes how a stream is intended to be used.\n> Stream roles are specified by applications and passed to cameras, that\n> then select the most appropriate streams and their default\n> configurations.\n> \n> > + */\n> > +\n> > +/**\n> > + * \\enum StreamRole::Role\n> > + * \\brief List of different stream roles\n> \n> Using the word role to name both the class and the enum makes\n> documentation a bit more difficult to write. You also need to document\n> the enum values by the way. How about something like the following ?\n> \n> /**\n> * \\enum StreamRole::Role\n> * Identify the role a stream is intended to play\n> * \\var StillCapture\n> * The stream is intended to capture high-resolution, high-quality still\n> * images with low frame rate. The captured frames may be exposed with\n> * flash.\n> * \\var VideoRecording\n> * The stream is intended to capture video for the purpose of recording\n> * or streaming. The video stream may produce a high frame rate and may\n> * be enhanced with video stabilization.\n> * \\var Viewfinder\n> * The stream is intended to capture video for the purpose of display on\n> * the local screen. The StreamRole includes the desired resolution.\n> * Trade-offs between quality and usage of system resources are\n> * acceptable.\n> */\n> \n> > + */\n> > +\n> > +/**\n> > + * \\fn StreamRole::role()\n> > + * \\brief Retrieve the stream role\n> > + *\n> > + * \\return The stream role hint\n> \n> s/ hint//\n> \n> > + */\n> > +\n> > +/**\n> > + * \\fn StreamRole::width()\n> > + * \\brief Retrieve desired width\n> > + *\n> > + * \\return The desired width if defined, -1 otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn StreamRole::height()\n> > + * \\brief Retrieve desired height\n> > + *\n> > + * \\return The desired height if defined, -1 otherwise\n> > + */\n> \n> After merging Jacopo's ImgU patch series I think we could use the Size\n> class, and modify it to use -1 as a marker of invalid dimensions.\n> \n> > +\n> > +/**\n> > + * \\brief Create a stream role\n> > + * \\param[in] role Stream role hint\n> \n> I would like to drop the word hint, but \"Stream role\" sounds weird. I\n> wonder if we should rename the StreamRole class to StreamUsage in order\n> to have two different words, usage and role. Feel free to propose an\n> alternative for \"hint\" that wouldn't require such a rename :-)\n> \n> > + */\n> > +StreamRole::StreamRole(Role role)\n> > +\t: role_(role), width_(-1), height_(-1)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Create a stream role with dimension hints\n> > + * \\param[in] role Stream role hint\n> > + * \\param[in] width Desired width\n> > + * \\param[in] height Desired height\n> > + */\n> > +StreamRole::StreamRole(Role role, int width, int height)\n> > +\t: role_(role), width_(width), height_(height)\n> > +{\n> > +}\n> > +\n> > /**\n> > * \\class Stream\n> > * \\brief Video stream for a camera\n> > @@ -78,6 +133,39 @@ namespace libcamera {\n> > * optimal stream for the task.\n> > */\n> > \n> > +/**\n> > + * \\class Stream::StillCapture\n> > + * \\brief Describes a still capture role\n> > + */\n> > +Stream::StillCapture::StillCapture()\n> > +\t: StreamRole(Role::StillCapture)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\class Stream::VideoRecording\n> > + * \\brief Describes a video recording role\n> > + */\n> > +Stream::VideoRecording::VideoRecording()\n> > +\t: StreamRole(Role::VideoRecording)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\class Stream::Viewfinder\n> > + * \\brief Describes a viewfinder role\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create a viewfinder role with dimension hints\n> > + * \\param[in] width Desired viewfinder width\n> > + * \\param[in] height Desired viewfinder height\n> > + */\n> > +Stream::Viewfinder::Viewfinder(int width, int height)\n> > +\t: StreamRole(Role::Viewfinder, width, height)\n> > +{\n> > +}\n> > +\n> > /**\n> > * \\brief Construct a stream with default parameters\n> > */\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 817C1610C5\n\tfor ;\n\tThu, 4 Apr 2019 01:20:09 +0200 (CEST)","by mail-lf1-x144.google.com with SMTP id b7so340639lfg.9\n\tfor ;\n\tWed, 03 Apr 2019 16:20:09 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tl13sm3535598lji.39.2019.04.03.16.20.07\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 03 Apr 2019 16:20:07 -0700 (PDT)"],"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=XmUdp2pe+rnJWvXNCFJHqG6Vxbm9p32TC90DJPPh0fI=;\n\tb=SBOkuF/0j2clbVb3wfTzTkpmXqWse9TS68fST0UW0IjzbpauGN3IjXCP3k9i+GV9XK\n\tpiU+e2+8Er9P/qmdb5Fm99P1Lz2RTOaLSHgKB6O9INbNFHJ0erRWPyig/1G/oWu4Czup\n\t/H3iretg9BmcnPhOgfCNbUfSjDtxcvphlq1I+F/RDsm6kwcVVVM7bI4ohSCJpLBvfIB/\n\tWLXVZ2MDoG12P/eCOpVNupCvhESR+O8AEuoiRdh2e9mag0Xjy9jxNC5dGeRDYY5Q9L6d\n\t8z18Co9vq9CWbUcJwx+C5ZV0vvhSc1CxViKH6U6D5pA4aVkBdbEHOBLqT/ePpjqsYHOG\n\t3U/g==","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=XmUdp2pe+rnJWvXNCFJHqG6Vxbm9p32TC90DJPPh0fI=;\n\tb=XleqzLsW/n6HMVI5fO4l0TrziTmBsWODKNuijNMfvHoIqwLwXZwvS6qB0F08DAOMZx\n\tIdV4zgH9bTxoOUKys68M7Yj5tyeoYH8PkzTR8l3RVQsH1tF0buS3hQc7NPelueg51bCJ\n\ttPChBEDA7nwyxmz9o7XgxKlq+RNvuLTY6bRCn0xoqKTBKLnI76aV07vR1SZdLZvmmyVv\n\txIlyXbPsMlGM3Lgms6MB5c6Q+MMFFZ2l0ArRrgY51Da9+uPQUjAV4HSxI3p7IFsAlN5O\n\taasw+R6gAk/YHdufpj0z7BDxhVu4IRvsny2HHZoA4wtrfs6/uuOpy/UUZudDT2tzFwRe\n\tGj7Q==","X-Gm-Message-State":"APjAAAXMO+kVqUmfWxYu8zJrmcBylCqxit+5Js9VvGq1OtEqDVdscLbV\n\t1FxN2oWOR8TvfEc3E18IUzjbbBvNMuM=","X-Google-Smtp-Source":"APXvYqwuUYr6ifINHVWug+vc3Iv/Ynvpou5flAKyfcxPvMFaPXcIcnO238h3vXrm8tdPlWspmdo07A==","X-Received":"by 2002:ac2:43c3:: with SMTP id u3mr1311966lfl.69.1554333608612; \n\tWed, 03 Apr 2019 16:20:08 -0700 (PDT)","Date":"Thu, 4 Apr 2019 01:20:07 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190403232007.GK23466@bigcity.dyn.berto.se>","References":"<20190403011221.12711-1-niklas.soderlund@ragnatech.se>\n\t<20190403011221.12711-5-niklas.soderlund@ragnatech.se>\n\t<20190403065652.GD4813@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":"<20190403065652.GD4813@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: stream: Add basic\n\tstream roles definitions","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":"Wed, 03 Apr 2019 23:20:09 -0000"}}]