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

2310 diff mbox series

From: Jacopo Mondi <jacopo@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@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@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

Hi Jacopo,

Thanks for your work.

On 2019-11-08 22:53:59 +0200, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@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@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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?

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

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

> +		write_ += size;
> +	}
> +
> +	return 0;
> +}

skip() and carveOut() share much code, could they be merged?

> +
> +/**
> + * \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
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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@jmondi.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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',

Hi Laurent, Niklas,

On Mon, Nov 18, 2019 at 08:39:23PM +0200, 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@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@jmondi.org>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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.
>

I agree and if I recall an attempt to store values with binary size <=
4 in the offset section of the serialized packet header it felt a bit
in the way. If it's not a huge work, I would rather drop it. Let me
know if I can help there.

Thanks
   j

> > > +
> > > +	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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Mon, Nov 18, 2019 at 11:59:19PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 18, 2019 at 08:39:23PM +0200, Laurent Pinchart wrote:
> > 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@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@jmondi.org>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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.
> 
> I agree and if I recall an attempt to store values with binary size <=
> 4 in the offset section of the serialized packet header it felt a bit
> in the way. If it's not a huge work, I would rather drop it. Let me
> know if I can help there.

I'm a bit confused, which part would you drop ? :-) Propagating the
overflow to the parent ? I'd prefer dropping it on top of this series,
along with a few other safety checks, in order to allow access to the
memory without any copy. Even better would be a compile-time flag that
would enable extra safety checks, but I'm probably just dreaming :-)

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

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@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@jmondi.org>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@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',
>

Hi Laurent,

On Tue, Nov 19, 2019 at 02:28:02AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Nov 18, 2019 at 11:59:19PM +0100, Jacopo Mondi wrote:
> > On Mon, Nov 18, 2019 at 08:39:23PM +0200, Laurent Pinchart wrote:
> > > 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@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@jmondi.org>
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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.
> >
> > I agree and if I recall an attempt to store values with binary size <=
> > 4 in the offset section of the serialized packet header it felt a bit
> > in the way. If it's not a huge work, I would rather drop it. Let me
> > know if I can help there.
>
> I'm a bit confused, which part would you drop ? :-) Propagating the

I meant the whole ByteStreamBuffer class :)

But then I had a look at how it is used and how it nicely abstract
away read/write implementation details, and then I kind of changed my
mind. I think we'll revist when/if we'll optimize storing values
in the offset section.

Thanks
   j

> overflow to the parent ? I'd prefer dropping it on top of this series,
> along with a few other safety checks, in order to allow access to the
> memory without any copy. Even better would be a compile-time flag that
> would enable extra safety checks, but I'm probably just dreaming :-)
>
> > > > > +
> > > > > +	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