[{"id":1304,"web_url":"https://patchwork.libcamera.org/comment/1304/","msgid":"<20190406172340.GG4817@pendragon.ideasonboard.com>","date":"2019-04-06T17:23:40","subject":"Re: [libcamera-devel] [PATCH v3 4/5] cam: Extend request completion\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\nOn Sat, Apr 06, 2019 at 01:59:28AM +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 request. The streams\n> are named automatically streamX, where X is the order of how the stream\n\ns/how/which/\n\n> was passed to configureStream().\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/main.cpp | 39 +++++++++++++++++++++++++++------------\n> 1 file changed, 27 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index da05612b1c347b31..da5ca3402ce1a823 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -23,6 +23,7 @@ using namespace libcamera;\n> \n> OptionsParser::Options options;\n> std::shared_ptr camera;\n> +std::map streamInfo;\n> EventLoop *loop;\n> BufferWriter *writer;\n\nUnrelated to this patch, we should really create a class to hold all\nthis.\n\n> \n> @@ -87,9 +88,12 @@ static int prepareCameraConfig(CameraConfiguration *config)\n> {\n> \tstd::vector roles;\n> \n> +\tstreamInfo.clear();\n> +\n> \t/* If no configuration is provided assume a single video stream. */\n> \tif (!options.isSet(OptStream)) {\n> \t\t*config = camera->streamConfiguration({ Stream::VideoRecording() });\n> +\t\tstreamInfo[config->front()] = \"stream0\";\n> \t\treturn 0;\n> \t}\n> \n> @@ -142,31 +146,42 @@ static int prepareCameraConfig(CameraConfiguration *config)\n> \t\t\t(*config)[stream].pixelFormat = conf[\"pixelformat\"];\n> \t}\n> \n> +\tfor (unsigned int i = 0; i < config->size(); i++)\n> +\t\tstreamInfo[(*config)[i]] = \"stream\" + std::to_string(i);\n\nHow about using iterators ?\n\n\tindex = 0;\n\tfor (Stream *stream : *config) {\n\t\tstreamInfo[stream] = \"stream\" + std::to_string(index);\n\t\tindex++;\n\t}\n\n> +\n> \treturn 0;\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 it = buffers.begin(); it != buffers.end(); ++it) {\n> +\t\tStream *stream = it->first;\n> +\t\tBuffer *buffer = it->second;\n> +\t\tstd::string name = streamInfo[stream];\n> \n> -\tdouble fps = buffer->timestamp() - last;\n> -\tfps = last && fps ? 1000000000.0 / fps : 0.0;\n> -\tlast = buffer->timestamp();\n> +\t\tif (it == buffers.begin()) {\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)\n> +\t\t\t << std::setfill('0') << buffer->sequence()\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\nAs the fps is per-request and not per-stream we should ideally print it\nonce only. One option would be to print a single line per request that\ncontains per-request information as well as per-stream information for\neach stream, but that may be difficult to read. Another option would be\nto first print per-request information, and then one line for each\nstream. This may be best, otherwise the log\n\nstream0 seq:...\nstream1 seq:...\n\ncould be understood as either one request containing both streams, or\ntwo requests containing stream0 and stream1 respectively.\n\nI'm tempted to see if we could go for the single line per request\noption while keeping it readable enough.\n\n> +\t\t\t << std::endl;\n> \n> -\tif (writer)\n> -\t\twriter->write(buffer, \"stream0\");\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 ABE7960B1B\n\tfor ;\n\tSat, 6 Apr 2019 19:23:52 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [91.183.39.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A663599F;\n\tSat, 6 Apr 2019 19:23:51 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554571432;\n\tbh=S/p5OFUiopLXfHUKiS4FPdT4R1dAFK6ek/XB6268soQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NSoy5ABwm5osSLVSuBtkrKDMijQCtG+LJPVIExEFF94Y68INPiaYeKMZtB4f1sfow\n\t0GG00dojR7OT8C2lw5LXPZ0ArzYV6AIxQtLM5CKyizqfQzK/JQl2NRiE9zIjyeQ2JN\n\tJGk9uWOFHAuXtYwWRn89JcG947JS62ejDXk9BPkw=","Date":"Sat, 6 Apr 2019 20:23:40 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190406172340.GG4817@pendragon.ideasonboard.com>","References":"<20190405235929.27987-1-niklas.soderlund@ragnatech.se>\n\t<20190405235929.27987-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":"<20190405235929.27987-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] cam: Extend request completion\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":"Sat, 06 Apr 2019 17:23:52 -0000"}},{"id":1317,"web_url":"https://patchwork.libcamera.org/comment/1317/","msgid":"<20190408132430.GI15350@bigcity.dyn.berto.se>","date":"2019-04-08T13:24:30","subject":"Re: [libcamera-devel] [PATCH v3 4/5] cam: Extend request completion\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-06 20:23:40 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sat, Apr 06, 2019 at 01:59:28AM +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 request. The streams\n> > are named automatically streamX, where X is the order of how the stream\n> \n> s/how/which/\n> \n> > was passed to configureStream().\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/cam/main.cpp | 39 +++++++++++++++++++++++++++------------\n> > 1 file changed, 27 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index da05612b1c347b31..da5ca3402ce1a823 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -23,6 +23,7 @@ using namespace libcamera;\n> > \n> > OptionsParser::Options options;\n> > std::shared_ptr camera;\n> > +std::map streamInfo;\n> > EventLoop *loop;\n> > BufferWriter *writer;\n> \n> Unrelated to this patch, we should really create a class to hold all\n> this.\n\nI agree, I feel bad when I have to add to this hack. I will try to find \ntime to rework this in a follow up patch.\n\n> \n> > \n> > @@ -87,9 +88,12 @@ static int prepareCameraConfig(CameraConfiguration *config)\n> > {\n> > \tstd::vector roles;\n> > \n> > +\tstreamInfo.clear();\n> > +\n> > \t/* If no configuration is provided assume a single video stream. */\n> > \tif (!options.isSet(OptStream)) {\n> > \t\t*config = camera->streamConfiguration({ Stream::VideoRecording() });\n> > +\t\tstreamInfo[config->front()] = \"stream0\";\n> > \t\treturn 0;\n> > \t}\n> > \n> > @@ -142,31 +146,42 @@ static int prepareCameraConfig(CameraConfiguration *config)\n> > \t\t\t(*config)[stream].pixelFormat = conf[\"pixelformat\"];\n> > \t}\n> > \n> > +\tfor (unsigned int i = 0; i < config->size(); i++)\n> > +\t\tstreamInfo[(*config)[i]] = \"stream\" + std::to_string(i);\n> \n> How about using iterators ?\n> \n> \tindex = 0;\n> \tfor (Stream *stream : *config) {\n> \t\tstreamInfo[stream] = \"stream\" + std::to_string(index);\n> \t\tindex++;\n> \t}\n\nSure.\n\n> \n> > +\n> > \treturn 0;\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 it = buffers.begin(); it != buffers.end(); ++it) {\n> > +\t\tStream *stream = it->first;\n> > +\t\tBuffer *buffer = it->second;\n> > +\t\tstd::string name = streamInfo[stream];\n> > \n> > -\tdouble fps = buffer->timestamp() - last;\n> > -\tfps = last && fps ? 1000000000.0 / fps : 0.0;\n> > -\tlast = buffer->timestamp();\n> > +\t\tif (it == buffers.begin()) {\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)\n> > +\t\t\t << std::setfill('0') << buffer->sequence()\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> \n> As the fps is per-request and not per-stream we should ideally print it\n> once only. One option would be to print a single line per request that\n> contains per-request information as well as per-stream information for\n> each stream, but that may be difficult to read. Another option would be\n> to first print per-request information, and then one line for each\n> stream. This may be best, otherwise the log\n> \n> stream0 seq:...\n> stream1 seq:...\n> \n> could be understood as either one request containing both streams, or\n> two requests containing stream0 and stream1 respectively.\n> \n> I'm tempted to see if we could go for the single line per request\n> option while keeping it readable enough.\n\nI had an attempt of makig this a single line, lets see how it plays out.\n\n> \n> > +\t\t\t << std::endl;\n> > \n> > -\tif (writer)\n> > -\t\twriter->write(buffer, \"stream0\");\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-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D16A960004\n\tfor ;\n\tMon, 8 Apr 2019 15:24:32 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id f23so11285751ljc.0\n\tfor ;\n\tMon, 08 Apr 2019 06:24:32 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tq5sm5997350lfn.85.2019.04.08.06.24.31\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 08 Apr 2019 06:24:31 -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=No61CVCfO/yfRyLyi5vmI3yucaeNC3CM2plRDst4Ac8=;\n\tb=Pm9wfSyX2Wnreuac7w4PDsG/FH653IEkIYlmlkDyQm11GLrqwiwjWeFAbBArMXZZDr\n\tFmw+ZXgU23YCiLCXz+ux4MffH3JE/MlCZW97y9Z1xfVAQrdE8DLdAewiNrqLcSUgA3Fo\n\trKgrTEcLGhitv+vchukDULvwcxuGkwwmlB9X2kMaFRL2I+I3u89wxR8PnYkiJCsZaPuD\n\tIv2nKTK1IReMYcloKbMibzgjLEdv8s2eeWu69ss069lq98HapAGO8IhFtpRQ2nhShE83\n\tFJFSypzgnGEtm421P0KPGB4qqElgDPky+B3eaH6K+Qg60CzJHa3wbAXpgIReYy69LYZX\n\tr8iw==","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=No61CVCfO/yfRyLyi5vmI3yucaeNC3CM2plRDst4Ac8=;\n\tb=V+Vy95ZUploXKvkUKfq44Dg5bRFFoCa2bliF1H8hegXbxjE2B1nazE+vLh+4zHgrFH\n\tXTo2TgTnAIDFeXfpo46O8/JBnA2CbVkLz9vWk50/s/C3rkmY6+IChPrIZh7vYOC3XTYa\n\tJtm52B/5mKN2JBKuuprJ9CDcIUS+iJJsLyqx0TsXOuWPiA+pMICayoezj0EBRwrABiKz\n\tZgEW1T7d0vOBq4vcR2flnXUuZNVohEPtwgI8tNaqW1CoInEcq5bfHb3pALhr99GXlV7b\n\tR5nDXC4g7n50MFwi5+yZ60b0SSYmUb1pqf0cb08xNJRxraQsbLvA9ca65taFqvnTr+Zr\n\t2NWA==","X-Gm-Message-State":"APjAAAVOxNF2V8Mea3HYuxiXEWeYNLwtoP6P1IstMNCzdj2rtniceAbw\n\t0uemULu6FQM2UMEIq4NevGKLJ+CqP6M=","X-Google-Smtp-Source":"APXvYqyLIznKjeUHHl0MdntKzmwRWVGYUnijDaE9sgtdTEK8dqqfx8rLd1NSJ3+c0/VRDYtUYhAmoQ==","X-Received":"by 2002:a2e:7c02:: with SMTP id\n\tx2mr12851626ljc.176.1554729872042; \n\tMon, 08 Apr 2019 06:24:32 -0700 (PDT)","Date":"Mon, 8 Apr 2019 15:24:30 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408132430.GI15350@bigcity.dyn.berto.se>","References":"<20190405235929.27987-1-niklas.soderlund@ragnatech.se>\n\t<20190405235929.27987-5-niklas.soderlund@ragnatech.se>\n\t<20190406172340.GG4817@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":"<20190406172340.GG4817@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] cam: Extend request completion\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":"Mon, 08 Apr 2019 13:24:33 -0000"}}]