[{"id":1108,"web_url":"https://patchwork.libcamera.org/comment/1108/","msgid":"<20190323133000.GE4587@pendragon.ideasonboard.com>","date":"2019-03-23T13:30:00","subject":"Re: [libcamera-devel] [PATCH v4 16/31] libcamera: request: Store\n\tthe streams in the request","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Mar 20, 2019 at 05:30:40PM +0100, Jacopo Mondi wrote:\n> Store the streams which the request applies to and provide an accessor\n> method to return them in a set.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/request.h | 2 ++\n> src/libcamera/request.cpp | 16 ++++++++++++++++\n> 2 files changed, 18 insertions(+)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 0dbd425115e8..1bf90de2c6f9 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -34,6 +34,7 @@ public:\n> \n> \tint setBuffers(const std::map &streamMap);\n> \tBuffer *findBuffer(Stream *stream) const;\n> +\tconst std::set &streams() const { return streams_; }\n> \n> \tStatus status() const { return status_; }\n> \n> @@ -49,6 +50,7 @@ private:\n> \tCamera *camera_;\n> \tstd::map bufferMap_;\n> \tstd::unordered_set pending_;\n> +\tstd::set streams_;\n\nThis duplicates the keys stored in bufferMap_. One option to avoid this\nwould be to implement the streams() function as\n\nstd::set &Request::streams() const\n{\n\tstd::set streams;\n\tfor (auto const &it: bufferMap_)\n\t\tstreams.insert(it.first);\n\treturn streams;\n}\n\nThis comes at an additional cost at runtime, whether this is preferable\nor not depends on the usage pattern, how many times do you expect this\nfunction to be called per request ?\n\nDepending on how the caller(s) use the returned set, an std::vector\ncould also be more efficient (slower lookup but faster push_back).\n\n> \n> \tStatus status_;\n> };\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index e0e77e972411..22c516208ede 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -51,6 +51,13 @@ Request::Request(Camera *camera)\n> {\n> }\n> \n> +/**\n> + * \\fn Request::streams()\n> + * \\brief Retrieve the set of streams contained in the request\n> + *\n> + * \\return The set of streams contained in the request\n> + */\n> +\n> /**\n> * \\brief Set the streams to capture with associated buffers\n> * \\param[in] streamMap The map of streams to buffers\n> @@ -65,6 +72,10 @@ int Request::setBuffers(const std::map &streamMap)\n> \t}\n> \n> \tbufferMap_ = streamMap;\n> +\n> +\tfor (const auto &map : streamMap)\n> +\t\tstreams_.insert(map.first);\n> +\n> \treturn 0;\n> }\n> \n> @@ -77,6 +88,11 @@ int Request::setBuffers(const std::map &streamMap)\n> * map.\n> */\n> \n> +/**\n> + * \\var Request::streams_\n> + * \\brief Set of streams in this request\n> + */\n> +\n> /**\n> * \\brief Return the buffer associated with a stream\n> * \\param[in] stream The stream the buffer is associated to","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 30838610B6\n\tfor ;\n\tSat, 23 Mar 2019 14:30:14 +0100 (CET)","from pendragon.ideasonboard.com\n\t(p5269001-ipngn11702marunouchi.tokyo.ocn.ne.jp [114.158.195.1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6FB1849;\n\tSat, 23 Mar 2019 14:30:12 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1553347813;\n\tbh=Ot/sdAn3njinWQml0OkNkQ3gejpyGVsAlAp5F1ZiobM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j3ccsx197QEeOjKTclzR7PNUwiow8kurFVDwS1pBYQ/doQ8TpqyXhQl810Fddk3EA\n\t28+j+7TyWSmN04j1fthKyc8GbTCwvFffNra/aQfBq5T0LPk9/ZNDjHA1RxoYtIKy39\n\t3tCU6l8BauiavcHa+GQ1aPmw8i+IKb7FRenVVfLI=","Date":"Sat, 23 Mar 2019 15:30:00 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190323133000.GE4587@pendragon.ideasonboard.com>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-17-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190320163055.22056-17-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 16/31] libcamera: request: Store\n\tthe streams in the request","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, 23 Mar 2019 13:30:14 -0000"}},{"id":1147,"web_url":"https://patchwork.libcamera.org/comment/1147/","msgid":"<20190327082801.7bbzlo2amixxp67v@uno.localdomain>","date":"2019-03-27T08:28:01","subject":"Re: [libcamera-devel] [PATCH v4 16/31] libcamera: request: Store\n\tthe streams in the request","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Mar 23, 2019 at 03:30:00PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Mar 20, 2019 at 05:30:40PM +0100, Jacopo Mondi wrote:\n> > Store the streams which the request applies to and provide an accessor\n> > method to return them in a set.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/request.h | 2 ++\n> > src/libcamera/request.cpp | 16 ++++++++++++++++\n> > 2 files changed, 18 insertions(+)\n> >\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 0dbd425115e8..1bf90de2c6f9 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -34,6 +34,7 @@ public:\n> >\n> > \tint setBuffers(const std::map &streamMap);\n> > \tBuffer *findBuffer(Stream *stream) const;\n> > +\tconst std::set &streams() const { return streams_; }\n> >\n> > \tStatus status() const { return status_; }\n> >\n> > @@ -49,6 +50,7 @@ private:\n> > \tCamera *camera_;\n> > \tstd::map bufferMap_;\n> > \tstd::unordered_set pending_;\n> > +\tstd::set streams_;\n>\n> This duplicates the keys stored in bufferMap_. One option to avoid this\n> would be to implement the streams() function as\n\nI don't think this is big, at all, we do expect a few streams per\ncamera, and the space required is for a few pointers and a container.\n\n>\n> std::set &Request::streams() const\n> {\n> \tstd::set streams;\n> \tfor (auto const &it: bufferMap_)\n> \t\tstreams.insert(it.first);\n> \treturn streams;\n> }\n>\n> This comes at an additional cost at runtime, whether this is preferable\n> or not depends on the usage pattern, how many times do you expect this\n> function to be called per request ?\n>\n\nIf we really want to profile this though, I do expect this to be\ncalled by pipeline handlers only at queueRequest time, where they do\nreceive a Request as paramter. Applications do set the buffers on the\nrequest, but they might want to have them back from the request itself\ntoo. So I expect this to be called once from pipeline handlers and\neventually by applications.\n\n> Depending on how the caller(s) use the returned set, an std::vector\n> could also be more efficient (slower lookup but faster push_back).\n\nI used std:set for consistency, as it is the container used by the\nCamera and the Request class when dealing with streams, but I do\nexpect callers to iterate on the stream list, and indeed we have to\npush the Stream * back when constructing it, so a vector might make\nmore sense.\n\nThanks\n j\n\n>\n> >\n> > \tStatus status_;\n> > };\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index e0e77e972411..22c516208ede 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -51,6 +51,13 @@ Request::Request(Camera *camera)\n> > {\n> > }\n> >\n> > +/**\n> > + * \\fn Request::streams()\n> > + * \\brief Retrieve the set of streams contained in the request\n> > + *\n> > + * \\return The set of streams contained in the request\n> > + */\n> > +\n> > /**\n> > * \\brief Set the streams to capture with associated buffers\n> > * \\param[in] streamMap The map of streams to buffers\n> > @@ -65,6 +72,10 @@ int Request::setBuffers(const std::map &streamMap)\n> > \t}\n> >\n> > \tbufferMap_ = streamMap;\n> > +\n> > +\tfor (const auto &map : streamMap)\n> > +\t\tstreams_.insert(map.first);\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -77,6 +88,11 @@ int Request::setBuffers(const std::map &streamMap)\n> > * map.\n> > */\n> >\n> > +/**\n> > + * \\var Request::streams_\n> > + * \\brief Set of streams in this request\n> > + */\n> > +\n> > /**\n> > * \\brief Return the buffer associated with a stream\n> > * \\param[in] stream The stream the buffer is associated to\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E20C1610B3\n\tfor ;\n\tWed, 27 Mar 2019 09:27:19 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 5A61FE000D;\n\tWed, 27 Mar 2019 08:27:18 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 27 Mar 2019 09:28:01 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190327082801.7bbzlo2amixxp67v@uno.localdomain>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-17-jacopo@jmondi.org>\n\t<20190323133000.GE4587@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"f6fjtjotwnrevu77\"","Content-Disposition":"inline","In-Reply-To":"<20190323133000.GE4587@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 16/31] libcamera: request: Store\n\tthe streams in the request","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, 27 Mar 2019 08:27:20 -0000"}},{"id":1159,"web_url":"https://patchwork.libcamera.org/comment/1159/","msgid":"<20190401214504.GI4787@pendragon.ideasonboard.com>","date":"2019-04-01T21:45:04","subject":"Re: [libcamera-devel] [PATCH v4 16/31] libcamera: request: Store\n\tthe streams in the request","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, Mar 27, 2019 at 09:28:01AM +0100, Jacopo Mondi wrote:\n> On Sat, Mar 23, 2019 at 03:30:00PM +0200, Laurent Pinchart wrote:\n> > On Wed, Mar 20, 2019 at 05:30:40PM +0100, Jacopo Mondi wrote:\n> >> Store the streams which the request applies to and provide an accessor\n> >> method to return them in a set.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> include/libcamera/request.h | 2 ++\n> >> src/libcamera/request.cpp | 16 ++++++++++++++++\n> >> 2 files changed, 18 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> >> index 0dbd425115e8..1bf90de2c6f9 100644\n> >> --- a/include/libcamera/request.h\n> >> +++ b/include/libcamera/request.h\n> >> @@ -34,6 +34,7 @@ public:\n> >>\n> >> \tint setBuffers(const std::map &streamMap);\n> >> \tBuffer *findBuffer(Stream *stream) const;\n> >> +\tconst std::set &streams() const { return streams_; }\n> >>\n> >> \tStatus status() const { return status_; }\n> >>\n> >> @@ -49,6 +50,7 @@ private:\n> >> \tCamera *camera_;\n> >> \tstd::map bufferMap_;\n> >> \tstd::unordered_set pending_;\n> >> +\tstd::set streams_;\n> >\n> > This duplicates the keys stored in bufferMap_. One option to avoid this\n> > would be to implement the streams() function as\n> \n> I don't think this is big, at all, we do expect a few streams per\n> camera, and the space required is for a few pointers and a container.\n\nIt's not just a matter of space, storing two separate copies of the same\ninformation is usually a way to get them out of sync at some point.\nUnless there's a good reason to do so (which could include various forms\nof optimization), it should be avoided.\n\n> > std::set &Request::streams() const\n> > {\n> > \tstd::set streams;\n> > \tfor (auto const &it: bufferMap_)\n> > \t\tstreams.insert(it.first);\n> > \treturn streams;\n> > }\n> >\n> > This comes at an additional cost at runtime, whether this is preferable\n> > or not depends on the usage pattern, how many times do you expect this\n> > function to be called per request ?\n> \n> If we really want to profile this though, I do expect this to be\n> called by pipeline handlers only at queueRequest time, where they do\n> receive a Request as paramter. Applications do set the buffers on the\n> request, but they might want to have them back from the request itself\n> too. So I expect this to be called once from pipeline handlers and\n> eventually by applications.\n> \n> > Depending on how the caller(s) use the returned set, an std::vector\n> > could also be more efficient (slower lookup but faster push_back).\n> \n> I used std:set for consistency, as it is the container used by the\n> Camera and the Request class when dealing with streams, but I do\n> expect callers to iterate on the stream list, and indeed we have to\n> push the Stream * back when constructing it, so a vector might make\n> more sense.\n\nstd::set is useful as an input parameter to methods of the Camera class\nto make it impossible for the caller to store multiple instances of the\nsame object in the container. This avoids potentialy bugs by making them\nimpossible in the first place. To pass information back to application,\nusage of other types of containers isn't ruled out. We have to decide in\neach case which container is the best for the task at hand.\n\n> >>\n> >> \tStatus status_;\n> >> };\n> >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> >> index e0e77e972411..22c516208ede 100644\n> >> --- a/src/libcamera/request.cpp\n> >> +++ b/src/libcamera/request.cpp\n> >> @@ -51,6 +51,13 @@ Request::Request(Camera *camera)\n> >> {\n> >> }\n> >>\n> >> +/**\n> >> + * \\fn Request::streams()\n> >> + * \\brief Retrieve the set of streams contained in the request\n> >> + *\n> >> + * \\return The set of streams contained in the request\n> >> + */\n> >> +\n> >> /**\n> >> * \\brief Set the streams to capture with associated buffers\n> >> * \\param[in] streamMap The map of streams to buffers\n> >> @@ -65,6 +72,10 @@ int Request::setBuffers(const std::map &streamMap)\n> >> \t}\n> >>\n> >> \tbufferMap_ = streamMap;\n> >> +\n> >> +\tfor (const auto &map : streamMap)\n> >> +\t\tstreams_.insert(map.first);\n> >> +\n> >> \treturn 0;\n> >> }\n> >>\n> >> @@ -77,6 +88,11 @@ int Request::setBuffers(const std::map &streamMap)\n> >> * map.\n> >> */\n> >>\n> >> +/**\n> >> + * \\var Request::streams_\n> >> + * \\brief Set of streams in this request\n> >> + */\n> >> +\n> >> /**\n> >> * \\brief Return the buffer associated with a stream\n> >> * \\param[in] stream The stream the buffer is associated to","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 15B40610B3\n\tfor ;\n\tMon, 1 Apr 2019 23:45:16 +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 615E7542;\n\tMon, 1 Apr 2019 23:45:15 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554155115;\n\tbh=cfpkS9wRFfs3EDJR3GDlI4AUGl5ixkeDjS4wbfE4WHo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wWwshJNzAYSCx+L3QWzUQ0RbZYJZTWpq9fS9F/nsYNAhtCvUrk4DAgh53J8LQrzGi\n\tkihfQueoaurIDm0tN/hl022ybRuNqgP24cw3RzkhWL3SMZ/4R1d8pEFy8+2woABs1+\n\t9AQDut9e/cE2Ktt8ypP3A0qM24S09AhpQCdc3Rmw=","Date":"Tue, 2 Apr 2019 00:45:04 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190401214504.GI4787@pendragon.ideasonboard.com>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-17-jacopo@jmondi.org>\n\t<20190323133000.GE4587@pendragon.ideasonboard.com>\n\t<20190327082801.7bbzlo2amixxp67v@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190327082801.7bbzlo2amixxp67v@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 16/31] libcamera: request: Store\n\tthe streams in the request","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, 01 Apr 2019 21:45:16 -0000"}}]