[{"id":4707,"web_url":"https://patchwork.libcamera.org/comment/4707/","msgid":"<20200501165031.GL5951@pendragon.ideasonboard.com>","date":"2020-05-01T16:50:31","subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 Fri, May 01, 2020 at 05:27:43PM +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 wishes to capture a raw frame.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n> * Changes since v1\n> - Keep doneQueue_ as a QQueue and keep the original processing\n>   granularity.\n> ---\n>  src/qcam/main_window.cpp | 125 +++++++++++++++++++++++++++------------\n>  src/qcam/main_window.h   |   8 ++-\n>  2 files changed, 94 insertions(+), 39 deletions(-)\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index b9348111dfa28914..dc8824dae4669a7e 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -281,17 +281,30 @@ void MainWindow::toggleCapture(bool start)\n>  int MainWindow::startCapture()\n>  {\n>  \tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> +\tstd::vector<Request *> requests;\n>  \tint ret;\n>  \n>  \t/* Verify roles are supported. */\n> -\tif (roles.size() != 1) {\n> -\t\tqCritical() << \"Only one stream supported\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\tif (roles[0] != StreamRole::Viewfinder) {\n> -\t\tqCritical() << \"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> @@ -301,17 +314,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<PixelFormat> formats = cfg.formats().pixelformats();\n> +\tstd::vector<PixelFormat> 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> @@ -331,7 +344,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> @@ -339,10 +352,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> @@ -350,16 +369,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<FrameBuffer> &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<Request *> requests;\n> -\tfor (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n> +\t/* Create requests and fill it with buffers from the viewfinder. */\n\ns/it/them/\n\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> @@ -367,19 +403,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> @@ -424,6 +454,8 @@ error:\n>  \t}\n>  \tmappedBuffers_.clear();\n>  \n> +\tfreeBuffers_.clear();\n> +\n>  \tdelete allocator_;\n>  \tallocator_ = nullptr;\n>  \n> @@ -466,6 +498,7 @@ 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> +\tfreeBuffers_.clear();\n>  \tdoneQueue_.clear();\n>  \n>  \ttitleTimer_.stop();\n> @@ -505,12 +538,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<Stream *, FrameBuffer *> &buffers = request->buffers();\n> -\tFrameBuffer *buffer = buffers.begin()->second;\n> -\n>  \t{\n>  \t\tQMutexLocker locker(&mutex_);\n> -\t\tdoneQueue_.enqueue(buffer);\n> +\t\tdoneQueue_.enqueue(request->buffers());\n>  \t}\n>  \n>  \tQCoreApplication::postEvent(this, new CaptureEvent);\n> @@ -523,16 +553,38 @@ 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> +\tstd::map<Stream *, FrameBuffer *> buffers;\n>  \n>  \t{\n>  \t\tQMutexLocker locker(&mutex_);\n>  \t\tif (doneQueue_.isEmpty())\n>  \t\t\treturn;\n>  \n> -\t\tbuffer = doneQueue_.dequeue();\n> +\t\tbuffers = doneQueue_.dequeue();\n>  \t}\n>  \n> +\t/* Process buffers. */\n> +\tif (buffers.count(vfStream_))\n> +\t\tprocessViewfinder(buffers[vfStream_]);\n> +\n> +\t/*\n> +\t * Return buffers so they can be reused. No processing involving\n> +\t * a buffer can happen after they are returned to the free list.\n> +\t */\n> +\tfor (auto &it : buffers) {\n> +\t\tStream *stream = it.first;\n> +\t\tFrameBuffer *buffer = it.second;\n> +\n> +\t\t/* The ViewFinder manages the viewfinder buffers. */\n> +\t\tif (stream == vfStream_)\n> +\t\t\tcontinue;\n> +\n> +\t\tfreeBuffers_[stream].enqueue(buffer);\n> +\t}\n\nThis looks a bit dodgy as the free buffers list is only used when\nstarting the stream, but it will get used for raw capture in the next\npatches, so it's fine.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +}\n> +\n> +void MainWindow::processViewfinder(FrameBuffer *buffer)\n> +{\n>  \tframesCaptured_++;\n>  \n>  \tconst FrameMetadata &metadata = buffer->metadata();\n> @@ -559,8 +611,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 aea1f1dee20fcbb6..4856ecc10729159c 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -69,6 +69,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> @@ -95,8 +96,11 @@ private:\n>  \n>  \t/* Capture state, buffers queue and statistics */\n>  \tbool isCapturing_;\n> -\tQQueue<FrameBuffer *> doneQueue_;\n> -\tQMutex mutex_;\t/* Protects doneQueue_ */\n> +\tStream *vfStream_;\n> +\tStream *rawStream_;\n> +\tstd::map<Stream *, QQueue<FrameBuffer *>> freeBuffers_;\n> +\tQQueue<std::map<Stream *, FrameBuffer *>> doneQueue_;\n> +\tQMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */\n>  \n>  \tuint64_t lastBufferTime_;\n>  \tQElapsedTimer frameRateInterval_;","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98427603F3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  1 May 2020 18:50:35 +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 0EC3E72C;\n\tFri,  1 May 2020 18:50:34 +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=\"ff79Jsy1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588351835;\n\tbh=2HDzmD8Dq5HuIJGZ7r7OaBo3OkXmcq6fkNK2VTZYobA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ff79Jsy1TNdLm3paNn2XagGgH7b6/tMtQbsblMrcRMS7MnJ+C5WIggFl0Ps5tgTNE\n\t9povfPlfvR+Hrlyvj0DJjBxcSbRhSH5AhUa+Y3Y77jYd0mbwedUW1lzFRy7+k2upCe\n\tcbbDOg4ZcwUnI1+k0rWYbr8/9qoaNBxMs6pQ7o5I=","Date":"Fri, 1 May 2020 19:50:31 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200501165031.GL5951@pendragon.ideasonboard.com>","References":"<20200501152745.437777-1-niklas.soderlund@ragnatech.se>\n\t<20200501152745.437777-2-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":"<20200501152745.437777-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 01 May 2020 16:50:35 -0000"}}]