[{"id":4304,"web_url":"https://patchwork.libcamera.org/comment/4304/","msgid":"<20200326140932.GO20581@pendragon.ideasonboard.com>","date":"2020-03-26T14:09:32","subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: FrameBuffer: Add a\n\tmethod to copy buffer content","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 Tue, Mar 24, 2020 at 04:51:42PM +0100, Niklas Söderlund wrote:\n> This method may be used to memory copy a whole FrameBuffer content to\n> from another buffer. The operation is not fast and should not be used\n\nto or from ? :-)\n\n> without great care by pipelines.\n> \n> The intended use-case is to have an option to copy out RAW buffers from\n> the middle of a pipeline.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since RFC\n> - Make method member of FrameBuffer\n> - Add documentation\n> - s/out/dstmem/ and s/in/src/mem/\n> ---\n> include/libcamera/buffer.h | 1 +\n> src/libcamera/buffer.cpp | 61 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 62 insertions(+)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 8e5ec699e3925eee..ef3a3b36cd4e4e17 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -57,6 +57,7 @@ public:\n> \tunsigned int cookie() const { return cookie_; }\n> \tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n> \n> +\tint copyFrom(const FrameBuffer *src);\n> private:\n> \tfriend class Request; /* Needed to update request_. */\n> \tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 673a63d3d1658190..0352917e9f2a3202 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -211,4 +211,65 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> * core never modifies the buffer cookie.\n> */\n> \n> +/**\n> + * \\brief Copy the content from another buffer\n\nI'm not entirely sure, but I think s/content/contents/ (same below).\nMaybe Kieran can shed some native speaker light here.\n\n> + * \\param[in] src Buffer to copy\n> + *\n> + * Copy the buffer content and metadata form \\a src. The receiving FrameBuffer\n\ns/form/from/\ns/src/src to this buffer/\ns/receiving/destination/\n\n> + * needs to have at least as many planes of equal or grater size then the source.\n\nThis doesn't match the implementation, should it be \"shall have the same\nnumber of planes as the source buffer, and each destination plane shall\nbe larger than or equal to the corresponding source plane.\" ?\n\nShould we add a sentence to explain how metadata is copied ? This could\ndo, but feel free to rephrase it:\n\n\"The complete metadata of the source buffer is copied to the destination\nbuffer. If an error occurs during the copy, the destination buffer's\nmetadata status is set to FrameMetadata::FrameError, and other metadata\nfields are not modified.\"\n\n> + *\n> + * The operation is performed using memcpy() so is very slow, users needs to\n> + * consider this before copying buffers.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int FrameBuffer::copyFrom(const FrameBuffer *src)\n> +{\n> +\tif (planes_.size() != src->planes_.size()) {\n> +\t\tLOG(Buffer, Error) << \"Different number of planes\";\n> +\t\tmetadata_.status = FrameMetadata::FrameError;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < planes_.size(); i++) {\n> +\t\tif (planes_[i].length < src->planes_[i].length) {\n> +\t\t\tLOG(Buffer, Error) << \"Plane \" << i << \" is too small\";\n> +\t\t\tmetadata_.status = FrameMetadata::FrameError;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < planes_.size(); i++) {\n> +\t\tvoid *dstmem = mmap(NULL, planes_[i].length, PROT_WRITE,\n\ns/NULL/nullptr/\n\n> +\t\t\t\t MAP_SHARED, planes_[i].fd.fd(), 0);\n> +\n> +\t\tif (dstmem == MAP_FAILED) {\n> +\t\t\tLOG(Buffer, Error)\n> +\t\t\t\t<< \"Failed to map destination plane \" << i;\n> +\t\t\tmetadata_.status = FrameMetadata::FrameError;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tvoid *srcmem = mmap(NULL, src->planes_[i].length, PROT_READ,\n\ns/NULL/nullptr/\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t\t\t MAP_SHARED, src->planes_[i].fd.fd(), 0);\n> +\n> +\t\tif (srcmem == MAP_FAILED) {\n> +\t\t\tmunmap(dstmem, planes_[i].length);\n> +\t\t\tLOG(Buffer, Error)\n> +\t\t\t\t<< \"Failed to map source plane \" << i;\n> +\t\t\tmetadata_.status = FrameMetadata::FrameError;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tmemcpy(dstmem, srcmem, src->planes_[i].length);\n> +\n> +\t\tmunmap(srcmem, src->planes_[i].length);\n> +\t\tmunmap(dstmem, planes_[i].length);\n> +\t}\n> +\n> +\tmetadata_ = src->metadata_;\n> +\n> +\treturn 0;\n> +}\n> +\n> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["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 EE96960410\n\tfor ;\n\tThu, 26 Mar 2020 15:09:36 +0100 (CET)","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 6AE352DC;\n\tThu, 26 Mar 2020 15:09:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dOxVj/sJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585231776;\n\tbh=3ADxyHjlHCuX4C7dhIFQ26JTla9QPFhujTywQKyqAw0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dOxVj/sJ4Igi1bhsedxlQavlgvRz2CEoXnLg+Ai1eOYUyG5j0zZWaGB8N/WoL6/cb\n\ty1Xz1TCu7nLuRGHVzVNhnNxBNqUEU8Q8o3lrV2e9giaJ7d7Qigj4UhCOVR9jwDhJDS\n\tWM94If5lZlx3HbrSvJNV6GJmANCoRf9AcU90a2O4=","Date":"Thu, 26 Mar 2020 16:09:32 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200326140932.GO20581@pendragon.ideasonboard.com>","References":"<20200324155145.3896183-1-niklas.soderlund@ragnatech.se>\n\t<20200324155145.3896183-5-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":"<20200324155145.3896183-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/7] libcamera: FrameBuffer: Add a\n\tmethod to copy buffer content","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, 26 Mar 2020 14:09:37 -0000"}}]