[libcamera-devel] [PATCH v2 14/24] libcamera: Add ByteStreamBuffer

[Kieran Bingham]

Hi Laurent,

On 18/11/2019 18:39, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Mon, Nov 18, 2019 at 07:13:32PM +0100, Niklas Söderlund wrote:
>> On 2019-11-08 22:53:59 +0200, Laurent Pinchart wrote:
>>> From: Jacopo Mondi <jacopo at jmondi.org>
>>>
>>> The ByteStreamBuffer class wraps a memory area, expected to be allocated
>>> by the user of the class and provides operations to perform sequential
>>> access in read and write modes.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>  src/libcamera/byte_stream_buffer.cpp       | 286 +++++++++++++++++++++
>>>  src/libcamera/include/byte_stream_buffer.h |  63 +++++
>>>  src/libcamera/include/meson.build          |   1 +
>>>  src/libcamera/meson.build                  |   1 +
>>>  4 files changed, 351 insertions(+)
>>>  create mode 100644 src/libcamera/byte_stream_buffer.cpp
>>>  create mode 100644 src/libcamera/include/byte_stream_buffer.h
>>>
>>> diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp
>>> new file mode 100644
>>> index 000000000000..23d624dd0a06
>>> --- /dev/null
>>> +++ b/src/libcamera/byte_stream_buffer.cpp
>>> @@ -0,0 +1,286 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2019, Google Inc.
>>> + *
>>> + * byte_stream_buffer.cpp - Byte stream buffer
>>> + */
>>> +
>>> +#include "byte_stream_buffer.h"
>>> +
>>> +#include <stdint.h>
>>> +#include <string.h>
>>> +
>>> +#include "log.h"
>>> +
>>> +namespace libcamera {
>>> +
>>> +LOG_DEFINE_CATEGORY(Serialization);
>>> +
>>> +/**
>>> + * \file byte_stream_buffer.h
>>> + * \brief Managed memory container for serialized data
>>> + */
>>> +
>>> +/**
>>> + * \class ByteStreamBuffer
>>> + * \brief Wrap a memory buffer and provide sequential data read and write
>>> + *
>>> + * The ByteStreamBuffer class wraps a memory buffer and exposes sequential read
>>> + * and write operation with integrated boundary checks. Access beyond the end
>>> + * of the buffer are blocked and logged, allowing error checks to take place at
>>> + * the of of access operations instead of at each access. This simplifies
>>> + * serialization and deserialization of data.
>>> + *
>>> + * A byte stream buffer is created with a base memory pointer and a size. If the
>>> + * memory pointer is const, the buffer operates in read-only mode, and write
>>> + * operations are denied. Otherwise the buffer operates in write-only mode, and
>>> + * read operations are denied.
>>> + *
>>> + * Once a buffer is created, data is read or written with read() and write()
>>> + * respectively. Access is strictly sequential, the buffer keeps track of the
>>> + * current access location and advances it automatically. Reading or writing
>>> + * the same location multiple times is thus not possible. Bytes may also be
>>> + * skipped with the skip() method.
>>> + *
>>> + * The ByteStreamBuffer also supports carving out pieces of memory into other
>>> + * ByteStreamBuffer instances. Like a read or write operation, a carveOut()
>>> + * advances the internal access location, but allows the carved out memory to
>>> + * be accessed at a later time.
>>> + *
>>> + * All accesses beyond the end of the buffer (read, write, skip or carve out)
>>> + * are blocked. The first of such accesses causes a message to be logged, and
>>> + * the buffer being marked as having overflown. If the buffer has been carved
>>> + * out from a parent buffer, the parent buffer is also marked as having
>>> + * overflown. Any later access on an overflown buffer is blocked. The buffer
>>> + * overflow status can be checked with the overflow() method.
>>> + */
>>> +
>>> +/**
>>> + * \brief Construct a read ByteStreamBuffer from the memory area \a base
>>> + * of \a size
>>> + * \param[in] base The address of the memory area to wrap
>>> + * \param[in] size The size of the memory area to wrap
>>> + */
>>> +ByteStreamBuffer::ByteStreamBuffer(const uint8_t *base, size_t size)
>>> +	: parent_(nullptr), base_(base), size_(size), overflow_(false),
>>> +	  read_(base), write_(nullptr)
>>> +{
>>> +}
>>> +
>>> +/**
>>> + * \brief Construct a write ByteStreamBuffer from the memory area \a base
>>> + * of \a size
>>> + * \param[in] base The address of the memory area to wrap
>>> + * \param[in] size The size of the memory area to wrap
>>> + */
>>> +ByteStreamBuffer::ByteStreamBuffer(uint8_t *base, size_t size)
>>> +	: parent_(nullptr), base_(base), size_(size), overflow_(false),
>>> +	  read_(nullptr), write_(base)
>>> +{
>>> +}
>>> +
>>> +/**
>>> + * \brief Construct a ByteStreamBuffer from the contents of \a other using move
>>> + * semantics
>>> + * \param[in] other The other buffer
>>> + *
>>> + * After the move construction the \a other buffer is invalidated. Any attempt
>>> + * to access its contents will be considered as an overflow.
>>> + */
>>> +ByteStreamBuffer::ByteStreamBuffer(ByteStreamBuffer &&other)
>>> +{
>>> +	*this = std::move(other);
>>> +}
>>> +
>>> +/**
>>> + * \brief Replace the contents of the buffer with those of \a other using move
>>> + * semantics
>>> + * \param[in] other The other buffer
>>> + *
>>> + * After the assignment the \a other buffer is invalidated. Any attempt to
>>> + * access its contents will be considered as an overflow.
>>> + */
>>> +ByteStreamBuffer &ByteStreamBuffer::operator=(ByteStreamBuffer &&other)
>>> +{
>>> +	parent_ = other.parent_;
>>> +	base_ = other.base_;
>>> +	size_ = other.size_;
>>> +	overflow_ = other.overflow_;
>>> +	read_ = other.read_;
>>> +	write_ = other.write_;
>>> +
>>> +	other.parent_ = nullptr;
>>> +	other.base_ = nullptr;
>>> +	other.size_ = 0;
>>> +	other.overflow_ = false;
>>> +	other.read_ = nullptr;
>>> +	other.write_ = nullptr;
>>> +
>>> +	return *this;
>>> +}
>>> +
>>> +/**
>>> + * \fn ByteStreamBuffer::base()
>>> + * \brief Retrieve a pointer to the start location of the managed memory buffer
>>> + * \return A pointer to the managed memory buffer
>>> + */
>>> +
>>> +/**
>>> + * \fn ByteStreamBuffer::offset()
>>> + * \brief Retrieve the offset of the current access location from the base
>>> + * \return The offset in bytes
>>> + */
>>> +
>>> +/**
>>> + * \fn ByteStreamBuffer::size()
>>> + * \brief Retrieve the size of the managed memory buffer
>>> + * \return The size of managed memory buffer
>>> + */
>>> +
>>> +/**
>>> + * \fn ByteStreamBuffer::overflow()
>>> + * \brief Check if the buffer has overflown
>>> + * \return True if the buffer has overflow, false otherwise
>>> + */
>>> +
>>> +void ByteStreamBuffer::setOverflow()
>>> +{
>>> +	if (parent_)
>>> +		parent_->setOverflow();
>>
>> This seems a bit more complex then it needs to be. Is it really needed 
>> to track a parent from a carve out and set overflow if the child tries 
>> to interact with too much data?
>>
>> It is not possible to create a carv out which would create a overflow 
>> and the overflow will be logged from the child right?
> 
> The idea here is that all ByteStreamBuffer operations (read, write and
> carve out) log overflow in the top-level object, allowing users of this
> class to not perform any error checking until the very end. Without
> propagating overflow to the parent, we would need to check each carved
> out buffer independently.
> 
> This being said, I think the ByteStreamBuffer class, while nice as a
> concept, may be overkill. It has a limited set of users, and gets a bit
> in the way by requiring copies of all data. I think we'll revisit it
> when moving value for simple controls inline (creating a union to store
> either the value or the offset in the packet entry), and relax the error
> checks then.

My original intention of the ByteStreamBuffer (in a very distant series)
was that it could directly map to the write/read calls of the IPC
without needing intermediate storage.

I.e. ByteStreamBuffer was just an implementation of a ByteStream
interface, and it could map directly to some ByteStreamIPC implementation.

Effectively serialising would go direct to the 'wire'. However, the
non-linear data storage that has been implemented and the carve-outs I
think will prevent this optimisation that I had planned, so if it is no
longer relevant then so be it.

--
Kieran



>>> +
>>> +	overflow_ = true;
>>> +}
>>> +
>>> +/**
>>> + * \brief Carve out an area of \a size bytes into a new ByteStreamBuffer
>>> + * \param[in] size The size of the newly created memory buffer
>>> + *
>>> + * This method carves out an area of \a size bytes from the buffer into a new
>>> + * ByteStreamBuffer, and returns the new buffer. It operates identically to a
>>> + * read or write access from the point of view of the current buffer, but allows
>>> + * the new buffer to be read or written at a later time after other read or
>>> + * write accesses on the current buffer.
>>> + *
>>> + * \return A newly created ByteStreamBuffer of \a size
>>> + */
>>> +ByteStreamBuffer ByteStreamBuffer::carveOut(size_t size)
>>> +{
>>> +	if (!size_ || overflow_)
>>> +		return ByteStreamBuffer(static_cast<const uint8_t *>(nullptr), 0);
>>> +
>>> +	const uint8_t *curr = read_ ? read_ : write_;
>>> +	if (curr + size > base_ + size_) {
>>> +		LOG(Serialization, Error)
>>> +			<< "Unable to reserve " << size << " bytes";
>>> +		setOverflow();
>>> +
>>> +		return ByteStreamBuffer(static_cast<const uint8_t *>(nullptr), 0);
>>> +	}
>>> +
>>> +	if (read_) {
>>> +		ByteStreamBuffer b(read_, size);
>>> +		b.parent_ = this;
>>> +		read_ += size;
>>> +		return b;
>>> +	} else {
>>> +		ByteStreamBuffer b(write_, size);
>>> +		b.parent_ = this;
>>> +		write_ += size;
>>> +		return b;
>>> +	}
>>> +}
>>> +
>>> +/**
>>> + * \brief Skip \a size bytes from the buffer
>>> + * \param[in] size The number of bytes to skip
>>> + *
>>> + * This method skips the next \a size bytes from the buffer.
>>> + *
>>> + * \return 0 on success, a negative error code otherwise
>>> + * \retval -ENOSPC no more space is available in the managed memory buffer
>>> + */
>>> +int ByteStreamBuffer::skip(size_t size)
>>> +{
>>> +	if (overflow_)
>>> +		return -ENOSPC;
>>
>> I think you should return early on !size.
> 
> This should never happen in practice, right ? Would you return before
> this check, or right after it ? If we return right after it will just be
> an optimization for a case that should never happen.
> 
>>> +
>>> +	const uint8_t *curr = read_ ? read_ : write_;
>>> +	if (curr + size > base_ + size_) {
>>> +		LOG(Serialization, Error)
>>> +			<< "Unable to skip " << size << " bytes";
>>> +		setOverflow();
>>> +
>>> +		return -ENOSPC;
>>> +	}
>>> +
>>> +	if (read_) {
>>> +		read_ += size;
>>> +	} else {
>>> +		memset(write_, 0, size);
>>
>> Do we want to memset her? I think we do but just checking.
> 
> I think we do, otherwise we could leak data over IPC.
> 
>>> +		write_ += size;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>
>> skip() and carveOut() share much code, could they be merged?
> 
> I've read both methods, and I think they don't share that much code.
> There's only the size check, and even that has a different error
> message, and a different return value. I don't think it's worth it, but
> maybe I'm missing something, so please feel free to propose a patch.
> 
>>> +
>>> +/**
>>> + * \fn template<typename T> int ByteStreamBuffer::read(T *t)
>>> + * \brief Read \a size \a data from the managed memory buffer
>>> + * \param[out] t Pointer to the memory containing the read data
>>> + * \return 0 on success, a negative error code otherwise
>>> + * \retval -EACCES attempting to read from a write buffer
>>> + * \retval -ENOSPC no more space is available in the managed memory buffer
>>> + */
>>> +
>>> +/**
>>> + * \fn template<typename T> int ByteStreamBuffer::write(const T *t)
>>> + * \brief Write \a data of \a size to the managed memory buffer
>>> + * \param[in] t The data to write to memory
>>> + * \return 0 on success, a negative error code otherwise
>>> + * \retval -EACCES attempting to write to a read buffer
>>> + * \retval -ENOSPC no more space is available in the managed memory buffer
>>> + */
>>> +
>>> +int ByteStreamBuffer::read(uint8_t *data, size_t size)
>>> +{
>>> +	if (!read_)
>>> +		return -EACCES;
>>> +
>>> +	if (overflow_)
>>> +		return -ENOSPC;
>>> +
>>> +	if (read_ + size > base_ + size_) {
>>> +		LOG(Serialization, Error)
>>> +			<< "Unable to read " << size << " bytes: out of bounds";
>>> +		setOverflow();
>>> +		return -ENOSPC;
>>> +	}
>>> +
>>> +	memcpy(data, read_, size);
>>> +	read_ += size;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int ByteStreamBuffer::write(const uint8_t *data, size_t size)
>>> +{
>>> +	if (!write_)
>>> +		return -EACCES;
>>> +
>>> +	if (overflow_)
>>> +		return -ENOSPC;
>>> +
>>> +	if (write_ + size > base_ + size_) {
>>> +		LOG(Serialization, Error)
>>> +			<< "Unable to write " << size << " bytes: no space left";
>>> +		setOverflow();
>>> +		return -ENOSPC;
>>> +	}
>>> +
>>> +	memcpy(write_, data, size);
>>> +	write_ += size;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h
>>> new file mode 100644
>>> index 000000000000..b5274c62b85e
>>> --- /dev/null
>>> +++ b/src/libcamera/include/byte_stream_buffer.h
>>> @@ -0,0 +1,63 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2019, Google Inc.
>>> + *
>>> + * byte_stream_buffer.h - Byte stream buffer
>>> + */
>>> +#ifndef __LIBCAMERA_BYTE_STREAM_BUFFER_H__
>>> +#define __LIBCAMERA_BYTE_STREAM_BUFFER_H__
>>> +
>>> +#include <stddef.h>
>>> +#include <stdint.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +class ByteStreamBuffer
>>> +{
>>> +public:
>>> +	ByteStreamBuffer(const uint8_t *base, size_t size);
>>> +	ByteStreamBuffer(uint8_t *base, size_t size);
>>> +	ByteStreamBuffer(ByteStreamBuffer &&other);
>>> +	ByteStreamBuffer &operator=(ByteStreamBuffer &&other);
>>> +
>>> +	const uint8_t *base() const { return base_; }
>>> +	uint32_t offset() const { return (write_ ? write_ : read_) - base_; }
>>> +	size_t size() const { return size_; }
>>> +	bool overflow() const { return overflow_; }
>>> +
>>> +	ByteStreamBuffer carveOut(size_t size);
>>> +	int skip(size_t size);
>>> +
>>> +	template<typename T>
>>> +	int read(T *t)
>>> +	{
>>> +		return read(reinterpret_cast<uint8_t *>(t), sizeof(*t));
>>> +	}
>>> +	template<typename T>
>>> +	int write(const T *t)
>>> +	{
>>> +		return write(reinterpret_cast<const uint8_t *>(t), sizeof(*t));
>>> +	}
>>> +
>>> +private:
>>> +	ByteStreamBuffer(const ByteStreamBuffer &other) = delete;
>>> +	ByteStreamBuffer &operator=(const ByteStreamBuffer &other) = delete;
>>> +
>>> +	void setOverflow();
>>> +
>>> +	int read(uint8_t *data, size_t size);
>>> +	int write(const uint8_t *data, size_t size);
>>> +
>>> +	ByteStreamBuffer *parent_;
>>> +
>>> +	const uint8_t *base_;
>>> +	size_t size_;
>>> +	bool overflow_;
>>> +
>>> +	const uint8_t *read_;
>>> +	uint8_t *write_;
>>> +};
>>> +
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_BYTE_STREAM_BUFFER_H__ */
>>> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build
>>> index 64c2155f90cf..1ff0198662cc 100644
>>> --- a/src/libcamera/include/meson.build
>>> +++ b/src/libcamera/include/meson.build
>>> @@ -1,4 +1,5 @@
>>>  libcamera_headers = files([
>>> +    'byte_stream_buffer.h',
>>>      'camera_controls.h',
>>>      'camera_sensor.h',
>>>      'control_validator.h',
>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>> index be0cd53f6f10..dab2d8ad2649 100644
>>> --- a/src/libcamera/meson.build
>>> +++ b/src/libcamera/meson.build
>>> @@ -1,6 +1,7 @@
>>>  libcamera_sources = files([
>>>      'bound_method.cpp',
>>>      'buffer.cpp',
>>> +    'byte_stream_buffer.cpp',
>>>      'camera.cpp',
>>>      'camera_controls.cpp',
>>>      'camera_manager.cpp',
> 

-- 
Regards
--
Kieran