[{"id":19509,"web_url":"https://patchwork.libcamera.org/comment/19509/","msgid":"","date":"2021-09-07T11:57:42","subject":"Re: [libcamera-devel] [PATCH v3 27/30] qcam: Use Image class to\n\taccess pixel data","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 06/09/2021 23:56, Laurent Pinchart wrote:\n> Replace the manual implementation of frame buffer mapping with the Image\n> class to improve code sharing. The ViewFinder API is updated to take an\n> Image pointer in the render() function to prepare for multi-planar\n> buffer support.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Jean-Michel Hautbois \n\nMuch better IMO.\n\nReviewed-by: Kieran Bingham \n\n\n> ---\n> src/qcam/main_window.cpp | 31 ++++++++-----------------------\n> src/qcam/main_window.h | 4 ++--\n> src/qcam/meson.build | 1 +\n> src/qcam/viewfinder.h | 6 +++---\n> src/qcam/viewfinder_gl.cpp | 7 ++++---\n> src/qcam/viewfinder_gl.h | 2 +-\n> src/qcam/viewfinder_qt.cpp | 6 +++---\n> src/qcam/viewfinder_qt.h | 2 +-\n> 8 files changed, 23 insertions(+), 36 deletions(-)\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 0a00b1001570..168dd5ce30e3 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -7,10 +7,9 @@\n> \n> #include \"main_window.h\"\n> \n> +#include \n> #include \n> #include \n> -#include \n> -#include \n> \n> #include \n> #include \n> @@ -29,6 +28,7 @@\n> #include \n> #include \n> \n> +#include \"../cam/image.h\"\n> #include \"dng_writer.h\"\n> #ifndef QT_NO_OPENGL\n> #include \"viewfinder_gl.h\"\n> @@ -473,15 +473,10 @@ int MainWindow::startCapture()\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\tsize_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> -\t\t\tvoid *memory = mmap(NULL, length, PROT_READ, MAP_SHARED,\n> -\t\t\t\t\t plane.fd.fd(), 0);\n> -\n> -\t\t\tmappedBuffers_[buffer.get()] = { static_cast(memory),\n> -\t\t\t\t\t\t\t plane.length };\n> -\t\t\tplaneData_[buffer.get()] = { static_cast(memory) + plane.offset,\n> -\t\t\t\t\t\t plane.length };\n> +\t\t\tstd::unique_ptr image =\n> +\t\t\t\tImage::fromFrameBuffer(buffer.get(), Image::MapMode::ReadOnly);\n> +\t\t\tassert(image != nullptr);\n> +\t\t\tmappedBuffers_[buffer.get()] = std::move(image);\n> \n> \t\t\t/* Store buffers on the free list. */\n> \t\t\tfreeBuffers_[stream].enqueue(buffer.get());\n> @@ -543,12 +538,7 @@ error_disconnect:\n> error:\n> \trequests_.clear();\n> \n> -\tfor (auto &iter : mappedBuffers_) {\n> -\t\tconst Span &buffer = iter.second;\n> -\t\tmunmap(buffer.data(), buffer.size());\n> -\t}\n> \tmappedBuffers_.clear();\n> -\tplaneData_.clear();\n> \n> \tfreeBuffers_.clear();\n> \n> @@ -580,12 +570,7 @@ void MainWindow::stopCapture()\n> \n> \tcamera_->requestCompleted.disconnect(this);\n> \n> -\tfor (auto &iter : mappedBuffers_) {\n> -\t\tconst Span &buffer = iter.second;\n> -\t\tmunmap(buffer.data(), buffer.size());\n> -\t}\n> \tmappedBuffers_.clear();\n> -\tplaneData_.clear();\n> \n> \trequests_.clear();\n> \tfreeQueue_.clear();\n> @@ -682,7 +667,7 @@ void MainWindow::processRaw(FrameBuffer *buffer,\n> \t\t\t\t\t\t\t\"DNG Files (*.dng)\");\n> \n> \tif (!filename.isEmpty()) {\n> -\t\tuint8_t *memory = planeData_[buffer].data();\n> +\t\tuint8_t *memory = mappedBuffers_[buffer]->data(0).data();\n> \t\tDNGWriter::write(filename.toStdString().c_str(), camera_.get(),\n> \t\t\t\t rawStream_->configuration(), metadata, buffer,\n> \t\t\t\t memory);\n> @@ -766,7 +751,7 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> \t\t<< \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> \n> \t/* Render the frame on the viewfinder. */\n> -\tviewfinder_->render(buffer, planeData_[buffer]);\n> +\tviewfinder_->render(buffer, mappedBuffers_[buffer].get());\n> }\n> \n> void MainWindow::queueRequest(FrameBuffer *buffer)\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 28244bca58b2..a16bea09eadc 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -34,6 +34,7 @@ using namespace libcamera;\n> class QAction;\n> class QComboBox;\n> \n> +class Image;\n> class HotplugEvent;\n> \n> enum {\n> @@ -106,8 +107,7 @@ private:\n> \tFrameBufferAllocator *allocator_;\n> \n> \tstd::unique_ptr config_;\n> -\tstd::map> mappedBuffers_;\n> -\tstd::map> planeData_;\n> +\tstd::map> mappedBuffers_;\n> \n> \t/* Capture state, buffers queue and statistics */\n> \tbool isCapturing_;\n> diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> index 7d3621c93d41..c46f463130cd 100644\n> --- a/src/qcam/meson.build\n> +++ b/src/qcam/meson.build\n> @@ -15,6 +15,7 @@ endif\n> qcam_enabled = true\n> \n> qcam_sources = files([\n> + '../cam/image.cpp',\n> '../cam/options.cpp',\n> '../cam/stream_options.cpp',\n> 'format_converter.cpp',\n> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> index 42d40f1f33f0..fb462835fb5f 100644\n> --- a/src/qcam/viewfinder.h\n> +++ b/src/qcam/viewfinder.h\n> @@ -11,11 +11,11 @@\n> #include \n> #include \n> \n> -#include \n> -\n> #include \n> #include \n> \n> +class Image;\n> +\n> class ViewFinder\n> {\n> public:\n> @@ -24,7 +24,7 @@ public:\n> \tvirtual const QList &nativeFormats() const = 0;\n> \n> \tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> -\tvirtual void render(libcamera::FrameBuffer *buffer, libcamera::Span mem) = 0;\n> +\tvirtual void render(libcamera::FrameBuffer *buffer, Image *image) = 0;\n> \tvirtual void stop() = 0;\n> \n> \tvirtual QImage getCurrentImage() = 0;\n> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> index d2ef036974f4..87e4fe03cb8d 100644\n> --- a/src/qcam/viewfinder_gl.cpp\n> +++ b/src/qcam/viewfinder_gl.cpp\n> @@ -13,6 +13,8 @@\n> \n> #include \n> \n> +#include \"../cam/image.h\"\n> +\n> static const QList supportedFormats{\n> \t/* YUV - packed (single plane) */\n> \tlibcamera::formats::UYVY,\n> @@ -110,8 +112,7 @@ QImage ViewFinderGL::getCurrentImage()\n> \treturn grabFramebuffer();\n> }\n> \n> -void ViewFinderGL::render(libcamera::FrameBuffer *buffer,\n> -\t\t\t libcamera::Span mem)\n> +void ViewFinderGL::render(libcamera::FrameBuffer *buffer, Image *image)\n> {\n> \tif (buffer->planes().size() != 1) {\n> \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> @@ -121,7 +122,7 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer,\n> \tif (buffer_)\n> \t\trenderComplete(buffer_);\n> \n> -\tdata_ = mem.data();\n> +\tdata_ = image->data(0).data();\n> \t/*\n> \t * \\todo Get the stride from the buffer instead of computing it naively\n> \t */\n> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> index 3334549e0be4..7cd8ef3316b9 100644\n> --- a/src/qcam/viewfinder_gl.h\n> +++ b/src/qcam/viewfinder_gl.h\n> @@ -39,7 +39,7 @@ public:\n> \tconst QList &nativeFormats() const override;\n> \n> \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> -\tvoid render(libcamera::FrameBuffer *buffer, libcamera::Span mem) override;\n> +\tvoid render(libcamera::FrameBuffer *buffer, Image *image) override;\n> \tvoid stop() override;\n> \n> \tQImage getCurrentImage() override;\n> diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> index a0bf99b0b522..fef6d53eef5e 100644\n> --- a/src/qcam/viewfinder_qt.cpp\n> +++ b/src/qcam/viewfinder_qt.cpp\n> @@ -19,6 +19,7 @@\n> \n> #include \n> \n> +#include \"../cam/image.h\"\n> #include \"format_converter.h\"\n> \n> static const QMap nativeFormats\n> @@ -78,15 +79,14 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> \treturn 0;\n> }\n> \n> -void ViewFinderQt::render(libcamera::FrameBuffer *buffer,\n> -\t\t\t libcamera::Span mem)\n> +void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n> {\n> \tif (buffer->planes().size() != 1) {\n> \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> \t\treturn;\n> \t}\n> \n> -\tunsigned char *memory = mem.data();\n> +\tunsigned char *memory = image->data(0).data();\n> \tsize_t size = buffer->metadata().planes()[0].bytesused;\n> \n> \t{\n> diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> index 1a569b9cee6e..6b48ef48a7d1 100644\n> --- a/src/qcam/viewfinder_qt.h\n> +++ b/src/qcam/viewfinder_qt.h\n> @@ -32,7 +32,7 @@ public:\n> \tconst QList &nativeFormats() const override;\n> \n> \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> -\tvoid render(libcamera::FrameBuffer *buffer, libcamera::Span mem) override;\n> +\tvoid render(libcamera::FrameBuffer *buffer, Image *image) override;\n> \tvoid stop() override;\n> \n> \tQImage getCurrentImage() override;\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 E3A7BBE175\n\tfor ;\n\tTue, 7 Sep 2021 11:57:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62EED60252;\n\tTue, 7 Sep 2021 13:57:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1D4460251\n\tfor ;\n\tTue, 7 Sep 2021 13:57:45 +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 67093499;\n\tTue, 7 Sep 2021 13:57:45 +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=\"f92kAvGY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631015865;\n\tbh=MBHPqstvE4Qys7O9jOcsqAE/KqPHF/i7CM4CDCkAqg4=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=f92kAvGYslJ2u/AF1JmjwqcPV+zvCeFXPYPBvFRmTOJHKNXLEkNaVWkNGvv829je1\n\thZj1sdlLWJgZJqj67Xj7FqVm3E53xNkZuIihnfivU6iFx0A7PoeFPQe4kJCw3jA6aB\n\tANKK4MeE8apbFWAq9gTOUXY1ZMMahX4jHuuZcq6A=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210906225420.13275-1-laurent.pinchart@ideasonboard.com>\n\t<20210906225636.14683-27-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Tue, 7 Sep 2021 12:57:42 +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":"<20210906225636.14683-27-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 27/30] qcam: Use Image class to\n\taccess pixel data","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\" "}}]