[{"id":3065,"web_url":"https://patchwork.libcamera.org/comment/3065/","msgid":"<20191118181332.GI8072@bigcity.dyn.berto.se>","date":"2019-11-18T18:13:32","subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-11-08 22:53:59 +0200, Laurent Pinchart wrote:\n> From: Jacopo Mondi \n> \n> The ByteStreamBuffer class wraps a memory area, expected to be allocated\n> by the user of the class and provides operations to perform sequential\n> access in read and write modes.\n> \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/byte_stream_buffer.cpp | 286 +++++++++++++++++++++\n> src/libcamera/include/byte_stream_buffer.h | 63 +++++\n> src/libcamera/include/meson.build | 1 +\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 351 insertions(+)\n> create mode 100644 src/libcamera/byte_stream_buffer.cpp\n> create mode 100644 src/libcamera/include/byte_stream_buffer.h\n> \n> diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp\n> new file mode 100644\n> index 000000000000..23d624dd0a06\n> --- /dev/null\n> +++ b/src/libcamera/byte_stream_buffer.cpp\n> @@ -0,0 +1,286 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * byte_stream_buffer.cpp - Byte stream buffer\n> + */\n> +\n> +#include \"byte_stream_buffer.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Serialization);\n> +\n> +/**\n> + * \\file byte_stream_buffer.h\n> + * \\brief Managed memory container for serialized data\n> + */\n> +\n> +/**\n> + * \\class ByteStreamBuffer\n> + * \\brief Wrap a memory buffer and provide sequential data read and write\n> + *\n> + * The ByteStreamBuffer class wraps a memory buffer and exposes sequential read\n> + * and write operation with integrated boundary checks. Access beyond the end\n> + * of the buffer are blocked and logged, allowing error checks to take place at\n> + * the of of access operations instead of at each access. This simplifies\n> + * serialization and deserialization of data.\n> + *\n> + * A byte stream buffer is created with a base memory pointer and a size. If the\n> + * memory pointer is const, the buffer operates in read-only mode, and write\n> + * operations are denied. Otherwise the buffer operates in write-only mode, and\n> + * read operations are denied.\n> + *\n> + * Once a buffer is created, data is read or written with read() and write()\n> + * respectively. Access is strictly sequential, the buffer keeps track of the\n> + * current access location and advances it automatically. Reading or writing\n> + * the same location multiple times is thus not possible. Bytes may also be\n> + * skipped with the skip() method.\n> + *\n> + * The ByteStreamBuffer also supports carving out pieces of memory into other\n> + * ByteStreamBuffer instances. Like a read or write operation, a carveOut()\n> + * advances the internal access location, but allows the carved out memory to\n> + * be accessed at a later time.\n> + *\n> + * All accesses beyond the end of the buffer (read, write, skip or carve out)\n> + * are blocked. The first of such accesses causes a message to be logged, and\n> + * the buffer being marked as having overflown. If the buffer has been carved\n> + * out from a parent buffer, the parent buffer is also marked as having\n> + * overflown. Any later access on an overflown buffer is blocked. The buffer\n> + * overflow status can be checked with the overflow() method.\n> + */\n> +\n> +/**\n> + * \\brief Construct a read ByteStreamBuffer from the memory area \\a base\n> + * of \\a size\n> + * \\param[in] base The address of the memory area to wrap\n> + * \\param[in] size The size of the memory area to wrap\n> + */\n> +ByteStreamBuffer::ByteStreamBuffer(const uint8_t *base, size_t size)\n> +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> +\t read_(base), write_(nullptr)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a write ByteStreamBuffer from the memory area \\a base\n> + * of \\a size\n> + * \\param[in] base The address of the memory area to wrap\n> + * \\param[in] size The size of the memory area to wrap\n> + */\n> +ByteStreamBuffer::ByteStreamBuffer(uint8_t *base, size_t size)\n> +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> +\t read_(nullptr), write_(base)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a ByteStreamBuffer from the contents of \\a other using move\n> + * semantics\n> + * \\param[in] other The other buffer\n> + *\n> + * After the move construction the \\a other buffer is invalidated. Any attempt\n> + * to access its contents will be considered as an overflow.\n> + */\n> +ByteStreamBuffer::ByteStreamBuffer(ByteStreamBuffer &&other)\n> +{\n> +\t*this = std::move(other);\n> +}\n> +\n> +/**\n> + * \\brief Replace the contents of the buffer with those of \\a other using move\n> + * semantics\n> + * \\param[in] other The other buffer\n> + *\n> + * After the assignment the \\a other buffer is invalidated. Any attempt to\n> + * access its contents will be considered as an overflow.\n> + */\n> +ByteStreamBuffer &ByteStreamBuffer::operator=(ByteStreamBuffer &&other)\n> +{\n> +\tparent_ = other.parent_;\n> +\tbase_ = other.base_;\n> +\tsize_ = other.size_;\n> +\toverflow_ = other.overflow_;\n> +\tread_ = other.read_;\n> +\twrite_ = other.write_;\n> +\n> +\tother.parent_ = nullptr;\n> +\tother.base_ = nullptr;\n> +\tother.size_ = 0;\n> +\tother.overflow_ = false;\n> +\tother.read_ = nullptr;\n> +\tother.write_ = nullptr;\n> +\n> +\treturn *this;\n> +}\n> +\n> +/**\n> + * \\fn ByteStreamBuffer::base()\n> + * \\brief Retrieve a pointer to the start location of the managed memory buffer\n> + * \\return A pointer to the managed memory buffer\n> + */\n> +\n> +/**\n> + * \\fn ByteStreamBuffer::offset()\n> + * \\brief Retrieve the offset of the current access location from the base\n> + * \\return The offset in bytes\n> + */\n> +\n> +/**\n> + * \\fn ByteStreamBuffer::size()\n> + * \\brief Retrieve the size of the managed memory buffer\n> + * \\return The size of managed memory buffer\n> + */\n> +\n> +/**\n> + * \\fn ByteStreamBuffer::overflow()\n> + * \\brief Check if the buffer has overflown\n> + * \\return True if the buffer has overflow, false otherwise\n> + */\n> +\n> +void ByteStreamBuffer::setOverflow()\n> +{\n> +\tif (parent_)\n> +\t\tparent_->setOverflow();\n\nThis seems a bit more complex then it needs to be. Is it really needed \nto track a parent from a carve out and set overflow if the child tries \nto interact with too much data?\n\nIt is not possible to create a carv out which would create a overflow \nand the overflow will be logged from the child right?\n\n> +\n> +\toverflow_ = true;\n> +}\n> +\n> +/**\n> + * \\brief Carve out an area of \\a size bytes into a new ByteStreamBuffer\n> + * \\param[in] size The size of the newly created memory buffer\n> + *\n> + * This method carves out an area of \\a size bytes from the buffer into a new\n> + * ByteStreamBuffer, and returns the new buffer. It operates identically to a\n> + * read or write access from the point of view of the current buffer, but allows\n> + * the new buffer to be read or written at a later time after other read or\n> + * write accesses on the current buffer.\n> + *\n> + * \\return A newly created ByteStreamBuffer of \\a size\n> + */\n> +ByteStreamBuffer ByteStreamBuffer::carveOut(size_t size)\n> +{\n> +\tif (!size_ || overflow_)\n> +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> +\n> +\tconst uint8_t *curr = read_ ? read_ : write_;\n> +\tif (curr + size > base_ + size_) {\n> +\t\tLOG(Serialization, Error)\n> +\t\t\t<< \"Unable to reserve \" << size << \" bytes\";\n> +\t\tsetOverflow();\n> +\n> +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> +\t}\n> +\n> +\tif (read_) {\n> +\t\tByteStreamBuffer b(read_, size);\n> +\t\tb.parent_ = this;\n> +\t\tread_ += size;\n> +\t\treturn b;\n> +\t} else {\n> +\t\tByteStreamBuffer b(write_, size);\n> +\t\tb.parent_ = this;\n> +\t\twrite_ += size;\n> +\t\treturn b;\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Skip \\a size bytes from the buffer\n> + * \\param[in] size The number of bytes to skip\n> + *\n> + * This method skips the next \\a size bytes from the buffer.\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> + */\n> +int ByteStreamBuffer::skip(size_t size)\n> +{\n> +\tif (overflow_)\n> +\t\treturn -ENOSPC;\n\nI think you should return early on !size.\n\n> +\n> +\tconst uint8_t *curr = read_ ? read_ : write_;\n> +\tif (curr + size > base_ + size_) {\n> +\t\tLOG(Serialization, Error)\n> +\t\t\t<< \"Unable to skip \" << size << \" bytes\";\n> +\t\tsetOverflow();\n> +\n> +\t\treturn -ENOSPC;\n> +\t}\n> +\n> +\tif (read_) {\n> +\t\tread_ += size;\n> +\t} else {\n> +\t\tmemset(write_, 0, size);\n\nDo we want to memset her? I think we do but just checking.\n\n> +\t\twrite_ += size;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nskip() and carveOut() share much code, could they be merged?\n\n> +\n> +/**\n> + * \\fn template int ByteStreamBuffer::read(T *t)\n> + * \\brief Read \\a size \\a data from the managed memory buffer\n> + * \\param[out] t Pointer to the memory containing the read data\n> + * \\return 0 on success, a negative error code otherwise\n> + * \\retval -EACCES attempting to read from a write buffer\n> + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> + */\n> +\n> +/**\n> + * \\fn template int ByteStreamBuffer::write(const T *t)\n> + * \\brief Write \\a data of \\a size to the managed memory buffer\n> + * \\param[in] t The data to write to memory\n> + * \\return 0 on success, a negative error code otherwise\n> + * \\retval -EACCES attempting to write to a read buffer\n> + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> + */\n> +\n> +int ByteStreamBuffer::read(uint8_t *data, size_t size)\n> +{\n> +\tif (!read_)\n> +\t\treturn -EACCES;\n> +\n> +\tif (overflow_)\n> +\t\treturn -ENOSPC;\n> +\n> +\tif (read_ + size > base_ + size_) {\n> +\t\tLOG(Serialization, Error)\n> +\t\t\t<< \"Unable to read \" << size << \" bytes: out of bounds\";\n> +\t\tsetOverflow();\n> +\t\treturn -ENOSPC;\n> +\t}\n> +\n> +\tmemcpy(data, read_, size);\n> +\tread_ += size;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int ByteStreamBuffer::write(const uint8_t *data, size_t size)\n> +{\n> +\tif (!write_)\n> +\t\treturn -EACCES;\n> +\n> +\tif (overflow_)\n> +\t\treturn -ENOSPC;\n> +\n> +\tif (write_ + size > base_ + size_) {\n> +\t\tLOG(Serialization, Error)\n> +\t\t\t<< \"Unable to write \" << size << \" bytes: no space left\";\n> +\t\tsetOverflow();\n> +\t\treturn -ENOSPC;\n> +\t}\n> +\n> +\tmemcpy(write_, data, size);\n> +\twrite_ += size;\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h\n> new file mode 100644\n> index 000000000000..b5274c62b85e\n> --- /dev/null\n> +++ b/src/libcamera/include/byte_stream_buffer.h\n> @@ -0,0 +1,63 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * byte_stream_buffer.h - Byte stream buffer\n> + */\n> +#ifndef __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> +#define __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class ByteStreamBuffer\n> +{\n> +public:\n> +\tByteStreamBuffer(const uint8_t *base, size_t size);\n> +\tByteStreamBuffer(uint8_t *base, size_t size);\n> +\tByteStreamBuffer(ByteStreamBuffer &&other);\n> +\tByteStreamBuffer &operator=(ByteStreamBuffer &&other);\n> +\n> +\tconst uint8_t *base() const { return base_; }\n> +\tuint32_t offset() const { return (write_ ? write_ : read_) - base_; }\n> +\tsize_t size() const { return size_; }\n> +\tbool overflow() const { return overflow_; }\n> +\n> +\tByteStreamBuffer carveOut(size_t size);\n> +\tint skip(size_t size);\n> +\n> +\ttemplate\n> +\tint read(T *t)\n> +\t{\n> +\t\treturn read(reinterpret_cast(t), sizeof(*t));\n> +\t}\n> +\ttemplate\n> +\tint write(const T *t)\n> +\t{\n> +\t\treturn write(reinterpret_cast(t), sizeof(*t));\n> +\t}\n> +\n> +private:\n> +\tByteStreamBuffer(const ByteStreamBuffer &other) = delete;\n> +\tByteStreamBuffer &operator=(const ByteStreamBuffer &other) = delete;\n> +\n> +\tvoid setOverflow();\n> +\n> +\tint read(uint8_t *data, size_t size);\n> +\tint write(const uint8_t *data, size_t size);\n> +\n> +\tByteStreamBuffer *parent_;\n> +\n> +\tconst uint8_t *base_;\n> +\tsize_t size_;\n> +\tbool overflow_;\n> +\n> +\tconst uint8_t *read_;\n> +\tuint8_t *write_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_BYTE_STREAM_BUFFER_H__ */\n> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> index 64c2155f90cf..1ff0198662cc 100644\n> --- a/src/libcamera/include/meson.build\n> +++ b/src/libcamera/include/meson.build\n> @@ -1,4 +1,5 @@\n> libcamera_headers = files([\n> + 'byte_stream_buffer.h',\n> 'camera_controls.h',\n> 'camera_sensor.h',\n> 'control_validator.h',\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index be0cd53f6f10..dab2d8ad2649 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,6 +1,7 @@\n> libcamera_sources = files([\n> 'bound_method.cpp',\n> 'buffer.cpp',\n> + 'byte_stream_buffer.cpp',\n> 'camera.cpp',\n> 'camera_controls.cpp',\n> 'camera_manager.cpp',\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 12AAE60F1C\n\tfor ;\n\tMon, 18 Nov 2019 19:13:35 +0100 (CET)","by mail-lj1-x236.google.com with SMTP id v8so20073388ljh.5\n\tfor ;\n\tMon, 18 Nov 2019 10:13:35 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tu12sm8972536lje.1.2019.11.18.10.13.33\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 18 Nov 2019 10:13:33 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=xt9XkJQye81fJRLc94AwH6MJ6lhtbeXxiuBctltTE5w=;\n\tb=S8TMBq6OQCqA4OzGZhQnLDLWZU9ntdBjtgR1NXpYWDC53JacyoZwbtoyYztSPU9clW\n\tTZT67+TRyQyqFoKgDEzEwyy33fQYhzIsJusKJ8FsNM4gBUbmwo0KFLk6LFBMUyvDXUXf\n\tRc0IqXvaF0LGY1j5RADxO+wLvGrg4ImfgNO5tRjrSeXAt7A8D5j/fDvoO5bhUpJY4K9Q\n\tcTgm6M4PELNe8Bn1ZqYnPZNwHtCA404ctXeP7rTiL179IYdC6RwAJ95+4DD2Qq3pbITd\n\tvrQ5jr2AQajLNon4Ioo3vauL3UKsKCt9kX+v9lBxLUEuaLxEkHA8OxaB+IBhGv38+wbC\n\tUQTg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=xt9XkJQye81fJRLc94AwH6MJ6lhtbeXxiuBctltTE5w=;\n\tb=EQfBPkfOA8heE6BatPiRWn+ftH5Sa7yb2U4xdQNTqDvfvBmyd6vzRPjUeSIVYDh6ZD\n\tFed5pDvMG6jgLKkLmL5Qf2MFZFvbHEAHAqQQv4q8w9DR+JWFD4CLPGV3G5h8YY26v3S4\n\tUP+LKFCqbBjB7MTcMW21M2LzPaDAa0SmZKIzoJhn0NhQxqNhQnHnRt3MxOimQrimxdWj\n\txGUN0ob24ihFk73IkD/szMsV4HUh6Ss6mf1RHI7vDNanpaPVrgcplYgIv1LLZ77EAxrs\n\tR1utY+MN2VZE3SyDgNt5VGH/hTtOjSv4b/TA8I5kADijQWMrdsWDw1XSSWXNr0AMlPNu\n\t+7eQ==","X-Gm-Message-State":"APjAAAWhZShJY8JkFIFdBBTnhF0zc3751p5X3c0ZrQzKJcSEgoEnYvBD\n\twbuVt15OiIdYtd5hNIt10cX1JQ==","X-Google-Smtp-Source":"APXvYqzDTur72PTzidPoca2sxu4UmbScKk/t3AvOZAHImfjIM2NuqVfZxulKqnBU/9im91en98exgw==","X-Received":"by 2002:a2e:6c0c:: with SMTP id h12mr620479ljc.24.1574100814297; \n\tMon, 18 Nov 2019 10:13:34 -0800 (PST)","Date":"Mon, 18 Nov 2019 19:13:32 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191118181332.GI8072@bigcity.dyn.berto.se>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-15-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191108205409.18845-15-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","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":"Mon, 18 Nov 2019 18:13:35 -0000"}},{"id":3067,"web_url":"https://patchwork.libcamera.org/comment/3067/","msgid":"<20191118183923.GA4888@pendragon.ideasonboard.com>","date":"2019-11-18T18:39:23","subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Mon, Nov 18, 2019 at 07:13:32PM +0100, Niklas Söderlund wrote:\n> On 2019-11-08 22:53:59 +0200, Laurent Pinchart wrote:\n> > From: Jacopo Mondi \n> > \n> > The ByteStreamBuffer class wraps a memory area, expected to be allocated\n> > by the user of the class and provides operations to perform sequential\n> > access in read and write modes.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/byte_stream_buffer.cpp | 286 +++++++++++++++++++++\n> > src/libcamera/include/byte_stream_buffer.h | 63 +++++\n> > src/libcamera/include/meson.build | 1 +\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 351 insertions(+)\n> > create mode 100644 src/libcamera/byte_stream_buffer.cpp\n> > create mode 100644 src/libcamera/include/byte_stream_buffer.h\n> > \n> > diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp\n> > new file mode 100644\n> > index 000000000000..23d624dd0a06\n> > --- /dev/null\n> > +++ b/src/libcamera/byte_stream_buffer.cpp\n> > @@ -0,0 +1,286 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * byte_stream_buffer.cpp - Byte stream buffer\n> > + */\n> > +\n> > +#include \"byte_stream_buffer.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(Serialization);\n> > +\n> > +/**\n> > + * \\file byte_stream_buffer.h\n> > + * \\brief Managed memory container for serialized data\n> > + */\n> > +\n> > +/**\n> > + * \\class ByteStreamBuffer\n> > + * \\brief Wrap a memory buffer and provide sequential data read and write\n> > + *\n> > + * The ByteStreamBuffer class wraps a memory buffer and exposes sequential read\n> > + * and write operation with integrated boundary checks. Access beyond the end\n> > + * of the buffer are blocked and logged, allowing error checks to take place at\n> > + * the of of access operations instead of at each access. This simplifies\n> > + * serialization and deserialization of data.\n> > + *\n> > + * A byte stream buffer is created with a base memory pointer and a size. If the\n> > + * memory pointer is const, the buffer operates in read-only mode, and write\n> > + * operations are denied. Otherwise the buffer operates in write-only mode, and\n> > + * read operations are denied.\n> > + *\n> > + * Once a buffer is created, data is read or written with read() and write()\n> > + * respectively. Access is strictly sequential, the buffer keeps track of the\n> > + * current access location and advances it automatically. Reading or writing\n> > + * the same location multiple times is thus not possible. Bytes may also be\n> > + * skipped with the skip() method.\n> > + *\n> > + * The ByteStreamBuffer also supports carving out pieces of memory into other\n> > + * ByteStreamBuffer instances. Like a read or write operation, a carveOut()\n> > + * advances the internal access location, but allows the carved out memory to\n> > + * be accessed at a later time.\n> > + *\n> > + * All accesses beyond the end of the buffer (read, write, skip or carve out)\n> > + * are blocked. The first of such accesses causes a message to be logged, and\n> > + * the buffer being marked as having overflown. If the buffer has been carved\n> > + * out from a parent buffer, the parent buffer is also marked as having\n> > + * overflown. Any later access on an overflown buffer is blocked. The buffer\n> > + * overflow status can be checked with the overflow() method.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a read ByteStreamBuffer from the memory area \\a base\n> > + * of \\a size\n> > + * \\param[in] base The address of the memory area to wrap\n> > + * \\param[in] size The size of the memory area to wrap\n> > + */\n> > +ByteStreamBuffer::ByteStreamBuffer(const uint8_t *base, size_t size)\n> > +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> > +\t read_(base), write_(nullptr)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Construct a write ByteStreamBuffer from the memory area \\a base\n> > + * of \\a size\n> > + * \\param[in] base The address of the memory area to wrap\n> > + * \\param[in] size The size of the memory area to wrap\n> > + */\n> > +ByteStreamBuffer::ByteStreamBuffer(uint8_t *base, size_t size)\n> > +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> > +\t read_(nullptr), write_(base)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Construct a ByteStreamBuffer from the contents of \\a other using move\n> > + * semantics\n> > + * \\param[in] other The other buffer\n> > + *\n> > + * After the move construction the \\a other buffer is invalidated. Any attempt\n> > + * to access its contents will be considered as an overflow.\n> > + */\n> > +ByteStreamBuffer::ByteStreamBuffer(ByteStreamBuffer &&other)\n> > +{\n> > +\t*this = std::move(other);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Replace the contents of the buffer with those of \\a other using move\n> > + * semantics\n> > + * \\param[in] other The other buffer\n> > + *\n> > + * After the assignment the \\a other buffer is invalidated. Any attempt to\n> > + * access its contents will be considered as an overflow.\n> > + */\n> > +ByteStreamBuffer &ByteStreamBuffer::operator=(ByteStreamBuffer &&other)\n> > +{\n> > +\tparent_ = other.parent_;\n> > +\tbase_ = other.base_;\n> > +\tsize_ = other.size_;\n> > +\toverflow_ = other.overflow_;\n> > +\tread_ = other.read_;\n> > +\twrite_ = other.write_;\n> > +\n> > +\tother.parent_ = nullptr;\n> > +\tother.base_ = nullptr;\n> > +\tother.size_ = 0;\n> > +\tother.overflow_ = false;\n> > +\tother.read_ = nullptr;\n> > +\tother.write_ = nullptr;\n> > +\n> > +\treturn *this;\n> > +}\n> > +\n> > +/**\n> > + * \\fn ByteStreamBuffer::base()\n> > + * \\brief Retrieve a pointer to the start location of the managed memory buffer\n> > + * \\return A pointer to the managed memory buffer\n> > + */\n> > +\n> > +/**\n> > + * \\fn ByteStreamBuffer::offset()\n> > + * \\brief Retrieve the offset of the current access location from the base\n> > + * \\return The offset in bytes\n> > + */\n> > +\n> > +/**\n> > + * \\fn ByteStreamBuffer::size()\n> > + * \\brief Retrieve the size of the managed memory buffer\n> > + * \\return The size of managed memory buffer\n> > + */\n> > +\n> > +/**\n> > + * \\fn ByteStreamBuffer::overflow()\n> > + * \\brief Check if the buffer has overflown\n> > + * \\return True if the buffer has overflow, false otherwise\n> > + */\n> > +\n> > +void ByteStreamBuffer::setOverflow()\n> > +{\n> > +\tif (parent_)\n> > +\t\tparent_->setOverflow();\n> \n> This seems a bit more complex then it needs to be. Is it really needed \n> to track a parent from a carve out and set overflow if the child tries \n> to interact with too much data?\n> \n> It is not possible to create a carv out which would create a overflow \n> and the overflow will be logged from the child right?\n\nThe idea here is that all ByteStreamBuffer operations (read, write and\ncarve out) log overflow in the top-level object, allowing users of this\nclass to not perform any error checking until the very end. Without\npropagating overflow to the parent, we would need to check each carved\nout buffer independently.\n\nThis being said, I think the ByteStreamBuffer class, while nice as a\nconcept, may be overkill. It has a limited set of users, and gets a bit\nin the way by requiring copies of all data. I think we'll revisit it\nwhen moving value for simple controls inline (creating a union to store\neither the value or the offset in the packet entry), and relax the error\nchecks then.\n\n> > +\n> > +\toverflow_ = true;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Carve out an area of \\a size bytes into a new ByteStreamBuffer\n> > + * \\param[in] size The size of the newly created memory buffer\n> > + *\n> > + * This method carves out an area of \\a size bytes from the buffer into a new\n> > + * ByteStreamBuffer, and returns the new buffer. It operates identically to a\n> > + * read or write access from the point of view of the current buffer, but allows\n> > + * the new buffer to be read or written at a later time after other read or\n> > + * write accesses on the current buffer.\n> > + *\n> > + * \\return A newly created ByteStreamBuffer of \\a size\n> > + */\n> > +ByteStreamBuffer ByteStreamBuffer::carveOut(size_t size)\n> > +{\n> > +\tif (!size_ || overflow_)\n> > +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> > +\n> > +\tconst uint8_t *curr = read_ ? read_ : write_;\n> > +\tif (curr + size > base_ + size_) {\n> > +\t\tLOG(Serialization, Error)\n> > +\t\t\t<< \"Unable to reserve \" << size << \" bytes\";\n> > +\t\tsetOverflow();\n> > +\n> > +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> > +\t}\n> > +\n> > +\tif (read_) {\n> > +\t\tByteStreamBuffer b(read_, size);\n> > +\t\tb.parent_ = this;\n> > +\t\tread_ += size;\n> > +\t\treturn b;\n> > +\t} else {\n> > +\t\tByteStreamBuffer b(write_, size);\n> > +\t\tb.parent_ = this;\n> > +\t\twrite_ += size;\n> > +\t\treturn b;\n> > +\t}\n> > +}\n> > +\n> > +/**\n> > + * \\brief Skip \\a size bytes from the buffer\n> > + * \\param[in] size The number of bytes to skip\n> > + *\n> > + * This method skips the next \\a size bytes from the buffer.\n> > + *\n> > + * \\return 0 on success, a negative error code otherwise\n> > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > + */\n> > +int ByteStreamBuffer::skip(size_t size)\n> > +{\n> > +\tif (overflow_)\n> > +\t\treturn -ENOSPC;\n> \n> I think you should return early on !size.\n\nThis should never happen in practice, right ? Would you return before\nthis check, or right after it ? If we return right after it will just be\nan optimization for a case that should never happen.\n\n> > +\n> > +\tconst uint8_t *curr = read_ ? read_ : write_;\n> > +\tif (curr + size > base_ + size_) {\n> > +\t\tLOG(Serialization, Error)\n> > +\t\t\t<< \"Unable to skip \" << size << \" bytes\";\n> > +\t\tsetOverflow();\n> > +\n> > +\t\treturn -ENOSPC;\n> > +\t}\n> > +\n> > +\tif (read_) {\n> > +\t\tread_ += size;\n> > +\t} else {\n> > +\t\tmemset(write_, 0, size);\n> \n> Do we want to memset her? I think we do but just checking.\n\nI think we do, otherwise we could leak data over IPC.\n\n> > +\t\twrite_ += size;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> \n> skip() and carveOut() share much code, could they be merged?\n\nI've read both methods, and I think they don't share that much code.\nThere's only the size check, and even that has a different error\nmessage, and a different return value. I don't think it's worth it, but\nmaybe I'm missing something, so please feel free to propose a patch.\n\n> > +\n> > +/**\n> > + * \\fn template int ByteStreamBuffer::read(T *t)\n> > + * \\brief Read \\a size \\a data from the managed memory buffer\n> > + * \\param[out] t Pointer to the memory containing the read data\n> > + * \\return 0 on success, a negative error code otherwise\n> > + * \\retval -EACCES attempting to read from a write buffer\n> > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > + */\n> > +\n> > +/**\n> > + * \\fn template int ByteStreamBuffer::write(const T *t)\n> > + * \\brief Write \\a data of \\a size to the managed memory buffer\n> > + * \\param[in] t The data to write to memory\n> > + * \\return 0 on success, a negative error code otherwise\n> > + * \\retval -EACCES attempting to write to a read buffer\n> > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > + */\n> > +\n> > +int ByteStreamBuffer::read(uint8_t *data, size_t size)\n> > +{\n> > +\tif (!read_)\n> > +\t\treturn -EACCES;\n> > +\n> > +\tif (overflow_)\n> > +\t\treturn -ENOSPC;\n> > +\n> > +\tif (read_ + size > base_ + size_) {\n> > +\t\tLOG(Serialization, Error)\n> > +\t\t\t<< \"Unable to read \" << size << \" bytes: out of bounds\";\n> > +\t\tsetOverflow();\n> > +\t\treturn -ENOSPC;\n> > +\t}\n> > +\n> > +\tmemcpy(data, read_, size);\n> > +\tread_ += size;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int ByteStreamBuffer::write(const uint8_t *data, size_t size)\n> > +{\n> > +\tif (!write_)\n> > +\t\treturn -EACCES;\n> > +\n> > +\tif (overflow_)\n> > +\t\treturn -ENOSPC;\n> > +\n> > +\tif (write_ + size > base_ + size_) {\n> > +\t\tLOG(Serialization, Error)\n> > +\t\t\t<< \"Unable to write \" << size << \" bytes: no space left\";\n> > +\t\tsetOverflow();\n> > +\t\treturn -ENOSPC;\n> > +\t}\n> > +\n> > +\tmemcpy(write_, data, size);\n> > +\twrite_ += size;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h\n> > new file mode 100644\n> > index 000000000000..b5274c62b85e\n> > --- /dev/null\n> > +++ b/src/libcamera/include/byte_stream_buffer.h\n> > @@ -0,0 +1,63 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * byte_stream_buffer.h - Byte stream buffer\n> > + */\n> > +#ifndef __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> > +#define __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class ByteStreamBuffer\n> > +{\n> > +public:\n> > +\tByteStreamBuffer(const uint8_t *base, size_t size);\n> > +\tByteStreamBuffer(uint8_t *base, size_t size);\n> > +\tByteStreamBuffer(ByteStreamBuffer &&other);\n> > +\tByteStreamBuffer &operator=(ByteStreamBuffer &&other);\n> > +\n> > +\tconst uint8_t *base() const { return base_; }\n> > +\tuint32_t offset() const { return (write_ ? write_ : read_) - base_; }\n> > +\tsize_t size() const { return size_; }\n> > +\tbool overflow() const { return overflow_; }\n> > +\n> > +\tByteStreamBuffer carveOut(size_t size);\n> > +\tint skip(size_t size);\n> > +\n> > +\ttemplate\n> > +\tint read(T *t)\n> > +\t{\n> > +\t\treturn read(reinterpret_cast(t), sizeof(*t));\n> > +\t}\n> > +\ttemplate\n> > +\tint write(const T *t)\n> > +\t{\n> > +\t\treturn write(reinterpret_cast(t), sizeof(*t));\n> > +\t}\n> > +\n> > +private:\n> > +\tByteStreamBuffer(const ByteStreamBuffer &other) = delete;\n> > +\tByteStreamBuffer &operator=(const ByteStreamBuffer &other) = delete;\n> > +\n> > +\tvoid setOverflow();\n> > +\n> > +\tint read(uint8_t *data, size_t size);\n> > +\tint write(const uint8_t *data, size_t size);\n> > +\n> > +\tByteStreamBuffer *parent_;\n> > +\n> > +\tconst uint8_t *base_;\n> > +\tsize_t size_;\n> > +\tbool overflow_;\n> > +\n> > +\tconst uint8_t *read_;\n> > +\tuint8_t *write_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_BYTE_STREAM_BUFFER_H__ */\n> > diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> > index 64c2155f90cf..1ff0198662cc 100644\n> > --- a/src/libcamera/include/meson.build\n> > +++ b/src/libcamera/include/meson.build\n> > @@ -1,4 +1,5 @@\n> > libcamera_headers = files([\n> > + 'byte_stream_buffer.h',\n> > 'camera_controls.h',\n> > 'camera_sensor.h',\n> > 'control_validator.h',\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index be0cd53f6f10..dab2d8ad2649 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -1,6 +1,7 @@\n> > libcamera_sources = files([\n> > 'bound_method.cpp',\n> > 'buffer.cpp',\n> > + 'byte_stream_buffer.cpp',\n> > 'camera.cpp',\n> > 'camera_controls.cpp',\n> > 'camera_manager.cpp',","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 1360760F1C\n\tfor ;\n\tMon, 18 Nov 2019 19:39:47 +0100 (CET)","from pendragon.ideasonboard.com (unknown [38.98.37.142])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2A3F2563;\n\tMon, 18 Nov 2019 19:39:39 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1574102386;\n\tbh=SAuEOm+cD1XJTJ7y/IUSiG5/2H9VPsHniQQHIofpPjE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V1aRgaJLQOCqoRzMgsH07aMyww2CMqJZn6I1FXRO3e4K3z90T5NX4jVsGTRiB1z5r\n\txWSxMU1uI0wO3f2AxF40PLh4gBKI2ak2iKgOpUTAVp1/iG5XSlvaw8WNrKoYniX3fZ\n\tGhv67VJTbsuRt/DcdNdlJ9u63TyXgc7ijtmqhEeE=","Date":"Mon, 18 Nov 2019 20:39:23 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191118183923.GA4888@pendragon.ideasonboard.com>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-15-laurent.pinchart@ideasonboard.com>\n\t<20191118181332.GI8072@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191118181332.GI8072@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","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":"Mon, 18 Nov 2019 18:39:47 -0000"}},{"id":3089,"web_url":"https://patchwork.libcamera.org/comment/3089/","msgid":"<20191118225919.y6paptuhsfpfmpre@uno.localdomain>","date":"2019-11-18T22:59:19","subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Niklas,\n\nOn Mon, Nov 18, 2019 at 08:39:23PM +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n>\n> On Mon, Nov 18, 2019 at 07:13:32PM +0100, Niklas Söderlund wrote:\n> > On 2019-11-08 22:53:59 +0200, Laurent Pinchart wrote:\n> > > From: Jacopo Mondi \n> > >\n> > > The ByteStreamBuffer class wraps a memory area, expected to be allocated\n> > > by the user of the class and provides operations to perform sequential\n> > > access in read and write modes.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/byte_stream_buffer.cpp | 286 +++++++++++++++++++++\n> > > src/libcamera/include/byte_stream_buffer.h | 63 +++++\n> > > src/libcamera/include/meson.build | 1 +\n> > > src/libcamera/meson.build | 1 +\n> > > 4 files changed, 351 insertions(+)\n> > > create mode 100644 src/libcamera/byte_stream_buffer.cpp\n> > > create mode 100644 src/libcamera/include/byte_stream_buffer.h\n> > >\n> > > diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp\n> > > new file mode 100644\n> > > index 000000000000..23d624dd0a06\n> > > --- /dev/null\n> > > +++ b/src/libcamera/byte_stream_buffer.cpp\n> > > @@ -0,0 +1,286 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * byte_stream_buffer.cpp - Byte stream buffer\n> > > + */\n> > > +\n> > > +#include \"byte_stream_buffer.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"log.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(Serialization);\n> > > +\n> > > +/**\n> > > + * \\file byte_stream_buffer.h\n> > > + * \\brief Managed memory container for serialized data\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\class ByteStreamBuffer\n> > > + * \\brief Wrap a memory buffer and provide sequential data read and write\n> > > + *\n> > > + * The ByteStreamBuffer class wraps a memory buffer and exposes sequential read\n> > > + * and write operation with integrated boundary checks. Access beyond the end\n> > > + * of the buffer are blocked and logged, allowing error checks to take place at\n> > > + * the of of access operations instead of at each access. This simplifies\n> > > + * serialization and deserialization of data.\n> > > + *\n> > > + * A byte stream buffer is created with a base memory pointer and a size. If the\n> > > + * memory pointer is const, the buffer operates in read-only mode, and write\n> > > + * operations are denied. Otherwise the buffer operates in write-only mode, and\n> > > + * read operations are denied.\n> > > + *\n> > > + * Once a buffer is created, data is read or written with read() and write()\n> > > + * respectively. Access is strictly sequential, the buffer keeps track of the\n> > > + * current access location and advances it automatically. Reading or writing\n> > > + * the same location multiple times is thus not possible. Bytes may also be\n> > > + * skipped with the skip() method.\n> > > + *\n> > > + * The ByteStreamBuffer also supports carving out pieces of memory into other\n> > > + * ByteStreamBuffer instances. Like a read or write operation, a carveOut()\n> > > + * advances the internal access location, but allows the carved out memory to\n> > > + * be accessed at a later time.\n> > > + *\n> > > + * All accesses beyond the end of the buffer (read, write, skip or carve out)\n> > > + * are blocked. The first of such accesses causes a message to be logged, and\n> > > + * the buffer being marked as having overflown. If the buffer has been carved\n> > > + * out from a parent buffer, the parent buffer is also marked as having\n> > > + * overflown. Any later access on an overflown buffer is blocked. The buffer\n> > > + * overflow status can be checked with the overflow() method.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct a read ByteStreamBuffer from the memory area \\a base\n> > > + * of \\a size\n> > > + * \\param[in] base The address of the memory area to wrap\n> > > + * \\param[in] size The size of the memory area to wrap\n> > > + */\n> > > +ByteStreamBuffer::ByteStreamBuffer(const uint8_t *base, size_t size)\n> > > +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> > > +\t read_(base), write_(nullptr)\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Construct a write ByteStreamBuffer from the memory area \\a base\n> > > + * of \\a size\n> > > + * \\param[in] base The address of the memory area to wrap\n> > > + * \\param[in] size The size of the memory area to wrap\n> > > + */\n> > > +ByteStreamBuffer::ByteStreamBuffer(uint8_t *base, size_t size)\n> > > +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> > > +\t read_(nullptr), write_(base)\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Construct a ByteStreamBuffer from the contents of \\a other using move\n> > > + * semantics\n> > > + * \\param[in] other The other buffer\n> > > + *\n> > > + * After the move construction the \\a other buffer is invalidated. Any attempt\n> > > + * to access its contents will be considered as an overflow.\n> > > + */\n> > > +ByteStreamBuffer::ByteStreamBuffer(ByteStreamBuffer &&other)\n> > > +{\n> > > +\t*this = std::move(other);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Replace the contents of the buffer with those of \\a other using move\n> > > + * semantics\n> > > + * \\param[in] other The other buffer\n> > > + *\n> > > + * After the assignment the \\a other buffer is invalidated. Any attempt to\n> > > + * access its contents will be considered as an overflow.\n> > > + */\n> > > +ByteStreamBuffer &ByteStreamBuffer::operator=(ByteStreamBuffer &&other)\n> > > +{\n> > > +\tparent_ = other.parent_;\n> > > +\tbase_ = other.base_;\n> > > +\tsize_ = other.size_;\n> > > +\toverflow_ = other.overflow_;\n> > > +\tread_ = other.read_;\n> > > +\twrite_ = other.write_;\n> > > +\n> > > +\tother.parent_ = nullptr;\n> > > +\tother.base_ = nullptr;\n> > > +\tother.size_ = 0;\n> > > +\tother.overflow_ = false;\n> > > +\tother.read_ = nullptr;\n> > > +\tother.write_ = nullptr;\n> > > +\n> > > +\treturn *this;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn ByteStreamBuffer::base()\n> > > + * \\brief Retrieve a pointer to the start location of the managed memory buffer\n> > > + * \\return A pointer to the managed memory buffer\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ByteStreamBuffer::offset()\n> > > + * \\brief Retrieve the offset of the current access location from the base\n> > > + * \\return The offset in bytes\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ByteStreamBuffer::size()\n> > > + * \\brief Retrieve the size of the managed memory buffer\n> > > + * \\return The size of managed memory buffer\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ByteStreamBuffer::overflow()\n> > > + * \\brief Check if the buffer has overflown\n> > > + * \\return True if the buffer has overflow, false otherwise\n> > > + */\n> > > +\n> > > +void ByteStreamBuffer::setOverflow()\n> > > +{\n> > > +\tif (parent_)\n> > > +\t\tparent_->setOverflow();\n> >\n> > This seems a bit more complex then it needs to be. Is it really needed\n> > to track a parent from a carve out and set overflow if the child tries\n> > to interact with too much data?\n> >\n> > It is not possible to create a carv out which would create a overflow\n> > and the overflow will be logged from the child right?\n>\n> The idea here is that all ByteStreamBuffer operations (read, write and\n> carve out) log overflow in the top-level object, allowing users of this\n> class to not perform any error checking until the very end. Without\n> propagating overflow to the parent, we would need to check each carved\n> out buffer independently.\n>\n> This being said, I think the ByteStreamBuffer class, while nice as a\n> concept, may be overkill. It has a limited set of users, and gets a bit\n> in the way by requiring copies of all data. I think we'll revisit it\n> when moving value for simple controls inline (creating a union to store\n> either the value or the offset in the packet entry), and relax the error\n> checks then.\n>\n\nI agree and if I recall an attempt to store values with binary size <=\n4 in the offset section of the serialized packet header it felt a bit\nin the way. If it's not a huge work, I would rather drop it. Let me\nknow if I can help there.\n\nThanks\n j\n\n> > > +\n> > > +\toverflow_ = true;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Carve out an area of \\a size bytes into a new ByteStreamBuffer\n> > > + * \\param[in] size The size of the newly created memory buffer\n> > > + *\n> > > + * This method carves out an area of \\a size bytes from the buffer into a new\n> > > + * ByteStreamBuffer, and returns the new buffer. It operates identically to a\n> > > + * read or write access from the point of view of the current buffer, but allows\n> > > + * the new buffer to be read or written at a later time after other read or\n> > > + * write accesses on the current buffer.\n> > > + *\n> > > + * \\return A newly created ByteStreamBuffer of \\a size\n> > > + */\n> > > +ByteStreamBuffer ByteStreamBuffer::carveOut(size_t size)\n> > > +{\n> > > +\tif (!size_ || overflow_)\n> > > +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> > > +\n> > > +\tconst uint8_t *curr = read_ ? read_ : write_;\n> > > +\tif (curr + size > base_ + size_) {\n> > > +\t\tLOG(Serialization, Error)\n> > > +\t\t\t<< \"Unable to reserve \" << size << \" bytes\";\n> > > +\t\tsetOverflow();\n> > > +\n> > > +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> > > +\t}\n> > > +\n> > > +\tif (read_) {\n> > > +\t\tByteStreamBuffer b(read_, size);\n> > > +\t\tb.parent_ = this;\n> > > +\t\tread_ += size;\n> > > +\t\treturn b;\n> > > +\t} else {\n> > > +\t\tByteStreamBuffer b(write_, size);\n> > > +\t\tb.parent_ = this;\n> > > +\t\twrite_ += size;\n> > > +\t\treturn b;\n> > > +\t}\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Skip \\a size bytes from the buffer\n> > > + * \\param[in] size The number of bytes to skip\n> > > + *\n> > > + * This method skips the next \\a size bytes from the buffer.\n> > > + *\n> > > + * \\return 0 on success, a negative error code otherwise\n> > > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > > + */\n> > > +int ByteStreamBuffer::skip(size_t size)\n> > > +{\n> > > +\tif (overflow_)\n> > > +\t\treturn -ENOSPC;\n> >\n> > I think you should return early on !size.\n>\n> This should never happen in practice, right ? Would you return before\n> this check, or right after it ? If we return right after it will just be\n> an optimization for a case that should never happen.\n>\n> > > +\n> > > +\tconst uint8_t *curr = read_ ? read_ : write_;\n> > > +\tif (curr + size > base_ + size_) {\n> > > +\t\tLOG(Serialization, Error)\n> > > +\t\t\t<< \"Unable to skip \" << size << \" bytes\";\n> > > +\t\tsetOverflow();\n> > > +\n> > > +\t\treturn -ENOSPC;\n> > > +\t}\n> > > +\n> > > +\tif (read_) {\n> > > +\t\tread_ += size;\n> > > +\t} else {\n> > > +\t\tmemset(write_, 0, size);\n> >\n> > Do we want to memset her? I think we do but just checking.\n>\n> I think we do, otherwise we could leak data over IPC.\n>\n> > > +\t\twrite_ += size;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> >\n> > skip() and carveOut() share much code, could they be merged?\n>\n> I've read both methods, and I think they don't share that much code.\n> There's only the size check, and even that has a different error\n> message, and a different return value. I don't think it's worth it, but\n> maybe I'm missing something, so please feel free to propose a patch.\n>\n> > > +\n> > > +/**\n> > > + * \\fn template int ByteStreamBuffer::read(T *t)\n> > > + * \\brief Read \\a size \\a data from the managed memory buffer\n> > > + * \\param[out] t Pointer to the memory containing the read data\n> > > + * \\return 0 on success, a negative error code otherwise\n> > > + * \\retval -EACCES attempting to read from a write buffer\n> > > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn template int ByteStreamBuffer::write(const T *t)\n> > > + * \\brief Write \\a data of \\a size to the managed memory buffer\n> > > + * \\param[in] t The data to write to memory\n> > > + * \\return 0 on success, a negative error code otherwise\n> > > + * \\retval -EACCES attempting to write to a read buffer\n> > > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > > + */\n> > > +\n> > > +int ByteStreamBuffer::read(uint8_t *data, size_t size)\n> > > +{\n> > > +\tif (!read_)\n> > > +\t\treturn -EACCES;\n> > > +\n> > > +\tif (overflow_)\n> > > +\t\treturn -ENOSPC;\n> > > +\n> > > +\tif (read_ + size > base_ + size_) {\n> > > +\t\tLOG(Serialization, Error)\n> > > +\t\t\t<< \"Unable to read \" << size << \" bytes: out of bounds\";\n> > > +\t\tsetOverflow();\n> > > +\t\treturn -ENOSPC;\n> > > +\t}\n> > > +\n> > > +\tmemcpy(data, read_, size);\n> > > +\tread_ += size;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int ByteStreamBuffer::write(const uint8_t *data, size_t size)\n> > > +{\n> > > +\tif (!write_)\n> > > +\t\treturn -EACCES;\n> > > +\n> > > +\tif (overflow_)\n> > > +\t\treturn -ENOSPC;\n> > > +\n> > > +\tif (write_ + size > base_ + size_) {\n> > > +\t\tLOG(Serialization, Error)\n> > > +\t\t\t<< \"Unable to write \" << size << \" bytes: no space left\";\n> > > +\t\tsetOverflow();\n> > > +\t\treturn -ENOSPC;\n> > > +\t}\n> > > +\n> > > +\tmemcpy(write_, data, size);\n> > > +\twrite_ += size;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h\n> > > new file mode 100644\n> > > index 000000000000..b5274c62b85e\n> > > --- /dev/null\n> > > +++ b/src/libcamera/include/byte_stream_buffer.h\n> > > @@ -0,0 +1,63 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * byte_stream_buffer.h - Byte stream buffer\n> > > + */\n> > > +#ifndef __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> > > +#define __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class ByteStreamBuffer\n> > > +{\n> > > +public:\n> > > +\tByteStreamBuffer(const uint8_t *base, size_t size);\n> > > +\tByteStreamBuffer(uint8_t *base, size_t size);\n> > > +\tByteStreamBuffer(ByteStreamBuffer &&other);\n> > > +\tByteStreamBuffer &operator=(ByteStreamBuffer &&other);\n> > > +\n> > > +\tconst uint8_t *base() const { return base_; }\n> > > +\tuint32_t offset() const { return (write_ ? write_ : read_) - base_; }\n> > > +\tsize_t size() const { return size_; }\n> > > +\tbool overflow() const { return overflow_; }\n> > > +\n> > > +\tByteStreamBuffer carveOut(size_t size);\n> > > +\tint skip(size_t size);\n> > > +\n> > > +\ttemplate\n> > > +\tint read(T *t)\n> > > +\t{\n> > > +\t\treturn read(reinterpret_cast(t), sizeof(*t));\n> > > +\t}\n> > > +\ttemplate\n> > > +\tint write(const T *t)\n> > > +\t{\n> > > +\t\treturn write(reinterpret_cast(t), sizeof(*t));\n> > > +\t}\n> > > +\n> > > +private:\n> > > +\tByteStreamBuffer(const ByteStreamBuffer &other) = delete;\n> > > +\tByteStreamBuffer &operator=(const ByteStreamBuffer &other) = delete;\n> > > +\n> > > +\tvoid setOverflow();\n> > > +\n> > > +\tint read(uint8_t *data, size_t size);\n> > > +\tint write(const uint8_t *data, size_t size);\n> > > +\n> > > +\tByteStreamBuffer *parent_;\n> > > +\n> > > +\tconst uint8_t *base_;\n> > > +\tsize_t size_;\n> > > +\tbool overflow_;\n> > > +\n> > > +\tconst uint8_t *read_;\n> > > +\tuint8_t *write_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_BYTE_STREAM_BUFFER_H__ */\n> > > diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> > > index 64c2155f90cf..1ff0198662cc 100644\n> > > --- a/src/libcamera/include/meson.build\n> > > +++ b/src/libcamera/include/meson.build\n> > > @@ -1,4 +1,5 @@\n> > > libcamera_headers = files([\n> > > + 'byte_stream_buffer.h',\n> > > 'camera_controls.h',\n> > > 'camera_sensor.h',\n> > > 'control_validator.h',\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index be0cd53f6f10..dab2d8ad2649 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -1,6 +1,7 @@\n> > > libcamera_sources = files([\n> > > 'bound_method.cpp',\n> > > 'buffer.cpp',\n> > > + 'byte_stream_buffer.cpp',\n> > > 'camera.cpp',\n> > > 'camera_controls.cpp',\n> > > 'camera_manager.cpp',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCCDE60F1C\n\tfor ;\n\tMon, 18 Nov 2019 23:57:16 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 0AF57C0009;\n\tMon, 18 Nov 2019 22:57:15 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 18 Nov 2019 23:59:19 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191118225919.y6paptuhsfpfmpre@uno.localdomain>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-15-laurent.pinchart@ideasonboard.com>\n\t<20191118181332.GI8072@bigcity.dyn.berto.se>\n\t<20191118183923.GA4888@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"5pbxihjkb5alfp4f\"","Content-Disposition":"inline","In-Reply-To":"<20191118183923.GA4888@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","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":"Mon, 18 Nov 2019 22:57:17 -0000"}},{"id":3096,"web_url":"https://patchwork.libcamera.org/comment/3096/","msgid":"<20191119002802.GN5171@pendragon.ideasonboard.com>","date":"2019-11-19T00:28:02","subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Nov 18, 2019 at 11:59:19PM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 18, 2019 at 08:39:23PM +0200, Laurent Pinchart wrote:\n> > On Mon, Nov 18, 2019 at 07:13:32PM +0100, Niklas Söderlund wrote:\n> > > On 2019-11-08 22:53:59 +0200, Laurent Pinchart wrote:\n> > > > From: Jacopo Mondi \n> > > >\n> > > > The ByteStreamBuffer class wraps a memory area, expected to be allocated\n> > > > by the user of the class and provides operations to perform sequential\n> > > > access in read and write modes.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/libcamera/byte_stream_buffer.cpp | 286 +++++++++++++++++++++\n> > > > src/libcamera/include/byte_stream_buffer.h | 63 +++++\n> > > > src/libcamera/include/meson.build | 1 +\n> > > > src/libcamera/meson.build | 1 +\n> > > > 4 files changed, 351 insertions(+)\n> > > > create mode 100644 src/libcamera/byte_stream_buffer.cpp\n> > > > create mode 100644 src/libcamera/include/byte_stream_buffer.h\n> > > >\n> > > > diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..23d624dd0a06\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/byte_stream_buffer.cpp\n> > > > @@ -0,0 +1,286 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2019, Google Inc.\n> > > > + *\n> > > > + * byte_stream_buffer.cpp - Byte stream buffer\n> > > > + */\n> > > > +\n> > > > +#include \"byte_stream_buffer.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"log.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(Serialization);\n> > > > +\n> > > > +/**\n> > > > + * \\file byte_stream_buffer.h\n> > > > + * \\brief Managed memory container for serialized data\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\class ByteStreamBuffer\n> > > > + * \\brief Wrap a memory buffer and provide sequential data read and write\n> > > > + *\n> > > > + * The ByteStreamBuffer class wraps a memory buffer and exposes sequential read\n> > > > + * and write operation with integrated boundary checks. Access beyond the end\n> > > > + * of the buffer are blocked and logged, allowing error checks to take place at\n> > > > + * the of of access operations instead of at each access. This simplifies\n> > > > + * serialization and deserialization of data.\n> > > > + *\n> > > > + * A byte stream buffer is created with a base memory pointer and a size. If the\n> > > > + * memory pointer is const, the buffer operates in read-only mode, and write\n> > > > + * operations are denied. Otherwise the buffer operates in write-only mode, and\n> > > > + * read operations are denied.\n> > > > + *\n> > > > + * Once a buffer is created, data is read or written with read() and write()\n> > > > + * respectively. Access is strictly sequential, the buffer keeps track of the\n> > > > + * current access location and advances it automatically. Reading or writing\n> > > > + * the same location multiple times is thus not possible. Bytes may also be\n> > > > + * skipped with the skip() method.\n> > > > + *\n> > > > + * The ByteStreamBuffer also supports carving out pieces of memory into other\n> > > > + * ByteStreamBuffer instances. Like a read or write operation, a carveOut()\n> > > > + * advances the internal access location, but allows the carved out memory to\n> > > > + * be accessed at a later time.\n> > > > + *\n> > > > + * All accesses beyond the end of the buffer (read, write, skip or carve out)\n> > > > + * are blocked. The first of such accesses causes a message to be logged, and\n> > > > + * the buffer being marked as having overflown. If the buffer has been carved\n> > > > + * out from a parent buffer, the parent buffer is also marked as having\n> > > > + * overflown. Any later access on an overflown buffer is blocked. The buffer\n> > > > + * overflow status can be checked with the overflow() method.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Construct a read ByteStreamBuffer from the memory area \\a base\n> > > > + * of \\a size\n> > > > + * \\param[in] base The address of the memory area to wrap\n> > > > + * \\param[in] size The size of the memory area to wrap\n> > > > + */\n> > > > +ByteStreamBuffer::ByteStreamBuffer(const uint8_t *base, size_t size)\n> > > > +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> > > > +\t read_(base), write_(nullptr)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Construct a write ByteStreamBuffer from the memory area \\a base\n> > > > + * of \\a size\n> > > > + * \\param[in] base The address of the memory area to wrap\n> > > > + * \\param[in] size The size of the memory area to wrap\n> > > > + */\n> > > > +ByteStreamBuffer::ByteStreamBuffer(uint8_t *base, size_t size)\n> > > > +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> > > > +\t read_(nullptr), write_(base)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Construct a ByteStreamBuffer from the contents of \\a other using move\n> > > > + * semantics\n> > > > + * \\param[in] other The other buffer\n> > > > + *\n> > > > + * After the move construction the \\a other buffer is invalidated. Any attempt\n> > > > + * to access its contents will be considered as an overflow.\n> > > > + */\n> > > > +ByteStreamBuffer::ByteStreamBuffer(ByteStreamBuffer &&other)\n> > > > +{\n> > > > +\t*this = std::move(other);\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Replace the contents of the buffer with those of \\a other using move\n> > > > + * semantics\n> > > > + * \\param[in] other The other buffer\n> > > > + *\n> > > > + * After the assignment the \\a other buffer is invalidated. Any attempt to\n> > > > + * access its contents will be considered as an overflow.\n> > > > + */\n> > > > +ByteStreamBuffer &ByteStreamBuffer::operator=(ByteStreamBuffer &&other)\n> > > > +{\n> > > > +\tparent_ = other.parent_;\n> > > > +\tbase_ = other.base_;\n> > > > +\tsize_ = other.size_;\n> > > > +\toverflow_ = other.overflow_;\n> > > > +\tread_ = other.read_;\n> > > > +\twrite_ = other.write_;\n> > > > +\n> > > > +\tother.parent_ = nullptr;\n> > > > +\tother.base_ = nullptr;\n> > > > +\tother.size_ = 0;\n> > > > +\tother.overflow_ = false;\n> > > > +\tother.read_ = nullptr;\n> > > > +\tother.write_ = nullptr;\n> > > > +\n> > > > +\treturn *this;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\fn ByteStreamBuffer::base()\n> > > > + * \\brief Retrieve a pointer to the start location of the managed memory buffer\n> > > > + * \\return A pointer to the managed memory buffer\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn ByteStreamBuffer::offset()\n> > > > + * \\brief Retrieve the offset of the current access location from the base\n> > > > + * \\return The offset in bytes\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn ByteStreamBuffer::size()\n> > > > + * \\brief Retrieve the size of the managed memory buffer\n> > > > + * \\return The size of managed memory buffer\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn ByteStreamBuffer::overflow()\n> > > > + * \\brief Check if the buffer has overflown\n> > > > + * \\return True if the buffer has overflow, false otherwise\n> > > > + */\n> > > > +\n> > > > +void ByteStreamBuffer::setOverflow()\n> > > > +{\n> > > > +\tif (parent_)\n> > > > +\t\tparent_->setOverflow();\n> > >\n> > > This seems a bit more complex then it needs to be. Is it really needed\n> > > to track a parent from a carve out and set overflow if the child tries\n> > > to interact with too much data?\n> > >\n> > > It is not possible to create a carv out which would create a overflow\n> > > and the overflow will be logged from the child right?\n> >\n> > The idea here is that all ByteStreamBuffer operations (read, write and\n> > carve out) log overflow in the top-level object, allowing users of this\n> > class to not perform any error checking until the very end. Without\n> > propagating overflow to the parent, we would need to check each carved\n> > out buffer independently.\n> >\n> > This being said, I think the ByteStreamBuffer class, while nice as a\n> > concept, may be overkill. It has a limited set of users, and gets a bit\n> > in the way by requiring copies of all data. I think we'll revisit it\n> > when moving value for simple controls inline (creating a union to store\n> > either the value or the offset in the packet entry), and relax the error\n> > checks then.\n> \n> I agree and if I recall an attempt to store values with binary size <=\n> 4 in the offset section of the serialized packet header it felt a bit\n> in the way. If it's not a huge work, I would rather drop it. Let me\n> know if I can help there.\n\nI'm a bit confused, which part would you drop ? :-) Propagating the\noverflow to the parent ? I'd prefer dropping it on top of this series,\nalong with a few other safety checks, in order to allow access to the\nmemory without any copy. Even better would be a compile-time flag that\nwould enable extra safety checks, but I'm probably just dreaming :-)\n\n> > > > +\n> > > > +\toverflow_ = true;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Carve out an area of \\a size bytes into a new ByteStreamBuffer\n> > > > + * \\param[in] size The size of the newly created memory buffer\n> > > > + *\n> > > > + * This method carves out an area of \\a size bytes from the buffer into a new\n> > > > + * ByteStreamBuffer, and returns the new buffer. It operates identically to a\n> > > > + * read or write access from the point of view of the current buffer, but allows\n> > > > + * the new buffer to be read or written at a later time after other read or\n> > > > + * write accesses on the current buffer.\n> > > > + *\n> > > > + * \\return A newly created ByteStreamBuffer of \\a size\n> > > > + */\n> > > > +ByteStreamBuffer ByteStreamBuffer::carveOut(size_t size)\n> > > > +{\n> > > > +\tif (!size_ || overflow_)\n> > > > +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> > > > +\n> > > > +\tconst uint8_t *curr = read_ ? read_ : write_;\n> > > > +\tif (curr + size > base_ + size_) {\n> > > > +\t\tLOG(Serialization, Error)\n> > > > +\t\t\t<< \"Unable to reserve \" << size << \" bytes\";\n> > > > +\t\tsetOverflow();\n> > > > +\n> > > > +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> > > > +\t}\n> > > > +\n> > > > +\tif (read_) {\n> > > > +\t\tByteStreamBuffer b(read_, size);\n> > > > +\t\tb.parent_ = this;\n> > > > +\t\tread_ += size;\n> > > > +\t\treturn b;\n> > > > +\t} else {\n> > > > +\t\tByteStreamBuffer b(write_, size);\n> > > > +\t\tb.parent_ = this;\n> > > > +\t\twrite_ += size;\n> > > > +\t\treturn b;\n> > > > +\t}\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Skip \\a size bytes from the buffer\n> > > > + * \\param[in] size The number of bytes to skip\n> > > > + *\n> > > > + * This method skips the next \\a size bytes from the buffer.\n> > > > + *\n> > > > + * \\return 0 on success, a negative error code otherwise\n> > > > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > > > + */\n> > > > +int ByteStreamBuffer::skip(size_t size)\n> > > > +{\n> > > > +\tif (overflow_)\n> > > > +\t\treturn -ENOSPC;\n> > >\n> > > I think you should return early on !size.\n> >\n> > This should never happen in practice, right ? Would you return before\n> > this check, or right after it ? If we return right after it will just be\n> > an optimization for a case that should never happen.\n> >\n> > > > +\n> > > > +\tconst uint8_t *curr = read_ ? read_ : write_;\n> > > > +\tif (curr + size > base_ + size_) {\n> > > > +\t\tLOG(Serialization, Error)\n> > > > +\t\t\t<< \"Unable to skip \" << size << \" bytes\";\n> > > > +\t\tsetOverflow();\n> > > > +\n> > > > +\t\treturn -ENOSPC;\n> > > > +\t}\n> > > > +\n> > > > +\tif (read_) {\n> > > > +\t\tread_ += size;\n> > > > +\t} else {\n> > > > +\t\tmemset(write_, 0, size);\n> > >\n> > > Do we want to memset her? I think we do but just checking.\n> >\n> > I think we do, otherwise we could leak data over IPC.\n> >\n> > > > +\t\twrite_ += size;\n> > > > +\t}\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > >\n> > > skip() and carveOut() share much code, could they be merged?\n> >\n> > I've read both methods, and I think they don't share that much code.\n> > There's only the size check, and even that has a different error\n> > message, and a different return value. I don't think it's worth it, but\n> > maybe I'm missing something, so please feel free to propose a patch.\n> >\n> > > > +\n> > > > +/**\n> > > > + * \\fn template int ByteStreamBuffer::read(T *t)\n> > > > + * \\brief Read \\a size \\a data from the managed memory buffer\n> > > > + * \\param[out] t Pointer to the memory containing the read data\n> > > > + * \\return 0 on success, a negative error code otherwise\n> > > > + * \\retval -EACCES attempting to read from a write buffer\n> > > > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn template int ByteStreamBuffer::write(const T *t)\n> > > > + * \\brief Write \\a data of \\a size to the managed memory buffer\n> > > > + * \\param[in] t The data to write to memory\n> > > > + * \\return 0 on success, a negative error code otherwise\n> > > > + * \\retval -EACCES attempting to write to a read buffer\n> > > > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > > > + */\n> > > > +\n> > > > +int ByteStreamBuffer::read(uint8_t *data, size_t size)\n> > > > +{\n> > > > +\tif (!read_)\n> > > > +\t\treturn -EACCES;\n> > > > +\n> > > > +\tif (overflow_)\n> > > > +\t\treturn -ENOSPC;\n> > > > +\n> > > > +\tif (read_ + size > base_ + size_) {\n> > > > +\t\tLOG(Serialization, Error)\n> > > > +\t\t\t<< \"Unable to read \" << size << \" bytes: out of bounds\";\n> > > > +\t\tsetOverflow();\n> > > > +\t\treturn -ENOSPC;\n> > > > +\t}\n> > > > +\n> > > > +\tmemcpy(data, read_, size);\n> > > > +\tread_ += size;\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +int ByteStreamBuffer::write(const uint8_t *data, size_t size)\n> > > > +{\n> > > > +\tif (!write_)\n> > > > +\t\treturn -EACCES;\n> > > > +\n> > > > +\tif (overflow_)\n> > > > +\t\treturn -ENOSPC;\n> > > > +\n> > > > +\tif (write_ + size > base_ + size_) {\n> > > > +\t\tLOG(Serialization, Error)\n> > > > +\t\t\t<< \"Unable to write \" << size << \" bytes: no space left\";\n> > > > +\t\tsetOverflow();\n> > > > +\t\treturn -ENOSPC;\n> > > > +\t}\n> > > > +\n> > > > +\tmemcpy(write_, data, size);\n> > > > +\twrite_ += size;\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h\n> > > > new file mode 100644\n> > > > index 000000000000..b5274c62b85e\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/include/byte_stream_buffer.h\n> > > > @@ -0,0 +1,63 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2019, Google Inc.\n> > > > + *\n> > > > + * byte_stream_buffer.h - Byte stream buffer\n> > > > + */\n> > > > +#ifndef __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> > > > +#define __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +class ByteStreamBuffer\n> > > > +{\n> > > > +public:\n> > > > +\tByteStreamBuffer(const uint8_t *base, size_t size);\n> > > > +\tByteStreamBuffer(uint8_t *base, size_t size);\n> > > > +\tByteStreamBuffer(ByteStreamBuffer &&other);\n> > > > +\tByteStreamBuffer &operator=(ByteStreamBuffer &&other);\n> > > > +\n> > > > +\tconst uint8_t *base() const { return base_; }\n> > > > +\tuint32_t offset() const { return (write_ ? write_ : read_) - base_; }\n> > > > +\tsize_t size() const { return size_; }\n> > > > +\tbool overflow() const { return overflow_; }\n> > > > +\n> > > > +\tByteStreamBuffer carveOut(size_t size);\n> > > > +\tint skip(size_t size);\n> > > > +\n> > > > +\ttemplate\n> > > > +\tint read(T *t)\n> > > > +\t{\n> > > > +\t\treturn read(reinterpret_cast(t), sizeof(*t));\n> > > > +\t}\n> > > > +\ttemplate\n> > > > +\tint write(const T *t)\n> > > > +\t{\n> > > > +\t\treturn write(reinterpret_cast(t), sizeof(*t));\n> > > > +\t}\n> > > > +\n> > > > +private:\n> > > > +\tByteStreamBuffer(const ByteStreamBuffer &other) = delete;\n> > > > +\tByteStreamBuffer &operator=(const ByteStreamBuffer &other) = delete;\n> > > > +\n> > > > +\tvoid setOverflow();\n> > > > +\n> > > > +\tint read(uint8_t *data, size_t size);\n> > > > +\tint write(const uint8_t *data, size_t size);\n> > > > +\n> > > > +\tByteStreamBuffer *parent_;\n> > > > +\n> > > > +\tconst uint8_t *base_;\n> > > > +\tsize_t size_;\n> > > > +\tbool overflow_;\n> > > > +\n> > > > +\tconst uint8_t *read_;\n> > > > +\tuint8_t *write_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > +\n> > > > +#endif /* __LIBCAMERA_BYTE_STREAM_BUFFER_H__ */\n> > > > diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> > > > index 64c2155f90cf..1ff0198662cc 100644\n> > > > --- a/src/libcamera/include/meson.build\n> > > > +++ b/src/libcamera/include/meson.build\n> > > > @@ -1,4 +1,5 @@\n> > > > libcamera_headers = files([\n> > > > + 'byte_stream_buffer.h',\n> > > > 'camera_controls.h',\n> > > > 'camera_sensor.h',\n> > > > 'control_validator.h',\n> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > index be0cd53f6f10..dab2d8ad2649 100644\n> > > > --- a/src/libcamera/meson.build\n> > > > +++ b/src/libcamera/meson.build\n> > > > @@ -1,6 +1,7 @@\n> > > > libcamera_sources = files([\n> > > > 'bound_method.cpp',\n> > > > 'buffer.cpp',\n> > > > + 'byte_stream_buffer.cpp',\n> > > > 'camera.cpp',\n> > > > 'camera_controls.cpp',\n> > > > 'camera_manager.cpp',","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 09A2460C12\n\tfor ;\n\tTue, 19 Nov 2019 01:28:22 +0100 (CET)","from pendragon.ideasonboard.com (unknown [38.98.37.142])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A3D39563;\n\tTue, 19 Nov 2019 01:28:13 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1574123301;\n\tbh=6mzlj9pFwlfwLZyd2DEwxtfgYGl2A8F2jABEejCD1jA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dU7EAy6J9HAHMOXaJjvxjsSbKPMktxDA/khFTB/GE85Hjf0rCDQPSAcLi9ESTRPu1\n\tj9BdtfEVJSbhYIyEL++uTvDSokPyn4veU0QITvv0rvvzG5eEhRKTTdX3n92BggMVfh\n\tOUsyhkUA0to/KwaK/Df8CSurWJPXcXNWmBfVUkF8=","Date":"Tue, 19 Nov 2019 02:28:02 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191119002802.GN5171@pendragon.ideasonboard.com>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-15-laurent.pinchart@ideasonboard.com>\n\t<20191118181332.GI8072@bigcity.dyn.berto.se>\n\t<20191118183923.GA4888@pendragon.ideasonboard.com>\n\t<20191118225919.y6paptuhsfpfmpre@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191118225919.y6paptuhsfpfmpre@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","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":"Tue, 19 Nov 2019 00:28:22 -0000"}},{"id":3105,"web_url":"https://patchwork.libcamera.org/comment/3105/","msgid":"<021ab685-3504-9b33-424d-1eea95caa83e@ideasonboard.com>","date":"2019-11-19T16:37:12","subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 18/11/2019 18:39, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> On Mon, Nov 18, 2019 at 07:13:32PM +0100, Niklas Söderlund wrote:\n>> On 2019-11-08 22:53:59 +0200, Laurent Pinchart wrote:\n>>> From: Jacopo Mondi \n>>>\n>>> The ByteStreamBuffer class wraps a memory area, expected to be allocated\n>>> by the user of the class and provides operations to perform sequential\n>>> access in read and write modes.\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> Signed-off-by: Laurent Pinchart \n>>> ---\n>>> src/libcamera/byte_stream_buffer.cpp | 286 +++++++++++++++++++++\n>>> src/libcamera/include/byte_stream_buffer.h | 63 +++++\n>>> src/libcamera/include/meson.build | 1 +\n>>> src/libcamera/meson.build | 1 +\n>>> 4 files changed, 351 insertions(+)\n>>> create mode 100644 src/libcamera/byte_stream_buffer.cpp\n>>> create mode 100644 src/libcamera/include/byte_stream_buffer.h\n>>>\n>>> diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp\n>>> new file mode 100644\n>>> index 000000000000..23d624dd0a06\n>>> --- /dev/null\n>>> +++ b/src/libcamera/byte_stream_buffer.cpp\n>>> @@ -0,0 +1,286 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2019, Google Inc.\n>>> + *\n>>> + * byte_stream_buffer.cpp - Byte stream buffer\n>>> + */\n>>> +\n>>> +#include \"byte_stream_buffer.h\"\n>>> +\n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \"log.h\"\n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +LOG_DEFINE_CATEGORY(Serialization);\n>>> +\n>>> +/**\n>>> + * \\file byte_stream_buffer.h\n>>> + * \\brief Managed memory container for serialized data\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\class ByteStreamBuffer\n>>> + * \\brief Wrap a memory buffer and provide sequential data read and write\n>>> + *\n>>> + * The ByteStreamBuffer class wraps a memory buffer and exposes sequential read\n>>> + * and write operation with integrated boundary checks. Access beyond the end\n>>> + * of the buffer are blocked and logged, allowing error checks to take place at\n>>> + * the of of access operations instead of at each access. This simplifies\n>>> + * serialization and deserialization of data.\n>>> + *\n>>> + * A byte stream buffer is created with a base memory pointer and a size. If the\n>>> + * memory pointer is const, the buffer operates in read-only mode, and write\n>>> + * operations are denied. Otherwise the buffer operates in write-only mode, and\n>>> + * read operations are denied.\n>>> + *\n>>> + * Once a buffer is created, data is read or written with read() and write()\n>>> + * respectively. Access is strictly sequential, the buffer keeps track of the\n>>> + * current access location and advances it automatically. Reading or writing\n>>> + * the same location multiple times is thus not possible. Bytes may also be\n>>> + * skipped with the skip() method.\n>>> + *\n>>> + * The ByteStreamBuffer also supports carving out pieces of memory into other\n>>> + * ByteStreamBuffer instances. Like a read or write operation, a carveOut()\n>>> + * advances the internal access location, but allows the carved out memory to\n>>> + * be accessed at a later time.\n>>> + *\n>>> + * All accesses beyond the end of the buffer (read, write, skip or carve out)\n>>> + * are blocked. The first of such accesses causes a message to be logged, and\n>>> + * the buffer being marked as having overflown. If the buffer has been carved\n>>> + * out from a parent buffer, the parent buffer is also marked as having\n>>> + * overflown. Any later access on an overflown buffer is blocked. The buffer\n>>> + * overflow status can be checked with the overflow() method.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Construct a read ByteStreamBuffer from the memory area \\a base\n>>> + * of \\a size\n>>> + * \\param[in] base The address of the memory area to wrap\n>>> + * \\param[in] size The size of the memory area to wrap\n>>> + */\n>>> +ByteStreamBuffer::ByteStreamBuffer(const uint8_t *base, size_t size)\n>>> +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n>>> +\t read_(base), write_(nullptr)\n>>> +{\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Construct a write ByteStreamBuffer from the memory area \\a base\n>>> + * of \\a size\n>>> + * \\param[in] base The address of the memory area to wrap\n>>> + * \\param[in] size The size of the memory area to wrap\n>>> + */\n>>> +ByteStreamBuffer::ByteStreamBuffer(uint8_t *base, size_t size)\n>>> +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n>>> +\t read_(nullptr), write_(base)\n>>> +{\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Construct a ByteStreamBuffer from the contents of \\a other using move\n>>> + * semantics\n>>> + * \\param[in] other The other buffer\n>>> + *\n>>> + * After the move construction the \\a other buffer is invalidated. Any attempt\n>>> + * to access its contents will be considered as an overflow.\n>>> + */\n>>> +ByteStreamBuffer::ByteStreamBuffer(ByteStreamBuffer &&other)\n>>> +{\n>>> +\t*this = std::move(other);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Replace the contents of the buffer with those of \\a other using move\n>>> + * semantics\n>>> + * \\param[in] other The other buffer\n>>> + *\n>>> + * After the assignment the \\a other buffer is invalidated. Any attempt to\n>>> + * access its contents will be considered as an overflow.\n>>> + */\n>>> +ByteStreamBuffer &ByteStreamBuffer::operator=(ByteStreamBuffer &&other)\n>>> +{\n>>> +\tparent_ = other.parent_;\n>>> +\tbase_ = other.base_;\n>>> +\tsize_ = other.size_;\n>>> +\toverflow_ = other.overflow_;\n>>> +\tread_ = other.read_;\n>>> +\twrite_ = other.write_;\n>>> +\n>>> +\tother.parent_ = nullptr;\n>>> +\tother.base_ = nullptr;\n>>> +\tother.size_ = 0;\n>>> +\tother.overflow_ = false;\n>>> +\tother.read_ = nullptr;\n>>> +\tother.write_ = nullptr;\n>>> +\n>>> +\treturn *this;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\fn ByteStreamBuffer::base()\n>>> + * \\brief Retrieve a pointer to the start location of the managed memory buffer\n>>> + * \\return A pointer to the managed memory buffer\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ByteStreamBuffer::offset()\n>>> + * \\brief Retrieve the offset of the current access location from the base\n>>> + * \\return The offset in bytes\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ByteStreamBuffer::size()\n>>> + * \\brief Retrieve the size of the managed memory buffer\n>>> + * \\return The size of managed memory buffer\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ByteStreamBuffer::overflow()\n>>> + * \\brief Check if the buffer has overflown\n>>> + * \\return True if the buffer has overflow, false otherwise\n>>> + */\n>>> +\n>>> +void ByteStreamBuffer::setOverflow()\n>>> +{\n>>> +\tif (parent_)\n>>> +\t\tparent_->setOverflow();\n>>\n>> This seems a bit more complex then it needs to be. Is it really needed \n>> to track a parent from a carve out and set overflow if the child tries \n>> to interact with too much data?\n>>\n>> It is not possible to create a carv out which would create a overflow \n>> and the overflow will be logged from the child right?\n> \n> The idea here is that all ByteStreamBuffer operations (read, write and\n> carve out) log overflow in the top-level object, allowing users of this\n> class to not perform any error checking until the very end. Without\n> propagating overflow to the parent, we would need to check each carved\n> out buffer independently.\n> \n> This being said, I think the ByteStreamBuffer class, while nice as a\n> concept, may be overkill. It has a limited set of users, and gets a bit\n> in the way by requiring copies of all data. I think we'll revisit it\n> when moving value for simple controls inline (creating a union to store\n> either the value or the offset in the packet entry), and relax the error\n> checks then.\n\nMy original intention of the ByteStreamBuffer (in a very distant series)\nwas that it could directly map to the write/read calls of the IPC\nwithout needing intermediate storage.\n\nI.e. ByteStreamBuffer was just an implementation of a ByteStream\ninterface, and it could map directly to some ByteStreamIPC implementation.\n\nEffectively serialising would go direct to the 'wire'. However, the\nnon-linear data storage that has been implemented and the carve-outs I\nthink will prevent this optimisation that I had planned, so if it is no\nlonger relevant then so be it.\n\n--\nKieran\n\n\n\n>>> +\n>>> +\toverflow_ = true;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Carve out an area of \\a size bytes into a new ByteStreamBuffer\n>>> + * \\param[in] size The size of the newly created memory buffer\n>>> + *\n>>> + * This method carves out an area of \\a size bytes from the buffer into a new\n>>> + * ByteStreamBuffer, and returns the new buffer. It operates identically to a\n>>> + * read or write access from the point of view of the current buffer, but allows\n>>> + * the new buffer to be read or written at a later time after other read or\n>>> + * write accesses on the current buffer.\n>>> + *\n>>> + * \\return A newly created ByteStreamBuffer of \\a size\n>>> + */\n>>> +ByteStreamBuffer ByteStreamBuffer::carveOut(size_t size)\n>>> +{\n>>> +\tif (!size_ || overflow_)\n>>> +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n>>> +\n>>> +\tconst uint8_t *curr = read_ ? read_ : write_;\n>>> +\tif (curr + size > base_ + size_) {\n>>> +\t\tLOG(Serialization, Error)\n>>> +\t\t\t<< \"Unable to reserve \" << size << \" bytes\";\n>>> +\t\tsetOverflow();\n>>> +\n>>> +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n>>> +\t}\n>>> +\n>>> +\tif (read_) {\n>>> +\t\tByteStreamBuffer b(read_, size);\n>>> +\t\tb.parent_ = this;\n>>> +\t\tread_ += size;\n>>> +\t\treturn b;\n>>> +\t} else {\n>>> +\t\tByteStreamBuffer b(write_, size);\n>>> +\t\tb.parent_ = this;\n>>> +\t\twrite_ += size;\n>>> +\t\treturn b;\n>>> +\t}\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Skip \\a size bytes from the buffer\n>>> + * \\param[in] size The number of bytes to skip\n>>> + *\n>>> + * This method skips the next \\a size bytes from the buffer.\n>>> + *\n>>> + * \\return 0 on success, a negative error code otherwise\n>>> + * \\retval -ENOSPC no more space is available in the managed memory buffer\n>>> + */\n>>> +int ByteStreamBuffer::skip(size_t size)\n>>> +{\n>>> +\tif (overflow_)\n>>> +\t\treturn -ENOSPC;\n>>\n>> I think you should return early on !size.\n> \n> This should never happen in practice, right ? Would you return before\n> this check, or right after it ? If we return right after it will just be\n> an optimization for a case that should never happen.\n> \n>>> +\n>>> +\tconst uint8_t *curr = read_ ? read_ : write_;\n>>> +\tif (curr + size > base_ + size_) {\n>>> +\t\tLOG(Serialization, Error)\n>>> +\t\t\t<< \"Unable to skip \" << size << \" bytes\";\n>>> +\t\tsetOverflow();\n>>> +\n>>> +\t\treturn -ENOSPC;\n>>> +\t}\n>>> +\n>>> +\tif (read_) {\n>>> +\t\tread_ += size;\n>>> +\t} else {\n>>> +\t\tmemset(write_, 0, size);\n>>\n>> Do we want to memset her? I think we do but just checking.\n> \n> I think we do, otherwise we could leak data over IPC.\n> \n>>> +\t\twrite_ += size;\n>>> +\t}\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>\n>> skip() and carveOut() share much code, could they be merged?\n> \n> I've read both methods, and I think they don't share that much code.\n> There's only the size check, and even that has a different error\n> message, and a different return value. I don't think it's worth it, but\n> maybe I'm missing something, so please feel free to propose a patch.\n> \n>>> +\n>>> +/**\n>>> + * \\fn template int ByteStreamBuffer::read(T *t)\n>>> + * \\brief Read \\a size \\a data from the managed memory buffer\n>>> + * \\param[out] t Pointer to the memory containing the read data\n>>> + * \\return 0 on success, a negative error code otherwise\n>>> + * \\retval -EACCES attempting to read from a write buffer\n>>> + * \\retval -ENOSPC no more space is available in the managed memory buffer\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn template int ByteStreamBuffer::write(const T *t)\n>>> + * \\brief Write \\a data of \\a size to the managed memory buffer\n>>> + * \\param[in] t The data to write to memory\n>>> + * \\return 0 on success, a negative error code otherwise\n>>> + * \\retval -EACCES attempting to write to a read buffer\n>>> + * \\retval -ENOSPC no more space is available in the managed memory buffer\n>>> + */\n>>> +\n>>> +int ByteStreamBuffer::read(uint8_t *data, size_t size)\n>>> +{\n>>> +\tif (!read_)\n>>> +\t\treturn -EACCES;\n>>> +\n>>> +\tif (overflow_)\n>>> +\t\treturn -ENOSPC;\n>>> +\n>>> +\tif (read_ + size > base_ + size_) {\n>>> +\t\tLOG(Serialization, Error)\n>>> +\t\t\t<< \"Unable to read \" << size << \" bytes: out of bounds\";\n>>> +\t\tsetOverflow();\n>>> +\t\treturn -ENOSPC;\n>>> +\t}\n>>> +\n>>> +\tmemcpy(data, read_, size);\n>>> +\tread_ += size;\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +int ByteStreamBuffer::write(const uint8_t *data, size_t size)\n>>> +{\n>>> +\tif (!write_)\n>>> +\t\treturn -EACCES;\n>>> +\n>>> +\tif (overflow_)\n>>> +\t\treturn -ENOSPC;\n>>> +\n>>> +\tif (write_ + size > base_ + size_) {\n>>> +\t\tLOG(Serialization, Error)\n>>> +\t\t\t<< \"Unable to write \" << size << \" bytes: no space left\";\n>>> +\t\tsetOverflow();\n>>> +\t\treturn -ENOSPC;\n>>> +\t}\n>>> +\n>>> +\tmemcpy(write_, data, size);\n>>> +\twrite_ += size;\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +} /* namespace libcamera */\n>>> diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h\n>>> new file mode 100644\n>>> index 000000000000..b5274c62b85e\n>>> --- /dev/null\n>>> +++ b/src/libcamera/include/byte_stream_buffer.h\n>>> @@ -0,0 +1,63 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2019, Google Inc.\n>>> + *\n>>> + * byte_stream_buffer.h - Byte stream buffer\n>>> + */\n>>> +#ifndef __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n>>> +#define __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n>>> +\n>>> +#include \n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +class ByteStreamBuffer\n>>> +{\n>>> +public:\n>>> +\tByteStreamBuffer(const uint8_t *base, size_t size);\n>>> +\tByteStreamBuffer(uint8_t *base, size_t size);\n>>> +\tByteStreamBuffer(ByteStreamBuffer &&other);\n>>> +\tByteStreamBuffer &operator=(ByteStreamBuffer &&other);\n>>> +\n>>> +\tconst uint8_t *base() const { return base_; }\n>>> +\tuint32_t offset() const { return (write_ ? write_ : read_) - base_; }\n>>> +\tsize_t size() const { return size_; }\n>>> +\tbool overflow() const { return overflow_; }\n>>> +\n>>> +\tByteStreamBuffer carveOut(size_t size);\n>>> +\tint skip(size_t size);\n>>> +\n>>> +\ttemplate\n>>> +\tint read(T *t)\n>>> +\t{\n>>> +\t\treturn read(reinterpret_cast(t), sizeof(*t));\n>>> +\t}\n>>> +\ttemplate\n>>> +\tint write(const T *t)\n>>> +\t{\n>>> +\t\treturn write(reinterpret_cast(t), sizeof(*t));\n>>> +\t}\n>>> +\n>>> +private:\n>>> +\tByteStreamBuffer(const ByteStreamBuffer &other) = delete;\n>>> +\tByteStreamBuffer &operator=(const ByteStreamBuffer &other) = delete;\n>>> +\n>>> +\tvoid setOverflow();\n>>> +\n>>> +\tint read(uint8_t *data, size_t size);\n>>> +\tint write(const uint8_t *data, size_t size);\n>>> +\n>>> +\tByteStreamBuffer *parent_;\n>>> +\n>>> +\tconst uint8_t *base_;\n>>> +\tsize_t size_;\n>>> +\tbool overflow_;\n>>> +\n>>> +\tconst uint8_t *read_;\n>>> +\tuint8_t *write_;\n>>> +};\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> +#endif /* __LIBCAMERA_BYTE_STREAM_BUFFER_H__ */\n>>> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n>>> index 64c2155f90cf..1ff0198662cc 100644\n>>> --- a/src/libcamera/include/meson.build\n>>> +++ b/src/libcamera/include/meson.build\n>>> @@ -1,4 +1,5 @@\n>>> libcamera_headers = files([\n>>> + 'byte_stream_buffer.h',\n>>> 'camera_controls.h',\n>>> 'camera_sensor.h',\n>>> 'control_validator.h',\n>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>> index be0cd53f6f10..dab2d8ad2649 100644\n>>> --- a/src/libcamera/meson.build\n>>> +++ b/src/libcamera/meson.build\n>>> @@ -1,6 +1,7 @@\n>>> libcamera_sources = files([\n>>> 'bound_method.cpp',\n>>> 'buffer.cpp',\n>>> + 'byte_stream_buffer.cpp',\n>>> 'camera.cpp',\n>>> 'camera_controls.cpp',\n>>> 'camera_manager.cpp',\n>","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 8C5A760BEA\n\tfor ;\n\tTue, 19 Nov 2019 17:37:15 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C2CBB311;\n\tTue, 19 Nov 2019 17:37:14 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1574181435;\n\tbh=Q/vZhBC6cmSKser0PmlWitgX332Tmbb5TpFogOKlk+w=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=BR4ruwAzjutiu3NyjoqwPenV9OGYPC6So98Cp+YaxnswO2sv7efKZSOaieFvMEyaW\n\tzeIzZk7NcFcmTjbHOqaVzeDN3Eh9UEBJ8KWAK+mp2NhjR58JORTdnwkUMcVEzoPCFD\n\tP+mkjFEaJiUhOKGDEXtePSpVdyM3E6a7Zm78J55Y=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart , =?utf-8?q?Niklas?=\n\t=?utf-8?q?_S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-15-laurent.pinchart@ideasonboard.com>\n\t<20191118181332.GI8072@bigcity.dyn.berto.se>\n\t<20191118183923.GA4888@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<021ab685-3504-9b33-424d-1eea95caa83e@ideasonboard.com>","Date":"Tue, 19 Nov 2019 16:37:12 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20191118183923.GA4888@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","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":"Tue, 19 Nov 2019 16:37:15 -0000"}},{"id":3118,"web_url":"https://patchwork.libcamera.org/comment/3118/","msgid":"<20191120110004.dbri2ianlbhjkuag@uno.localdomain>","date":"2019-11-20T11:00:04","subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Nov 19, 2019 at 02:28:02AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Nov 18, 2019 at 11:59:19PM +0100, Jacopo Mondi wrote:\n> > On Mon, Nov 18, 2019 at 08:39:23PM +0200, Laurent Pinchart wrote:\n> > > On Mon, Nov 18, 2019 at 07:13:32PM +0100, Niklas Söderlund wrote:\n> > > > On 2019-11-08 22:53:59 +0200, Laurent Pinchart wrote:\n> > > > > From: Jacopo Mondi \n> > > > >\n> > > > > The ByteStreamBuffer class wraps a memory area, expected to be allocated\n> > > > > by the user of the class and provides operations to perform sequential\n> > > > > access in read and write modes.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/libcamera/byte_stream_buffer.cpp | 286 +++++++++++++++++++++\n> > > > > src/libcamera/include/byte_stream_buffer.h | 63 +++++\n> > > > > src/libcamera/include/meson.build | 1 +\n> > > > > src/libcamera/meson.build | 1 +\n> > > > > 4 files changed, 351 insertions(+)\n> > > > > create mode 100644 src/libcamera/byte_stream_buffer.cpp\n> > > > > create mode 100644 src/libcamera/include/byte_stream_buffer.h\n> > > > >\n> > > > > diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp\n> > > > > new file mode 100644\n> > > > > index 000000000000..23d624dd0a06\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/byte_stream_buffer.cpp\n> > > > > @@ -0,0 +1,286 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2019, Google Inc.\n> > > > > + *\n> > > > > + * byte_stream_buffer.cpp - Byte stream buffer\n> > > > > + */\n> > > > > +\n> > > > > +#include \"byte_stream_buffer.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \"log.h\"\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +LOG_DEFINE_CATEGORY(Serialization);\n> > > > > +\n> > > > > +/**\n> > > > > + * \\file byte_stream_buffer.h\n> > > > > + * \\brief Managed memory container for serialized data\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\class ByteStreamBuffer\n> > > > > + * \\brief Wrap a memory buffer and provide sequential data read and write\n> > > > > + *\n> > > > > + * The ByteStreamBuffer class wraps a memory buffer and exposes sequential read\n> > > > > + * and write operation with integrated boundary checks. Access beyond the end\n> > > > > + * of the buffer are blocked and logged, allowing error checks to take place at\n> > > > > + * the of of access operations instead of at each access. This simplifies\n> > > > > + * serialization and deserialization of data.\n> > > > > + *\n> > > > > + * A byte stream buffer is created with a base memory pointer and a size. If the\n> > > > > + * memory pointer is const, the buffer operates in read-only mode, and write\n> > > > > + * operations are denied. Otherwise the buffer operates in write-only mode, and\n> > > > > + * read operations are denied.\n> > > > > + *\n> > > > > + * Once a buffer is created, data is read or written with read() and write()\n> > > > > + * respectively. Access is strictly sequential, the buffer keeps track of the\n> > > > > + * current access location and advances it automatically. Reading or writing\n> > > > > + * the same location multiple times is thus not possible. Bytes may also be\n> > > > > + * skipped with the skip() method.\n> > > > > + *\n> > > > > + * The ByteStreamBuffer also supports carving out pieces of memory into other\n> > > > > + * ByteStreamBuffer instances. Like a read or write operation, a carveOut()\n> > > > > + * advances the internal access location, but allows the carved out memory to\n> > > > > + * be accessed at a later time.\n> > > > > + *\n> > > > > + * All accesses beyond the end of the buffer (read, write, skip or carve out)\n> > > > > + * are blocked. The first of such accesses causes a message to be logged, and\n> > > > > + * the buffer being marked as having overflown. If the buffer has been carved\n> > > > > + * out from a parent buffer, the parent buffer is also marked as having\n> > > > > + * overflown. Any later access on an overflown buffer is blocked. The buffer\n> > > > > + * overflow status can be checked with the overflow() method.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Construct a read ByteStreamBuffer from the memory area \\a base\n> > > > > + * of \\a size\n> > > > > + * \\param[in] base The address of the memory area to wrap\n> > > > > + * \\param[in] size The size of the memory area to wrap\n> > > > > + */\n> > > > > +ByteStreamBuffer::ByteStreamBuffer(const uint8_t *base, size_t size)\n> > > > > +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> > > > > +\t read_(base), write_(nullptr)\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Construct a write ByteStreamBuffer from the memory area \\a base\n> > > > > + * of \\a size\n> > > > > + * \\param[in] base The address of the memory area to wrap\n> > > > > + * \\param[in] size The size of the memory area to wrap\n> > > > > + */\n> > > > > +ByteStreamBuffer::ByteStreamBuffer(uint8_t *base, size_t size)\n> > > > > +\t: parent_(nullptr), base_(base), size_(size), overflow_(false),\n> > > > > +\t read_(nullptr), write_(base)\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Construct a ByteStreamBuffer from the contents of \\a other using move\n> > > > > + * semantics\n> > > > > + * \\param[in] other The other buffer\n> > > > > + *\n> > > > > + * After the move construction the \\a other buffer is invalidated. Any attempt\n> > > > > + * to access its contents will be considered as an overflow.\n> > > > > + */\n> > > > > +ByteStreamBuffer::ByteStreamBuffer(ByteStreamBuffer &&other)\n> > > > > +{\n> > > > > +\t*this = std::move(other);\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Replace the contents of the buffer with those of \\a other using move\n> > > > > + * semantics\n> > > > > + * \\param[in] other The other buffer\n> > > > > + *\n> > > > > + * After the assignment the \\a other buffer is invalidated. Any attempt to\n> > > > > + * access its contents will be considered as an overflow.\n> > > > > + */\n> > > > > +ByteStreamBuffer &ByteStreamBuffer::operator=(ByteStreamBuffer &&other)\n> > > > > +{\n> > > > > +\tparent_ = other.parent_;\n> > > > > +\tbase_ = other.base_;\n> > > > > +\tsize_ = other.size_;\n> > > > > +\toverflow_ = other.overflow_;\n> > > > > +\tread_ = other.read_;\n> > > > > +\twrite_ = other.write_;\n> > > > > +\n> > > > > +\tother.parent_ = nullptr;\n> > > > > +\tother.base_ = nullptr;\n> > > > > +\tother.size_ = 0;\n> > > > > +\tother.overflow_ = false;\n> > > > > +\tother.read_ = nullptr;\n> > > > > +\tother.write_ = nullptr;\n> > > > > +\n> > > > > +\treturn *this;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn ByteStreamBuffer::base()\n> > > > > + * \\brief Retrieve a pointer to the start location of the managed memory buffer\n> > > > > + * \\return A pointer to the managed memory buffer\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn ByteStreamBuffer::offset()\n> > > > > + * \\brief Retrieve the offset of the current access location from the base\n> > > > > + * \\return The offset in bytes\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn ByteStreamBuffer::size()\n> > > > > + * \\brief Retrieve the size of the managed memory buffer\n> > > > > + * \\return The size of managed memory buffer\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn ByteStreamBuffer::overflow()\n> > > > > + * \\brief Check if the buffer has overflown\n> > > > > + * \\return True if the buffer has overflow, false otherwise\n> > > > > + */\n> > > > > +\n> > > > > +void ByteStreamBuffer::setOverflow()\n> > > > > +{\n> > > > > +\tif (parent_)\n> > > > > +\t\tparent_->setOverflow();\n> > > >\n> > > > This seems a bit more complex then it needs to be. Is it really needed\n> > > > to track a parent from a carve out and set overflow if the child tries\n> > > > to interact with too much data?\n> > > >\n> > > > It is not possible to create a carv out which would create a overflow\n> > > > and the overflow will be logged from the child right?\n> > >\n> > > The idea here is that all ByteStreamBuffer operations (read, write and\n> > > carve out) log overflow in the top-level object, allowing users of this\n> > > class to not perform any error checking until the very end. Without\n> > > propagating overflow to the parent, we would need to check each carved\n> > > out buffer independently.\n> > >\n> > > This being said, I think the ByteStreamBuffer class, while nice as a\n> > > concept, may be overkill. It has a limited set of users, and gets a bit\n> > > in the way by requiring copies of all data. I think we'll revisit it\n> > > when moving value for simple controls inline (creating a union to store\n> > > either the value or the offset in the packet entry), and relax the error\n> > > checks then.\n> >\n> > I agree and if I recall an attempt to store values with binary size <=\n> > 4 in the offset section of the serialized packet header it felt a bit\n> > in the way. If it's not a huge work, I would rather drop it. Let me\n> > know if I can help there.\n>\n> I'm a bit confused, which part would you drop ? :-) Propagating the\n\nI meant the whole ByteStreamBuffer class :)\n\nBut then I had a look at how it is used and how it nicely abstract\naway read/write implementation details, and then I kind of changed my\nmind. I think we'll revist when/if we'll optimize storing values\nin the offset section.\n\nThanks\n j\n\n> overflow to the parent ? I'd prefer dropping it on top of this series,\n> along with a few other safety checks, in order to allow access to the\n> memory without any copy. Even better would be a compile-time flag that\n> would enable extra safety checks, but I'm probably just dreaming :-)\n>\n> > > > > +\n> > > > > +\toverflow_ = true;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Carve out an area of \\a size bytes into a new ByteStreamBuffer\n> > > > > + * \\param[in] size The size of the newly created memory buffer\n> > > > > + *\n> > > > > + * This method carves out an area of \\a size bytes from the buffer into a new\n> > > > > + * ByteStreamBuffer, and returns the new buffer. It operates identically to a\n> > > > > + * read or write access from the point of view of the current buffer, but allows\n> > > > > + * the new buffer to be read or written at a later time after other read or\n> > > > > + * write accesses on the current buffer.\n> > > > > + *\n> > > > > + * \\return A newly created ByteStreamBuffer of \\a size\n> > > > > + */\n> > > > > +ByteStreamBuffer ByteStreamBuffer::carveOut(size_t size)\n> > > > > +{\n> > > > > +\tif (!size_ || overflow_)\n> > > > > +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> > > > > +\n> > > > > +\tconst uint8_t *curr = read_ ? read_ : write_;\n> > > > > +\tif (curr + size > base_ + size_) {\n> > > > > +\t\tLOG(Serialization, Error)\n> > > > > +\t\t\t<< \"Unable to reserve \" << size << \" bytes\";\n> > > > > +\t\tsetOverflow();\n> > > > > +\n> > > > > +\t\treturn ByteStreamBuffer(static_cast(nullptr), 0);\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (read_) {\n> > > > > +\t\tByteStreamBuffer b(read_, size);\n> > > > > +\t\tb.parent_ = this;\n> > > > > +\t\tread_ += size;\n> > > > > +\t\treturn b;\n> > > > > +\t} else {\n> > > > > +\t\tByteStreamBuffer b(write_, size);\n> > > > > +\t\tb.parent_ = this;\n> > > > > +\t\twrite_ += size;\n> > > > > +\t\treturn b;\n> > > > > +\t}\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Skip \\a size bytes from the buffer\n> > > > > + * \\param[in] size The number of bytes to skip\n> > > > > + *\n> > > > > + * This method skips the next \\a size bytes from the buffer.\n> > > > > + *\n> > > > > + * \\return 0 on success, a negative error code otherwise\n> > > > > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > > > > + */\n> > > > > +int ByteStreamBuffer::skip(size_t size)\n> > > > > +{\n> > > > > +\tif (overflow_)\n> > > > > +\t\treturn -ENOSPC;\n> > > >\n> > > > I think you should return early on !size.\n> > >\n> > > This should never happen in practice, right ? Would you return before\n> > > this check, or right after it ? If we return right after it will just be\n> > > an optimization for a case that should never happen.\n> > >\n> > > > > +\n> > > > > +\tconst uint8_t *curr = read_ ? read_ : write_;\n> > > > > +\tif (curr + size > base_ + size_) {\n> > > > > +\t\tLOG(Serialization, Error)\n> > > > > +\t\t\t<< \"Unable to skip \" << size << \" bytes\";\n> > > > > +\t\tsetOverflow();\n> > > > > +\n> > > > > +\t\treturn -ENOSPC;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (read_) {\n> > > > > +\t\tread_ += size;\n> > > > > +\t} else {\n> > > > > +\t\tmemset(write_, 0, size);\n> > > >\n> > > > Do we want to memset her? I think we do but just checking.\n> > >\n> > > I think we do, otherwise we could leak data over IPC.\n> > >\n> > > > > +\t\twrite_ += size;\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > >\n> > > > skip() and carveOut() share much code, could they be merged?\n> > >\n> > > I've read both methods, and I think they don't share that much code.\n> > > There's only the size check, and even that has a different error\n> > > message, and a different return value. I don't think it's worth it, but\n> > > maybe I'm missing something, so please feel free to propose a patch.\n> > >\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn template int ByteStreamBuffer::read(T *t)\n> > > > > + * \\brief Read \\a size \\a data from the managed memory buffer\n> > > > > + * \\param[out] t Pointer to the memory containing the read data\n> > > > > + * \\return 0 on success, a negative error code otherwise\n> > > > > + * \\retval -EACCES attempting to read from a write buffer\n> > > > > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn template int ByteStreamBuffer::write(const T *t)\n> > > > > + * \\brief Write \\a data of \\a size to the managed memory buffer\n> > > > > + * \\param[in] t The data to write to memory\n> > > > > + * \\return 0 on success, a negative error code otherwise\n> > > > > + * \\retval -EACCES attempting to write to a read buffer\n> > > > > + * \\retval -ENOSPC no more space is available in the managed memory buffer\n> > > > > + */\n> > > > > +\n> > > > > +int ByteStreamBuffer::read(uint8_t *data, size_t size)\n> > > > > +{\n> > > > > +\tif (!read_)\n> > > > > +\t\treturn -EACCES;\n> > > > > +\n> > > > > +\tif (overflow_)\n> > > > > +\t\treturn -ENOSPC;\n> > > > > +\n> > > > > +\tif (read_ + size > base_ + size_) {\n> > > > > +\t\tLOG(Serialization, Error)\n> > > > > +\t\t\t<< \"Unable to read \" << size << \" bytes: out of bounds\";\n> > > > > +\t\tsetOverflow();\n> > > > > +\t\treturn -ENOSPC;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tmemcpy(data, read_, size);\n> > > > > +\tread_ += size;\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +int ByteStreamBuffer::write(const uint8_t *data, size_t size)\n> > > > > +{\n> > > > > +\tif (!write_)\n> > > > > +\t\treturn -EACCES;\n> > > > > +\n> > > > > +\tif (overflow_)\n> > > > > +\t\treturn -ENOSPC;\n> > > > > +\n> > > > > +\tif (write_ + size > base_ + size_) {\n> > > > > +\t\tLOG(Serialization, Error)\n> > > > > +\t\t\t<< \"Unable to write \" << size << \" bytes: no space left\";\n> > > > > +\t\tsetOverflow();\n> > > > > +\t\treturn -ENOSPC;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tmemcpy(write_, data, size);\n> > > > > +\twrite_ += size;\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h\n> > > > > new file mode 100644\n> > > > > index 000000000000..b5274c62b85e\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/include/byte_stream_buffer.h\n> > > > > @@ -0,0 +1,63 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2019, Google Inc.\n> > > > > + *\n> > > > > + * byte_stream_buffer.h - Byte stream buffer\n> > > > > + */\n> > > > > +#ifndef __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> > > > > +#define __LIBCAMERA_BYTE_STREAM_BUFFER_H__\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +class ByteStreamBuffer\n> > > > > +{\n> > > > > +public:\n> > > > > +\tByteStreamBuffer(const uint8_t *base, size_t size);\n> > > > > +\tByteStreamBuffer(uint8_t *base, size_t size);\n> > > > > +\tByteStreamBuffer(ByteStreamBuffer &&other);\n> > > > > +\tByteStreamBuffer &operator=(ByteStreamBuffer &&other);\n> > > > > +\n> > > > > +\tconst uint8_t *base() const { return base_; }\n> > > > > +\tuint32_t offset() const { return (write_ ? write_ : read_) - base_; }\n> > > > > +\tsize_t size() const { return size_; }\n> > > > > +\tbool overflow() const { return overflow_; }\n> > > > > +\n> > > > > +\tByteStreamBuffer carveOut(size_t size);\n> > > > > +\tint skip(size_t size);\n> > > > > +\n> > > > > +\ttemplate\n> > > > > +\tint read(T *t)\n> > > > > +\t{\n> > > > > +\t\treturn read(reinterpret_cast(t), sizeof(*t));\n> > > > > +\t}\n> > > > > +\ttemplate\n> > > > > +\tint write(const T *t)\n> > > > > +\t{\n> > > > > +\t\treturn write(reinterpret_cast(t), sizeof(*t));\n> > > > > +\t}\n> > > > > +\n> > > > > +private:\n> > > > > +\tByteStreamBuffer(const ByteStreamBuffer &other) = delete;\n> > > > > +\tByteStreamBuffer &operator=(const ByteStreamBuffer &other) = delete;\n> > > > > +\n> > > > > +\tvoid setOverflow();\n> > > > > +\n> > > > > +\tint read(uint8_t *data, size_t size);\n> > > > > +\tint write(const uint8_t *data, size_t size);\n> > > > > +\n> > > > > +\tByteStreamBuffer *parent_;\n> > > > > +\n> > > > > +\tconst uint8_t *base_;\n> > > > > +\tsize_t size_;\n> > > > > +\tbool overflow_;\n> > > > > +\n> > > > > +\tconst uint8_t *read_;\n> > > > > +\tuint8_t *write_;\n> > > > > +};\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > +\n> > > > > +#endif /* __LIBCAMERA_BYTE_STREAM_BUFFER_H__ */\n> > > > > diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> > > > > index 64c2155f90cf..1ff0198662cc 100644\n> > > > > --- a/src/libcamera/include/meson.build\n> > > > > +++ b/src/libcamera/include/meson.build\n> > > > > @@ -1,4 +1,5 @@\n> > > > > libcamera_headers = files([\n> > > > > + 'byte_stream_buffer.h',\n> > > > > 'camera_controls.h',\n> > > > > 'camera_sensor.h',\n> > > > > 'control_validator.h',\n> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > > index be0cd53f6f10..dab2d8ad2649 100644\n> > > > > --- a/src/libcamera/meson.build\n> > > > > +++ b/src/libcamera/meson.build\n> > > > > @@ -1,6 +1,7 @@\n> > > > > libcamera_sources = files([\n> > > > > 'bound_method.cpp',\n> > > > > 'buffer.cpp',\n> > > > > + 'byte_stream_buffer.cpp',\n> > > > > 'camera.cpp',\n> > > > > 'camera_controls.cpp',\n> > > > > 'camera_manager.cpp',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03C3660C22\n\tfor ;\n\tWed, 20 Nov 2019 11:58:06 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 0A63FFF81B;\n\tWed, 20 Nov 2019 10:58:05 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 20 Nov 2019 12:00:04 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191120110004.dbri2ianlbhjkuag@uno.localdomain>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-15-laurent.pinchart@ideasonboard.com>\n\t<20191118181332.GI8072@bigcity.dyn.berto.se>\n\t<20191118183923.GA4888@pendragon.ideasonboard.com>\n\t<20191118225919.y6paptuhsfpfmpre@uno.localdomain>\n\t<20191119002802.GN5171@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"axyd2dq5rguak5wj\"","Content-Disposition":"inline","In-Reply-To":"<20191119002802.GN5171@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 14/24] libcamera: Add\n\tByteStreamBuffer","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":"Wed, 20 Nov 2019 10:58:07 -0000"}}]