[{"id":18450,"web_url":"https://patchwork.libcamera.org/comment/18450/","msgid":"","date":"2021-07-31T23:08:14","subject":"Re: [libcamera-devel] [PATCH v2 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 2021-07-30 04:03:01 +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\nI really like the end result of this patch, the patch itself touches \nmany different topics and could be split up to ease review, you know \nthis is my pet peeve ;-)\n\nReview is still not too harsh and I like the end result so no need to \nsplit it for my benefit,\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> Changes since v1:\n> \n> - Print message if the file can't be opened\n> ---\n> src/cam/buffer_writer.cpp | 33 +++++++++++++++++---\n> src/cam/buffer_writer.h | 14 ++++++---\n> src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------\n> src/cam/camera_session.h | 6 ++--\n> 4 files changed, 96 insertions(+), 21 deletions(-)\n> \n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> \t}\n> }\n> \n> -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> +bool BufferWriter::consumeRequest(Request *request)\n> {\n> +\tconst Stream *stream = request->buffers().begin()->first;\n> +\tFrameBuffer *buffer = request->buffers().begin()->second;\n> +\n> \tstd::string filename;\n> \tsize_t pos;\n> \tint fd, ret = 0;\n> @@ -58,7 +78,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> @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> \tfd = open(filename.c_str(), O_CREAT | O_WRONLY |\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> +\tif (fd == -1) {\n> +\t\tstd::cerr << \"failed to open file \" << filename << \": \"\n> +\t\t\t << strerror(-ret) << std::endl;\n> +\t\treturn true;\n> +\t}\n> \n> \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n> @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644\n> --- a/src/cam/buffer_writer.h\n> +++ b/src/cam/buffer_writer.h\n> @@ -10,20 +10,24 @@\n> #include \n> #include \n> \n> -#include \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 = \"\");\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 consumeRequest(libcamera::Request *request) override;\n> \n> private:\n> +\tstd::map streamNames_;\n> \tstd::string pattern_;\n> \tstd::map> mappedBuffers_;\n> };\n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index 0d49fc1ade83..465c8e24190e 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> \n> +#include \"buffer_writer.h\"\n> #include \"camera_session.h\"\n> #include \"event_loop.h\"\n> #include \"main.h\"\n> @@ -162,9 +163,20 @@ int CameraSession::start()\n> \n> \tif (options_.isSet(OptFile)) {\n> \t\tif (!options_[OptFile].toString().empty())\n> -\t\t\twriter_ = std::make_unique(options_[OptFile]);\n> +\t\t\tsink_ = std::make_unique(options_[OptFile]);\n> \t\telse\n> -\t\t\twriter_ = std::make_unique();\n> +\t\t\tsink_ = std::make_unique();\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_->requestReleased.connect(this, &CameraSession::sinkRelease);\n> \t}\n> \n> \tallocator_ = std::make_unique(camera_);\n> @@ -178,7 +190,13 @@ void CameraSession::stop()\n> \tif (ret)\n> \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> \n> -\twriter_.reset();\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> +\tsink_.reset();\n> \n> \trequests_.clear();\n> \n> @@ -227,16 +245,26 @@ int CameraSession::startCapture()\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(std::move(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> @@ -245,6 +273,8 @@ int CameraSession::startCapture()\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> @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)\n> \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> \tlast_ = ts;\n> \n> +\tbool requeue = true;\n> +\n> \tstd::stringstream info;\n> \tinfo << ts / 1000000000 << \".\"\n> \t << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000\n> @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)\n> \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> \t\tconst Stream *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> @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)\n> \t\t\tif (++nplane < metadata.planes.size())\n> \t\t\t\tinfo << \"/\";\n> \t\t}\n> +\t}\n> \n> -\t\tif (writer_)\n> -\t\t\twriter_->write(buffer, name);\n> +\tif (sink_) {\n> +\t\tif (!sink_->consumeRequest(request))\n> +\t\t\trequeue = false;\n> \t}\n> \n> \tstd::cout << info.str() << std::endl;\n> @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)\n> \t\treturn;\n> \t}\n> \n> +\t/*\n> +\t * If the frame sink holds on the request, we'll requeue it later in the\n> +\t * complete handler.\n> +\t */\n> +\tif (!requeue)\n> +\t\treturn;\n> +\n> +\trequest->reuse(Request::ReuseBuffers);\n> +\tcamera_->queueRequest(request);\n> +}\n> +\n> +void CameraSession::sinkRelease(Request *request)\n> +{\n> \trequest->reuse(Request::ReuseBuffers);\n> \tqueueRequest(request);\n> }\n> diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> index b0f50e7f998e..2ccc71977a99 100644\n> --- a/src/cam/camera_session.h\n> +++ b/src/cam/camera_session.h\n> @@ -21,9 +21,10 @@\n> #include \n> #include \n> \n> -#include \"buffer_writer.h\"\n> #include \"options.h\"\n> \n> +class FrameSink;\n> +\n> class CameraSession\n> {\n> public:\n> @@ -53,13 +54,14 @@ private:\n> \tint queueRequest(libcamera::Request *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> \tvoid processRequest(libcamera::Request *request);\n> +\tvoid sinkRelease(libcamera::Request *request);\n> \n> \tconst OptionsParser::Options &options_;\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> \n> \tstd::map streamName_;\n> -\tstd::unique_ptr writer_;\n> +\tstd::unique_ptr sink_;\n> \tunsigned int cameraIndex_;\n> \n> \tuint64_t last_;\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 816C5C3232\n\tfor ;\n\tSat, 31 Jul 2021 23:08:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA4F5687C3;\n\tSun, 1 Aug 2021 01:08:17 +0200 (CEST)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4CDA7687BE\n\tfor ;\n\tSun, 1 Aug 2021 01:08:16 +0200 (CEST)","by mail-lf1-x12d.google.com with SMTP id p38so11146590lfa.0\n\tfor ;\n\tSat, 31 Jul 2021 16:08:16 -0700 (PDT)","from localhost (h-46-59-88-219.A463.priv.bahnhof.se.\n\t[46.59.88.219]) by smtp.gmail.com with ESMTPSA id\n\ti11sm505423lfe.215.2021.07.31.16.08.14\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 31 Jul 2021 16:08:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"NYsT3W3V\"; dkim-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=WxdwV+XfpkBYcz+Bxpuwh0tMC3fIbGWNOFZ2mK2VJqY=;\n\tb=NYsT3W3V5cUGJJ1InsA++zoomQAGjFmsO1GiuumRlwBbPpMxDzLElCsf4RJRgMzwsY\n\tk/vpDRYjkGxQotW5mb3zcOJq9ow4W5kxZg6iAvlF6/VcgCGn14V5zq1Aa1ycLeLD58Ct\n\tAgSxfTrIBRp4vjssjlqirgCVEXB1lnLs9VHr3/CxyXcQ4hFIscnjfuJNri+ov2lAXIti\n\tmQR8HhsA8VVb9N8lw+QTdsmDxA4Np2Oim41pQ2h4KcU0MqxGO43tc4M7WmEmkJ3t+zfC\n\ttUSkU6AsC3LGX/nNT5l/5xVz26KF+AY20lhbx/EMIvj1wx0+c9N3nHEcWDZ3P+0VCyMb\n\tun+Q==","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=WxdwV+XfpkBYcz+Bxpuwh0tMC3fIbGWNOFZ2mK2VJqY=;\n\tb=ePeSGClzTqVSxfxKxlqiUsU6Atl8H3YzzMyYDlKwuNJVHr5HSS8z31k327fUZ0ZxQy\n\tc6mZ0fCDs28+XLdqXEurxABxSK5AdwVQEsJ2e7QK1gNQtQV0kdPHdP7Fo7mYEplMKKEV\n\tZsePK2DTdo1bo6wGh1oRH993gkhKsIA3IikE6L2JGd8C4fe4bx6T8plpssnJ5I+Zsq/D\n\tdkv+xz6t0jx2tP/ydDLc0Ctb1XI4f6uRB/uk59e4uQHz1ciFmd6ic7nok2d3Y2GW5b4K\n\tUCKNfHsKVtcZB/D7bXW6NuUM89IPQkBxDgIbbNiJlhyNiYZXY8UEmDJAwLgkBmKc0kLv\n\tCQNA==","X-Gm-Message-State":"AOAM533sSO/G59VFYItKLR+JIBwCe/XtZgTe6NPqIjmzSdocSnrZ8eq1\n\tKvzAHA+V0It2XAyML1oPMdnVohuBa7Pjnw==","X-Google-Smtp-Source":"ABdhPJwC4gd1pL1QkApP/KF65/xpeguFTYvDnw+vrHTASwstTfvy5zkgiOAwRX65AMy6cq+/V2gtPA==","X-Received":"by 2002:a05:6512:200d:: with SMTP id\n\ta13mr6819130lfb.251.1627772895572; \n\tSat, 31 Jul 2021 16:08:15 -0700 (PDT)","Date":"Sun, 1 Aug 2021 01:08:14 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-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":"<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18503,"web_url":"https://patchwork.libcamera.org/comment/18503/","msgid":"<20210803070601.GZ2167@pyrite.rasen.tech>","date":"2021-08-03T07:06:01","subject":"Re: [libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a\n\tFrameSink","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Fri, Jul 30, 2021 at 04:03:01AM +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> Changes since v1:\n> \n> - Print message if the file can't be opened\n> ---\n> src/cam/buffer_writer.cpp | 33 +++++++++++++++++---\n> src/cam/buffer_writer.h | 14 ++++++---\n> src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------\n> src/cam/camera_session.h | 6 ++--\n> 4 files changed, 96 insertions(+), 21 deletions(-)\n> \n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> \t}\n> }\n> \n> -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> +bool BufferWriter::consumeRequest(Request *request)\n\nSo this return value signifies whether or not the FrameSink will hold on\nto the request, and it's not a success value (?). And if it does hold on\nto the request then it needs to emit it in requestReleased.\n\nShould this be documented anywhere, or is the KMSSink later sufficient\nas an \"example\"?\n\n> {\n> +\tconst Stream *stream = request->buffers().begin()->first;\n> +\tFrameBuffer *buffer = request->buffers().begin()->second;\n> +\n> \tstd::string filename;\n> \tsize_t pos;\n> \tint fd, ret = 0;\n> @@ -58,7 +78,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> @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> \tfd = open(filename.c_str(), O_CREAT | O_WRONLY |\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> +\tif (fd == -1) {\n> +\t\tstd::cerr << \"failed to open file \" << filename << \": \"\n> +\t\t\t << strerror(-ret) << std::endl;\n> +\t\treturn true;\n> +\t}\n> \n> \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n> @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644\n> --- a/src/cam/buffer_writer.h\n> +++ b/src/cam/buffer_writer.h\n> @@ -10,20 +10,24 @@\n> #include \n> #include \n> \n> -#include \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 = \"\");\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 consumeRequest(libcamera::Request *request) override;\n> \n> private:\n> +\tstd::map streamNames_;\n> \tstd::string pattern_;\n> \tstd::map> mappedBuffers_;\n> };\n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index 0d49fc1ade83..465c8e24190e 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> \n> +#include \"buffer_writer.h\"\n> #include \"camera_session.h\"\n> #include \"event_loop.h\"\n> #include \"main.h\"\n> @@ -162,9 +163,20 @@ int CameraSession::start()\n> \n> \tif (options_.isSet(OptFile)) {\n> \t\tif (!options_[OptFile].toString().empty())\n> -\t\t\twriter_ = std::make_unique(options_[OptFile]);\n> +\t\t\tsink_ = std::make_unique(options_[OptFile]);\n> \t\telse\n> -\t\t\twriter_ = std::make_unique();\n> +\t\t\tsink_ = std::make_unique();\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_->requestReleased.connect(this, &CameraSession::sinkRelease);\n> \t}\n> \n> \tallocator_ = std::make_unique(camera_);\n> @@ -178,7 +190,13 @@ void CameraSession::stop()\n> \tif (ret)\n> \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> \n> -\twriter_.reset();\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> +\tsink_.reset();\n> \n> \trequests_.clear();\n> \n> @@ -227,16 +245,26 @@ int CameraSession::startCapture()\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(std::move(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> @@ -245,6 +273,8 @@ int CameraSession::startCapture()\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> @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)\n> \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> \tlast_ = ts;\n> \n> +\tbool requeue = true;\n> +\n> \tstd::stringstream info;\n> \tinfo << ts / 1000000000 << \".\"\n> \t << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000\n> @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)\n> \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> \t\tconst Stream *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> @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)\n> \t\t\tif (++nplane < metadata.planes.size())\n> \t\t\t\tinfo << \"/\";\n> \t\t}\n> +\t}\n> \n> -\t\tif (writer_)\n> -\t\t\twriter_->write(buffer, name);\n> +\tif (sink_) {\n> +\t\tif (!sink_->consumeRequest(request))\n> +\t\t\trequeue = false;\n\nSo we're not writing every buffer in each request anymore :D Should this\nbe pointed out in the commit message?\n\n\nOther than that, looks good.\n\nReviewed-by: Paul Elder \n\n> \t}\n> \n> \tstd::cout << info.str() << std::endl;\n> @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)\n> \t\treturn;\n> \t}\n> \n> +\t/*\n> +\t * If the frame sink holds on the request, we'll requeue it later in the\n> +\t * complete handler.\n> +\t */\n> +\tif (!requeue)\n> +\t\treturn;\n> +\n> +\trequest->reuse(Request::ReuseBuffers);\n> +\tcamera_->queueRequest(request);\n> +}\n> +\n> +void CameraSession::sinkRelease(Request *request)\n> +{\n> \trequest->reuse(Request::ReuseBuffers);\n> \tqueueRequest(request);\n> }\n> diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> index b0f50e7f998e..2ccc71977a99 100644\n> --- a/src/cam/camera_session.h\n> +++ b/src/cam/camera_session.h\n> @@ -21,9 +21,10 @@\n> #include \n> #include \n> \n> -#include \"buffer_writer.h\"\n> #include \"options.h\"\n> \n> +class FrameSink;\n> +\n> class CameraSession\n> {\n> public:\n> @@ -53,13 +54,14 @@ private:\n> \tint queueRequest(libcamera::Request *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> \tvoid processRequest(libcamera::Request *request);\n> +\tvoid sinkRelease(libcamera::Request *request);\n> \n> \tconst OptionsParser::Options &options_;\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> \n> \tstd::map streamName_;\n> -\tstd::unique_ptr writer_;\n> +\tstd::unique_ptr sink_;\n> \tunsigned int cameraIndex_;\n> \n> \tuint64_t last_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0FB00C3232\n\tfor ;\n\tTue, 3 Aug 2021 07:06:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69EC3687CF;\n\tTue, 3 Aug 2021 09:06:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A8C5E6026D\n\tfor ;\n\tTue, 3 Aug 2021 09:06:08 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 20ADC4A3;\n\tTue, 3 Aug 2021 09:06:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Yl57LDdc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627974368;\n\tbh=LzK6MGMpTqy2fijfU2aL7gEUjxGwxs0M+kwdaqX651o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Yl57LDdcelIdiFOtAyJJl3+SM7qFLL2lGL4kIaFJfIfIcSZprUuWYJjdOQcQTpaPW\n\taIqvuf1Di/Qxdns2qPsTdyl8apLuXlD5fi1qD+pv/JfIgzJFCYxj8U2/ojzsa5KlUo\n\t337Tjcrv3u69Js2jllSBn2JuiD7GMYfrXwrm8CHM=","Date":"Tue, 3 Aug 2021 16:06:01 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210803070601.GZ2167@pyrite.rasen.tech>","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18504,"web_url":"https://patchwork.libcamera.org/comment/18504/","msgid":"","date":"2021-08-03T07:14:11","subject":"Re: [libcamera-devel] [PATCH v2 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 Paul,\n\nOn Tue, Aug 03, 2021 at 04:06:01PM +0900, paul.elder@ideasonboard.com wrote:\n> On Fri, Jul 30, 2021 at 04:03:01AM +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> > Changes since v1:\n> > \n> > - Print message if the file can't be opened\n> > ---\n> > src/cam/buffer_writer.cpp | 33 +++++++++++++++++---\n> > src/cam/buffer_writer.h | 14 ++++++---\n> > src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------\n> > src/cam/camera_session.h | 6 ++--\n> > 4 files changed, 96 insertions(+), 21 deletions(-)\n> > \n> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > \t}\n> > }\n> > \n> > -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > +bool BufferWriter::consumeRequest(Request *request)\n> \n> So this return value signifies whether or not the FrameSink will hold on\n> to the request, and it's not a success value (?). And if it does hold on\n> to the request then it needs to emit it in requestReleased.\n> \n> Should this be documented anywhere, or is the KMSSink later sufficient\n> as an \"example\"?\n\nDocumentation is always good. I'll add a comment block in\nframe_sink.cpp.\n\n> > {\n> > +\tconst Stream *stream = request->buffers().begin()->first;\n> > +\tFrameBuffer *buffer = request->buffers().begin()->second;\n> > +\n> > \tstd::string filename;\n> > \tsize_t pos;\n> > \tint fd, ret = 0;\n> > @@ -58,7 +78,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> > @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > \tfd = open(filename.c_str(), O_CREAT | O_WRONLY |\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> > +\tif (fd == -1) {\n> > +\t\tstd::cerr << \"failed to open file \" << filename << \": \"\n> > +\t\t\t << strerror(-ret) << std::endl;\n> > +\t\treturn true;\n> > +\t}\n> > \n> > \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n> > @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644\n> > --- a/src/cam/buffer_writer.h\n> > +++ b/src/cam/buffer_writer.h\n> > @@ -10,20 +10,24 @@\n> > #include \n> > #include \n> > \n> > -#include \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 = \"\");\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 consumeRequest(libcamera::Request *request) override;\n> > \n> > private:\n> > +\tstd::map streamNames_;\n> > \tstd::string pattern_;\n> > \tstd::map> mappedBuffers_;\n> > };\n> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > index 0d49fc1ade83..465c8e24190e 100644\n> > --- a/src/cam/camera_session.cpp\n> > +++ b/src/cam/camera_session.cpp\n> > @@ -13,6 +13,7 @@\n> > #include \n> > #include \n> > \n> > +#include \"buffer_writer.h\"\n> > #include \"camera_session.h\"\n> > #include \"event_loop.h\"\n> > #include \"main.h\"\n> > @@ -162,9 +163,20 @@ int CameraSession::start()\n> > \n> > \tif (options_.isSet(OptFile)) {\n> > \t\tif (!options_[OptFile].toString().empty())\n> > -\t\t\twriter_ = std::make_unique(options_[OptFile]);\n> > +\t\t\tsink_ = std::make_unique(options_[OptFile]);\n> > \t\telse\n> > -\t\t\twriter_ = std::make_unique();\n> > +\t\t\tsink_ = std::make_unique();\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_->requestReleased.connect(this, &CameraSession::sinkRelease);\n> > \t}\n> > \n> > \tallocator_ = std::make_unique(camera_);\n> > @@ -178,7 +190,13 @@ void CameraSession::stop()\n> > \tif (ret)\n> > \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> > \n> > -\twriter_.reset();\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> > +\tsink_.reset();\n> > \n> > \trequests_.clear();\n> > \n> > @@ -227,16 +245,26 @@ int CameraSession::startCapture()\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(std::move(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> > @@ -245,6 +273,8 @@ int CameraSession::startCapture()\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> > @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)\n> > \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> > \tlast_ = ts;\n> > \n> > +\tbool requeue = true;\n> > +\n> > \tstd::stringstream info;\n> > \tinfo << ts / 1000000000 << \".\"\n> > \t << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000\n> > @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)\n> > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > \t\tconst Stream *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> > @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)\n> > \t\t\tif (++nplane < metadata.planes.size())\n> > \t\t\t\tinfo << \"/\";\n> > \t\t}\n> > +\t}\n> > \n> > -\t\tif (writer_)\n> > -\t\t\twriter_->write(buffer, name);\n> > +\tif (sink_) {\n> > +\t\tif (!sink_->consumeRequest(request))\n> > +\t\t\trequeue = false;\n> \n> So we're not writing every buffer in each request anymore :D Should this\n> be pointed out in the commit message?\n\nNot quite, we should instead fix the BufferWriter :-) Good catch, I'll\nfix this.\n\n> Other than that, looks good.\n> \n> Reviewed-by: Paul Elder \n> \n> > \t}\n> > \n> > \tstd::cout << info.str() << std::endl;\n> > @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)\n> > \t\treturn;\n> > \t}\n> > \n> > +\t/*\n> > +\t * If the frame sink holds on the request, we'll requeue it later in the\n> > +\t * complete handler.\n> > +\t */\n> > +\tif (!requeue)\n> > +\t\treturn;\n> > +\n> > +\trequest->reuse(Request::ReuseBuffers);\n> > +\tcamera_->queueRequest(request);\n> > +}\n> > +\n> > +void CameraSession::sinkRelease(Request *request)\n> > +{\n> > \trequest->reuse(Request::ReuseBuffers);\n> > \tqueueRequest(request);\n> > }\n> > diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> > index b0f50e7f998e..2ccc71977a99 100644\n> > --- a/src/cam/camera_session.h\n> > +++ b/src/cam/camera_session.h\n> > @@ -21,9 +21,10 @@\n> > #include \n> > #include \n> > \n> > -#include \"buffer_writer.h\"\n> > #include \"options.h\"\n> > \n> > +class FrameSink;\n> > +\n> > class CameraSession\n> > {\n> > public:\n> > @@ -53,13 +54,14 @@ private:\n> > \tint queueRequest(libcamera::Request *request);\n> > \tvoid requestComplete(libcamera::Request *request);\n> > \tvoid processRequest(libcamera::Request *request);\n> > +\tvoid sinkRelease(libcamera::Request *request);\n> > \n> > \tconst OptionsParser::Options &options_;\n> > \tstd::shared_ptr camera_;\n> > \tstd::unique_ptr config_;\n> > \n> > \tstd::map streamName_;\n> > -\tstd::unique_ptr writer_;\n> > +\tstd::unique_ptr sink_;\n> > \tunsigned int cameraIndex_;\n> > \n> > \tuint64_t last_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F3433C3235\n\tfor ;\n\tTue, 3 Aug 2021 07:14:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79B72687CF;\n\tTue, 3 Aug 2021 09:14:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 96D516026D\n\tfor ;\n\tTue, 3 Aug 2021 09:14:23 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09CEC4A3;\n\tTue, 3 Aug 2021 09:14:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RULfc7X+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627974863;\n\tbh=5edz66cknsbFR1s4a4OddXShIwCsPtI/mmwphtyIW84=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RULfc7X+she06FZ1UQUBP+OjyTIP5J1xC7SzWMpCx3Wuv3efNQo1sVMNi8zMkvxYr\n\tpdnzxLNHsVBWM+KoSbBfNUPbgBeDLaYJx324WxZ/dP9HY8lMqYWQOpjM/0mTVY0P53\n\tbqHE++lb8+vTjV9k2gcwmL6TU2wUNXQvacGHb4Tk=","Date":"Tue, 3 Aug 2021 10:14:11 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>\n\t<20210803070601.GZ2167@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210803070601.GZ2167@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v2 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18517,"web_url":"https://patchwork.libcamera.org/comment/18517/","msgid":"<2faa2fb2-8c1d-55b4-f612-2b47835c2f7b@ideasonboard.com>","date":"2021-08-03T14:28:59","subject":"Re: [libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a\n\tFrameSink","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 30/07/2021 02:03, 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> Changes since v1:\n> \n> - Print message if the file can't be opened\n> ---\n> src/cam/buffer_writer.cpp | 33 +++++++++++++++++---\n> src/cam/buffer_writer.h | 14 ++++++---\n> src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------\n> src/cam/camera_session.h | 6 ++--\n> 4 files changed, 96 insertions(+), 21 deletions(-)\n> \n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> \t}\n> }\n> \n> -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> +bool BufferWriter::consumeRequest(Request *request)\n> {\n> +\tconst Stream *stream = request->buffers().begin()->first;\n> +\tFrameBuffer *buffer = request->buffers().begin()->second;\n> +\n> \tstd::string filename;\n> \tsize_t pos;\n> \tint fd, ret = 0;\n> @@ -58,7 +78,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> @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> \tfd = open(filename.c_str(), O_CREAT | O_WRONLY |\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> +\tif (fd == -1) {\n> +\t\tstd::cerr << \"failed to open file \" << filename << \": \"\n> +\t\t\t << strerror(-ret) << std::endl;\n> +\t\treturn true;\n> +\t}\n> \n> \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n> @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644\n> --- a/src/cam/buffer_writer.h\n> +++ b/src/cam/buffer_writer.h\n> @@ -10,20 +10,24 @@\n> #include \n> #include \n> \n> -#include \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 = \"\");\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 consumeRequest(libcamera::Request *request) override;\n> \n> private:\n> +\tstd::map streamNames_;\n> \tstd::string pattern_;\n> \tstd::map> mappedBuffers_;\n> };\n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index 0d49fc1ade83..465c8e24190e 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> \n> +#include \"buffer_writer.h\"\n> #include \"camera_session.h\"\n> #include \"event_loop.h\"\n> #include \"main.h\"\n> @@ -162,9 +163,20 @@ int CameraSession::start()\n> \n> \tif (options_.isSet(OptFile)) {\n> \t\tif (!options_[OptFile].toString().empty())\n> -\t\t\twriter_ = std::make_unique(options_[OptFile]);\n> +\t\t\tsink_ = std::make_unique(options_[OptFile]);\n> \t\telse\n> -\t\t\twriter_ = std::make_unique();\n> +\t\t\tsink_ = std::make_unique();\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_->requestReleased.connect(this, &CameraSession::sinkRelease);\n> \t}\n> \n> \tallocator_ = std::make_unique(camera_);\n> @@ -178,7 +190,13 @@ void CameraSession::stop()\n> \tif (ret)\n> \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> \n> -\twriter_.reset();\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> +\tsink_.reset();\n> \n> \trequests_.clear();\n> \n> @@ -227,16 +245,26 @@ int CameraSession::startCapture()\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(std::move(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> @@ -245,6 +273,8 @@ int CameraSession::startCapture()\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> @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)\n> \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> \tlast_ = ts;\n> \n> +\tbool requeue = true;\n> +\n> \tstd::stringstream info;\n> \tinfo << ts / 1000000000 << \".\"\n> \t << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000\n> @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)\n> \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> \t\tconst Stream *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> @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)\n> \t\t\tif (++nplane < metadata.planes.size())\n> \t\t\t\tinfo << \"/\";\n> \t\t}\n> +\t}\n> \n> -\t\tif (writer_)\n> -\t\t\twriter_->write(buffer, name);\n> +\tif (sink_) {\n> +\t\tif (!sink_->consumeRequest(request))\n> +\t\t\trequeue = false;\n\nThis reads counter intuitively.\n\n\"If the request was not consumed, don't requeue it.\"\n\nI wonder if we can we 'move' the request into the consumeRequest?\n\nAlthough then we have to 'return' the request if it's not consumed...\nand I'm not sure if that's better anyway.\n\nAny better ideas? if not ... I'll look the other way.\n\n> \t}\n> \n> \tstd::cout << info.str() << std::endl;\n> @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)\n> \t\treturn;\n> \t}\n> \n> +\t/*\n> +\t * If the frame sink holds on the request, we'll requeue it later in the\n\n'holds on to the'\n\nReviewed-by: Kieran Bingham \n\n> +\t * complete handler.\n> +\t */\n> +\tif (!requeue)\n> +\t\treturn;\n> +\n> +\trequest->reuse(Request::ReuseBuffers);\n> +\tcamera_->queueRequest(request);\n> +}\n> +\n> +void CameraSession::sinkRelease(Request *request)\n> +{\n> \trequest->reuse(Request::ReuseBuffers);\n> \tqueueRequest(request);\n> }\n> diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> index b0f50e7f998e..2ccc71977a99 100644\n> --- a/src/cam/camera_session.h\n> +++ b/src/cam/camera_session.h\n> @@ -21,9 +21,10 @@\n> #include \n> #include \n> \n> -#include \"buffer_writer.h\"\n> #include \"options.h\"\n> \n> +class FrameSink;\n> +\n> class CameraSession\n> {\n> public:\n> @@ -53,13 +54,14 @@ private:\n> \tint queueRequest(libcamera::Request *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> \tvoid processRequest(libcamera::Request *request);\n> +\tvoid sinkRelease(libcamera::Request *request);\n> \n> \tconst OptionsParser::Options &options_;\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> \n> \tstd::map streamName_;\n> -\tstd::unique_ptr writer_;\n> +\tstd::unique_ptr sink_;\n> \tunsigned int cameraIndex_;\n> \n> \tuint64_t last_;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C83C8C3232\n\tfor ;\n\tTue, 3 Aug 2021 14:29:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 983D468536;\n\tTue, 3 Aug 2021 16:29:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C9AC6026A\n\tfor ;\n\tTue, 3 Aug 2021 16:29:02 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 683A93F0;\n\tTue, 3 Aug 2021 16:29:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LPGuBCws\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628000941;\n\tbh=nLycdZyjKJPJ1xWa+mPFaoIvQPzEPoDeYFO5mv1zc8o=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=LPGuBCws25VrJEvdjd3M+sxfrJCFd01U8xH+TBwEKlIcKZxKk/4JNzQRh5B44iBzJ\n\tPOvt3b+bYdVMyZwzFwfbMCkKVgR//aSFDtIbJpY/BWDtZmRxBAiLkrj/75CvFIl/yl\n\tIgjiQuoQlImWGGO3qHGX8I2y0J1Zf5LW1esk8KYY=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<2faa2fb2-8c1d-55b4-f612-2b47835c2f7b@ideasonboard.com>","Date":"Tue, 3 Aug 2021 15:28:59 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18519,"web_url":"https://patchwork.libcamera.org/comment/18519/","msgid":"","date":"2021-08-03T14:40:19","subject":"Re: [libcamera-devel] [PATCH v2 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 Kieran,\n\nOn Tue, Aug 03, 2021 at 03:28:59PM +0100, Kieran Bingham wrote:\n> On 30/07/2021 02:03, 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> > Changes since v1:\n> > \n> > - Print message if the file can't be opened\n> > ---\n> > src/cam/buffer_writer.cpp | 33 +++++++++++++++++---\n> > src/cam/buffer_writer.h | 14 ++++++---\n> > src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------\n> > src/cam/camera_session.h | 6 ++--\n> > 4 files changed, 96 insertions(+), 21 deletions(-)\n> > \n> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > \t}\n> > }\n> > \n> > -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > +bool BufferWriter::consumeRequest(Request *request)\n> > {\n> > +\tconst Stream *stream = request->buffers().begin()->first;\n> > +\tFrameBuffer *buffer = request->buffers().begin()->second;\n> > +\n> > \tstd::string filename;\n> > \tsize_t pos;\n> > \tint fd, ret = 0;\n> > @@ -58,7 +78,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> > @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > \tfd = open(filename.c_str(), O_CREAT | O_WRONLY |\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> > +\tif (fd == -1) {\n> > +\t\tstd::cerr << \"failed to open file \" << filename << \": \"\n> > +\t\t\t << strerror(-ret) << std::endl;\n> > +\t\treturn true;\n> > +\t}\n> > \n> > \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n> > @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644\n> > --- a/src/cam/buffer_writer.h\n> > +++ b/src/cam/buffer_writer.h\n> > @@ -10,20 +10,24 @@\n> > #include \n> > #include \n> > \n> > -#include \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 = \"\");\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 consumeRequest(libcamera::Request *request) override;\n> > \n> > private:\n> > +\tstd::map streamNames_;\n> > \tstd::string pattern_;\n> > \tstd::map> mappedBuffers_;\n> > };\n> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > index 0d49fc1ade83..465c8e24190e 100644\n> > --- a/src/cam/camera_session.cpp\n> > +++ b/src/cam/camera_session.cpp\n> > @@ -13,6 +13,7 @@\n> > #include \n> > #include \n> > \n> > +#include \"buffer_writer.h\"\n> > #include \"camera_session.h\"\n> > #include \"event_loop.h\"\n> > #include \"main.h\"\n> > @@ -162,9 +163,20 @@ int CameraSession::start()\n> > \n> > \tif (options_.isSet(OptFile)) {\n> > \t\tif (!options_[OptFile].toString().empty())\n> > -\t\t\twriter_ = std::make_unique(options_[OptFile]);\n> > +\t\t\tsink_ = std::make_unique(options_[OptFile]);\n> > \t\telse\n> > -\t\t\twriter_ = std::make_unique();\n> > +\t\t\tsink_ = std::make_unique();\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_->requestReleased.connect(this, &CameraSession::sinkRelease);\n> > \t}\n> > \n> > \tallocator_ = std::make_unique(camera_);\n> > @@ -178,7 +190,13 @@ void CameraSession::stop()\n> > \tif (ret)\n> > \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> > \n> > -\twriter_.reset();\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> > +\tsink_.reset();\n> > \n> > \trequests_.clear();\n> > \n> > @@ -227,16 +245,26 @@ int CameraSession::startCapture()\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(std::move(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> > @@ -245,6 +273,8 @@ int CameraSession::startCapture()\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> > @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)\n> > \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> > \tlast_ = ts;\n> > \n> > +\tbool requeue = true;\n> > +\n> > \tstd::stringstream info;\n> > \tinfo << ts / 1000000000 << \".\"\n> > \t << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000\n> > @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)\n> > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > \t\tconst Stream *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> > @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)\n> > \t\t\tif (++nplane < metadata.planes.size())\n> > \t\t\t\tinfo << \"/\";\n> > \t\t}\n> > +\t}\n> > \n> > -\t\tif (writer_)\n> > -\t\t\twriter_->write(buffer, name);\n> > +\tif (sink_) {\n> > +\t\tif (!sink_->consumeRequest(request))\n> > +\t\t\trequeue = false;\n> \n> This reads counter intuitively.\n> \n> \"If the request was not consumed, don't requeue it.\"\n\nThat's what it does, if the request wasn't consumed synchronously, don't\nrequeue it.\n\n> I wonder if we can we 'move' the request into the consumeRequest?\n> \n> Although then we have to 'return' the request if it's not consumed...\n> and I'm not sure if that's better anyway.\n\nYes it's not very nice.\n\n> Any better ideas? if not ... I'll look the other way.\n\n> > +\t\tif (!sink_->consumeRequest(request))\n> > +\t\t\trequeue = false;\n\nWe could define an enum with two values. I'm sure we would bikeshed the\nenum and enumerator names though :-)\n\nAnother option would be always have the sink emit the requestConsumed\nsignal. It would require more intrusive changes in\nCameraSession::processRequest() though, to correctly handle the\nsynchronous case.\n\n> > \t}\n> > \n> > \tstd::cout << info.str() << std::endl;\n> > @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)\n> > \t\treturn;\n> > \t}\n> > \n> > +\t/*\n> > +\t * If the frame sink holds on the request, we'll requeue it later in the\n> \n> 'holds on to the'\n\n\"on to\" or \"onto\" ?\n\n> Reviewed-by: Kieran Bingham \n> \n> > +\t * complete handler.\n> > +\t */\n> > +\tif (!requeue)\n> > +\t\treturn;\n> > +\n> > +\trequest->reuse(Request::ReuseBuffers);\n> > +\tcamera_->queueRequest(request);\n> > +}\n> > +\n> > +void CameraSession::sinkRelease(Request *request)\n> > +{\n> > \trequest->reuse(Request::ReuseBuffers);\n> > \tqueueRequest(request);\n> > }\n> > diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> > index b0f50e7f998e..2ccc71977a99 100644\n> > --- a/src/cam/camera_session.h\n> > +++ b/src/cam/camera_session.h\n> > @@ -21,9 +21,10 @@\n> > #include \n> > #include \n> > \n> > -#include \"buffer_writer.h\"\n> > #include \"options.h\"\n> > \n> > +class FrameSink;\n> > +\n> > class CameraSession\n> > {\n> > public:\n> > @@ -53,13 +54,14 @@ private:\n> > \tint queueRequest(libcamera::Request *request);\n> > \tvoid requestComplete(libcamera::Request *request);\n> > \tvoid processRequest(libcamera::Request *request);\n> > +\tvoid sinkRelease(libcamera::Request *request);\n> > \n> > \tconst OptionsParser::Options &options_;\n> > \tstd::shared_ptr camera_;\n> > \tstd::unique_ptr config_;\n> > \n> > \tstd::map streamName_;\n> > -\tstd::unique_ptr writer_;\n> > +\tstd::unique_ptr sink_;\n> > \tunsigned int cameraIndex_;\n> > \n> > \tuint64_t last_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0B450C3235\n\tfor ;\n\tTue, 3 Aug 2021 14:40:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7ECD568536;\n\tTue, 3 Aug 2021 16:40:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2EC276026A\n\tfor ;\n\tTue, 3 Aug 2021 16:40:32 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A532D3F0;\n\tTue, 3 Aug 2021 16:40:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XDITiyhQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628001631;\n\tbh=AFj6OO1yZMoji5crmcZmGfy1h1wsGdCk+y5e9GGjyPc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XDITiyhQD8YF5y+s1Qv1rIMARNH8ZdtTjsWEXD5fJPB4fnDY3EYYb5gRFfBiKuoiM\n\t0nQRqXAp2EEsEdaPmR25bJMe3EdgcnYTbyZpXEmSRxpgDisS4uPibYR37kZcmyPLLv\n\t5p7U+rhuCM2C6n9i7xiMXgPTViuiA0blrLgT805M=","Date":"Tue, 3 Aug 2021 17:40:19 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>\n\t<2faa2fb2-8c1d-55b4-f612-2b47835c2f7b@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<2faa2fb2-8c1d-55b4-f612-2b47835c2f7b@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18521,"web_url":"https://patchwork.libcamera.org/comment/18521/","msgid":"<8ab371e0-0f1a-60a3-3144-b0bb556bbe76@ideasonboard.com>","date":"2021-08-03T15:49:32","subject":"Re: [libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a\n\tFrameSink","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 03/08/2021 15:40, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Tue, Aug 03, 2021 at 03:28:59PM +0100, Kieran Bingham wrote:\n>> On 30/07/2021 02:03, 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>>> Changes since v1:\n>>>\n>>> - Print message if the file can't be opened\n>>> ---\n>>> src/cam/buffer_writer.cpp | 33 +++++++++++++++++---\n>>> src/cam/buffer_writer.h | 14 ++++++---\n>>> src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------\n>>> src/cam/camera_session.h | 6 ++--\n>>> 4 files changed, 96 insertions(+), 21 deletions(-)\n>>>\n>>> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n>>> index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n>>> \t}\n>>> }\n>>> \n>>> -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n>>> +bool BufferWriter::consumeRequest(Request *request)\n>>> {\n>>> +\tconst Stream *stream = request->buffers().begin()->first;\n>>> +\tFrameBuffer *buffer = request->buffers().begin()->second;\n>>> +\n>>> \tstd::string filename;\n>>> \tsize_t pos;\n>>> \tint fd, ret = 0;\n>>> @@ -58,7 +78,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>>> @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n>>> \tfd = open(filename.c_str(), O_CREAT | O_WRONLY |\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>>> +\tif (fd == -1) {\n>>> +\t\tstd::cerr << \"failed to open file \" << filename << \": \"\n>>> +\t\t\t << strerror(-ret) << std::endl;\n>>> +\t\treturn true;\n>>> +\t}\n>>> \n>>> \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n>>> \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n>>> @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644\n>>> --- a/src/cam/buffer_writer.h\n>>> +++ b/src/cam/buffer_writer.h\n>>> @@ -10,20 +10,24 @@\n>>> #include \n>>> #include \n>>> \n>>> -#include \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 = \"\");\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 consumeRequest(libcamera::Request *request) override;\n>>> \n>>> private:\n>>> +\tstd::map streamNames_;\n>>> \tstd::string pattern_;\n>>> \tstd::map> mappedBuffers_;\n>>> };\n>>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n>>> index 0d49fc1ade83..465c8e24190e 100644\n>>> --- a/src/cam/camera_session.cpp\n>>> +++ b/src/cam/camera_session.cpp\n>>> @@ -13,6 +13,7 @@\n>>> #include \n>>> #include \n>>> \n>>> +#include \"buffer_writer.h\"\n>>> #include \"camera_session.h\"\n>>> #include \"event_loop.h\"\n>>> #include \"main.h\"\n>>> @@ -162,9 +163,20 @@ int CameraSession::start()\n>>> \n>>> \tif (options_.isSet(OptFile)) {\n>>> \t\tif (!options_[OptFile].toString().empty())\n>>> -\t\t\twriter_ = std::make_unique(options_[OptFile]);\n>>> +\t\t\tsink_ = std::make_unique(options_[OptFile]);\n>>> \t\telse\n>>> -\t\t\twriter_ = std::make_unique();\n>>> +\t\t\tsink_ = std::make_unique();\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_->requestReleased.connect(this, &CameraSession::sinkRelease);\n>>> \t}\n>>> \n>>> \tallocator_ = std::make_unique(camera_);\n>>> @@ -178,7 +190,13 @@ void CameraSession::stop()\n>>> \tif (ret)\n>>> \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n>>> \n>>> -\twriter_.reset();\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>>> +\tsink_.reset();\n>>> \n>>> \trequests_.clear();\n>>> \n>>> @@ -227,16 +245,26 @@ int CameraSession::startCapture()\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(std::move(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>>> @@ -245,6 +273,8 @@ int CameraSession::startCapture()\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>>> @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)\n>>> \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n>>> \tlast_ = ts;\n>>> \n>>> +\tbool requeue = true;\n>>> +\n>>> \tstd::stringstream info;\n>>> \tinfo << ts / 1000000000 << \".\"\n>>> \t << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000\n>>> @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)\n>>> \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n>>> \t\tconst Stream *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>>> @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)\n>>> \t\t\tif (++nplane < metadata.planes.size())\n>>> \t\t\t\tinfo << \"/\";\n>>> \t\t}\n>>> +\t}\n>>> \n>>> -\t\tif (writer_)\n>>> -\t\t\twriter_->write(buffer, name);\n>>> +\tif (sink_) {\n>>> +\t\tif (!sink_->consumeRequest(request))\n>>> +\t\t\trequeue = false;\n>>\n>> This reads counter intuitively.\n>>\n>> \"If the request was not consumed, don't requeue it.\"\n> \n> That's what it does, if the request wasn't consumed synchronously, don't\n> requeue it.\n\n\nTo me, if something has consumed an item - then ... that recipient now\nowns it ... and has perhaps in some form destroyed it.\n\nOr in this instance, it would be the sink's responsibility to deal with\nthe request from there on if it has consumed it, so it should be the one\nthat is going to requeue it when it has completed (presumably\nasynchronously if it consumed the item/object)\n\n\nIt could be that changing the 'verb' from consumeRequest to something\nelse will convey the meaning better, and remove my confusion ;-)\n\n\n>> I wonder if we can we 'move' the request into the consumeRequest?\n>>\n>> Although then we have to 'return' the request if it's not consumed...\n>> and I'm not sure if that's better anyway.\n> \n> Yes it's not very nice.\n> \n>> Any better ideas? if not ... I'll look the other way.\n> \n>>> +\t\tif (!sink_->consumeRequest(request))\n>>> +\t\t\trequeue = false;\n> \n> We could define an enum with two values. I'm sure we would bikeshed the\n> enum and enumerator names though :-)\n> \n> Another option would be always have the sink emit the requestConsumed\n> signal. It would require more intrusive changes in\n> CameraSession::processRequest() though, to correctly handle the\n> synchronous case.\n\n\nI don't think the sink needs to always be forced to emit the\nrequestConsumed.\n\nHowever, I would (later) envisage that there might be multiple sinks\nthat could operate on a Request, either in parallel or in a chain.\n\nFor example, perhaps there might be a desire to both write out the\nbuffer with a FileSink, while simultaneously displaying it on the KMSSink.\n\n(Hrm, you might have mentioned that use case already too recently)\n\n\n>>> \t}\n>>> \n>>> \tstd::cout << info.str() << std::endl;\n>>> @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)\n>>> \t\treturn;\n>>> \t}\n>>> \n>>> +\t/*\n>>> +\t * If the frame sink holds on the request, we'll requeue it later in the\n>>\n>> 'holds on to the'\n> \n> \"on to\" or \"onto\" ?\n\nEither of those ;-)\n\n\n\n>> Reviewed-by: Kieran Bingham \n>>\n>>> +\t * complete handler.\n>>> +\t */\n>>> +\tif (!requeue)\n>>> +\t\treturn;\n>>> +\n>>> +\trequest->reuse(Request::ReuseBuffers);\n>>> +\tcamera_->queueRequest(request);\n>>> +}\n>>> +\n>>> +void CameraSession::sinkRelease(Request *request)\n>>> +{\n>>> \trequest->reuse(Request::ReuseBuffers);\n>>> \tqueueRequest(request);\n>>> }\n>>> diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n>>> index b0f50e7f998e..2ccc71977a99 100644\n>>> --- a/src/cam/camera_session.h\n>>> +++ b/src/cam/camera_session.h\n>>> @@ -21,9 +21,10 @@\n>>> #include \n>>> #include \n>>> \n>>> -#include \"buffer_writer.h\"\n>>> #include \"options.h\"\n>>> \n>>> +class FrameSink;\n>>> +\n>>> class CameraSession\n>>> {\n>>> public:\n>>> @@ -53,13 +54,14 @@ private:\n>>> \tint queueRequest(libcamera::Request *request);\n>>> \tvoid requestComplete(libcamera::Request *request);\n>>> \tvoid processRequest(libcamera::Request *request);\n>>> +\tvoid sinkRelease(libcamera::Request *request);\n>>> \n>>> \tconst OptionsParser::Options &options_;\n>>> \tstd::shared_ptr camera_;\n>>> \tstd::unique_ptr config_;\n>>> \n>>> \tstd::map streamName_;\n>>> -\tstd::unique_ptr writer_;\n>>> +\tstd::unique_ptr sink_;\n>>> \tunsigned int cameraIndex_;\n>>> \n>>> \tuint64_t last_;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DDDDDC3235\n\tfor ;\n\tTue, 3 Aug 2021 15:49:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C468687CC;\n\tTue, 3 Aug 2021 17:49:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 31D576026A\n\tfor ;\n\tTue, 3 Aug 2021 17:49:35 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A940924F;\n\tTue, 3 Aug 2021 17:49:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"E93pUg6r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628005774;\n\tbh=vCgVE/sZxezbj0YzsD3N4dhXGHFJLZjxonf0qK4692o=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=E93pUg6rRYAjRWqJQp+bNOCkw1b1GEYOOssMGQM0hZUnZBSvKF0RUw4b0kuoVmW16\n\tId2HyYyPQPnANFTxOA1qyyRzYKfFnCb8/OMYWQWNAzgDjZmrIhCmbGiGKTeAHHlUQj\n\txRoSKLMXCV8zbpK0uuTsT3wHQ1GUvPrxkcDNkA1s=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>\n\t<2faa2fb2-8c1d-55b4-f612-2b47835c2f7b@ideasonboard.com>\n\t","Message-ID":"<8ab371e0-0f1a-60a3-3144-b0bb556bbe76@ideasonboard.com>","Date":"Tue, 3 Aug 2021 16:49:32 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18522,"web_url":"https://patchwork.libcamera.org/comment/18522/","msgid":"","date":"2021-08-03T16:01:31","subject":"Re: [libcamera-devel] [PATCH v2 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 Kieran,\n\nOn Tue, Aug 03, 2021 at 04:49:32PM +0100, Kieran Bingham wrote:\n> On 03/08/2021 15:40, Laurent Pinchart wrote:\n> > On Tue, Aug 03, 2021 at 03:28:59PM +0100, Kieran Bingham wrote:\n> >> On 30/07/2021 02:03, 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> >>> Changes since v1:\n> >>>\n> >>> - Print message if the file can't be opened\n> >>> ---\n> >>> src/cam/buffer_writer.cpp | 33 +++++++++++++++++---\n> >>> src/cam/buffer_writer.h | 14 ++++++---\n> >>> src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------\n> >>> src/cam/camera_session.h | 6 ++--\n> >>> 4 files changed, 96 insertions(+), 21 deletions(-)\n> >>>\n> >>> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> >>> index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> >>> \t}\n> >>> }\n> >>> \n> >>> -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> >>> +bool BufferWriter::consumeRequest(Request *request)\n> >>> {\n> >>> +\tconst Stream *stream = request->buffers().begin()->first;\n> >>> +\tFrameBuffer *buffer = request->buffers().begin()->second;\n> >>> +\n> >>> \tstd::string filename;\n> >>> \tsize_t pos;\n> >>> \tint fd, ret = 0;\n> >>> @@ -58,7 +78,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> >>> @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> >>> \tfd = open(filename.c_str(), O_CREAT | O_WRONLY |\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> >>> +\tif (fd == -1) {\n> >>> +\t\tstd::cerr << \"failed to open file \" << filename << \": \"\n> >>> +\t\t\t << strerror(-ret) << std::endl;\n> >>> +\t\treturn true;\n> >>> +\t}\n> >>> \n> >>> \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> >>> \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n> >>> @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644\n> >>> --- a/src/cam/buffer_writer.h\n> >>> +++ b/src/cam/buffer_writer.h\n> >>> @@ -10,20 +10,24 @@\n> >>> #include \n> >>> #include \n> >>> \n> >>> -#include \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 = \"\");\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 consumeRequest(libcamera::Request *request) override;\n> >>> \n> >>> private:\n> >>> +\tstd::map streamNames_;\n> >>> \tstd::string pattern_;\n> >>> \tstd::map> mappedBuffers_;\n> >>> };\n> >>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> >>> index 0d49fc1ade83..465c8e24190e 100644\n> >>> --- a/src/cam/camera_session.cpp\n> >>> +++ b/src/cam/camera_session.cpp\n> >>> @@ -13,6 +13,7 @@\n> >>> #include \n> >>> #include \n> >>> \n> >>> +#include \"buffer_writer.h\"\n> >>> #include \"camera_session.h\"\n> >>> #include \"event_loop.h\"\n> >>> #include \"main.h\"\n> >>> @@ -162,9 +163,20 @@ int CameraSession::start()\n> >>> \n> >>> \tif (options_.isSet(OptFile)) {\n> >>> \t\tif (!options_[OptFile].toString().empty())\n> >>> -\t\t\twriter_ = std::make_unique(options_[OptFile]);\n> >>> +\t\t\tsink_ = std::make_unique(options_[OptFile]);\n> >>> \t\telse\n> >>> -\t\t\twriter_ = std::make_unique();\n> >>> +\t\t\tsink_ = std::make_unique();\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_->requestReleased.connect(this, &CameraSession::sinkRelease);\n> >>> \t}\n> >>> \n> >>> \tallocator_ = std::make_unique(camera_);\n> >>> @@ -178,7 +190,13 @@ void CameraSession::stop()\n> >>> \tif (ret)\n> >>> \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> >>> \n> >>> -\twriter_.reset();\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> >>> +\tsink_.reset();\n> >>> \n> >>> \trequests_.clear();\n> >>> \n> >>> @@ -227,16 +245,26 @@ int CameraSession::startCapture()\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(std::move(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> >>> @@ -245,6 +273,8 @@ int CameraSession::startCapture()\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> >>> @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)\n> >>> \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> >>> \tlast_ = ts;\n> >>> \n> >>> +\tbool requeue = true;\n> >>> +\n> >>> \tstd::stringstream info;\n> >>> \tinfo << ts / 1000000000 << \".\"\n> >>> \t << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000\n> >>> @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)\n> >>> \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> >>> \t\tconst Stream *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> >>> @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)\n> >>> \t\t\tif (++nplane < metadata.planes.size())\n> >>> \t\t\t\tinfo << \"/\";\n> >>> \t\t}\n> >>> +\t}\n> >>> \n> >>> -\t\tif (writer_)\n> >>> -\t\t\twriter_->write(buffer, name);\n> >>> +\tif (sink_) {\n> >>> +\t\tif (!sink_->consumeRequest(request))\n> >>> +\t\t\trequeue = false;\n> >>\n> >> This reads counter intuitively.\n> >>\n> >> \"If the request was not consumed, don't requeue it.\"\n> > \n> > That's what it does, if the request wasn't consumed synchronously, don't\n> > requeue it.\n> \n> To me, if something has consumed an item - then ... that recipient now\n> owns it ... and has perhaps in some form destroyed it.\n> \n> Or in this instance, it would be the sink's responsibility to deal with\n> the request from there on if it has consumed it, so it should be the one\n> that is going to requeue it when it has completed (presumably\n> asynchronously if it consumed the item/object)\n> \n> \n> It could be that changing the 'verb' from consumeRequest to something\n> else will convey the meaning better, and remove my confusion ;-)\n\nIt will be processRequest() and requestProcessed in v3.\n\n> >> I wonder if we can we 'move' the request into the consumeRequest?\n> >>\n> >> Although then we have to 'return' the request if it's not consumed...\n> >> and I'm not sure if that's better anyway.\n> > \n> > Yes it's not very nice.\n> > \n> >> Any better ideas? if not ... I'll look the other way.\n> > \n> >>> +\t\tif (!sink_->consumeRequest(request))\n> >>> +\t\t\trequeue = false;\n> > \n> > We could define an enum with two values. I'm sure we would bikeshed the\n> > enum and enumerator names though :-)\n> > \n> > Another option would be always have the sink emit the requestConsumed\n> > signal. It would require more intrusive changes in\n> > CameraSession::processRequest() though, to correctly handle the\n> > synchronous case.\n> \n> I don't think the sink needs to always be forced to emit the\n> requestConsumed.\n> \n> However, I would (later) envisage that there might be multiple sinks\n> that could operate on a Request, either in parallel or in a chain.\n> \n> For example, perhaps there might be a desire to both write out the\n> buffer with a FileSink, while simultaneously displaying it on the KMSSink.\n> \n> (Hrm, you might have mentioned that use case already too recently)\n\nThat will be another occasion to rename the function again :-)\n\n> >>> \t}\n> >>> \n> >>> \tstd::cout << info.str() << std::endl;\n> >>> @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)\n> >>> \t\treturn;\n> >>> \t}\n> >>> \n> >>> +\t/*\n> >>> +\t * If the frame sink holds on the request, we'll requeue it later in the\n> >>\n> >> 'holds on to the'\n> > \n> > \"on to\" or \"onto\" ?\n> \n> Either of those ;-)\n> \n> >> Reviewed-by: Kieran Bingham \n> >>\n> >>> +\t * complete handler.\n> >>> +\t */\n> >>> +\tif (!requeue)\n> >>> +\t\treturn;\n> >>> +\n> >>> +\trequest->reuse(Request::ReuseBuffers);\n> >>> +\tcamera_->queueRequest(request);\n> >>> +}\n> >>> +\n> >>> +void CameraSession::sinkRelease(Request *request)\n> >>> +{\n> >>> \trequest->reuse(Request::ReuseBuffers);\n> >>> \tqueueRequest(request);\n> >>> }\n> >>> diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> >>> index b0f50e7f998e..2ccc71977a99 100644\n> >>> --- a/src/cam/camera_session.h\n> >>> +++ b/src/cam/camera_session.h\n> >>> @@ -21,9 +21,10 @@\n> >>> #include \n> >>> #include \n> >>> \n> >>> -#include \"buffer_writer.h\"\n> >>> #include \"options.h\"\n> >>> \n> >>> +class FrameSink;\n> >>> +\n> >>> class CameraSession\n> >>> {\n> >>> public:\n> >>> @@ -53,13 +54,14 @@ private:\n> >>> \tint queueRequest(libcamera::Request *request);\n> >>> \tvoid requestComplete(libcamera::Request *request);\n> >>> \tvoid processRequest(libcamera::Request *request);\n> >>> +\tvoid sinkRelease(libcamera::Request *request);\n> >>> \n> >>> \tconst OptionsParser::Options &options_;\n> >>> \tstd::shared_ptr camera_;\n> >>> \tstd::unique_ptr config_;\n> >>> \n> >>> \tstd::map streamName_;\n> >>> -\tstd::unique_ptr writer_;\n> >>> +\tstd::unique_ptr sink_;\n> >>> \tunsigned int cameraIndex_;\n> >>> \n> >>> \tuint64_t last_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4C4BCC3232\n\tfor ;\n\tTue, 3 Aug 2021 16:01:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C357C68536;\n\tTue, 3 Aug 2021 18:01:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F03436026A\n\tfor ;\n\tTue, 3 Aug 2021 18:01:43 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 738DF3F0;\n\tTue, 3 Aug 2021 18:01:43 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Hqm61HQe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628006503;\n\tbh=3WQKLBSF8zJ8+gtrhVsAiixzhWzB3PGNSSmLPWUlY7Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Hqm61HQeZtOIuHjXWwWJikPOFyZj8FmT+01UJ20Y3Xcv0bvqJguYvRpyu6McnAJ+X\n\tL96v7HAXpuzCEMy3vNp+69wLsYZjHB6JkZNpWHGfJ9xvK3sLnHVvrhSxweS1KOCyre\n\tOWGrIB3URCVXXc4Nkkzs7TkTgO3GYpqMHb1SlHqI=","Date":"Tue, 3 Aug 2021 19:01:31 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>\n\t<2faa2fb2-8c1d-55b4-f612-2b47835c2f7b@ideasonboard.com>\n\t\n\t<8ab371e0-0f1a-60a3-3144-b0bb556bbe76@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<8ab371e0-0f1a-60a3-3144-b0bb556bbe76@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18540,"web_url":"https://patchwork.libcamera.org/comment/18540/","msgid":"<206e8370-1719-207b-8d4e-00f8f7906a3f@ideasonboard.com>","date":"2021-08-04T08:35:56","subject":"Re: [libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a\n\tFrameSink","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch.\n\nOn 7/30/21 6:33 AM, 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> Changes since v1:\n>\n> - Print message if the file can't be opened\n> ---\n> src/cam/buffer_writer.cpp | 33 +++++++++++++++++---\n> src/cam/buffer_writer.h | 14 ++++++---\n> src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------\n> src/cam/camera_session.h | 6 ++--\n> 4 files changed, 96 insertions(+), 21 deletions(-)\n>\n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> \t}\n> }\n> \n> -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> +bool BufferWriter::consumeRequest(Request *request)\nI'm still processing what the return value signifies here. In one of the \nother mails, you mentioned to insert a comment/doc about it, so I'll \nwait for that in further versions.\n> {\n> +\tconst Stream *stream = request->buffers().begin()->first;\n> +\tFrameBuffer *buffer = request->buffers().begin()->second;\n> +\n> \tstd::string filename;\n> \tsize_t pos;\n> \tint fd, ret = 0;\n> @@ -58,7 +78,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> @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> \tfd = open(filename.c_str(), O_CREAT | O_WRONLY |\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> +\tif (fd == -1) {\n> +\t\tstd::cerr << \"failed to open file \" << filename << \": \"\n> +\t\t\t << strerror(-ret) << std::endl;\n\nWhat exactly is ret here? I see it's initialized to '0' but never again \nset till here. Am I missing something?\n\nRest looks good:\nReviewed-by: Umang Jain \n\n> +\t\treturn true;\n> +\t}\n> \n> \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n> @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644\n> --- a/src/cam/buffer_writer.h\n> +++ b/src/cam/buffer_writer.h\n> @@ -10,20 +10,24 @@\n> #include \n> #include \n> \n> -#include \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 = \"\");\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 consumeRequest(libcamera::Request *request) override;\n> \n> private:\n> +\tstd::map streamNames_;\n> \tstd::string pattern_;\n> \tstd::map> mappedBuffers_;\n> };\n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index 0d49fc1ade83..465c8e24190e 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> \n> +#include \"buffer_writer.h\"\n> #include \"camera_session.h\"\n> #include \"event_loop.h\"\n> #include \"main.h\"\n> @@ -162,9 +163,20 @@ int CameraSession::start()\n> \n> \tif (options_.isSet(OptFile)) {\n> \t\tif (!options_[OptFile].toString().empty())\n> -\t\t\twriter_ = std::make_unique(options_[OptFile]);\n> +\t\t\tsink_ = std::make_unique(options_[OptFile]);\n> \t\telse\n> -\t\t\twriter_ = std::make_unique();\n> +\t\t\tsink_ = std::make_unique();\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_->requestReleased.connect(this, &CameraSession::sinkRelease);\n> \t}\n> \n> \tallocator_ = std::make_unique(camera_);\n> @@ -178,7 +190,13 @@ void CameraSession::stop()\n> \tif (ret)\n> \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> \n> -\twriter_.reset();\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> +\tsink_.reset();\n> \n> \trequests_.clear();\n> \n> @@ -227,16 +245,26 @@ int CameraSession::startCapture()\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(std::move(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> @@ -245,6 +273,8 @@ int CameraSession::startCapture()\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> @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)\n> \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> \tlast_ = ts;\n> \n> +\tbool requeue = true;\n> +\n> \tstd::stringstream info;\n> \tinfo << ts / 1000000000 << \".\"\n> \t << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000\n> @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)\n> \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> \t\tconst Stream *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> @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)\n> \t\t\tif (++nplane < metadata.planes.size())\n> \t\t\t\tinfo << \"/\";\n> \t\t}\n> +\t}\n> \n> -\t\tif (writer_)\n> -\t\t\twriter_->write(buffer, name);\n> +\tif (sink_) {\n> +\t\tif (!sink_->consumeRequest(request))\n> +\t\t\trequeue = false;\n> \t}\n> \n> \tstd::cout << info.str() << std::endl;\n> @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)\n> \t\treturn;\n> \t}\n> \n> +\t/*\n> +\t * If the frame sink holds on the request, we'll requeue it later in the\n> +\t * complete handler.\n> +\t */\n> +\tif (!requeue)\n> +\t\treturn;\n> +\n> +\trequest->reuse(Request::ReuseBuffers);\n> +\tcamera_->queueRequest(request);\n> +}\n> +\n> +void CameraSession::sinkRelease(Request *request)\n> +{\n> \trequest->reuse(Request::ReuseBuffers);\n> \tqueueRequest(request);\n> }\n> diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> index b0f50e7f998e..2ccc71977a99 100644\n> --- a/src/cam/camera_session.h\n> +++ b/src/cam/camera_session.h\n> @@ -21,9 +21,10 @@\n> #include \n> #include \n> \n> -#include \"buffer_writer.h\"\n> #include \"options.h\"\n> \n> +class FrameSink;\n> +\n> class CameraSession\n> {\n> public:\n> @@ -53,13 +54,14 @@ private:\n> \tint queueRequest(libcamera::Request *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> \tvoid processRequest(libcamera::Request *request);\n> +\tvoid sinkRelease(libcamera::Request *request);\n> \n> \tconst OptionsParser::Options &options_;\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> \n> \tstd::map streamName_;\n> -\tstd::unique_ptr writer_;\n> +\tstd::unique_ptr sink_;\n> \tunsigned int cameraIndex_;\n> \n> \tuint64_t last_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9FC62C3235\n\tfor ;\n\tWed, 4 Aug 2021 08:36:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DC2E6880F;\n\tWed, 4 Aug 2021 10:36:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1F186026C\n\tfor ;\n\tWed, 4 Aug 2021 10:36:01 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.40])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DDE3824F;\n\tWed, 4 Aug 2021 10:36:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XxWrloTs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628066161;\n\tbh=1yBdRh3kWo5SSQpqInd6C1Qiof+pwGVP/ueBIG94mY8=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=XxWrloTsvod53hsMQP0Ofc9nYx8wjNTIpYea9RKA2wx94sdZt4J+IHP8SAjRrou/5\n\tWqirvXc0vKy1qjX/Snhqk7WzmdCKU5DTVFzQsbr9+RVjRztbfqn9WuAbcbHssPb8q5\n\tDST/WR4srQBrqsiM/C6JrTVty5pxSJBYzcBV6+ls=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<206e8370-1719-207b-8d4e-00f8f7906a3f@ideasonboard.com>","Date":"Wed, 4 Aug 2021 14:05:56 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18543,"web_url":"https://patchwork.libcamera.org/comment/18543/","msgid":"","date":"2021-08-04T08:55:22","subject":"Re: [libcamera-devel] [PATCH v2 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 Umang,\n\nOn Wed, Aug 04, 2021 at 02:05:56PM +0530, Umang Jain wrote:\n> On 7/30/21 6:33 AM, 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> > Changes since v1:\n> >\n> > - Print message if the file can't be opened\n> > ---\n> > src/cam/buffer_writer.cpp | 33 +++++++++++++++++---\n> > src/cam/buffer_writer.h | 14 ++++++---\n> > src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------\n> > src/cam/camera_session.h | 6 ++--\n> > 4 files changed, 96 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)\n> > \t}\n> > }\n> > \n> > -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > +bool BufferWriter::consumeRequest(Request *request)\n>\n> I'm still processing what the return value signifies here. In one of the \n> other mails, you mentioned to insert a comment/doc about it, so I'll \n> wait for that in further versions.\n>\n> > {\n> > +\tconst Stream *stream = request->buffers().begin()->first;\n> > +\tFrameBuffer *buffer = request->buffers().begin()->second;\n> > +\n> > \tstd::string filename;\n> > \tsize_t pos;\n> > \tint fd, ret = 0;\n> > @@ -58,7 +78,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> > @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n> > \tfd = open(filename.c_str(), O_CREAT | O_WRONLY |\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> > +\tif (fd == -1) {\n> > +\t\tstd::cerr << \"failed to open file \" << filename << \": \"\n> > +\t\t\t << strerror(-ret) << std::endl;\n> \n> What exactly is ret here? I see it's initialized to '0' but never again \n> set till here. Am I missing something?\n\nOops. There should be a\n\n\tret = -errno;\n\njust before this line. I'll fix it.\n\n> Rest looks good:\n> Reviewed-by: Umang Jain \n> \n> > +\t\treturn true;\n> > +\t}\n> > \n> > \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n> > @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644\n> > --- a/src/cam/buffer_writer.h\n> > +++ b/src/cam/buffer_writer.h\n> > @@ -10,20 +10,24 @@\n> > #include \n> > #include \n> > \n> > -#include \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 = \"\");\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 consumeRequest(libcamera::Request *request) override;\n> > \n> > private:\n> > +\tstd::map streamNames_;\n> > \tstd::string pattern_;\n> > \tstd::map> mappedBuffers_;\n> > };\n> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > index 0d49fc1ade83..465c8e24190e 100644\n> > --- a/src/cam/camera_session.cpp\n> > +++ b/src/cam/camera_session.cpp\n> > @@ -13,6 +13,7 @@\n> > #include \n> > #include \n> > \n> > +#include \"buffer_writer.h\"\n> > #include \"camera_session.h\"\n> > #include \"event_loop.h\"\n> > #include \"main.h\"\n> > @@ -162,9 +163,20 @@ int CameraSession::start()\n> > \n> > \tif (options_.isSet(OptFile)) {\n> > \t\tif (!options_[OptFile].toString().empty())\n> > -\t\t\twriter_ = std::make_unique(options_[OptFile]);\n> > +\t\t\tsink_ = std::make_unique(options_[OptFile]);\n> > \t\telse\n> > -\t\t\twriter_ = std::make_unique();\n> > +\t\t\tsink_ = std::make_unique();\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_->requestReleased.connect(this, &CameraSession::sinkRelease);\n> > \t}\n> > \n> > \tallocator_ = std::make_unique(camera_);\n> > @@ -178,7 +190,13 @@ void CameraSession::stop()\n> > \tif (ret)\n> > \t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> > \n> > -\twriter_.reset();\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> > +\tsink_.reset();\n> > \n> > \trequests_.clear();\n> > \n> > @@ -227,16 +245,26 @@ int CameraSession::startCapture()\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(std::move(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> > @@ -245,6 +273,8 @@ int CameraSession::startCapture()\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> > @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)\n> > \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> > \tlast_ = ts;\n> > \n> > +\tbool requeue = true;\n> > +\n> > \tstd::stringstream info;\n> > \tinfo << ts / 1000000000 << \".\"\n> > \t << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000\n> > @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)\n> > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > \t\tconst Stream *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> > @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)\n> > \t\t\tif (++nplane < metadata.planes.size())\n> > \t\t\t\tinfo << \"/\";\n> > \t\t}\n> > +\t}\n> > \n> > -\t\tif (writer_)\n> > -\t\t\twriter_->write(buffer, name);\n> > +\tif (sink_) {\n> > +\t\tif (!sink_->consumeRequest(request))\n> > +\t\t\trequeue = false;\n> > \t}\n> > \n> > \tstd::cout << info.str() << std::endl;\n> > @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)\n> > \t\treturn;\n> > \t}\n> > \n> > +\t/*\n> > +\t * If the frame sink holds on the request, we'll requeue it later in the\n> > +\t * complete handler.\n> > +\t */\n> > +\tif (!requeue)\n> > +\t\treturn;\n> > +\n> > +\trequest->reuse(Request::ReuseBuffers);\n> > +\tcamera_->queueRequest(request);\n> > +}\n> > +\n> > +void CameraSession::sinkRelease(Request *request)\n> > +{\n> > \trequest->reuse(Request::ReuseBuffers);\n> > \tqueueRequest(request);\n> > }\n> > diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> > index b0f50e7f998e..2ccc71977a99 100644\n> > --- a/src/cam/camera_session.h\n> > +++ b/src/cam/camera_session.h\n> > @@ -21,9 +21,10 @@\n> > #include \n> > #include \n> > \n> > -#include \"buffer_writer.h\"\n> > #include \"options.h\"\n> > \n> > +class FrameSink;\n> > +\n> > class CameraSession\n> > {\n> > public:\n> > @@ -53,13 +54,14 @@ private:\n> > \tint queueRequest(libcamera::Request *request);\n> > \tvoid requestComplete(libcamera::Request *request);\n> > \tvoid processRequest(libcamera::Request *request);\n> > +\tvoid sinkRelease(libcamera::Request *request);\n> > \n> > \tconst OptionsParser::Options &options_;\n> > \tstd::shared_ptr camera_;\n> > \tstd::unique_ptr config_;\n> > \n> > \tstd::map streamName_;\n> > -\tstd::unique_ptr writer_;\n> > +\tstd::unique_ptr sink_;\n> > \tunsigned int cameraIndex_;\n> > \n> > \tuint64_t last_;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6D312C3235\n\tfor ;\n\tWed, 4 Aug 2021 08:55:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFFD168811;\n\tWed, 4 Aug 2021 10:55:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 123366026C\n\tfor ;\n\tWed, 4 Aug 2021 10:55:35 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 883F024F;\n\tWed, 4 Aug 2021 10:55:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oySB31/h\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628067334;\n\tbh=S16I/IpshndG6ow5YWk5ndgGUl915XdJ7rTA5oziemE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oySB31/hcrBI+GMgFTxQwlTaklTSELZDpC5OzoFIzQMkYoV2lEt3mUdyG93o1rMXC\n\tWrYce5v5y+Q+lR89B5NUQtw310SP44HrygYw8N3K4xylwhIDNuFkqMO7gc7xbqZId+\n\t1on8wgdiA5rFO1MliiJH5RaHpejzIte5TFXKMnOE=","Date":"Wed, 4 Aug 2021 11:55:22 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-4-laurent.pinchart@ideasonboard.com>\n\t<206e8370-1719-207b-8d4e-00f8f7906a3f@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<206e8370-1719-207b-8d4e-00f8f7906a3f@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]