[{"id":19010,"web_url":"https://patchwork.libcamera.org/comment/19010/","msgid":"","date":"2021-08-23T21:50:15","subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Aug 23, 2021 at 09:43:20PM +0900, Hirokazu Honda wrote:\n> CameraBuffer implementation maps a given buffer_handle_t in\n> constructor. Mapping is redundant to only know the plane info like\n> stride and offset. Mapping should be executed rater in the first\n> plane() call.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/mm/cros_camera_buffer.cpp | 71 ++++++++++--------\n> src/android/mm/generic_camera_buffer.cpp | 93 +++++++++++++++---------\n> 2 files changed, 100 insertions(+), 64 deletions(-)\n> \n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index 50732637..85ef6480 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -25,7 +25,7 @@ public:\n> \t\tint flags);\n> \t~Private();\n> \n> -\tbool isValid() const { return valid_; }\n> +\tbool isValid() const { return registered_; }\n> \n> \tunsigned int numPlanes() const;\n> \n> @@ -34,10 +34,12 @@ public:\n> \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> \n> private:\n> +\tbool Map();\n\ns/Map/map/\n\n> +\n> \tcros::CameraBufferManager *bufferManager_;\n> \tbuffer_handle_t handle_;\n> \tunsigned int numPlanes_;\n> -\tbool valid_;\n> +\tbool mapped_;\n> \tbool registered_;\n> \tunion {\n> \t\tvoid *addr;\n> @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\t\t [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> \t\t\t [[maybe_unused]] const libcamera::Size &size,\n> \t\t\t [[maybe_unused]] int flags)\n> -\t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> +\t: handle_(camera3Buffer), numPlanes_(0), mapped_(false),\n> \t registered_(false)\n> {\n> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \n> \tregistered_ = true;\n> \tnumPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> -\tswitch (numPlanes_) {\n> -\tcase 1: {\n> -\t\tret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> -\t\tif (ret) {\n> -\t\t\tLOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> -\t\t\treturn;\n> -\t\t}\n> -\t\tbreak;\n> -\t}\n> -\tcase 2:\n> -\tcase 3: {\n> -\t\tret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> -\t\t\t\t\t\t&mem.ycbcr);\n> -\t\tif (ret) {\n> -\t\t\tLOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> -\t\t\treturn;\n> -\t\t}\n> -\t\tbreak;\n> -\t}\n> -\tdefault:\n> -\t\tLOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> -\t\treturn;\n> -\t}\n> -\n> -\tvalid_ = true;\n> }\n> \n> CameraBuffer::Private::~Private()\n> {\n> -\tif (valid_)\n> +\tif (mapped_)\n> \t\tbufferManager_->Unlock(handle_);\n> \tif (registered_)\n> \t\tbufferManager_->Deregister(handle_);\n> @@ -105,6 +82,12 @@ unsigned int CameraBuffer::Private::numPlanes() const\n> \n> Span CameraBuffer::Private::plane(unsigned int plane)\n> {\n> +\tif (!mapped_) {\n> +\t\tmapped_ = Map();\n> +\t\tif (!mapped_)\n> +\t\t\treturn {};\n> +\t}\n> +\n> \tvoid *addr;\n> \n> \tswitch (numPlanes()) {\n> @@ -134,4 +117,34 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> \treturn bufferManager_->GetPlaneSize(handle_, 0);\n> }\n> \n> +bool CameraBuffer::Private::Map()\n> +{\n> +\tint ret;\n> +\tswitch (numPlanes_) {\n> +\tcase 1: {\n> +\t\tret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> +\t\t\treturn false;\n> +\t\t}\n> +\t\tbreak;\n> +\t}\n> +\tcase 2:\n> +\tcase 3: {\n> +\t\tret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> +\t\t\t\t\t\t&mem.ycbcr);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> +\t\t\treturn false;\n> +\t\t}\n> +\t\tbreak;\n> +\t}\n> +\tdefault:\n> +\t\tLOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index 3127069f..6c1e4611 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -37,6 +37,12 @@ public:\n> \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> \n> private:\n> +\tbool Map();\n\ns/Map/map/\n\n> +\n> +\tint fd_;\n> +\tint flags_;\n> +\tlibcamera::Size size_;\n> +\tlibcamera::PixelFormatInfo info_;\n\nMaybe a const pointer, to avoid a copy ? Or a const reference,\ninitialized in the constructor's initializer list.\n\n> \t/* \\todo remove planes_ is added to MappedBuffer. */\n> \tstd::vector> planes_;\n> };\n> @@ -45,83 +51,100 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\t\t buffer_handle_t camera3Buffer,\n> \t\t\t libcamera::PixelFormat pixelFormat,\n> \t\t\t const libcamera::Size &size, int flags)\n> +\t: fd_(-1), flags_(flags), size_(size)\n> {\n> \terror_ = 0;\n> \n> -\tint fd = -1;\n> \tfor (int i = 0; i < camera3Buffer->numFds; i++) {\n> \t\tif (camera3Buffer->data[i] != -1) {\n> -\t\t\tfd = camera3Buffer->data[i];\n> +\t\t\tfd_ = camera3Buffer->data[i];\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> \n> -\tif (fd != -1) {\n> +\tif (fd_ != -1) {\n> \t\terror_ = EINVAL;\n> \t\tLOG(HAL, Error) << \"No valid file descriptor\";\n> \t\treturn;\n> \t}\n> \n> -\tconst auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> -\tif (!info.isValid()) {\n> +\tinfo_ = libcamera::PixelFormatInfo::info(pixelFormat);\n> +\tif (!info_.isValid()) {\n> \t\terror_ = EINVAL;\n> \t\tLOG(HAL, Error) << \"Invalid pixel format: \"\n> \t\t\t\t<< pixelFormat.toString();\n> \t\treturn;\n> \t}\n> +}\n> +\n> +CameraBuffer::Private::~Private()\n> +{\n> +}\n> +\n> +unsigned int CameraBuffer::Private::numPlanes() const\n> +{\n> +\treturn info_.numPlanes();\n> +}\n> +\n> +Span CameraBuffer::Private::plane(unsigned int plane)\n> +{\n> +\tif (planes_.empty() && !Map())\n> +\t\treturn {};\n> \n> -\tsize_t bufferLength = lseek(fd, 0, SEEK_END);\n> +\tif (plane >= planes_.size())\n> +\t\treturn {};\n> +\n> +\treturn planes_[plane];\n> +}\n> +\n> +size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> +{\n> +\tif (maps_.empty()) {\n> +\t\tsize_t bufferLength = lseek(fd_, 0, SEEK_END);\n> +\t\tif (bufferLength < 0)\n> +\t\t\tLOG(HAL, Error) << \"Failed to get buffer length\";\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn std::min(maps_[0].size(),\n> +\t\t\t\t maxJpegBufferSize);\n> +}\n> +\n> +bool CameraBuffer::Private::Map()\n> +{\n> +\tASSERT(fd_ != -1);\n> +\n> +\tsize_t bufferLength = lseek(fd_, 0, SEEK_END);\n> \tif (bufferLength < 0) {\n> \t\terror_ = -errno;\n> \t\tLOG(HAL, Error) << \"Failed to get buffer length\";\n> -\t\treturn;\n> +\t\treturn false;\n> \t}\n\nI would keep this in the constructor, to avoid the lseek() call in\nCameraBuffer::Private::jpegBufferSize(), as lseek() isn't a very\nexpensive operation. It would also be useful to check, in the\nconstructor, that the buffer size as reported by lseek() is equal to or\nlarger than the total size computed from the format, as a sanity check.\nI think that should go in patch 1/3.\n\n> \n> -\tvoid *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> +\tvoid *address = mmap(nullptr, bufferLength, flags_, MAP_SHARED, fd_, 0);\n> \tif (address == MAP_FAILED) {\n> \t\terror_ = -errno;\n> \t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> -\t\treturn;\n> +\t\treturn false;\n> \t}\n> \tmaps_.emplace_back(static_cast(address), bufferLength);\n> \n> -\tconst unsigned int numPlanes = info.numPlanes();\n> +\tconst unsigned int numPlanes = info_.numPlanes();\n> \tplanes_.resize(numPlanes);\n> \tunsigned int offset = 0;\n> \tfor (unsigned int i = 0; i < numPlanes; ++i) {\n> -\t\tconst unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> -\t\tconst unsigned int stride = info.stride(size.width, i, 1u);\n> +\t\tconst unsigned int vertSubSample = info_.planes[i].verticalSubSampling;\n> +\t\tconst unsigned int stride = info_.stride(size_.width, i, 1u);\n> \t\tconst unsigned int planeSize =\n> -\t\t\tstride * ((size.height + vertSubSample - 1) / vertSubSample);\n> +\t\t\tstride * ((size_.height + vertSubSample - 1) / vertSubSample);\n> \n> \t\tplanes_[i] = libcamera::Span(\n> \t\t\tstatic_cast(address) + offset, planeSize);\n> \n> \t\toffset += planeSize;\n> \t}\n\nThis should also be kept in the constructor, as otherwise calling\nCameraBuffer::size() will be valid on an unmapped before on CrOS, but\nnot on Android.\n\n> -}\n> -\n> -CameraBuffer::Private::~Private()\n> -{\n> -}\n> -\n> -unsigned int CameraBuffer::Private::numPlanes() const\n> -{\n> -\treturn planes_.size();\n> -}\n> \n> -Span CameraBuffer::Private::plane(unsigned int plane)\n> -{\n> -\tif (plane >= planes_.size())\n> -\t\treturn {};\n> -\n> -\treturn planes_[plane];\n> -}\n> -\n> -size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> -{\n> -\treturn std::min(maps_[0].size(),\n> -\t\t\t\t maxJpegBufferSize);\n> +\treturn true;\n> }\n> \n> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION","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 0B071BD87D\n\tfor ;\n\tMon, 23 Aug 2021 21:50:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7150B688A3;\n\tMon, 23 Aug 2021 23:50:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D24206025B\n\tfor ;\n\tMon, 23 Aug 2021 23:50:25 +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 46A0D2A5;\n\tMon, 23 Aug 2021 23:50:25 +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=\"fKCVUl9H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629755425;\n\tbh=/RteK2D3QTWZHvMzdI8bgmai23MIuBXqrAR7np+Z4xU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fKCVUl9H+6rWrwWpVwToKEmaJEI/n0va6gVNTrGZV6e4EPJ8avfogndnK4AIw3e4C\n\tv0wYPEMYJyZzCViDKPdyDpnRb1Egw6zZKSgHkhCRiPCSCxwJWAN/NRJsQb4yWYOe2K\n\tos0oC4i+srgqcrbZwuN//R48A5iv+c9TM7RXUuqg=","Date":"Tue, 24 Aug 2021 00:50:15 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210823124321.980847-1-hiroh@chromium.org>\n\t<20210823124321.980847-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210823124321.980847-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","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\" "}}]