[{"id":4863,"web_url":"https://patchwork.libcamera.org/comment/4863/","msgid":"<20200519143814.GN470768@oden.dyn.berto.se>","date":"2020-05-19T14:38:14","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","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 work.\n\nOn 2020-05-19 06:25:00 +0300, Laurent Pinchart wrote:\n> Make the BufferWriter class inherit from FrameSink, and use the\n> FrameSink API to manage it. This makes the code more generic, and will\n> allow usage of other sinks.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/cam/buffer_writer.cpp | 25 ++++++++++---\n> src/cam/buffer_writer.h | 13 ++++---\n> src/cam/capture.cpp | 76 ++++++++++++++++++++++++++++++++-------\n> src/cam/capture.h | 6 ++--\n> 4 files changed, 97 insertions(+), 23 deletions(-)\n> \n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index c5a5eb46224a..2bec4b132155 100644\n> --- a/src/cam/buffer_writer.cpp\n> +++ b/src/cam/buffer_writer.cpp\n> @@ -13,6 +13,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"buffer_writer.h\"\n> \n> using namespace libcamera;\n> @@ -32,6 +34,21 @@ BufferWriter::~BufferWriter()\n> \tmappedBuffers_.clear();\n> }\n> \n> +int BufferWriter::configure(const libcamera::CameraConfiguration &config)\n> +{\n> +\tint ret = FrameSink::configure(config);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tstreamNames_.clear();\n> +\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> +\t\tconst StreamConfiguration &cfg = config.at(index);\n> +\t\tstreamNames_[cfg.stream()] = \"stream\" + std::to_string(index);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> {\n> \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> @@ -43,7 +60,7 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> \t}\n> }\n> \n> -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> +bool BufferWriter::consumeBuffer(const Stream *stream, FrameBuffer *buffer)\n> {\n> \tstd::string filename;\n> \tsize_t pos;\n> @@ -53,7 +70,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> \tpos = filename.find_first_of('#');\n> \tif (pos != std::string::npos) {\n> \t\tstd::stringstream ss;\n> -\t\tss << streamName << \"-\" << std::setw(6)\n> +\t\tss << streamNames_[stream] << \"-\" << std::setw(6)\n> \t\t << std::setfill('0') << buffer->metadata().sequence;\n> \t\tfilename.replace(pos, 1, ss.str());\n> \t}\n> @@ -62,7 +79,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> \t\t (pos == std::string::npos ? O_APPEND : O_TRUNC),\n> \t\t S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);\n> \tif (fd == -1)\n> -\t\treturn -errno;\n> +\t\treturn true;\n\nWould it make sens to log an error here as the error no longer is \npropagated to the caller?\n\n> \n> \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> \t\tvoid *data = mappedBuffers_[plane.fd.fd()].first;\n> @@ -84,5 +101,5 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> \n> \tclose(fd);\n> \n> -\treturn ret;\n> +\treturn true;\n> }\n> diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> index 47e26103e13e..5a5b176f73d8 100644\n> --- a/src/cam/buffer_writer.h\n> +++ b/src/cam/buffer_writer.h\n> @@ -12,18 +12,23 @@\n> \n> #include \n> \n> -class BufferWriter\n> +#include \"frame_sink.h\"\n> +\n> +class BufferWriter : public FrameSink\n> {\n> public:\n> \tBufferWriter(const std::string &pattern = \"frame-#.bin\");\n> \t~BufferWriter();\n> \n> -\tvoid mapBuffer(libcamera::FrameBuffer *buffer);\n> +\tint configure(const libcamera::CameraConfiguration &config) override;\n> \n> -\tint write(libcamera::FrameBuffer *buffer,\n> -\t\t const std::string &streamName);\n> +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> +\n> +\tbool consumeBuffer(const libcamera::Stream *stream,\n> +\t\t\t libcamera::FrameBuffer *buffer) override;\n> \n> private:\n> +\tstd::map streamNames_;\n> \tstd::string pattern_;\n> \tstd::map> mappedBuffers_;\n> };\n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index b7e06bcc9463..7fc9cba48892 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> \n> +#include \"buffer_writer.h\"\n> #include \"capture.h\"\n> #include \"main.h\"\n> \n> @@ -18,7 +19,7 @@ using namespace libcamera;\n> \n> Capture::Capture(std::shared_ptr camera, CameraConfiguration *config,\n> \t\t const StreamRoles &roles)\n> -\t: camera_(camera), config_(config), roles_(roles), writer_(nullptr)\n> +\t: camera_(camera), config_(config), roles_(roles), sink_(nullptr)\n> {\n> }\n> \n> @@ -47,20 +48,28 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n> \n> \tif (options.isSet(OptFile)) {\n> \t\tif (!options[OptFile].toString().empty())\n> -\t\t\twriter_ = new BufferWriter(options[OptFile]);\n> +\t\t\tsink_ = new BufferWriter(options[OptFile]);\n> \t\telse\n> -\t\t\twriter_ = new BufferWriter();\n> +\t\t\tsink_ = new BufferWriter();\n> \t}\n> \n> +\tif (sink_) {\n> +\t\tret = sink_->configure(*config_);\n> +\t\tif (ret < 0) {\n> +\t\t\tstd::cout << \"Failed to configure frame sink\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tsink_->bufferReleased.connect(this, &Capture::sinkRelease);\n> +\t}\n> \n> \tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> \n> \tret = capture(loop, allocator);\n> \n> -\tif (options.isSet(OptFile)) {\n> -\t\tdelete writer_;\n> -\t\twriter_ = nullptr;\n> -\t}\n> +\tdelete sink_;\n> +\tsink_ = nullptr;\n> \n> \tdelete allocator;\n> \n> @@ -110,16 +119,26 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> \t\t\t\treturn ret;\n> \t\t\t}\n> \n> -\t\t\tif (writer_)\n> -\t\t\t\twriter_->mapBuffer(buffer.get());\n> +\t\t\tif (sink_)\n> +\t\t\t\tsink_->mapBuffer(buffer.get());\n> \t\t}\n> \n> \t\trequests.push_back(request);\n> \t}\n> \n> +\tif (sink_) {\n> +\t\tret = sink_->start();\n> +\t\tif (ret) {\n> +\t\t\tstd::cout << \"Failed to start frame sink\" << std::endl;\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\n> \tret = camera_->start();\n> \tif (ret) {\n> \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> +\t\tif (sink_)\n> +\t\t\tsink_->stop();\n> \t\treturn ret;\n> \t}\n> \n> @@ -128,6 +147,8 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> \t\tif (ret < 0) {\n> \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> \t\t\tcamera_->stop();\n> +\t\t\tif (sink_)\n> +\t\t\t\tsink_->stop();\n> \t\t\treturn ret;\n> \t\t}\n> \t}\n> @@ -141,6 +162,12 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> \tif (ret)\n> \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> \n> +\tif (sink_) {\n> +\t\tret = sink_->stop();\n> +\t\tif (ret)\n> +\t\t\tstd::cout << \"Failed to stop frame sink\" << std::endl;\n> +\t}\n> +\n> \treturn ret;\n> }\n> \n> @@ -157,17 +184,18 @@ void Capture::requestComplete(Request *request)\n> \t ? 1000.0 / fps : 0.0;\n> \tlast_ = now;\n> \n> +\tbool requeue = true;\n> +\n> \tstd::stringstream info;\n> \tinfo << \"fps: \" << std::fixed << std::setprecision(2) << fps;\n> \n> \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> \t\tStream *stream = it->first;\n> \t\tFrameBuffer *buffer = it->second;\n> -\t\tconst std::string &name = streamName_[stream];\n> \n> \t\tconst FrameMetadata &metadata = buffer->metadata();\n> \n> -\t\tinfo << \" \" << name\n> +\t\tinfo << \" \" << streamName_[stream]\n> \t\t << \" seq: \" << std::setw(6) << std::setfill('0') << metadata.sequence\n> \t\t << \" bytesused: \";\n> \n> @@ -178,12 +206,21 @@ void Capture::requestComplete(Request *request)\n> \t\t\t\tinfo << \"/\";\n> \t\t}\n> \n> -\t\tif (writer_)\n> -\t\t\twriter_->write(buffer, name);\n> +\t\tif (sink_) {\n> +\t\t\tif (!sink_->consumeBuffer(stream, buffer))\n> +\t\t\t\trequeue = false;\n\nThis will only work for requests that contains one buffer. In this \nchange it will works as BufferWriter::consumeBuffer() returns true and \nthe while request is requeued as the buffer writer does not need to hold \nonto the buffers. I know that the new sink added later in this sereis \nonly supports one stream so it returning false and holding onto the \nbuffer will also work with this logic.\n\nWould it instead make sens to rework this so that the sink always would \nneed to signal that it's done with the buffer(s) instead of requing the \nrequest from the requestComplete() callback? That way the behavior the \nsinks would be the same disregarding for how long it needs to hold on to \nthe buffer.\n\n> +\t\t}\n> \t}\n> \n> \tstd::cout << info.str() << std::endl;\n> \n> +\t/*\n> +\t * If the frame sink holds on the buffer, we'll requeue it later in the\n> +\t * complete handler.\n> +\t */\n> +\tif (!requeue)\n> +\t\treturn;\n> +\n> \t/*\n> \t * Create a new request and populate it with one buffer for each\n> \t * stream.\n> @@ -203,3 +240,16 @@ void Capture::requestComplete(Request *request)\n> \n> \tcamera_->queueRequest(request);\n> }\n> +\n> +void Capture::sinkRelease(libcamera::FrameBuffer *buffer)\n> +{\n> +\tRequest *request = camera_->createRequest();\n> +\tif (!request) {\n> +\t\tstd::cerr << \"Can't create request\" << std::endl;\n> +\t\treturn;\n> +\t}\n> +\n> +\trequest->addBuffer(config_->at(0).stream(), buffer);\n> +\n> +\tcamera_->queueRequest(request);\n> +}\n> diff --git a/src/cam/capture.h b/src/cam/capture.h\n> index c0e697b831fb..3be1d98e4d3e 100644\n> --- a/src/cam/capture.h\n> +++ b/src/cam/capture.h\n> @@ -16,10 +16,11 @@\n> #include \n> #include \n> \n> -#include \"buffer_writer.h\"\n> #include \"event_loop.h\"\n> #include \"options.h\"\n> \n> +class FrameSink;\n> +\n> class Capture\n> {\n> public:\n> @@ -33,13 +34,14 @@ private:\n> \t\t libcamera::FrameBufferAllocator *allocator);\n> \n> \tvoid requestComplete(libcamera::Request *request);\n> +\tvoid sinkRelease(libcamera::FrameBuffer *buffer);\n> \n> \tstd::shared_ptr camera_;\n> \tlibcamera::CameraConfiguration *config_;\n> \tlibcamera::StreamRoles roles_;\n> \n> \tstd::map streamName_;\n> -\tBufferWriter *writer_;\n> +\tFrameSink *sink_;\n> \tstd::chrono::steady_clock::time_point last_;\n> };\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \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 mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D983C603D9\n\tfor ;\n\tTue, 19 May 2020 16:38:16 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id g4so3257ljl.2\n\tfor ;\n\tTue, 19 May 2020 07:38:16 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tk24sm7546537ljg.92.2020.05.19.07.38.15\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 19 May 2020 07:38:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"oc5fwixi\"; \n\tdkim-atps=neutral","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\tbh=FDaBoAnUJ6AAmuBXHOQsXkk21JOOjHq0flgejlZRhM0=;\n\tb=oc5fwixivBomPFsHrEhUQ4OXN7P2oi9PUcGglbFa/Ag0DbXRapbxV2N0eo088gfL02\n\tI5Q/PAOC44hnVGk2D+yB1hwE6AZL7Yv6lSD3l0VIKePWanpBkfyY2ayzE7jOHcvmoLKL\n\tbl7Fy7HnJMs6X/Lq5pC9w67+lpI2OnKLqcFNT0RTaf53zJWsTw0z8IxxcvAczwns5dDk\n\tmCSik3Wb1Rn4fJENMSo2U0NKzaHlpabcSuTjawxqxqchlJn0+3yZXzMV4QQVJHSUToFM\n\txlxAwy5HOLi5hsy8pUoNBiOO5cPAU5dL5eGCB4sH+DCqJdNsS+vIkU68C/En/VoJchgn\n\tWxcw==","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;\n\tbh=FDaBoAnUJ6AAmuBXHOQsXkk21JOOjHq0flgejlZRhM0=;\n\tb=oO9qq/gVuatmFQfhCC3pMe1V6IiAW/63IwuyDD7i22UIldbekO1TQODWkV0bKHAiJU\n\tTQr8/MJmcT/vf5HkrPBL60FXZCEdh+0nyg/DzdEnC9a20LIRN/7+jWCoFlnGrm2nK6Zk\n\tRlq/lqnw7UkPyjS47QfYhMz0r7nET1l+f8qSRHQgVciBAW9diWK7w7WR+gaoW21NTaCE\n\tZjiYpoTFQbAMTTu1xFecfp/YoXqVn/gQID0ZDgJ9BU3ZSCmWpzFFwgnEhcmi/4tfwxa1\n\tyLVqfh+7/mkDKwZ8LDQWTdOMrFtScGp1MqhzmDa5bp+ipSZUVa6fNRft53IEKMdyzDCX\n\t63Vw==","X-Gm-Message-State":"AOAM533yFDZGHkc3d2VCjggrl50YOGJlpkZ2TT3oY39NAEISGyFvKos/\n\tXCE8BbfqWEevgbrxSJ8ZC5hYCLHAkVYNng==","X-Google-Smtp-Source":"ABdhPJxHugJd1JTzKY2GiLXuOfwR5/Au7U527fCMccCcwsQ3Q8stiamF6jsisp7DXA6LNki9V+SncA==","X-Received":"by 2002:a05:651c:490:: with SMTP id\n\ts16mr13751465ljc.152.1589899095978; \n\tTue, 19 May 2020 07:38:15 -0700 (PDT)","Date":"Tue, 19 May 2020 16:38:14 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200519143814.GN470768@oden.dyn.berto.se>","References":"<20200519032505.17307-1-laurent.pinchart@ideasonboard.com>\n\t<20200519032505.17307-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200519032505.17307-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 19 May 2020 14:38:17 -0000"}},{"id":4867,"web_url":"https://patchwork.libcamera.org/comment/4867/","msgid":"<20200519144332.GF3820@pendragon.ideasonboard.com>","date":"2020-05-19T14:43:32","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Tue, May 19, 2020 at 04:38:14PM +0200, Niklas Söderlund wrote:\n> On 2020-05-19 06:25:00 +0300, Laurent Pinchart wrote:\n> > Make the BufferWriter class inherit from FrameSink, and use the\n> > FrameSink API to manage it. This makes the code more generic, and will\n> > allow usage of other sinks.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/cam/buffer_writer.cpp | 25 ++++++++++---\n> > src/cam/buffer_writer.h | 13 ++++---\n> > src/cam/capture.cpp | 76 ++++++++++++++++++++++++++++++++-------\n> > src/cam/capture.h | 6 ++--\n> > 4 files changed, 97 insertions(+), 23 deletions(-)\n> > \n> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > index c5a5eb46224a..2bec4b132155 100644\n> > --- a/src/cam/buffer_writer.cpp\n> > +++ b/src/cam/buffer_writer.cpp\n> > @@ -13,6 +13,8 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > +\n> > #include \"buffer_writer.h\"\n> > \n> > using namespace libcamera;\n> > @@ -32,6 +34,21 @@ BufferWriter::~BufferWriter()\n> > \tmappedBuffers_.clear();\n> > }\n> > \n> > +int BufferWriter::configure(const libcamera::CameraConfiguration &config)\n> > +{\n> > +\tint ret = FrameSink::configure(config);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tstreamNames_.clear();\n> > +\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> > +\t\tconst StreamConfiguration &cfg = config.at(index);\n> > +\t\tstreamNames_[cfg.stream()] = \"stream\" + std::to_string(index);\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > {\n> > \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > @@ -43,7 +60,7 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > \t}\n> > }\n> > \n> > -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > +bool BufferWriter::consumeBuffer(const Stream *stream, FrameBuffer *buffer)\n> > {\n> > \tstd::string filename;\n> > \tsize_t pos;\n> > @@ -53,7 +70,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > \tpos = filename.find_first_of('#');\n> > \tif (pos != std::string::npos) {\n> > \t\tstd::stringstream ss;\n> > -\t\tss << streamName << \"-\" << std::setw(6)\n> > +\t\tss << streamNames_[stream] << \"-\" << std::setw(6)\n> > \t\t << std::setfill('0') << buffer->metadata().sequence;\n> > \t\tfilename.replace(pos, 1, ss.str());\n> > \t}\n> > @@ -62,7 +79,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > \t\t (pos == std::string::npos ? O_APPEND : O_TRUNC),\n> > \t\t S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);\n> > \tif (fd == -1)\n> > -\t\treturn -errno;\n> > +\t\treturn true;\n> \n> Would it make sens to log an error here as the error no longer is \n> propagated to the caller?\n\nGood point, I'll do so.\n\n> > \n> > \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > \t\tvoid *data = mappedBuffers_[plane.fd.fd()].first;\n> > @@ -84,5 +101,5 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > \n> > \tclose(fd);\n> > \n> > -\treturn ret;\n> > +\treturn true;\n> > }\n> > diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> > index 47e26103e13e..5a5b176f73d8 100644\n> > --- a/src/cam/buffer_writer.h\n> > +++ b/src/cam/buffer_writer.h\n> > @@ -12,18 +12,23 @@\n> > \n> > #include \n> > \n> > -class BufferWriter\n> > +#include \"frame_sink.h\"\n> > +\n> > +class BufferWriter : public FrameSink\n> > {\n> > public:\n> > \tBufferWriter(const std::string &pattern = \"frame-#.bin\");\n> > \t~BufferWriter();\n> > \n> > -\tvoid mapBuffer(libcamera::FrameBuffer *buffer);\n> > +\tint configure(const libcamera::CameraConfiguration &config) override;\n> > \n> > -\tint write(libcamera::FrameBuffer *buffer,\n> > -\t\t const std::string &streamName);\n> > +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > +\n> > +\tbool consumeBuffer(const libcamera::Stream *stream,\n> > +\t\t\t libcamera::FrameBuffer *buffer) override;\n> > \n> > private:\n> > +\tstd::map streamNames_;\n> > \tstd::string pattern_;\n> > \tstd::map> mappedBuffers_;\n> > };\n> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > index b7e06bcc9463..7fc9cba48892 100644\n> > --- a/src/cam/capture.cpp\n> > +++ b/src/cam/capture.cpp\n> > @@ -11,6 +11,7 @@\n> > #include \n> > #include \n> > \n> > +#include \"buffer_writer.h\"\n> > #include \"capture.h\"\n> > #include \"main.h\"\n> > \n> > @@ -18,7 +19,7 @@ using namespace libcamera;\n> > \n> > Capture::Capture(std::shared_ptr camera, CameraConfiguration *config,\n> > \t\t const StreamRoles &roles)\n> > -\t: camera_(camera), config_(config), roles_(roles), writer_(nullptr)\n> > +\t: camera_(camera), config_(config), roles_(roles), sink_(nullptr)\n> > {\n> > }\n> > \n> > @@ -47,20 +48,28 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n> > \n> > \tif (options.isSet(OptFile)) {\n> > \t\tif (!options[OptFile].toString().empty())\n> > -\t\t\twriter_ = new BufferWriter(options[OptFile]);\n> > +\t\t\tsink_ = new BufferWriter(options[OptFile]);\n> > \t\telse\n> > -\t\t\twriter_ = new BufferWriter();\n> > +\t\t\tsink_ = new BufferWriter();\n> > \t}\n> > \n> > +\tif (sink_) {\n> > +\t\tret = sink_->configure(*config_);\n> > +\t\tif (ret < 0) {\n> > +\t\t\tstd::cout << \"Failed to configure frame sink\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tsink_->bufferReleased.connect(this, &Capture::sinkRelease);\n> > +\t}\n> > \n> > \tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> > \n> > \tret = capture(loop, allocator);\n> > \n> > -\tif (options.isSet(OptFile)) {\n> > -\t\tdelete writer_;\n> > -\t\twriter_ = nullptr;\n> > -\t}\n> > +\tdelete sink_;\n> > +\tsink_ = nullptr;\n> > \n> > \tdelete allocator;\n> > \n> > @@ -110,16 +119,26 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > \t\t\t\treturn ret;\n> > \t\t\t}\n> > \n> > -\t\t\tif (writer_)\n> > -\t\t\t\twriter_->mapBuffer(buffer.get());\n> > +\t\t\tif (sink_)\n> > +\t\t\t\tsink_->mapBuffer(buffer.get());\n> > \t\t}\n> > \n> > \t\trequests.push_back(request);\n> > \t}\n> > \n> > +\tif (sink_) {\n> > +\t\tret = sink_->start();\n> > +\t\tif (ret) {\n> > +\t\t\tstd::cout << \"Failed to start frame sink\" << std::endl;\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\t}\n> > +\n> > \tret = camera_->start();\n> > \tif (ret) {\n> > \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> > +\t\tif (sink_)\n> > +\t\t\tsink_->stop();\n> > \t\treturn ret;\n> > \t}\n> > \n> > @@ -128,6 +147,8 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > \t\tif (ret < 0) {\n> > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > \t\t\tcamera_->stop();\n> > +\t\t\tif (sink_)\n> > +\t\t\t\tsink_->stop();\n> > \t\t\treturn ret;\n> > \t\t}\n> > \t}\n> > @@ -141,6 +162,12 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > \tif (ret)\n> > \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> > \n> > +\tif (sink_) {\n> > +\t\tret = sink_->stop();\n> > +\t\tif (ret)\n> > +\t\t\tstd::cout << \"Failed to stop frame sink\" << std::endl;\n> > +\t}\n> > +\n> > \treturn ret;\n> > }\n> > \n> > @@ -157,17 +184,18 @@ void Capture::requestComplete(Request *request)\n> > \t ? 1000.0 / fps : 0.0;\n> > \tlast_ = now;\n> > \n> > +\tbool requeue = true;\n> > +\n> > \tstd::stringstream info;\n> > \tinfo << \"fps: \" << std::fixed << std::setprecision(2) << fps;\n> > \n> > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > \t\tStream *stream = it->first;\n> > \t\tFrameBuffer *buffer = it->second;\n> > -\t\tconst std::string &name = streamName_[stream];\n> > \n> > \t\tconst FrameMetadata &metadata = buffer->metadata();\n> > \n> > -\t\tinfo << \" \" << name\n> > +\t\tinfo << \" \" << streamName_[stream]\n> > \t\t << \" seq: \" << std::setw(6) << std::setfill('0') << metadata.sequence\n> > \t\t << \" bytesused: \";\n> > \n> > @@ -178,12 +206,21 @@ void Capture::requestComplete(Request *request)\n> > \t\t\t\tinfo << \"/\";\n> > \t\t}\n> > \n> > -\t\tif (writer_)\n> > -\t\t\twriter_->write(buffer, name);\n> > +\t\tif (sink_) {\n> > +\t\t\tif (!sink_->consumeBuffer(stream, buffer))\n> > +\t\t\t\trequeue = false;\n> \n> This will only work for requests that contains one buffer. In this \n> change it will works as BufferWriter::consumeBuffer() returns true and \n> the while request is requeued as the buffer writer does not need to hold \n> onto the buffers. I know that the new sink added later in this sereis \n> only supports one stream so it returning false and holding onto the \n> buffer will also work with this logic.\n\nDamn, you noticed ;-)\n\n> Would it instead make sens to rework this so that the sink always would \n> need to signal that it's done with the buffer(s) instead of requing the \n> request from the requestComplete() callback? That way the behavior the \n> sinks would be the same disregarding for how long it needs to hold on to \n> the buffer.\n\nI think this mechanism is indeed a bit fragile, and needs to be\nreworked. It works in the two cases we support (file sink and kms sink),\nbut isn't future-proof. That being said, fixing it properly isn't\ntrivial. We can only requeue the request when all buffers have been\nprocessed (or, rather, maybe we'll need to requeue a request with other\nbuffers instead). We can't unconditionally queue a new request in the\nbufferReleased callback of the sink, as that would only work for a\nsingle stream.\n\nIn addition to that, I think the cam application needs to be reworked to\nmove processing of buffers to its main thread. The file sink, in\nparticular, is too expensive to run in the request completion callback,\nwhich runs in the camera manager thread. I think it could make sense to\naddress the two issues together. Do you think it needs to be done as\npart of this series ? Or could I just add a \\todo to explain the issue\nand make sure we don't forget about it ?\n\n> > +\t\t}\n> > \t}\n> > \n> > \tstd::cout << info.str() << std::endl;\n> > \n> > +\t/*\n> > +\t * If the frame sink holds on the buffer, we'll requeue it later in the\n> > +\t * complete handler.\n> > +\t */\n> > +\tif (!requeue)\n> > +\t\treturn;\n> > +\n> > \t/*\n> > \t * Create a new request and populate it with one buffer for each\n> > \t * stream.\n> > @@ -203,3 +240,16 @@ void Capture::requestComplete(Request *request)\n> > \n> > \tcamera_->queueRequest(request);\n> > }\n> > +\n> > +void Capture::sinkRelease(libcamera::FrameBuffer *buffer)\n> > +{\n> > +\tRequest *request = camera_->createRequest();\n> > +\tif (!request) {\n> > +\t\tstd::cerr << \"Can't create request\" << std::endl;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\trequest->addBuffer(config_->at(0).stream(), buffer);\n> > +\n> > +\tcamera_->queueRequest(request);\n> > +}\n> > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > index c0e697b831fb..3be1d98e4d3e 100644\n> > --- a/src/cam/capture.h\n> > +++ b/src/cam/capture.h\n> > @@ -16,10 +16,11 @@\n> > #include \n> > #include \n> > \n> > -#include \"buffer_writer.h\"\n> > #include \"event_loop.h\"\n> > #include \"options.h\"\n> > \n> > +class FrameSink;\n> > +\n> > class Capture\n> > {\n> > public:\n> > @@ -33,13 +34,14 @@ private:\n> > \t\t libcamera::FrameBufferAllocator *allocator);\n> > \n> > \tvoid requestComplete(libcamera::Request *request);\n> > +\tvoid sinkRelease(libcamera::FrameBuffer *buffer);\n> > \n> > \tstd::shared_ptr camera_;\n> > \tlibcamera::CameraConfiguration *config_;\n> > \tlibcamera::StreamRoles roles_;\n> > \n> > \tstd::map streamName_;\n> > -\tBufferWriter *writer_;\n> > +\tFrameSink *sink_;\n> > \tstd::chrono::steady_clock::time_point last_;\n> > };\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 360FE603D9\n\tfor ;\n\tTue, 19 May 2020 16:43:44 +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 3F19C30C;\n\tTue, 19 May 2020 16:43:43 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gRJtdjNr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1589899423;\n\tbh=wQWT2Y+FI03od67obEEZjUC/ul4esIGUxGTi58YGxTE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gRJtdjNr5zrtACUq8ADlPu4k+TYB5XYXHmvksxMo89AL2sTvBpFhlAPnoDPtNf6PI\n\tA3MnPidQeCQgb/OCqoC7OL5c72XcyqMHwdF1DpXrL/rBTWQK5dDJ0W4fZHYyfH7E8T\n\tTrtz3kn+X970oJcA0cwOk2IILFOpfFQRpweEL9Rc=","Date":"Tue, 19 May 2020 17:43:32 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200519144332.GF3820@pendragon.ideasonboard.com>","References":"<20200519032505.17307-1-laurent.pinchart@ideasonboard.com>\n\t<20200519032505.17307-4-laurent.pinchart@ideasonboard.com>\n\t<20200519143814.GN470768@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200519143814.GN470768@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 19 May 2020 14:43:44 -0000"}},{"id":4868,"web_url":"https://patchwork.libcamera.org/comment/4868/","msgid":"<20200519145928.GP470768@oden.dyn.berto.se>","date":"2020-05-19T14:59:28","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2020-05-19 17:43:32 +0300, Laurent Pinchart wrote:\n> Hi Laurent,\n> \n> On Tue, May 19, 2020 at 04:38:14PM +0200, Niklas Söderlund wrote:\n> > On 2020-05-19 06:25:00 +0300, Laurent Pinchart wrote:\n> > > Make the BufferWriter class inherit from FrameSink, and use the\n> > > FrameSink API to manage it. This makes the code more generic, and will\n> > > allow usage of other sinks.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/cam/buffer_writer.cpp | 25 ++++++++++---\n> > > src/cam/buffer_writer.h | 13 ++++---\n> > > src/cam/capture.cpp | 76 ++++++++++++++++++++++++++++++++-------\n> > > src/cam/capture.h | 6 ++--\n> > > 4 files changed, 97 insertions(+), 23 deletions(-)\n> > > \n> > > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > > index c5a5eb46224a..2bec4b132155 100644\n> > > --- a/src/cam/buffer_writer.cpp\n> > > +++ b/src/cam/buffer_writer.cpp\n> > > @@ -13,6 +13,8 @@\n> > > #include \n> > > #include \n> > > \n> > > +#include \n> > > +\n> > > #include \"buffer_writer.h\"\n> > > \n> > > using namespace libcamera;\n> > > @@ -32,6 +34,21 @@ BufferWriter::~BufferWriter()\n> > > \tmappedBuffers_.clear();\n> > > }\n> > > \n> > > +int BufferWriter::configure(const libcamera::CameraConfiguration &config)\n> > > +{\n> > > +\tint ret = FrameSink::configure(config);\n> > > +\tif (ret < 0)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tstreamNames_.clear();\n> > > +\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> > > +\t\tconst StreamConfiguration &cfg = config.at(index);\n> > > +\t\tstreamNames_[cfg.stream()] = \"stream\" + std::to_string(index);\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > > {\n> > > \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > > @@ -43,7 +60,7 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > > \t}\n> > > }\n> > > \n> > > -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > +bool BufferWriter::consumeBuffer(const Stream *stream, FrameBuffer *buffer)\n> > > {\n> > > \tstd::string filename;\n> > > \tsize_t pos;\n> > > @@ -53,7 +70,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > \tpos = filename.find_first_of('#');\n> > > \tif (pos != std::string::npos) {\n> > > \t\tstd::stringstream ss;\n> > > -\t\tss << streamName << \"-\" << std::setw(6)\n> > > +\t\tss << streamNames_[stream] << \"-\" << std::setw(6)\n> > > \t\t << std::setfill('0') << buffer->metadata().sequence;\n> > > \t\tfilename.replace(pos, 1, ss.str());\n> > > \t}\n> > > @@ -62,7 +79,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > \t\t (pos == std::string::npos ? O_APPEND : O_TRUNC),\n> > > \t\t S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);\n> > > \tif (fd == -1)\n> > > -\t\treturn -errno;\n> > > +\t\treturn true;\n> > \n> > Would it make sens to log an error here as the error no longer is \n> > propagated to the caller?\n> \n> Good point, I'll do so.\n> \n> > > \n> > > \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > > \t\tvoid *data = mappedBuffers_[plane.fd.fd()].first;\n> > > @@ -84,5 +101,5 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > \n> > > \tclose(fd);\n> > > \n> > > -\treturn ret;\n> > > +\treturn true;\n> > > }\n> > > diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> > > index 47e26103e13e..5a5b176f73d8 100644\n> > > --- a/src/cam/buffer_writer.h\n> > > +++ b/src/cam/buffer_writer.h\n> > > @@ -12,18 +12,23 @@\n> > > \n> > > #include \n> > > \n> > > -class BufferWriter\n> > > +#include \"frame_sink.h\"\n> > > +\n> > > +class BufferWriter : public FrameSink\n> > > {\n> > > public:\n> > > \tBufferWriter(const std::string &pattern = \"frame-#.bin\");\n> > > \t~BufferWriter();\n> > > \n> > > -\tvoid mapBuffer(libcamera::FrameBuffer *buffer);\n> > > +\tint configure(const libcamera::CameraConfiguration &config) override;\n> > > \n> > > -\tint write(libcamera::FrameBuffer *buffer,\n> > > -\t\t const std::string &streamName);\n> > > +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > +\n> > > +\tbool consumeBuffer(const libcamera::Stream *stream,\n> > > +\t\t\t libcamera::FrameBuffer *buffer) override;\n> > > \n> > > private:\n> > > +\tstd::map streamNames_;\n> > > \tstd::string pattern_;\n> > > \tstd::map> mappedBuffers_;\n> > > };\n> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > > index b7e06bcc9463..7fc9cba48892 100644\n> > > --- a/src/cam/capture.cpp\n> > > +++ b/src/cam/capture.cpp\n> > > @@ -11,6 +11,7 @@\n> > > #include \n> > > #include \n> > > \n> > > +#include \"buffer_writer.h\"\n> > > #include \"capture.h\"\n> > > #include \"main.h\"\n> > > \n> > > @@ -18,7 +19,7 @@ using namespace libcamera;\n> > > \n> > > Capture::Capture(std::shared_ptr camera, CameraConfiguration *config,\n> > > \t\t const StreamRoles &roles)\n> > > -\t: camera_(camera), config_(config), roles_(roles), writer_(nullptr)\n> > > +\t: camera_(camera), config_(config), roles_(roles), sink_(nullptr)\n> > > {\n> > > }\n> > > \n> > > @@ -47,20 +48,28 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n> > > \n> > > \tif (options.isSet(OptFile)) {\n> > > \t\tif (!options[OptFile].toString().empty())\n> > > -\t\t\twriter_ = new BufferWriter(options[OptFile]);\n> > > +\t\t\tsink_ = new BufferWriter(options[OptFile]);\n> > > \t\telse\n> > > -\t\t\twriter_ = new BufferWriter();\n> > > +\t\t\tsink_ = new BufferWriter();\n> > > \t}\n> > > \n> > > +\tif (sink_) {\n> > > +\t\tret = sink_->configure(*config_);\n> > > +\t\tif (ret < 0) {\n> > > +\t\t\tstd::cout << \"Failed to configure frame sink\"\n> > > +\t\t\t\t << std::endl;\n> > > +\t\t\treturn ret;\n> > > +\t\t}\n> > > +\n> > > +\t\tsink_->bufferReleased.connect(this, &Capture::sinkRelease);\n> > > +\t}\n> > > \n> > > \tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> > > \n> > > \tret = capture(loop, allocator);\n> > > \n> > > -\tif (options.isSet(OptFile)) {\n> > > -\t\tdelete writer_;\n> > > -\t\twriter_ = nullptr;\n> > > -\t}\n> > > +\tdelete sink_;\n> > > +\tsink_ = nullptr;\n> > > \n> > > \tdelete allocator;\n> > > \n> > > @@ -110,16 +119,26 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > > \t\t\t\treturn ret;\n> > > \t\t\t}\n> > > \n> > > -\t\t\tif (writer_)\n> > > -\t\t\t\twriter_->mapBuffer(buffer.get());\n> > > +\t\t\tif (sink_)\n> > > +\t\t\t\tsink_->mapBuffer(buffer.get());\n> > > \t\t}\n> > > \n> > > \t\trequests.push_back(request);\n> > > \t}\n> > > \n> > > +\tif (sink_) {\n> > > +\t\tret = sink_->start();\n> > > +\t\tif (ret) {\n> > > +\t\t\tstd::cout << \"Failed to start frame sink\" << std::endl;\n> > > +\t\t\treturn ret;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > \tret = camera_->start();\n> > > \tif (ret) {\n> > > \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> > > +\t\tif (sink_)\n> > > +\t\t\tsink_->stop();\n> > > \t\treturn ret;\n> > > \t}\n> > > \n> > > @@ -128,6 +147,8 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > > \t\tif (ret < 0) {\n> > > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > > \t\t\tcamera_->stop();\n> > > +\t\t\tif (sink_)\n> > > +\t\t\t\tsink_->stop();\n> > > \t\t\treturn ret;\n> > > \t\t}\n> > > \t}\n> > > @@ -141,6 +162,12 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > > \tif (ret)\n> > > \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> > > \n> > > +\tif (sink_) {\n> > > +\t\tret = sink_->stop();\n> > > +\t\tif (ret)\n> > > +\t\t\tstd::cout << \"Failed to stop frame sink\" << std::endl;\n> > > +\t}\n> > > +\n> > > \treturn ret;\n> > > }\n> > > \n> > > @@ -157,17 +184,18 @@ void Capture::requestComplete(Request *request)\n> > > \t ? 1000.0 / fps : 0.0;\n> > > \tlast_ = now;\n> > > \n> > > +\tbool requeue = true;\n> > > +\n> > > \tstd::stringstream info;\n> > > \tinfo << \"fps: \" << std::fixed << std::setprecision(2) << fps;\n> > > \n> > > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > > \t\tStream *stream = it->first;\n> > > \t\tFrameBuffer *buffer = it->second;\n> > > -\t\tconst std::string &name = streamName_[stream];\n> > > \n> > > \t\tconst FrameMetadata &metadata = buffer->metadata();\n> > > \n> > > -\t\tinfo << \" \" << name\n> > > +\t\tinfo << \" \" << streamName_[stream]\n> > > \t\t << \" seq: \" << std::setw(6) << std::setfill('0') << metadata.sequence\n> > > \t\t << \" bytesused: \";\n> > > \n> > > @@ -178,12 +206,21 @@ void Capture::requestComplete(Request *request)\n> > > \t\t\t\tinfo << \"/\";\n> > > \t\t}\n> > > \n> > > -\t\tif (writer_)\n> > > -\t\t\twriter_->write(buffer, name);\n> > > +\t\tif (sink_) {\n> > > +\t\t\tif (!sink_->consumeBuffer(stream, buffer))\n> > > +\t\t\t\trequeue = false;\n> > \n> > This will only work for requests that contains one buffer. In this \n> > change it will works as BufferWriter::consumeBuffer() returns true and \n> > the while request is requeued as the buffer writer does not need to hold \n> > onto the buffers. I know that the new sink added later in this sereis \n> > only supports one stream so it returning false and holding onto the \n> > buffer will also work with this logic.\n> \n> Damn, you noticed ;-)\n\nSorry, I will pay less attention in the future ;-P\n\n> \n> > Would it instead make sens to rework this so that the sink always would \n> > need to signal that it's done with the buffer(s) instead of requing the \n> > request from the requestComplete() callback? That way the behavior the \n> > sinks would be the same disregarding for how long it needs to hold on to \n> > the buffer.\n> \n> I think this mechanism is indeed a bit fragile, and needs to be\n> reworked. It works in the two cases we support (file sink and kms sink),\n> but isn't future-proof. That being said, fixing it properly isn't\n> trivial. We can only requeue the request when all buffers have been\n> processed (or, rather, maybe we'll need to requeue a request with other\n> buffers instead). We can't unconditionally queue a new request in the\n> bufferReleased callback of the sink, as that would only work for a\n> single stream.\n> \n> In addition to that, I think the cam application needs to be reworked to\n> move processing of buffers to its main thread. The file sink, in\n> particular, is too expensive to run in the request completion callback,\n> which runs in the camera manager thread. I think it could make sense to\n> address the two issues together. Do you think it needs to be done as\n> part of this series ? Or could I just add a \\todo to explain the issue\n> and make sure we don't forget about it ?\n\nI don't think this needs to be solved the perfect way now, but I do \nthink we need something where adding a 3rd sink that supports more then \none stream and needs to hold onto buffers do not break, but they might \nnot need to function in the most efficient way.\n\nAs we already discussed that for now cam would only support a single \nsink would it make sens to rework the interface to deal with a Request \ninstead of a FrameBuffer? That way the logic you have here could still \nbe used while allowing new sinks to be added which would be less \nfragile.\n\nThat being said, I do agree with you that deepening on what end gaols we \nhave for cam we will likely need to rework how requests are constructed. \nWe still have the whole concept of request reuse and request templates \nto bash out :-)\n\nI'm however not convinced I like the idea of making cam an interactive \nconsole tool. Things like push spacebar to save frame to disk while \nviewing the viewfinder on the framebuffer I think maybe is another \napplication, but I might be wrong.\n\n> \n> > > +\t\t}\n> > > \t}\n> > > \n> > > \tstd::cout << info.str() << std::endl;\n> > > \n> > > +\t/*\n> > > +\t * If the frame sink holds on the buffer, we'll requeue it later in the\n> > > +\t * complete handler.\n> > > +\t */\n> > > +\tif (!requeue)\n> > > +\t\treturn;\n> > > +\n> > > \t/*\n> > > \t * Create a new request and populate it with one buffer for each\n> > > \t * stream.\n> > > @@ -203,3 +240,16 @@ void Capture::requestComplete(Request *request)\n> > > \n> > > \tcamera_->queueRequest(request);\n> > > }\n> > > +\n> > > +void Capture::sinkRelease(libcamera::FrameBuffer *buffer)\n> > > +{\n> > > +\tRequest *request = camera_->createRequest();\n> > > +\tif (!request) {\n> > > +\t\tstd::cerr << \"Can't create request\" << std::endl;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\trequest->addBuffer(config_->at(0).stream(), buffer);\n> > > +\n> > > +\tcamera_->queueRequest(request);\n> > > +}\n> > > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > > index c0e697b831fb..3be1d98e4d3e 100644\n> > > --- a/src/cam/capture.h\n> > > +++ b/src/cam/capture.h\n> > > @@ -16,10 +16,11 @@\n> > > #include \n> > > #include \n> > > \n> > > -#include \"buffer_writer.h\"\n> > > #include \"event_loop.h\"\n> > > #include \"options.h\"\n> > > \n> > > +class FrameSink;\n> > > +\n> > > class Capture\n> > > {\n> > > public:\n> > > @@ -33,13 +34,14 @@ private:\n> > > \t\t libcamera::FrameBufferAllocator *allocator);\n> > > \n> > > \tvoid requestComplete(libcamera::Request *request);\n> > > +\tvoid sinkRelease(libcamera::FrameBuffer *buffer);\n> > > \n> > > \tstd::shared_ptr camera_;\n> > > \tlibcamera::CameraConfiguration *config_;\n> > > \tlibcamera::StreamRoles roles_;\n> > > \n> > > \tstd::map streamName_;\n> > > -\tBufferWriter *writer_;\n> > > +\tFrameSink *sink_;\n> > > \tstd::chrono::steady_clock::time_point last_;\n> > > };\n> > > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 364A4603D9\n\tfor ;\n\tTue, 19 May 2020 16:59:31 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id d21so35324ljg.9\n\tfor ;\n\tTue, 19 May 2020 07:59:31 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tw10sm6127321ljm.26.2020.05.19.07.59.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 19 May 2020 07:59:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"Mo8EAzUE\"; \n\tdkim-atps=neutral","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\tbh=MUg0Htw4fghk1Y+ZeskbZOsY85aC6H/SOAYswp+aksU=;\n\tb=Mo8EAzUEO7L9/EK6sdNWLhwRNAqsvtSp1xCCY5Df4JzIhdDLJ33B5H+q3Q1FRmw0B9\n\tf6vQU7TvBZOiPv8A3bSwhsw4QIp3eYjqN7UAsE/pZgmjftodLDW7q77duExuX9gN7IZ2\n\teoobueeG1WDubAPYHGV7tkHg2r36buxU+KsicO6Jc/6WRU6DjeJVYmRjiPmnnph3JO3K\n\tAhEZHeH4FmNJ/HTVDxd5l32vwvaVz1/QThdUu/6pZ0XnSb20XFOuH1bUyHaqCJlHGqXY\n\tfE8hgUjsupFKxHjxVZEA0CKz15ltIrzQM9ytHqatazOy8axGd3IRIm0Lb/Jkl69rzypB\n\ttQrQ==","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;\n\tbh=MUg0Htw4fghk1Y+ZeskbZOsY85aC6H/SOAYswp+aksU=;\n\tb=Ha47IO4b8qK9QARuoF9pXLdEIhFsc+5QkxSYSsgQd+N6Jnz7YSX8sevVuq6Mss/HVK\n\tWyxZsV6pP+GiSdJqgR/t5VDRguMgUuxcP7ixWkk+wFkBoFmQJKo2vzdFWp170NV0hsYb\n\ty+T/HT+Ts7U4F9hrKdtIKNz1v/+4lifi9cKHNZeHW7qcuBrAiimfnFBjUq0N5Hg5MUCh\n\tkPlFOxN6exdLAbJPtlWe0QnuoovAFP8PO0DuwHmySKiWMDfVJuG1+yZsFowRcrN5s707\n\t/jjVi6tuzat4v8awt/gzHOJvg0Lm1kEZr6VKqVSU8YMJiMgEsm+soBng97NbBDJOvUYH\n\t0ucw==","X-Gm-Message-State":"AOAM532Qg+0cbTWpNKDarvaeCEevgAW93bOLbzykOUJjsQj9cz+4RIAJ\n\tGwPuawuev3arwoXeghCOiCEzuuvj9isZow==","X-Google-Smtp-Source":"ABdhPJydlt3y7FeDdHQQ9a4RuswBmF3OYFEZdY/gfufwrvOxor9+THIuwXuoNlfD53bHpM+Ml97oGA==","X-Received":"by 2002:a2e:b8cd:: with SMTP id s13mr9591283ljp.93.1589900370254;\n\tTue, 19 May 2020 07:59:30 -0700 (PDT)","Date":"Tue, 19 May 2020 16:59:28 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200519145928.GP470768@oden.dyn.berto.se>","References":"<20200519032505.17307-1-laurent.pinchart@ideasonboard.com>\n\t<20200519032505.17307-4-laurent.pinchart@ideasonboard.com>\n\t<20200519143814.GN470768@oden.dyn.berto.se>\n\t<20200519144332.GF3820@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":"<20200519144332.GF3820@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 19 May 2020 14:59:31 -0000"}},{"id":4870,"web_url":"https://patchwork.libcamera.org/comment/4870/","msgid":"<20200519222610.GG3820@pendragon.ideasonboard.com>","date":"2020-05-19T22:26:10","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, May 19, 2020 at 04:59:28PM +0200, Niklas Söderlund wrote:\n> On 2020-05-19 17:43:32 +0300, Laurent Pinchart wrote:\n> > On Tue, May 19, 2020 at 04:38:14PM +0200, Niklas Söderlund wrote:\n> > > On 2020-05-19 06:25:00 +0300, Laurent Pinchart wrote:\n> > > > Make the BufferWriter class inherit from FrameSink, and use the\n> > > > FrameSink API to manage it. This makes the code more generic, and will\n> > > > allow usage of other sinks.\n> > > > \n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/cam/buffer_writer.cpp | 25 ++++++++++---\n> > > > src/cam/buffer_writer.h | 13 ++++---\n> > > > src/cam/capture.cpp | 76 ++++++++++++++++++++++++++++++++-------\n> > > > src/cam/capture.h | 6 ++--\n> > > > 4 files changed, 97 insertions(+), 23 deletions(-)\n> > > > \n> > > > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > > > index c5a5eb46224a..2bec4b132155 100644\n> > > > --- a/src/cam/buffer_writer.cpp\n> > > > +++ b/src/cam/buffer_writer.cpp\n> > > > @@ -13,6 +13,8 @@\n> > > > #include \n> > > > #include \n> > > > \n> > > > +#include \n> > > > +\n> > > > #include \"buffer_writer.h\"\n> > > > \n> > > > using namespace libcamera;\n> > > > @@ -32,6 +34,21 @@ BufferWriter::~BufferWriter()\n> > > > \tmappedBuffers_.clear();\n> > > > }\n> > > > \n> > > > +int BufferWriter::configure(const libcamera::CameraConfiguration &config)\n> > > > +{\n> > > > +\tint ret = FrameSink::configure(config);\n> > > > +\tif (ret < 0)\n> > > > +\t\treturn ret;\n> > > > +\n> > > > +\tstreamNames_.clear();\n> > > > +\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> > > > +\t\tconst StreamConfiguration &cfg = config.at(index);\n> > > > +\t\tstreamNames_[cfg.stream()] = \"stream\" + std::to_string(index);\n> > > > +\t}\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > > > {\n> > > > \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > > > @@ -43,7 +60,7 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > > > \t}\n> > > > }\n> > > > \n> > > > -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > > +bool BufferWriter::consumeBuffer(const Stream *stream, FrameBuffer *buffer)\n> > > > {\n> > > > \tstd::string filename;\n> > > > \tsize_t pos;\n> > > > @@ -53,7 +70,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > > \tpos = filename.find_first_of('#');\n> > > > \tif (pos != std::string::npos) {\n> > > > \t\tstd::stringstream ss;\n> > > > -\t\tss << streamName << \"-\" << std::setw(6)\n> > > > +\t\tss << streamNames_[stream] << \"-\" << std::setw(6)\n> > > > \t\t << std::setfill('0') << buffer->metadata().sequence;\n> > > > \t\tfilename.replace(pos, 1, ss.str());\n> > > > \t}\n> > > > @@ -62,7 +79,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > > \t\t (pos == std::string::npos ? O_APPEND : O_TRUNC),\n> > > > \t\t S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);\n> > > > \tif (fd == -1)\n> > > > -\t\treturn -errno;\n> > > > +\t\treturn true;\n> > > \n> > > Would it make sens to log an error here as the error no longer is \n> > > propagated to the caller?\n> > \n> > Good point, I'll do so.\n> > \n> > > > \n> > > > \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > > > \t\tvoid *data = mappedBuffers_[plane.fd.fd()].first;\n> > > > @@ -84,5 +101,5 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > > \n> > > > \tclose(fd);\n> > > > \n> > > > -\treturn ret;\n> > > > +\treturn true;\n> > > > }\n> > > > diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> > > > index 47e26103e13e..5a5b176f73d8 100644\n> > > > --- a/src/cam/buffer_writer.h\n> > > > +++ b/src/cam/buffer_writer.h\n> > > > @@ -12,18 +12,23 @@\n> > > > \n> > > > #include \n> > > > \n> > > > -class BufferWriter\n> > > > +#include \"frame_sink.h\"\n> > > > +\n> > > > +class BufferWriter : public FrameSink\n> > > > {\n> > > > public:\n> > > > \tBufferWriter(const std::string &pattern = \"frame-#.bin\");\n> > > > \t~BufferWriter();\n> > > > \n> > > > -\tvoid mapBuffer(libcamera::FrameBuffer *buffer);\n> > > > +\tint configure(const libcamera::CameraConfiguration &config) override;\n> > > > \n> > > > -\tint write(libcamera::FrameBuffer *buffer,\n> > > > -\t\t const std::string &streamName);\n> > > > +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > > +\n> > > > +\tbool consumeBuffer(const libcamera::Stream *stream,\n> > > > +\t\t\t libcamera::FrameBuffer *buffer) override;\n> > > > \n> > > > private:\n> > > > +\tstd::map streamNames_;\n> > > > \tstd::string pattern_;\n> > > > \tstd::map> mappedBuffers_;\n> > > > };\n> > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > > > index b7e06bcc9463..7fc9cba48892 100644\n> > > > --- a/src/cam/capture.cpp\n> > > > +++ b/src/cam/capture.cpp\n> > > > @@ -11,6 +11,7 @@\n> > > > #include \n> > > > #include \n> > > > \n> > > > +#include \"buffer_writer.h\"\n> > > > #include \"capture.h\"\n> > > > #include \"main.h\"\n> > > > \n> > > > @@ -18,7 +19,7 @@ using namespace libcamera;\n> > > > \n> > > > Capture::Capture(std::shared_ptr camera, CameraConfiguration *config,\n> > > > \t\t const StreamRoles &roles)\n> > > > -\t: camera_(camera), config_(config), roles_(roles), writer_(nullptr)\n> > > > +\t: camera_(camera), config_(config), roles_(roles), sink_(nullptr)\n> > > > {\n> > > > }\n> > > > \n> > > > @@ -47,20 +48,28 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n> > > > \n> > > > \tif (options.isSet(OptFile)) {\n> > > > \t\tif (!options[OptFile].toString().empty())\n> > > > -\t\t\twriter_ = new BufferWriter(options[OptFile]);\n> > > > +\t\t\tsink_ = new BufferWriter(options[OptFile]);\n> > > > \t\telse\n> > > > -\t\t\twriter_ = new BufferWriter();\n> > > > +\t\t\tsink_ = new BufferWriter();\n> > > > \t}\n> > > > \n> > > > +\tif (sink_) {\n> > > > +\t\tret = sink_->configure(*config_);\n> > > > +\t\tif (ret < 0) {\n> > > > +\t\t\tstd::cout << \"Failed to configure frame sink\"\n> > > > +\t\t\t\t << std::endl;\n> > > > +\t\t\treturn ret;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tsink_->bufferReleased.connect(this, &Capture::sinkRelease);\n> > > > +\t}\n> > > > \n> > > > \tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> > > > \n> > > > \tret = capture(loop, allocator);\n> > > > \n> > > > -\tif (options.isSet(OptFile)) {\n> > > > -\t\tdelete writer_;\n> > > > -\t\twriter_ = nullptr;\n> > > > -\t}\n> > > > +\tdelete sink_;\n> > > > +\tsink_ = nullptr;\n> > > > \n> > > > \tdelete allocator;\n> > > > \n> > > > @@ -110,16 +119,26 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > > > \t\t\t\treturn ret;\n> > > > \t\t\t}\n> > > > \n> > > > -\t\t\tif (writer_)\n> > > > -\t\t\t\twriter_->mapBuffer(buffer.get());\n> > > > +\t\t\tif (sink_)\n> > > > +\t\t\t\tsink_->mapBuffer(buffer.get());\n> > > > \t\t}\n> > > > \n> > > > \t\trequests.push_back(request);\n> > > > \t}\n> > > > \n> > > > +\tif (sink_) {\n> > > > +\t\tret = sink_->start();\n> > > > +\t\tif (ret) {\n> > > > +\t\t\tstd::cout << \"Failed to start frame sink\" << std::endl;\n> > > > +\t\t\treturn ret;\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > \tret = camera_->start();\n> > > > \tif (ret) {\n> > > > \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> > > > +\t\tif (sink_)\n> > > > +\t\t\tsink_->stop();\n> > > > \t\treturn ret;\n> > > > \t}\n> > > > \n> > > > @@ -128,6 +147,8 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > > > \t\tif (ret < 0) {\n> > > > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > > > \t\t\tcamera_->stop();\n> > > > +\t\t\tif (sink_)\n> > > > +\t\t\t\tsink_->stop();\n> > > > \t\t\treturn ret;\n> > > > \t\t}\n> > > > \t}\n> > > > @@ -141,6 +162,12 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > > > \tif (ret)\n> > > > \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> > > > \n> > > > +\tif (sink_) {\n> > > > +\t\tret = sink_->stop();\n> > > > +\t\tif (ret)\n> > > > +\t\t\tstd::cout << \"Failed to stop frame sink\" << std::endl;\n> > > > +\t}\n> > > > +\n> > > > \treturn ret;\n> > > > }\n> > > > \n> > > > @@ -157,17 +184,18 @@ void Capture::requestComplete(Request *request)\n> > > > \t ? 1000.0 / fps : 0.0;\n> > > > \tlast_ = now;\n> > > > \n> > > > +\tbool requeue = true;\n> > > > +\n> > > > \tstd::stringstream info;\n> > > > \tinfo << \"fps: \" << std::fixed << std::setprecision(2) << fps;\n> > > > \n> > > > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > > > \t\tStream *stream = it->first;\n> > > > \t\tFrameBuffer *buffer = it->second;\n> > > > -\t\tconst std::string &name = streamName_[stream];\n> > > > \n> > > > \t\tconst FrameMetadata &metadata = buffer->metadata();\n> > > > \n> > > > -\t\tinfo << \" \" << name\n> > > > +\t\tinfo << \" \" << streamName_[stream]\n> > > > \t\t << \" seq: \" << std::setw(6) << std::setfill('0') << metadata.sequence\n> > > > \t\t << \" bytesused: \";\n> > > > \n> > > > @@ -178,12 +206,21 @@ void Capture::requestComplete(Request *request)\n> > > > \t\t\t\tinfo << \"/\";\n> > > > \t\t}\n> > > > \n> > > > -\t\tif (writer_)\n> > > > -\t\t\twriter_->write(buffer, name);\n> > > > +\t\tif (sink_) {\n> > > > +\t\t\tif (!sink_->consumeBuffer(stream, buffer))\n> > > > +\t\t\t\trequeue = false;\n> > > \n> > > This will only work for requests that contains one buffer. In this \n> > > change it will works as BufferWriter::consumeBuffer() returns true and \n> > > the while request is requeued as the buffer writer does not need to hold \n> > > onto the buffers. I know that the new sink added later in this sereis \n> > > only supports one stream so it returning false and holding onto the \n> > > buffer will also work with this logic.\n> > \n> > Damn, you noticed ;-)\n> \n> Sorry, I will pay less attention in the future ;-P\n> \n> > > Would it instead make sens to rework this so that the sink always would \n> > > need to signal that it's done with the buffer(s) instead of requing the \n> > > request from the requestComplete() callback? That way the behavior the \n> > > sinks would be the same disregarding for how long it needs to hold on to \n> > > the buffer.\n> > \n> > I think this mechanism is indeed a bit fragile, and needs to be\n> > reworked. It works in the two cases we support (file sink and kms sink),\n> > but isn't future-proof. That being said, fixing it properly isn't\n> > trivial. We can only requeue the request when all buffers have been\n> > processed (or, rather, maybe we'll need to requeue a request with other\n> > buffers instead). We can't unconditionally queue a new request in the\n> > bufferReleased callback of the sink, as that would only work for a\n> > single stream.\n> > \n> > In addition to that, I think the cam application needs to be reworked to\n> > move processing of buffers to its main thread. The file sink, in\n> > particular, is too expensive to run in the request completion callback,\n> > which runs in the camera manager thread. I think it could make sense to\n> > address the two issues together. Do you think it needs to be done as\n> > part of this series ? Or could I just add a \\todo to explain the issue\n> > and make sure we don't forget about it ?\n> \n> I don't think this needs to be solved the perfect way now, but I do \n> think we need something where adding a 3rd sink that supports more then \n> one stream and needs to hold onto buffers do not break, but they might \n> not need to function in the most efficient way.\n> \n> As we already discussed that for now cam would only support a single \n> sink would it make sens to rework the interface to deal with a Request \n> instead of a FrameBuffer? That way the logic you have here could still \n> be used while allowing new sinks to be added which would be less \n> fragile.\n\nJust to make it clear, how much rework would you do as part of this\nseries, and how much should be delayed ?\n\n> That being said, I do agree with you that deepening on what end gaols we \n> have for cam we will likely need to rework how requests are constructed. \n> We still have the whole concept of request reuse and request templates \n> to bash out :-)\n> \n> I'm however not convinced I like the idea of making cam an interactive \n> console tool. Things like push spacebar to save frame to disk while \n> viewing the viewfinder on the framebuffer I think maybe is another \n> application, but I might be wrong.\n\nYou're probably right there, it's not a direction I intend to push for.\n\n> > > > +\t\t}\n> > > > \t}\n> > > > \n> > > > \tstd::cout << info.str() << std::endl;\n> > > > \n> > > > +\t/*\n> > > > +\t * If the frame sink holds on the buffer, we'll requeue it later in the\n> > > > +\t * complete handler.\n> > > > +\t */\n> > > > +\tif (!requeue)\n> > > > +\t\treturn;\n> > > > +\n> > > > \t/*\n> > > > \t * Create a new request and populate it with one buffer for each\n> > > > \t * stream.\n> > > > @@ -203,3 +240,16 @@ void Capture::requestComplete(Request *request)\n> > > > \n> > > > \tcamera_->queueRequest(request);\n> > > > }\n> > > > +\n> > > > +void Capture::sinkRelease(libcamera::FrameBuffer *buffer)\n> > > > +{\n> > > > +\tRequest *request = camera_->createRequest();\n> > > > +\tif (!request) {\n> > > > +\t\tstd::cerr << \"Can't create request\" << std::endl;\n> > > > +\t\treturn;\n> > > > +\t}\n> > > > +\n> > > > +\trequest->addBuffer(config_->at(0).stream(), buffer);\n> > > > +\n> > > > +\tcamera_->queueRequest(request);\n> > > > +}\n> > > > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > > > index c0e697b831fb..3be1d98e4d3e 100644\n> > > > --- a/src/cam/capture.h\n> > > > +++ b/src/cam/capture.h\n> > > > @@ -16,10 +16,11 @@\n> > > > #include \n> > > > #include \n> > > > \n> > > > -#include \"buffer_writer.h\"\n> > > > #include \"event_loop.h\"\n> > > > #include \"options.h\"\n> > > > \n> > > > +class FrameSink;\n> > > > +\n> > > > class Capture\n> > > > {\n> > > > public:\n> > > > @@ -33,13 +34,14 @@ private:\n> > > > \t\t libcamera::FrameBufferAllocator *allocator);\n> > > > \n> > > > \tvoid requestComplete(libcamera::Request *request);\n> > > > +\tvoid sinkRelease(libcamera::FrameBuffer *buffer);\n> > > > \n> > > > \tstd::shared_ptr camera_;\n> > > > \tlibcamera::CameraConfiguration *config_;\n> > > > \tlibcamera::StreamRoles roles_;\n> > > > \n> > > > \tstd::map streamName_;\n> > > > -\tBufferWriter *writer_;\n> > > > +\tFrameSink *sink_;\n> > > > \tstd::chrono::steady_clock::time_point last_;\n> > > > };\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 761D4603DB\n\tfor ;\n\tWed, 20 May 2020 00:26:22 +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 B257230C;\n\tWed, 20 May 2020 00:26:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"T76dZmzV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1589927181;\n\tbh=EWD8F1kcuMicoHO6C+rPFot93jHkSMeJZYHvEeUNjGw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T76dZmzVxDIsUdNJAJerfbz6lkkAW+pgo0B31fQvUDrGi+vTNz53nVRL01JfrWkj4\n\tMRuDpDvWaRyZKCA7RWYaiqMHeXKZzQF8u8xmCHy3YWmBW0Y6XKgUAte2pKmXQCdjNI\n\tIHfZzO69R2Lxw5KZdjLiC7T8FbYIZOXACLnCuGKA=","Date":"Wed, 20 May 2020 01:26:10 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200519222610.GG3820@pendragon.ideasonboard.com>","References":"<20200519032505.17307-1-laurent.pinchart@ideasonboard.com>\n\t<20200519032505.17307-4-laurent.pinchart@ideasonboard.com>\n\t<20200519143814.GN470768@oden.dyn.berto.se>\n\t<20200519144332.GF3820@pendragon.ideasonboard.com>\n\t<20200519145928.GP470768@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200519145928.GP470768@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 19 May 2020 22:26:22 -0000"}},{"id":4884,"web_url":"https://patchwork.libcamera.org/comment/4884/","msgid":"<20200522010214.GB2165767@oden.dyn.berto.se>","date":"2020-05-22T01:02:14","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2020-05-20 01:26:10 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> On Tue, May 19, 2020 at 04:59:28PM +0200, Niklas Söderlund wrote:\n> > On 2020-05-19 17:43:32 +0300, Laurent Pinchart wrote:\n> > > On Tue, May 19, 2020 at 04:38:14PM +0200, Niklas Söderlund wrote:\n> > > > On 2020-05-19 06:25:00 +0300, Laurent Pinchart wrote:\n> > > > > Make the BufferWriter class inherit from FrameSink, and use the\n> > > > > FrameSink API to manage it. This makes the code more generic, and will\n> > > > > allow usage of other sinks.\n> > > > > \n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/cam/buffer_writer.cpp | 25 ++++++++++---\n> > > > > src/cam/buffer_writer.h | 13 ++++---\n> > > > > src/cam/capture.cpp | 76 ++++++++++++++++++++++++++++++++-------\n> > > > > src/cam/capture.h | 6 ++--\n> > > > > 4 files changed, 97 insertions(+), 23 deletions(-)\n> > > > > \n> > > > > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > > > > index c5a5eb46224a..2bec4b132155 100644\n> > > > > --- a/src/cam/buffer_writer.cpp\n> > > > > +++ b/src/cam/buffer_writer.cpp\n> > > > > @@ -13,6 +13,8 @@\n> > > > > #include \n> > > > > #include \n> > > > > \n> > > > > +#include \n> > > > > +\n> > > > > #include \"buffer_writer.h\"\n> > > > > \n> > > > > using namespace libcamera;\n> > > > > @@ -32,6 +34,21 @@ BufferWriter::~BufferWriter()\n> > > > > \tmappedBuffers_.clear();\n> > > > > }\n> > > > > \n> > > > > +int BufferWriter::configure(const libcamera::CameraConfiguration &config)\n> > > > > +{\n> > > > > +\tint ret = FrameSink::configure(config);\n> > > > > +\tif (ret < 0)\n> > > > > +\t\treturn ret;\n> > > > > +\n> > > > > +\tstreamNames_.clear();\n> > > > > +\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> > > > > +\t\tconst StreamConfiguration &cfg = config.at(index);\n> > > > > +\t\tstreamNames_[cfg.stream()] = \"stream\" + std::to_string(index);\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > > > > {\n> > > > > \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > > > > @@ -43,7 +60,7 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > > > > \t}\n> > > > > }\n> > > > > \n> > > > > -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > > > +bool BufferWriter::consumeBuffer(const Stream *stream, FrameBuffer *buffer)\n> > > > > {\n> > > > > \tstd::string filename;\n> > > > > \tsize_t pos;\n> > > > > @@ -53,7 +70,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > > > \tpos = filename.find_first_of('#');\n> > > > > \tif (pos != std::string::npos) {\n> > > > > \t\tstd::stringstream ss;\n> > > > > -\t\tss << streamName << \"-\" << std::setw(6)\n> > > > > +\t\tss << streamNames_[stream] << \"-\" << std::setw(6)\n> > > > > \t\t << std::setfill('0') << buffer->metadata().sequence;\n> > > > > \t\tfilename.replace(pos, 1, ss.str());\n> > > > > \t}\n> > > > > @@ -62,7 +79,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > > > \t\t (pos == std::string::npos ? O_APPEND : O_TRUNC),\n> > > > > \t\t S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);\n> > > > > \tif (fd == -1)\n> > > > > -\t\treturn -errno;\n> > > > > +\t\treturn true;\n> > > > \n> > > > Would it make sens to log an error here as the error no longer is \n> > > > propagated to the caller?\n> > > \n> > > Good point, I'll do so.\n> > > \n> > > > > \n> > > > > \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > > > > \t\tvoid *data = mappedBuffers_[plane.fd.fd()].first;\n> > > > > @@ -84,5 +101,5 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > > > > \n> > > > > \tclose(fd);\n> > > > > \n> > > > > -\treturn ret;\n> > > > > +\treturn true;\n> > > > > }\n> > > > > diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> > > > > index 47e26103e13e..5a5b176f73d8 100644\n> > > > > --- a/src/cam/buffer_writer.h\n> > > > > +++ b/src/cam/buffer_writer.h\n> > > > > @@ -12,18 +12,23 @@\n> > > > > \n> > > > > #include \n> > > > > \n> > > > > -class BufferWriter\n> > > > > +#include \"frame_sink.h\"\n> > > > > +\n> > > > > +class BufferWriter : public FrameSink\n> > > > > {\n> > > > > public:\n> > > > > \tBufferWriter(const std::string &pattern = \"frame-#.bin\");\n> > > > > \t~BufferWriter();\n> > > > > \n> > > > > -\tvoid mapBuffer(libcamera::FrameBuffer *buffer);\n> > > > > +\tint configure(const libcamera::CameraConfiguration &config) override;\n> > > > > \n> > > > > -\tint write(libcamera::FrameBuffer *buffer,\n> > > > > -\t\t const std::string &streamName);\n> > > > > +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > > > > +\n> > > > > +\tbool consumeBuffer(const libcamera::Stream *stream,\n> > > > > +\t\t\t libcamera::FrameBuffer *buffer) override;\n> > > > > \n> > > > > private:\n> > > > > +\tstd::map streamNames_;\n> > > > > \tstd::string pattern_;\n> > > > > \tstd::map> mappedBuffers_;\n> > > > > };\n> > > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > > > > index b7e06bcc9463..7fc9cba48892 100644\n> > > > > --- a/src/cam/capture.cpp\n> > > > > +++ b/src/cam/capture.cpp\n> > > > > @@ -11,6 +11,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > \n> > > > > +#include \"buffer_writer.h\"\n> > > > > #include \"capture.h\"\n> > > > > #include \"main.h\"\n> > > > > \n> > > > > @@ -18,7 +19,7 @@ using namespace libcamera;\n> > > > > \n> > > > > Capture::Capture(std::shared_ptr camera, CameraConfiguration *config,\n> > > > > \t\t const StreamRoles &roles)\n> > > > > -\t: camera_(camera), config_(config), roles_(roles), writer_(nullptr)\n> > > > > +\t: camera_(camera), config_(config), roles_(roles), sink_(nullptr)\n> > > > > {\n> > > > > }\n> > > > > \n> > > > > @@ -47,20 +48,28 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n> > > > > \n> > > > > \tif (options.isSet(OptFile)) {\n> > > > > \t\tif (!options[OptFile].toString().empty())\n> > > > > -\t\t\twriter_ = new BufferWriter(options[OptFile]);\n> > > > > +\t\t\tsink_ = new BufferWriter(options[OptFile]);\n> > > > > \t\telse\n> > > > > -\t\t\twriter_ = new BufferWriter();\n> > > > > +\t\t\tsink_ = new BufferWriter();\n> > > > > \t}\n> > > > > \n> > > > > +\tif (sink_) {\n> > > > > +\t\tret = sink_->configure(*config_);\n> > > > > +\t\tif (ret < 0) {\n> > > > > +\t\t\tstd::cout << \"Failed to configure frame sink\"\n> > > > > +\t\t\t\t << std::endl;\n> > > > > +\t\t\treturn ret;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tsink_->bufferReleased.connect(this, &Capture::sinkRelease);\n> > > > > +\t}\n> > > > > \n> > > > > \tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> > > > > \n> > > > > \tret = capture(loop, allocator);\n> > > > > \n> > > > > -\tif (options.isSet(OptFile)) {\n> > > > > -\t\tdelete writer_;\n> > > > > -\t\twriter_ = nullptr;\n> > > > > -\t}\n> > > > > +\tdelete sink_;\n> > > > > +\tsink_ = nullptr;\n> > > > > \n> > > > > \tdelete allocator;\n> > > > > \n> > > > > @@ -110,16 +119,26 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > > > > \t\t\t\treturn ret;\n> > > > > \t\t\t}\n> > > > > \n> > > > > -\t\t\tif (writer_)\n> > > > > -\t\t\t\twriter_->mapBuffer(buffer.get());\n> > > > > +\t\t\tif (sink_)\n> > > > > +\t\t\t\tsink_->mapBuffer(buffer.get());\n> > > > > \t\t}\n> > > > > \n> > > > > \t\trequests.push_back(request);\n> > > > > \t}\n> > > > > \n> > > > > +\tif (sink_) {\n> > > > > +\t\tret = sink_->start();\n> > > > > +\t\tif (ret) {\n> > > > > +\t\t\tstd::cout << \"Failed to start frame sink\" << std::endl;\n> > > > > +\t\t\treturn ret;\n> > > > > +\t\t}\n> > > > > +\t}\n> > > > > +\n> > > > > \tret = camera_->start();\n> > > > > \tif (ret) {\n> > > > > \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> > > > > +\t\tif (sink_)\n> > > > > +\t\t\tsink_->stop();\n> > > > > \t\treturn ret;\n> > > > > \t}\n> > > > > \n> > > > > @@ -128,6 +147,8 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > > > > \t\tif (ret < 0) {\n> > > > > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > > > > \t\t\tcamera_->stop();\n> > > > > +\t\t\tif (sink_)\n> > > > > +\t\t\t\tsink_->stop();\n> > > > > \t\t\treturn ret;\n> > > > > \t\t}\n> > > > > \t}\n> > > > > @@ -141,6 +162,12 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> > > > > \tif (ret)\n> > > > > \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> > > > > \n> > > > > +\tif (sink_) {\n> > > > > +\t\tret = sink_->stop();\n> > > > > +\t\tif (ret)\n> > > > > +\t\t\tstd::cout << \"Failed to stop frame sink\" << std::endl;\n> > > > > +\t}\n> > > > > +\n> > > > > \treturn ret;\n> > > > > }\n> > > > > \n> > > > > @@ -157,17 +184,18 @@ void Capture::requestComplete(Request *request)\n> > > > > \t ? 1000.0 / fps : 0.0;\n> > > > > \tlast_ = now;\n> > > > > \n> > > > > +\tbool requeue = true;\n> > > > > +\n> > > > > \tstd::stringstream info;\n> > > > > \tinfo << \"fps: \" << std::fixed << std::setprecision(2) << fps;\n> > > > > \n> > > > > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > > > > \t\tStream *stream = it->first;\n> > > > > \t\tFrameBuffer *buffer = it->second;\n> > > > > -\t\tconst std::string &name = streamName_[stream];\n> > > > > \n> > > > > \t\tconst FrameMetadata &metadata = buffer->metadata();\n> > > > > \n> > > > > -\t\tinfo << \" \" << name\n> > > > > +\t\tinfo << \" \" << streamName_[stream]\n> > > > > \t\t << \" seq: \" << std::setw(6) << std::setfill('0') << metadata.sequence\n> > > > > \t\t << \" bytesused: \";\n> > > > > \n> > > > > @@ -178,12 +206,21 @@ void Capture::requestComplete(Request *request)\n> > > > > \t\t\t\tinfo << \"/\";\n> > > > > \t\t}\n> > > > > \n> > > > > -\t\tif (writer_)\n> > > > > -\t\t\twriter_->write(buffer, name);\n> > > > > +\t\tif (sink_) {\n> > > > > +\t\t\tif (!sink_->consumeBuffer(stream, buffer))\n> > > > > +\t\t\t\trequeue = false;\n> > > > \n> > > > This will only work for requests that contains one buffer. In this \n> > > > change it will works as BufferWriter::consumeBuffer() returns true and \n> > > > the while request is requeued as the buffer writer does not need to hold \n> > > > onto the buffers. I know that the new sink added later in this sereis \n> > > > only supports one stream so it returning false and holding onto the \n> > > > buffer will also work with this logic.\n> > > \n> > > Damn, you noticed ;-)\n> > \n> > Sorry, I will pay less attention in the future ;-P\n> > \n> > > > Would it instead make sens to rework this so that the sink always would \n> > > > need to signal that it's done with the buffer(s) instead of requing the \n> > > > request from the requestComplete() callback? That way the behavior the \n> > > > sinks would be the same disregarding for how long it needs to hold on to \n> > > > the buffer.\n> > > \n> > > I think this mechanism is indeed a bit fragile, and needs to be\n> > > reworked. It works in the two cases we support (file sink and kms sink),\n> > > but isn't future-proof. That being said, fixing it properly isn't\n> > > trivial. We can only requeue the request when all buffers have been\n> > > processed (or, rather, maybe we'll need to requeue a request with other\n> > > buffers instead). We can't unconditionally queue a new request in the\n> > > bufferReleased callback of the sink, as that would only work for a\n> > > single stream.\n> > > \n> > > In addition to that, I think the cam application needs to be reworked to\n> > > move processing of buffers to its main thread. The file sink, in\n> > > particular, is too expensive to run in the request completion callback,\n> > > which runs in the camera manager thread. I think it could make sense to\n> > > address the two issues together. Do you think it needs to be done as\n> > > part of this series ? Or could I just add a \\todo to explain the issue\n> > > and make sure we don't forget about it ?\n> > \n> > I don't think this needs to be solved the perfect way now, but I do \n> > think we need something where adding a 3rd sink that supports more then \n> > one stream and needs to hold onto buffers do not break, but they might \n> > not need to function in the most efficient way.\n> > \n> > As we already discussed that for now cam would only support a single \n> > sink would it make sens to rework the interface to deal with a Request \n> > instead of a FrameBuffer? That way the logic you have here could still \n> > be used while allowing new sinks to be added which would be less \n> > fragile.\n> \n> Just to make it clear, how much rework would you do as part of this\n> series, and how much should be delayed ?\n\nFor me the least amount needed to allow for a sink that supports more \nthen one stream but needs to hold onto the buffers/request. I would be \nOK to pass the whole request to the sink instead of individual buffers. \nOr if you can think of something else more clever.\n\nWhat I don't like is that the code is fragile and without looking at \nthis change in isolation it's not clear what requirements are put on a \nsink so adding a new one could explode and take time to figure why it \ndid so. On the other hand I really like the sink concept you add here, \nmaybe it can be shared with qcam sometime far into the future. :-)\n\n> \n> > That being said, I do agree with you that deepening on what end gaols we \n> > have for cam we will likely need to rework how requests are constructed. \n> > We still have the whole concept of request reuse and request templates \n> > to bash out :-)\n> > \n> > I'm however not convinced I like the idea of making cam an interactive \n> > console tool. Things like push spacebar to save frame to disk while \n> > viewing the viewfinder on the framebuffer I think maybe is another \n> > application, but I might be wrong.\n> \n> You're probably right there, it's not a direction I intend to push for.\n> \n> > > > > +\t\t}\n> > > > > \t}\n> > > > > \n> > > > > \tstd::cout << info.str() << std::endl;\n> > > > > \n> > > > > +\t/*\n> > > > > +\t * If the frame sink holds on the buffer, we'll requeue it later in the\n> > > > > +\t * complete handler.\n> > > > > +\t */\n> > > > > +\tif (!requeue)\n> > > > > +\t\treturn;\n> > > > > +\n> > > > > \t/*\n> > > > > \t * Create a new request and populate it with one buffer for each\n> > > > > \t * stream.\n> > > > > @@ -203,3 +240,16 @@ void Capture::requestComplete(Request *request)\n> > > > > \n> > > > > \tcamera_->queueRequest(request);\n> > > > > }\n> > > > > +\n> > > > > +void Capture::sinkRelease(libcamera::FrameBuffer *buffer)\n> > > > > +{\n> > > > > +\tRequest *request = camera_->createRequest();\n> > > > > +\tif (!request) {\n> > > > > +\t\tstd::cerr << \"Can't create request\" << std::endl;\n> > > > > +\t\treturn;\n> > > > > +\t}\n> > > > > +\n> > > > > +\trequest->addBuffer(config_->at(0).stream(), buffer);\n> > > > > +\n> > > > > +\tcamera_->queueRequest(request);\n> > > > > +}\n> > > > > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > > > > index c0e697b831fb..3be1d98e4d3e 100644\n> > > > > --- a/src/cam/capture.h\n> > > > > +++ b/src/cam/capture.h\n> > > > > @@ -16,10 +16,11 @@\n> > > > > #include \n> > > > > #include \n> > > > > \n> > > > > -#include \"buffer_writer.h\"\n> > > > > #include \"event_loop.h\"\n> > > > > #include \"options.h\"\n> > > > > \n> > > > > +class FrameSink;\n> > > > > +\n> > > > > class Capture\n> > > > > {\n> > > > > public:\n> > > > > @@ -33,13 +34,14 @@ private:\n> > > > > \t\t libcamera::FrameBufferAllocator *allocator);\n> > > > > \n> > > > > \tvoid requestComplete(libcamera::Request *request);\n> > > > > +\tvoid sinkRelease(libcamera::FrameBuffer *buffer);\n> > > > > \n> > > > > \tstd::shared_ptr camera_;\n> > > > > \tlibcamera::CameraConfiguration *config_;\n> > > > > \tlibcamera::StreamRoles roles_;\n> > > > > \n> > > > > \tstd::map streamName_;\n> > > > > -\tBufferWriter *writer_;\n> > > > > +\tFrameSink *sink_;\n> > > > > \tstd::chrono::steady_clock::time_point last_;\n> > > > > };\n> > > > > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A3C5603D3\n\tfor ;\n\tFri, 22 May 2020 03:02:17 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id u15so10625403ljd.3\n\tfor ;\n\tThu, 21 May 2020 18:02:17 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv2sm2018259ljj.96.2020.05.21.18.02.15\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 21 May 2020 18:02:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"Fdn55u+N\"; \n\tdkim-atps=neutral","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\tbh=eXF6YpLxkDAxmCxEAZ1GbGbDLYIP31y/Ik0bVifRQQs=;\n\tb=Fdn55u+NxtOe3ahI89EIZwx/Vj4bR3v6UOUmDCMQdhk7L2CZ3q3UJFteXctXQMaS9M\n\tY41HTSgjYCWWJjnhCXf6F6zO4tmm/LwfKOYd2/ltCsU+vOvKyeDAOnWNE66hEO8UnhTK\n\tksXA9I5byPBe9QwNHf/1zb7ejDvBUhodAYsx8O1awa21z9vEg2VTI9a9pnp4+nkbYyzD\n\tixCRyTtjNDM2RTahCdpDLx/UGwqd0RSPiDceI26mJxHBhuoN/PpGiCoPJEuZvc6cGfqb\n\tQ0gzIMPSKtwX85IZaqXAxl+XGBFh6HbBDMSfIZ05Lc1fwDhyW2ULKjeS8gElnRCD4zt8\n\tZxsg==","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;\n\tbh=eXF6YpLxkDAxmCxEAZ1GbGbDLYIP31y/Ik0bVifRQQs=;\n\tb=ZESfPdMoTSnurlNm/xihc9e6gO8FdIjd8ru1gK9/bLMK62KTECVNB4mcmYI/KYmUvU\n\tzkJ+S484AP9J+n+z5P7LUM3MZVYHvga3L9p82iLlRAPa0i07MqMpyXd66HjKh+iVpgdw\n\tPHoBgdaGiskzuV0yehD1dWKllDkTksZ/iV06AyxgEKQozQhT21iyzBuKSfnKG4LXaWxg\n\tHLCwTxNgYm/9JZj5ZfT8tGc+ZDqWRkemCZRRcC7F/r26ey0/3sFVdWPfrsOMYUSNX6D2\n\towHYWKHiX1iiEs7XGTGh2/OVlyIgcJz2yDTiHjL5ZV26xgC2tCUMK6GUZnl/n5mwPk6i\n\tkiuA==","X-Gm-Message-State":"AOAM530sdqab4nCQw643vV1ZvS0MoBhfkrqjYbcNpbkv9JXfw3R2UTJf\n\tmYbyUAW9Jkw/Rrjt2+7u7EMBcBW1XPjHxQ==","X-Google-Smtp-Source":"ABdhPJwYSj4zMfgOiGnSbCqHMiKVu9uTNBEmd+7GgaoHWaIH7z79b2aIRRomwGYgVAh+RpYPPcqruQ==","X-Received":"by 2002:a2e:81c7:: with SMTP id s7mr6106148ljg.203.1590109336313;\n\tThu, 21 May 2020 18:02:16 -0700 (PDT)","Date":"Fri, 22 May 2020 03:02:14 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200522010214.GB2165767@oden.dyn.berto.se>","References":"<20200519032505.17307-1-laurent.pinchart@ideasonboard.com>\n\t<20200519032505.17307-4-laurent.pinchart@ideasonboard.com>\n\t<20200519143814.GN470768@oden.dyn.berto.se>\n\t<20200519144332.GF3820@pendragon.ideasonboard.com>\n\t<20200519145928.GP470768@oden.dyn.berto.se>\n\t<20200519222610.GG3820@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":"<20200519222610.GG3820@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 22 May 2020 01:02:17 -0000"}},{"id":4891,"web_url":"https://patchwork.libcamera.org/comment/4891/","msgid":"<20200522213226.GI5824@pendragon.ideasonboard.com>","date":"2020-05-22T21:32:26","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Fri, May 22, 2020 at 03:02:14AM +0200, Niklas Söderlund wrote:\n> On 2020-05-20 01:26:10 +0300, Laurent Pinchart wrote:\n> > On Tue, May 19, 2020 at 04:59:28PM +0200, Niklas Söderlund wrote:\n> >> On 2020-05-19 17:43:32 +0300, Laurent Pinchart wrote:\n> >>> On Tue, May 19, 2020 at 04:38:14PM +0200, Niklas Söderlund wrote:\n> >>>> On 2020-05-19 06:25:00 +0300, Laurent Pinchart wrote:\n> >>>>> Make the BufferWriter class inherit from FrameSink, and use the\n> >>>>> FrameSink API to manage it. This makes the code more generic, and will\n> >>>>> allow usage of other sinks.\n> >>>>> \n> >>>>> Signed-off-by: Laurent Pinchart \n> >>>>> ---\n> >>>>> src/cam/buffer_writer.cpp | 25 ++++++++++---\n> >>>>> src/cam/buffer_writer.h | 13 ++++---\n> >>>>> src/cam/capture.cpp | 76 ++++++++++++++++++++++++++++++++-------\n> >>>>> src/cam/capture.h | 6 ++--\n> >>>>> 4 files changed, 97 insertions(+), 23 deletions(-)\n> >>>>> \n> >>>>> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> >>>>> index c5a5eb46224a..2bec4b132155 100644\n> >>>>> --- a/src/cam/buffer_writer.cpp\n> >>>>> +++ b/src/cam/buffer_writer.cpp\n> >>>>> @@ -13,6 +13,8 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>> \n> >>>>> +#include \n> >>>>> +\n> >>>>> #include \"buffer_writer.h\"\n> >>>>> \n> >>>>> using namespace libcamera;\n> >>>>> @@ -32,6 +34,21 @@ BufferWriter::~BufferWriter()\n> >>>>> \tmappedBuffers_.clear();\n> >>>>> }\n> >>>>> \n> >>>>> +int BufferWriter::configure(const libcamera::CameraConfiguration &config)\n> >>>>> +{\n> >>>>> +\tint ret = FrameSink::configure(config);\n> >>>>> +\tif (ret < 0)\n> >>>>> +\t\treturn ret;\n> >>>>> +\n> >>>>> +\tstreamNames_.clear();\n> >>>>> +\tfor (unsigned int index = 0; index < config.size(); ++index) {\n> >>>>> +\t\tconst StreamConfiguration &cfg = config.at(index);\n> >>>>> +\t\tstreamNames_[cfg.stream()] = \"stream\" + std::to_string(index);\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\treturn 0;\n> >>>>> +}\n> >>>>> +\n> >>>>> void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> >>>>> {\n> >>>>> \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> >>>>> @@ -43,7 +60,7 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> >>>>> \t}\n> >>>>> }\n> >>>>> \n> >>>>> -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> >>>>> +bool BufferWriter::consumeBuffer(const Stream *stream, FrameBuffer *buffer)\n> >>>>> {\n> >>>>> \tstd::string filename;\n> >>>>> \tsize_t pos;\n> >>>>> @@ -53,7 +70,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> >>>>> \tpos = filename.find_first_of('#');\n> >>>>> \tif (pos != std::string::npos) {\n> >>>>> \t\tstd::stringstream ss;\n> >>>>> -\t\tss << streamName << \"-\" << std::setw(6)\n> >>>>> +\t\tss << streamNames_[stream] << \"-\" << std::setw(6)\n> >>>>> \t\t << std::setfill('0') << buffer->metadata().sequence;\n> >>>>> \t\tfilename.replace(pos, 1, ss.str());\n> >>>>> \t}\n> >>>>> @@ -62,7 +79,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> >>>>> \t\t (pos == std::string::npos ? O_APPEND : O_TRUNC),\n> >>>>> \t\t S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);\n> >>>>> \tif (fd == -1)\n> >>>>> -\t\treturn -errno;\n> >>>>> +\t\treturn true;\n> >>>> \n> >>>> Would it make sens to log an error here as the error no longer is \n> >>>> propagated to the caller?\n> >>> \n> >>> Good point, I'll do so.\n> >>> \n> >>>>> \n> >>>>> \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> >>>>> \t\tvoid *data = mappedBuffers_[plane.fd.fd()].first;\n> >>>>> @@ -84,5 +101,5 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> >>>>> \n> >>>>> \tclose(fd);\n> >>>>> \n> >>>>> -\treturn ret;\n> >>>>> +\treturn true;\n> >>>>> }\n> >>>>> diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> >>>>> index 47e26103e13e..5a5b176f73d8 100644\n> >>>>> --- a/src/cam/buffer_writer.h\n> >>>>> +++ b/src/cam/buffer_writer.h\n> >>>>> @@ -12,18 +12,23 @@\n> >>>>> \n> >>>>> #include \n> >>>>> \n> >>>>> -class BufferWriter\n> >>>>> +#include \"frame_sink.h\"\n> >>>>> +\n> >>>>> +class BufferWriter : public FrameSink\n> >>>>> {\n> >>>>> public:\n> >>>>> \tBufferWriter(const std::string &pattern = \"frame-#.bin\");\n> >>>>> \t~BufferWriter();\n> >>>>> \n> >>>>> -\tvoid mapBuffer(libcamera::FrameBuffer *buffer);\n> >>>>> +\tint configure(const libcamera::CameraConfiguration &config) override;\n> >>>>> \n> >>>>> -\tint write(libcamera::FrameBuffer *buffer,\n> >>>>> -\t\t const std::string &streamName);\n> >>>>> +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> >>>>> +\n> >>>>> +\tbool consumeBuffer(const libcamera::Stream *stream,\n> >>>>> +\t\t\t libcamera::FrameBuffer *buffer) override;\n> >>>>> \n> >>>>> private:\n> >>>>> +\tstd::map streamNames_;\n> >>>>> \tstd::string pattern_;\n> >>>>> \tstd::map> mappedBuffers_;\n> >>>>> };\n> >>>>> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> >>>>> index b7e06bcc9463..7fc9cba48892 100644\n> >>>>> --- a/src/cam/capture.cpp\n> >>>>> +++ b/src/cam/capture.cpp\n> >>>>> @@ -11,6 +11,7 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>> \n> >>>>> +#include \"buffer_writer.h\"\n> >>>>> #include \"capture.h\"\n> >>>>> #include \"main.h\"\n> >>>>> \n> >>>>> @@ -18,7 +19,7 @@ using namespace libcamera;\n> >>>>> \n> >>>>> Capture::Capture(std::shared_ptr camera, CameraConfiguration *config,\n> >>>>> \t\t const StreamRoles &roles)\n> >>>>> -\t: camera_(camera), config_(config), roles_(roles), writer_(nullptr)\n> >>>>> +\t: camera_(camera), config_(config), roles_(roles), sink_(nullptr)\n> >>>>> {\n> >>>>> }\n> >>>>> \n> >>>>> @@ -47,20 +48,28 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n> >>>>> \n> >>>>> \tif (options.isSet(OptFile)) {\n> >>>>> \t\tif (!options[OptFile].toString().empty())\n> >>>>> -\t\t\twriter_ = new BufferWriter(options[OptFile]);\n> >>>>> +\t\t\tsink_ = new BufferWriter(options[OptFile]);\n> >>>>> \t\telse\n> >>>>> -\t\t\twriter_ = new BufferWriter();\n> >>>>> +\t\t\tsink_ = new BufferWriter();\n> >>>>> \t}\n> >>>>> \n> >>>>> +\tif (sink_) {\n> >>>>> +\t\tret = sink_->configure(*config_);\n> >>>>> +\t\tif (ret < 0) {\n> >>>>> +\t\t\tstd::cout << \"Failed to configure frame sink\"\n> >>>>> +\t\t\t\t << std::endl;\n> >>>>> +\t\t\treturn ret;\n> >>>>> +\t\t}\n> >>>>> +\n> >>>>> +\t\tsink_->bufferReleased.connect(this, &Capture::sinkRelease);\n> >>>>> +\t}\n> >>>>> \n> >>>>> \tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> >>>>> \n> >>>>> \tret = capture(loop, allocator);\n> >>>>> \n> >>>>> -\tif (options.isSet(OptFile)) {\n> >>>>> -\t\tdelete writer_;\n> >>>>> -\t\twriter_ = nullptr;\n> >>>>> -\t}\n> >>>>> +\tdelete sink_;\n> >>>>> +\tsink_ = nullptr;\n> >>>>> \n> >>>>> \tdelete allocator;\n> >>>>> \n> >>>>> @@ -110,16 +119,26 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> >>>>> \t\t\t\treturn ret;\n> >>>>> \t\t\t}\n> >>>>> \n> >>>>> -\t\t\tif (writer_)\n> >>>>> -\t\t\t\twriter_->mapBuffer(buffer.get());\n> >>>>> +\t\t\tif (sink_)\n> >>>>> +\t\t\t\tsink_->mapBuffer(buffer.get());\n> >>>>> \t\t}\n> >>>>> \n> >>>>> \t\trequests.push_back(request);\n> >>>>> \t}\n> >>>>> \n> >>>>> +\tif (sink_) {\n> >>>>> +\t\tret = sink_->start();\n> >>>>> +\t\tif (ret) {\n> >>>>> +\t\t\tstd::cout << \"Failed to start frame sink\" << std::endl;\n> >>>>> +\t\t\treturn ret;\n> >>>>> +\t\t}\n> >>>>> +\t}\n> >>>>> +\n> >>>>> \tret = camera_->start();\n> >>>>> \tif (ret) {\n> >>>>> \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> >>>>> +\t\tif (sink_)\n> >>>>> +\t\t\tsink_->stop();\n> >>>>> \t\treturn ret;\n> >>>>> \t}\n> >>>>> \n> >>>>> @@ -128,6 +147,8 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> >>>>> \t\tif (ret < 0) {\n> >>>>> \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> >>>>> \t\t\tcamera_->stop();\n> >>>>> +\t\t\tif (sink_)\n> >>>>> +\t\t\t\tsink_->stop();\n> >>>>> \t\t\treturn ret;\n> >>>>> \t\t}\n> >>>>> \t}\n> >>>>> @@ -141,6 +162,12 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n> >>>>> \tif (ret)\n> >>>>> \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> >>>>> \n> >>>>> +\tif (sink_) {\n> >>>>> +\t\tret = sink_->stop();\n> >>>>> +\t\tif (ret)\n> >>>>> +\t\t\tstd::cout << \"Failed to stop frame sink\" << std::endl;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> \treturn ret;\n> >>>>> }\n> >>>>> \n> >>>>> @@ -157,17 +184,18 @@ void Capture::requestComplete(Request *request)\n> >>>>> \t ? 1000.0 / fps : 0.0;\n> >>>>> \tlast_ = now;\n> >>>>> \n> >>>>> +\tbool requeue = true;\n> >>>>> +\n> >>>>> \tstd::stringstream info;\n> >>>>> \tinfo << \"fps: \" << std::fixed << std::setprecision(2) << fps;\n> >>>>> \n> >>>>> \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> >>>>> \t\tStream *stream = it->first;\n> >>>>> \t\tFrameBuffer *buffer = it->second;\n> >>>>> -\t\tconst std::string &name = streamName_[stream];\n> >>>>> \n> >>>>> \t\tconst FrameMetadata &metadata = buffer->metadata();\n> >>>>> \n> >>>>> -\t\tinfo << \" \" << name\n> >>>>> +\t\tinfo << \" \" << streamName_[stream]\n> >>>>> \t\t << \" seq: \" << std::setw(6) << std::setfill('0') << metadata.sequence\n> >>>>> \t\t << \" bytesused: \";\n> >>>>> \n> >>>>> @@ -178,12 +206,21 @@ void Capture::requestComplete(Request *request)\n> >>>>> \t\t\t\tinfo << \"/\";\n> >>>>> \t\t}\n> >>>>> \n> >>>>> -\t\tif (writer_)\n> >>>>> -\t\t\twriter_->write(buffer, name);\n> >>>>> +\t\tif (sink_) {\n> >>>>> +\t\t\tif (!sink_->consumeBuffer(stream, buffer))\n> >>>>> +\t\t\t\trequeue = false;\n> >>>> \n> >>>> This will only work for requests that contains one buffer. In this \n> >>>> change it will works as BufferWriter::consumeBuffer() returns true and \n> >>>> the while request is requeued as the buffer writer does not need to hold \n> >>>> onto the buffers. I know that the new sink added later in this sereis \n> >>>> only supports one stream so it returning false and holding onto the \n> >>>> buffer will also work with this logic.\n> >>> \n> >>> Damn, you noticed ;-)\n> >> \n> >> Sorry, I will pay less attention in the future ;-P\n> >> \n> >>>> Would it instead make sens to rework this so that the sink always would \n> >>>> need to signal that it's done with the buffer(s) instead of requing the \n> >>>> request from the requestComplete() callback? That way the behavior the \n> >>>> sinks would be the same disregarding for how long it needs to hold on to \n> >>>> the buffer.\n> >>> \n> >>> I think this mechanism is indeed a bit fragile, and needs to be\n> >>> reworked. It works in the two cases we support (file sink and kms sink),\n> >>> but isn't future-proof. That being said, fixing it properly isn't\n> >>> trivial. We can only requeue the request when all buffers have been\n> >>> processed (or, rather, maybe we'll need to requeue a request with other\n> >>> buffers instead). We can't unconditionally queue a new request in the\n> >>> bufferReleased callback of the sink, as that would only work for a\n> >>> single stream.\n> >>> \n> >>> In addition to that, I think the cam application needs to be reworked to\n> >>> move processing of buffers to its main thread. The file sink, in\n> >>> particular, is too expensive to run in the request completion callback,\n> >>> which runs in the camera manager thread. I think it could make sense to\n> >>> address the two issues together. Do you think it needs to be done as\n> >>> part of this series ? Or could I just add a \\todo to explain the issue\n> >>> and make sure we don't forget about it ?\n> >> \n> >> I don't think this needs to be solved the perfect way now, but I do \n> >> think we need something where adding a 3rd sink that supports more then \n> >> one stream and needs to hold onto buffers do not break, but they might \n> >> not need to function in the most efficient way.\n> >> \n> >> As we already discussed that for now cam would only support a single \n> >> sink would it make sens to rework the interface to deal with a Request \n> >> instead of a FrameBuffer? That way the logic you have here could still \n> >> be used while allowing new sinks to be added which would be less \n> >> fragile.\n> > \n> > Just to make it clear, how much rework would you do as part of this\n> > series, and how much should be delayed ?\n> \n> For me the least amount needed to allow for a sink that supports more \n> then one stream but needs to hold onto the buffers/request. I would be \n> OK to pass the whole request to the sink instead of individual buffers. \n> Or if you can think of something else more clever.\n> \n> What I don't like is that the code is fragile and without looking at \n> this change in isolation it's not clear what requirements are put on a \n> sink so adding a new one could explode and take time to figure why it \n> did so. On the other hand I really like the sink concept you add here, \n> maybe it can be shared with qcam sometime far into the future. :-)\n\nI agree with you, moving the sink API from buffer to request is a good\nidea. There's one issue though, the Request objects are transient, and\nwhile they could be passed synchronously to the sink, the sink can't\nhold to them until it completes processing, as they are deleted\nautomatically by the libcamera core in Camera::requestComplete().\n\nI could work around that by only holding on the streams to buffers map\nand passing that from the sink to the Capture class when processing\nfinishes. The alternative is to already turn the Request class into a\nnon-transient object, which we are planning to do anyway. I was hoping\nDRM/KMS support could be merged in cam without that, but maybe I should\nbite the bullet and rework the Request object first ?\n\n> >> That being said, I do agree with you that deepening on what end gaols we \n> >> have for cam we will likely need to rework how requests are constructed. \n> >> We still have the whole concept of request reuse and request templates \n> >> to bash out :-)\n> >> \n> >> I'm however not convinced I like the idea of making cam an interactive \n> >> console tool. Things like push spacebar to save frame to disk while \n> >> viewing the viewfinder on the framebuffer I think maybe is another \n> >> application, but I might be wrong.\n> > \n> > You're probably right there, it's not a direction I intend to push for.\n> > \n> >>>>> +\t\t}\n> >>>>> \t}\n> >>>>> \n> >>>>> \tstd::cout << info.str() << std::endl;\n> >>>>> \n> >>>>> +\t/*\n> >>>>> +\t * If the frame sink holds on the buffer, we'll requeue it later in the\n> >>>>> +\t * complete handler.\n> >>>>> +\t */\n> >>>>> +\tif (!requeue)\n> >>>>> +\t\treturn;\n> >>>>> +\n> >>>>> \t/*\n> >>>>> \t * Create a new request and populate it with one buffer for each\n> >>>>> \t * stream.\n> >>>>> @@ -203,3 +240,16 @@ void Capture::requestComplete(Request *request)\n> >>>>> \n> >>>>> \tcamera_->queueRequest(request);\n> >>>>> }\n> >>>>> +\n> >>>>> +void Capture::sinkRelease(libcamera::FrameBuffer *buffer)\n> >>>>> +{\n> >>>>> +\tRequest *request = camera_->createRequest();\n> >>>>> +\tif (!request) {\n> >>>>> +\t\tstd::cerr << \"Can't create request\" << std::endl;\n> >>>>> +\t\treturn;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\trequest->addBuffer(config_->at(0).stream(), buffer);\n> >>>>> +\n> >>>>> +\tcamera_->queueRequest(request);\n> >>>>> +}\n> >>>>> diff --git a/src/cam/capture.h b/src/cam/capture.h\n> >>>>> index c0e697b831fb..3be1d98e4d3e 100644\n> >>>>> --- a/src/cam/capture.h\n> >>>>> +++ b/src/cam/capture.h\n> >>>>> @@ -16,10 +16,11 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>> \n> >>>>> -#include \"buffer_writer.h\"\n> >>>>> #include \"event_loop.h\"\n> >>>>> #include \"options.h\"\n> >>>>> \n> >>>>> +class FrameSink;\n> >>>>> +\n> >>>>> class Capture\n> >>>>> {\n> >>>>> public:\n> >>>>> @@ -33,13 +34,14 @@ private:\n> >>>>> \t\t libcamera::FrameBufferAllocator *allocator);\n> >>>>> \n> >>>>> \tvoid requestComplete(libcamera::Request *request);\n> >>>>> +\tvoid sinkRelease(libcamera::FrameBuffer *buffer);\n> >>>>> \n> >>>>> \tstd::shared_ptr camera_;\n> >>>>> \tlibcamera::CameraConfiguration *config_;\n> >>>>> \tlibcamera::StreamRoles roles_;\n> >>>>> \n> >>>>> \tstd::map streamName_;\n> >>>>> -\tBufferWriter *writer_;\n> >>>>> +\tFrameSink *sink_;\n> >>>>> \tstd::chrono::steady_clock::time_point last_;\n> >>>>> };\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 26E6A603D7\n\tfor ;\n\tFri, 22 May 2020 23:32:39 +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 8C784B55;\n\tFri, 22 May 2020 23:32:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"S/RUHu6z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1590183158;\n\tbh=KNM3hQFLutIaX6KCYoZZXL5fcMSIEfgKSv+NRWQirGk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S/RUHu6zOReITjeFsx/fV1RFL3cUxX573xIym9pMQfVBCTaqSQF1BTcf9Tr7/eJbP\n\tfxDcmRFk5YFMov3Mq2cPQhi+6oTuTZEhiC9NctHZJD9D3qIbMqPF0kw3sb44Mn6pYt\n\tzOOHfcJMB4gTB1DOUv3x4ipKHxbpr1rCt9iyL4WQ=","Date":"Sat, 23 May 2020 00:32:26 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200522213226.GI5824@pendragon.ideasonboard.com>","References":"<20200519032505.17307-1-laurent.pinchart@ideasonboard.com>\n\t<20200519032505.17307-4-laurent.pinchart@ideasonboard.com>\n\t<20200519143814.GN470768@oden.dyn.berto.se>\n\t<20200519144332.GF3820@pendragon.ideasonboard.com>\n\t<20200519145928.GP470768@oden.dyn.berto.se>\n\t<20200519222610.GG3820@pendragon.ideasonboard.com>\n\t<20200522010214.GB2165767@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200522010214.GB2165767@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a\n\tFrameSink","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 22 May 2020 21:32:39 -0000"}}]