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

[Laurent Pinchart]

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.

> > +
> > +	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,

Laurent Pinchart