[{"id":1213,"web_url":"https://patchwork.libcamera.org/comment/1213/","msgid":"<20190402154902.GH4805@pendragon.ideasonboard.com>","date":"2019-04-02T15:49:02","subject":"Re: [libcamera-devel] [PATCH 3/4] cam: Extend request complete\n\thandler to deal with multiple streams","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\nIn the subject line, s/complete/completion/\n\nOn Tue, Apr 02, 2019 at 02:54:05AM +0200, Niklas Söderlund wrote:\n> The completion handler needs to handle all buffers in the request. Solve\n> this by iterating over all buffers in the completed and assign each\n\ns/completed/completed request/ ?\n\n> stream a name as we encounter them. The buffer writer needs to be\n> extended to learn about stream names so it can prefix the files it\n> writes with it.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/buffer_writer.cpp | 4 ++--\n> src/cam/buffer_writer.h | 2 +-\n> src/cam/main.cpp | 41 +++++++++++++++++++++++++++------------\n> 3 files changed, 32 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index 2d2258b4cd1cbbc2..02a9610b8b3caa31 100644\n> --- a/src/cam/buffer_writer.cpp\n> +++ b/src/cam/buffer_writer.cpp\n> @@ -19,13 +19,13 @@ BufferWriter::BufferWriter(const std::string &pattern)\n> {\n> }\n> \n> -int BufferWriter::write(libcamera::Buffer *buffer)\n> +int BufferWriter::write(libcamera::Buffer *buffer, const std::string &prefix)\n> {\n> \tstd::string filename;\n> \tsize_t pos;\n> \tint fd, ret = 0;\n> \n> -\tfilename = pattern_;\n> +\tfilename = prefix + \"-\" + pattern_;\n\nShould the pattern syntax be extended to include a placeholder for the\nstream name ?\n\n> \tpos = filename.find_first_of('#');\n> \tif (pos != std::string::npos) {\n> \t\tstd::stringstream ss;\n> diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> index 9705773e0e397d45..ebc6c24cd8f8d43a 100644\n> --- a/src/cam/buffer_writer.h\n> +++ b/src/cam/buffer_writer.h\n> @@ -16,7 +16,7 @@ class BufferWriter\n> public:\n> \tBufferWriter(const std::string &pattern = \"frame-#.bin\");\n> \n> -\tint write(libcamera::Buffer *buffer);\n> +\tint write(libcamera::Buffer *buffer, const std::string &prefix);\n\ns/prefix/streamName/ ?\n\n> \n> private:\n> \tstd::string pattern_;\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index 1bf28ca8eb8c6da7..6aed4073f70d37a2 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -143,28 +143,45 @@ static int prepare_camera_config(std::map *config\n> \treturn 0;\n> }\n> \n> +static std::string streamToName(const Stream *stream)\n> +{\n> +\tstatic std::map names;\n> +\n> +\tif (names.find(stream) == names.end())\n> +\t\tnames[stream] = std::string(\"stream\") + std::to_string(names.size());\n> +\n> +\treturn names[stream];\n\nThis will end up creating stream names based on the order of the streams\nto buffers map, and there's no clear guarantee it will match the streams\norder as specified on the command line. I think it would be best to\ncreate the names map at init time to ensure consistent naming.\n\nI also wonder if the --stream option should support a name parameter.\n\n> +}\n> +\n> static void requestComplete(Request *request, const std::map &buffers)\n> {\n> \tstatic uint64_t last = 0;\n> +\tdouble fps = 0.0;\n> \n> \tif (request->status() == Request::RequestCancelled)\n> \t\treturn;\n> \n> -\tBuffer *buffer = buffers.begin()->second;\n> +\tfor (auto const &it : buffers) {\n> +\t\tStream *stream = it.first;\n> +\t\tBuffer *buffer = it.second;\n> +\t\tstd::string name = streamToName(stream);\n> \n> -\tdouble fps = buffer->timestamp() - last;\n> -\tfps = last && fps ? 1000000000.0 / fps : 0.0;\n> -\tlast = buffer->timestamp();\n> +\t\tif (fps == 0.0) {\n\nHow about if (it == buffers.begin()) to emphasize you compute the fps\nfor the first stream only ? I got a bit confused at first.\n\n> +\t\t\tfps = buffer->timestamp() - last;\n> +\t\t\tfps = last && fps ? 1000000000.0 / fps : 0.0;\n> +\t\t\tlast = buffer->timestamp();\n> +\t\t}\n> \n> -\tstd::cout << \"seq: \" << std::setw(6) << std::setfill('0') << buffer->sequence()\n> -\t\t << \" buf: \" << buffer->index()\n> -\t\t << \" bytesused: \" << buffer->bytesused()\n> -\t\t << \" timestamp: \" << buffer->timestamp()\n> -\t\t << \" fps: \" << std::fixed << std::setprecision(2) << fps\n> -\t\t << std::endl;\n> +\t\tstd::cout << name << \" seq: \" << std::setw(6) << std::setfill('0') << buffer->sequence()\n\nCould we wrap this a bit ?\n\n> +\t\t\t << \" buf: \" << buffer->index()\n> +\t\t\t << \" bytesused: \" << buffer->bytesused()\n> +\t\t\t << \" timestamp: \" << buffer->timestamp()\n> +\t\t\t << \" fps: \" << std::fixed << std::setprecision(2) << fps\n> +\t\t\t << std::endl;\n> \n> -\tif (writer)\n> -\t\twriter->write(buffer);\n> +\t\tif (writer)\n> +\t\t\twriter->write(buffer, name);\n> +\t}\n> \n> \trequest = camera->createRequest();\n> \tif (!request) {","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 88A67600FB\n\tfor ;\n\tTue, 2 Apr 2019 17:49:13 +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 F135F2F9;\n\tTue, 2 Apr 2019 17:49:12 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554220153;\n\tbh=n4LfkPIp7ErKBVzrSF8pUxhPvlnrrSCKM8FslfhPpTQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MU8Frmme0cNih+T3ZqFe0fljrnx1eh6zO/8fydEOgghc9qoVbFl30YhxBcpoX/xKj\n\tYNz4IL3Obm9jpWsmaea1A1PqCK0lPgNieYJv1Y70Cd70l+DPgMzQULpCsXSZXFXJGu\n\tDcN/Byipot5IJi5V1r9bJmxBBIbEyjWz+9IQo0DQ=","Date":"Tue, 2 Apr 2019 18:49:02 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402154902.GH4805@pendragon.ideasonboard.com>","References":"<20190402005406.25097-1-niklas.soderlund@ragnatech.se>\n\t<20190402005406.25097-4-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":"<20190402005406.25097-4-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/4] cam: Extend request complete\n\thandler to deal with multiple streams","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":"Tue, 02 Apr 2019 15:49:13 -0000"}},{"id":1235,"web_url":"https://patchwork.libcamera.org/comment/1235/","msgid":"<20190403003008.GJ23466@bigcity.dyn.berto.se>","date":"2019-04-03T00:30:08","subject":"Re: [libcamera-devel] [PATCH 3/4] cam: Extend request complete\n\thandler to deal with multiple streams","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-02 18:49:02 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> In the subject line, s/complete/completion/\n> \n> On Tue, Apr 02, 2019 at 02:54:05AM +0200, Niklas Söderlund wrote:\n> > The completion handler needs to handle all buffers in the request. Solve\n> > this by iterating over all buffers in the completed and assign each\n> \n> s/completed/completed request/ ?\n> \n> > stream a name as we encounter them. The buffer writer needs to be\n> > extended to learn about stream names so it can prefix the files it\n> > writes with it.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/cam/buffer_writer.cpp | 4 ++--\n> > src/cam/buffer_writer.h | 2 +-\n> > src/cam/main.cpp | 41 +++++++++++++++++++++++++++------------\n> > 3 files changed, 32 insertions(+), 15 deletions(-)\n> > \n> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > index 2d2258b4cd1cbbc2..02a9610b8b3caa31 100644\n> > --- a/src/cam/buffer_writer.cpp\n> > +++ b/src/cam/buffer_writer.cpp\n> > @@ -19,13 +19,13 @@ BufferWriter::BufferWriter(const std::string &pattern)\n> > {\n> > }\n> > \n> > -int BufferWriter::write(libcamera::Buffer *buffer)\n> > +int BufferWriter::write(libcamera::Buffer *buffer, const std::string &prefix)\n> > {\n> > \tstd::string filename;\n> > \tsize_t pos;\n> > \tint fd, ret = 0;\n> > \n> > -\tfilename = pattern_;\n> > +\tfilename = prefix + \"-\" + pattern_;\n> \n> Should the pattern syntax be extended to include a placeholder for the\n> stream name ?\n\nIt could be done, but maybe it's providing too much customization then \nit's worth? I'm open to this but in a follow up patch, see bellow.\n\n> \n> > \tpos = filename.find_first_of('#');\n> > \tif (pos != std::string::npos) {\n> > \t\tstd::stringstream ss;\n> > diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> > index 9705773e0e397d45..ebc6c24cd8f8d43a 100644\n> > --- a/src/cam/buffer_writer.h\n> > +++ b/src/cam/buffer_writer.h\n> > @@ -16,7 +16,7 @@ class BufferWriter\n> > public:\n> > \tBufferWriter(const std::string &pattern = \"frame-#.bin\");\n> > \n> > -\tint write(libcamera::Buffer *buffer);\n> > +\tint write(libcamera::Buffer *buffer, const std::string &prefix);\n> \n> s/prefix/streamName/ ?\n> \n> > \n> > private:\n> > \tstd::string pattern_;\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index 1bf28ca8eb8c6da7..6aed4073f70d37a2 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -143,28 +143,45 @@ static int prepare_camera_config(std::map *config\n> > \treturn 0;\n> > }\n> > \n> > +static std::string streamToName(const Stream *stream)\n> > +{\n> > +\tstatic std::map names;\n> > +\n> > +\tif (names.find(stream) == names.end())\n> > +\t\tnames[stream] = std::string(\"stream\") + std::to_string(names.size());\n> > +\n> > +\treturn names[stream];\n> \n> This will end up creating stream names based on the order of the streams\n> to buffers map, and there's no clear guarantee it will match the streams\n> order as specified on the command line. I think it would be best to\n> create the names map at init time to ensure consistent naming.\n\nI agree and it should be fixed, this will however depend on the issue \nyou point out in the roles sereis that it's hard to associate the stream \nreturned from streamConfiguration() to the roles requested. Lets fix \nboth issues as a series on top.\n\n> \n> I also wonder if the --stream option should support a name parameter.\n\nAgain in the category if it's feature bloat or not ;-)\n\n> \n> > +}\n> > +\n> > static void requestComplete(Request *request, const std::map &buffers)\n> > {\n> > \tstatic uint64_t last = 0;\n> > +\tdouble fps = 0.0;\n> > \n> > \tif (request->status() == Request::RequestCancelled)\n> > \t\treturn;\n> > \n> > -\tBuffer *buffer = buffers.begin()->second;\n> > +\tfor (auto const &it : buffers) {\n> > +\t\tStream *stream = it.first;\n> > +\t\tBuffer *buffer = it.second;\n> > +\t\tstd::string name = streamToName(stream);\n> > \n> > -\tdouble fps = buffer->timestamp() - last;\n> > -\tfps = last && fps ? 1000000000.0 / fps : 0.0;\n> > -\tlast = buffer->timestamp();\n> > +\t\tif (fps == 0.0) {\n> \n> How about if (it == buffers.begin()) to emphasize you compute the fps\n> for the first stream only ? I got a bit confused at first.\n\nGood point.\n\n> \n> > +\t\t\tfps = buffer->timestamp() - last;\n> > +\t\t\tfps = last && fps ? 1000000000.0 / fps : 0.0;\n> > +\t\t\tlast = buffer->timestamp();\n> > +\t\t}\n> > \n> > -\tstd::cout << \"seq: \" << std::setw(6) << std::setfill('0') << buffer->sequence()\n> > -\t\t << \" buf: \" << buffer->index()\n> > -\t\t << \" bytesused: \" << buffer->bytesused()\n> > -\t\t << \" timestamp: \" << buffer->timestamp()\n> > -\t\t << \" fps: \" << std::fixed << std::setprecision(2) << fps\n> > -\t\t << std::endl;\n> > +\t\tstd::cout << name << \" seq: \" << std::setw(6) << std::setfill('0') << buffer->sequence()\n> \n> Could we wrap this a bit ?\n> \n> > +\t\t\t << \" buf: \" << buffer->index()\n> > +\t\t\t << \" bytesused: \" << buffer->bytesused()\n> > +\t\t\t << \" timestamp: \" << buffer->timestamp()\n> > +\t\t\t << \" fps: \" << std::fixed << std::setprecision(2) << fps\n> > +\t\t\t << std::endl;\n> > \n> > -\tif (writer)\n> > -\t\twriter->write(buffer);\n> > +\t\tif (writer)\n> > +\t\t\twriter->write(buffer, name);\n> > +\t}\n> > \n> > \trequest = camera->createRequest();\n> > \tif (!request) {\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEFCC610B3\n\tfor ;\n\tWed, 3 Apr 2019 02:30:09 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id k8so4839989lja.8\n\tfor ;\n\tTue, 02 Apr 2019 17:30: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\te17sm3001745ljj.20.2019.04.02.17.30.08\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 02 Apr 2019 17:30:08 -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=ezbsoT8fpa+rVKwPaZQ/4Gb+dAGZoMCzz+ZlCY/pEAw=;\n\tb=G3pjX1DtmwzQtljD5T+u+dALLGWrUrcVUPz7AX9avSk9BriR97lcqneKqnFkrkP6rF\n\tSOjyaP3JswDXgc9vim+EEQRXNqHjgk5EmNQYTROzAgYLJmvcUG+KD3rM/16zn22GZeSs\n\trOPPro87ossO1FfLc4q7yEQIq8IVJiRItXSP9fwNVK2Vj3UHLRMACCUlvz4HmBBK5ySX\n\tVNC5RIYd3dYXLxV997loNcpYiEZuYgIhITdoITCqC8H897WK9JX0sHwsr2aLBiAe1b9e\n\tKCXkZbaJCASNeh2H8nSe1KpfBjjIVZrbZ/Khq5A583oDscQqnmaqNAtbl7udKcjNbseL\n\tlV7A==","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=ezbsoT8fpa+rVKwPaZQ/4Gb+dAGZoMCzz+ZlCY/pEAw=;\n\tb=hXjHwaPkqWNksvXlMVtJ4duJn0dlADSnu1rdR1A6v16EAYpRBvNxyyerEwlBAgSS9F\n\tX2CnNif6ypuREawALCJ+lrXRYeOW7HYImEtY9jWgY7BtU2OWppYnW8cfbFSL3rTFgWFp\n\tGMwVCqu+FI88y1e1YXpld8cm4f2cLLqGwEbcVAmQgqVv1Qvzz4xs73CI57lasnp8i/2z\n\tI+cNGfNlspNLjo4IrY/FjVQMkjF0w8XNXGE5zu2W40udRTVmvZvKeCcOKnHINHmpoJGh\n\tiRHQPag1SUHtHgSBTEWSmBEfOnDYLSKD1RaYhGt95m+gNI15o3nRqjxJY6td6LCnvCz6\n\tQ7xg==","X-Gm-Message-State":"APjAAAWI3TRiveEA4jv3ZsYb+UXkghokXbSf/qq4onxlInIZ2+1EmwjF\n\t4sfZst/Ds4j6l8oSZNT46Ufm32RVbKc=","X-Google-Smtp-Source":"APXvYqwS94hUVgHSIrO1pfnJHmNyX6TMjzCdxdL3qDnMdUiDB0lSXWAYHZHQ7nk1Tr2gvXYdzb0l8g==","X-Received":"by 2002:a2e:93c7:: with SMTP id p7mr31623704ljh.32.1554251409280;\n\tTue, 02 Apr 2019 17:30:09 -0700 (PDT)","Date":"Wed, 3 Apr 2019 02:30:08 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190403003008.GJ23466@bigcity.dyn.berto.se>","References":"<20190402005406.25097-1-niklas.soderlund@ragnatech.se>\n\t<20190402005406.25097-4-niklas.soderlund@ragnatech.se>\n\t<20190402154902.GH4805@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":"<20190402154902.GH4805@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH 3/4] cam: Extend request complete\n\thandler to deal with multiple streams","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 00:30:10 -0000"}}]