[{"id":4687,"web_url":"https://patchwork.libcamera.org/comment/4687/","msgid":"<20200430191945.GT5856@pendragon.ideasonboard.com>","date":"2020-04-30T19:19:45","subject":"Re: [libcamera-devel] [RFC/PATCH 2/5] qcam: Allow for a second raw\n\tstream to be configured","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Thu, Apr 30, 2020 at 02:36:01AM +0200, Niklas Söderlund wrote:\n> Allow a second stream to be configured for raw capture. This change only\n> adds support for configuring and allocating buffers for the second\n> stream. Later changes are needed to queue the allocated buffers to the\n> camera when the user wish to capture a raw frame.\n\ns/wish/wishes/\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/qcam/main_window.cpp | 133 +++++++++++++++++++++++++++------------\n> src/qcam/main_window.h | 8 ++-\n> 2 files changed, 99 insertions(+), 42 deletions(-)\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index b683c2e00d317307..e77bc01df8f3edfe 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -278,17 +278,30 @@ void MainWindow::toggleCapture(bool start)\n> int MainWindow::startCapture()\n> {\n> \tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> +\tstd::vector requests;\n\nDo you need to have this at the beginning of the function ? I suppose\nit's needed because of a goto below ?\n\nThe function is getting fairly big, please feel free to split it in more\nmanageable chunks.\n\n> \tint ret;\n> \n> \t/* Verify roles are supported. */\n> -\tif (roles.size() != 1) {\n> -\t\tqWarning() << \"Only one stream supported\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\tif (roles[0] != StreamRole::Viewfinder) {\n> -\t\tqWarning() << \"Only viewfinder supported\";\n> -\t\treturn -EINVAL;\n> +\tswitch (roles.size()) {\n> +\tcase 1:\n> +\t\tif (roles[0] != StreamRole::Viewfinder) {\n> +\t\t\tqWarning() << \"Only viewfinder supported for single stream\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t\tbreak;\n> +\tcase 2:\n> +\t\tif (roles[0] != StreamRole::Viewfinder ||\n> +\t\t roles[1] != StreamRole::StillCaptureRaw) {\n> +\t\t\tqWarning() << \"Only viewfinder + raw supported for dual streams\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tif (roles.size() != 1) {\n> +\t\t\tqWarning() << \"Unsuported stream configuration\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t\tbreak;\n> \t}\n> \n> \t/* Configure the camera. */\n> @@ -298,17 +311,17 @@ int MainWindow::startCapture()\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tStreamConfiguration &cfg = config_->at(0);\n> +\tStreamConfiguration &vfConfig = config_->at(0);\n> \n> \t/* Use a format supported by the viewfinder if available. */\n> -\tstd::vector formats = cfg.formats().pixelformats();\n> +\tstd::vector formats = vfConfig.formats().pixelformats();\n> \tfor (const PixelFormat &format : viewfinder_->nativeFormats()) {\n> \t\tauto match = std::find_if(formats.begin(), formats.end(),\n> \t\t\t\t\t [&](const PixelFormat &f) {\n> \t\t\t\t\t\t return f == format;\n> \t\t\t\t\t });\n> \t\tif (match != formats.end()) {\n> -\t\t\tcfg.pixelFormat = format;\n> +\t\t\tvfConfig.pixelFormat = format;\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> @@ -326,7 +339,7 @@ int MainWindow::startCapture()\n> \n> \tif (validation == CameraConfiguration::Adjusted)\n> \t\tqInfo() << \"Stream configuration adjusted to \"\n> -\t\t\t<< cfg.toString().c_str();\n> +\t\t\t<< vfConfig.toString().c_str();\n> \n> \tret = camera_->configure(config_.get());\n> \tif (ret < 0) {\n> @@ -334,10 +347,16 @@ int MainWindow::startCapture()\n> \t\treturn ret;\n> \t}\n> \n> +\t/* Store stream allocation. */\n> +\tvfStream_ = config_->at(0).stream();\n> +\tif (config_->size() == 2)\n> +\t\trawStream_ = config_->at(1).stream();\n> +\telse\n> +\t\trawStream_ = nullptr;\n> +\n> \t/* Configure the viewfinder. */\n> -\tStream *stream = cfg.stream();\n> -\tret = viewfinder_->setFormat(cfg.pixelFormat,\n> -\t\t\t\t QSize(cfg.size.width, cfg.size.height));\n> +\tret = viewfinder_->setFormat(vfConfig.pixelFormat,\n> +\t\t\t\t QSize(vfConfig.size.width, vfConfig.size.height));\n> \tif (ret < 0) {\n> \t\tqInfo() << \"Failed to set viewfinder format\";\n> \t\treturn ret;\n> @@ -345,16 +364,33 @@ int MainWindow::startCapture()\n> \n> \tadjustSize();\n> \n> -\t/* Allocate buffers and requests. */\n> +\t/* Allocate and map buffers. */\n> \tallocator_ = new FrameBufferAllocator(camera_);\n> -\tret = allocator_->allocate(stream);\n> -\tif (ret < 0) {\n> -\t\tqWarning() << \"Failed to allocate capture buffers\";\n> -\t\treturn ret;\n> +\tfor (StreamConfiguration &config : *config_) {\n> +\t\tStream *stream = config.stream();\n> +\n> +\t\tret = allocator_->allocate(stream);\n> +\t\tif (ret < 0) {\n> +\t\t\tqWarning() << \"Failed to allocate capture buffers\";\n> +\t\t\tgoto error;\n> +\t\t}\n> +\n> +\t\tfor (const std::unique_ptr &buffer : allocator_->buffers(stream)) {\n> +\t\t\t/* Map memory buffers and cache the mappings. */\n> +\t\t\tconst FrameBuffer::Plane &plane = buffer->planes().front();\n> +\t\t\tvoid *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n> +\t\t\t\t\t plane.fd.fd(), 0);\n> +\t\t\tmappedBuffers_[buffer.get()] = { memory, plane.length };\n> +\n> +\t\t\t/* Store buffers on the free list. */\n> +\t\t\tfreeBuffers_[stream].enqueue(buffer.get());\n> +\t\t}\n> \t}\n> \n> -\tstd::vector requests;\n> -\tfor (const std::unique_ptr &buffer : allocator_->buffers(stream)) {\n> +\t/* Create requests and fill it with buffers from the viewfinder. */\n> +\twhile (!freeBuffers_[vfStream_].isEmpty()) {\n> +\t\tFrameBuffer *buffer = freeBuffers_[vfStream_].dequeue();\n> +\n> \t\tRequest *request = camera_->createRequest();\n> \t\tif (!request) {\n> \t\t\tqWarning() << \"Can't create request\";\n> @@ -362,19 +398,13 @@ int MainWindow::startCapture()\n> \t\t\tgoto error;\n> \t\t}\n> \n> -\t\tret = request->addBuffer(stream, buffer.get());\n> +\t\tret = request->addBuffer(vfStream_, buffer);\n> \t\tif (ret < 0) {\n> \t\t\tqWarning() << \"Can't set buffer for request\";\n> \t\t\tgoto error;\n> \t\t}\n> \n> \t\trequests.push_back(request);\n> -\n> -\t\t/* Map memory buffers and cache the mappings. */\n> -\t\tconst FrameBuffer::Plane &plane = buffer->planes().front();\n> -\t\tvoid *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n> -\t\t\t\t plane.fd.fd(), 0);\n> -\t\tmappedBuffers_[buffer.get()] = { memory, plane.length };\n> \t}\n> \n> \t/* Start the title timer and the camera. */\n> @@ -419,6 +449,8 @@ error:\n> \t}\n> \tmappedBuffers_.clear();\n> \n> +\tfreeBuffers_.clear();\n> +\n> \tdelete allocator_;\n> \tallocator_ = nullptr;\n> \n> @@ -461,7 +493,8 @@ void MainWindow::stopCapture()\n> \t * but not processed yet. Clear the queue of done buffers to avoid\n> \t * racing with the event handler.\n> \t */\n> -\tdoneQueue_.clear();\n> +\tfreeBuffers_.clear();\n> +\tdoneBuffers_.clear();\n> \n> \ttitleTimer_.stop();\n> \tsetWindowTitle(title_);\n> @@ -500,12 +533,9 @@ void MainWindow::requestComplete(Request *request)\n> \t * are not allowed. Add the buffer to the done queue and post a\n> \t * CaptureEvent for the application thread to handle.\n> \t */\n> -\tconst std::map &buffers = request->buffers();\n> -\tFrameBuffer *buffer = buffers.begin()->second;\n> -\n> \t{\n> \t\tQMutexLocker locker(&mutex_);\n> -\t\tdoneQueue_.enqueue(buffer);\n> +\t\tdoneBuffers_.push_back(request->buffers());\n> \t}\n> \n> \tQCoreApplication::postEvent(this, new CaptureEvent);\n> @@ -518,16 +548,40 @@ void MainWindow::processCapture()\n> \t * if stopCapture() has been called while a CaptureEvent was posted but\n> \t * not processed yet. Return immediately in that case.\n> \t */\n> -\tFrameBuffer *buffer;\n> -\n> +\tstd::vector> doneBuffers;\n\nEmpty line to show the { } isn't a statement belonging to the previous\nline.\n\n> \t{\n> \t\tQMutexLocker locker(&mutex_);\n> -\t\tif (doneQueue_.isEmpty())\n> +\t\tif (doneBuffers_.empty())\n> \t\t\treturn;\n> \n> -\t\tbuffer = doneQueue_.dequeue();\n> +\t\tdoneBuffers = std::move(doneBuffers_);\n> +\t\tdoneBuffers_.clear();\n\nI don't clear() is needed as you use std::move().\n\n> \t}\n> \n> +\t/* Process buffers. */\n> +\tfor (std::map &buffers : doneBuffers) {\n\nIs there a reason while you batch processing of all done buffers,\ninstead of the queue head as done before ? I can't recall if there was a\nreason to do so in my original code, or if I just considered it could be\ndone later :-S\n\n> +\t\tif (buffers.count(vfStream_))\n> +\t\t\tprocessViewfinder(buffers[vfStream_]);\n\nDo it make sense to push all frames to the viewfinder if the done queue\nhas more than one element, or should we skip all of them but the last ?\nThe ViewFinder class will process every frame it gets, but if the code\ndoesn't go through the Qt event loop in between frames, only the last\none will be pushed to the screen. We would thus convert frames\nunnecessarily.\n\n> +\n> +\t\t/*\n> +\t\t * Return buffers so they can be reused. No processing involving\n> +\t\t * a buffer can happen after they are returned to the free list.\n> +\t\t */\n> +\t\tfor (auto &it : buffers) {\n> +\t\t\tStream *stream = it.first;\n> +\t\t\tFrameBuffer *buffer = it.second;\n> +\n> +\t\t\t/* The ViewFinder manages the viewfinder buffers. */\n> +\t\t\tif (stream == vfStream_)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tfreeBuffers_[stream].enqueue(buffer);\n> +\t\t}\n\nI think this would look nicer if you turned doneBuffers_ from\n\n\tstd::vector> doneBuffers_;\n\nto\n\n\tstd::map> doneBuffers_;\n\nbut now that I've said this, it may make sense to keep it the way it is,\nto retain the relation between elements of doneBuffers_ and requests.\n\n> +\t}\n> +}\n> +\n> +void MainWindow::processViewfinder(FrameBuffer *buffer)\n> +{\n> \tframesCaptured_++;\n> \n> \tconst FrameMetadata &metadata = buffer->metadata();\n> @@ -554,8 +608,7 @@ void MainWindow::queueRequest(FrameBuffer *buffer)\n> \t\treturn;\n> \t}\n> \n> -\tStream *stream = config_->at(0).stream();\n> -\trequest->addBuffer(stream, buffer);\n> +\trequest->addBuffer(vfStream_, buffer);\n> \n> \tcamera_->queueRequest(request);\n> }\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 5e9d9b8d9c6b2d6d..c2040c0ebcd61bfa 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -68,6 +68,7 @@ private:\n> \n> \tvoid requestComplete(Request *request);\n> \tvoid processCapture();\n> +\tvoid processViewfinder(FrameBuffer *buffer);\n> \n> \t/* UI elements */\n> \tQToolBar *toolbar_;\n> @@ -93,8 +94,11 @@ private:\n> \n> \t/* Capture state, buffers queue and statistics */\n> \tbool isCapturing_;\n> -\tQQueue doneQueue_;\n> -\tQMutex mutex_;\t/* Protects doneQueue_ */\n> +\tStream *vfStream_;\n> +\tStream *rawStream_;\n> +\tstd::map> freeBuffers_;\n> +\tstd::vector> doneBuffers_;\n\nAny reason not to keep the QQueue ? If there is, have you considered\nQList (which I assume would suffer from the same issue as QQueue\ninherits from QList), or std::deque ? I would also prefer keeping the\nname doneQueue_ to show it's a queue.\n\n> +\tQMutex mutex_; /* Protects freeBuffers_ and doneBuffers_ */\n> \n> \tuint64_t lastBufferTime_;\n> \tQElapsedTimer frameRateInterval_;","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07A77613A8\n\tfor ;\n\tThu, 30 Apr 2020 21:19:48 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6D2D672C;\n\tThu, 30 Apr 2020 21:19:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tsZvrt6z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588274387;\n\tbh=x79SK/leWsXlubRGJX4f5BMpgttvpFjKTcDadAWFn8A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tsZvrt6zPuwR4s64Lv4ipi7X/QDks0Wv6mTPS+Fr18M5PwjbbxvS1TrHt7DyGK7tx\n\te8ep2ye8EvQOdB247A4WWK/ZI9b9o5/3l6zSl/fQxfTC9jWup3ZkKX23BnBL0otuYP\n\tSFtRGHjmwqA5YzIJR9aSf6yTyVYz7BT7EQRlWbw4=","Date":"Thu, 30 Apr 2020 22:19:45 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200430191945.GT5856@pendragon.ideasonboard.com>","References":"<20200430003604.2423018-1-niklas.soderlund@ragnatech.se>\n\t<20200430003604.2423018-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200430003604.2423018-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [RFC/PATCH 2/5] qcam: Allow for a second raw\n\tstream to be configured","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 30 Apr 2020 19:19:48 -0000"}}]