[libcamera-devel] [PATCH v2 14/24] libcamera: Add ByteStreamBuffer
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Nov 18 19:13:32 CET 2019
Hi Jacopo,
Thanks for your work.
On 2019-11-08 22:53:59 +0200, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo at jmondi.org>
>
> The ByteStreamBuffer class wraps a memory area, expected to be allocated
> by the user of the class and provides operations to perform sequential
> access in read and write modes.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/libcamera/byte_stream_buffer.cpp | 286 +++++++++++++++++++++
> src/libcamera/include/byte_stream_buffer.h | 63 +++++
> src/libcamera/include/meson.build | 1 +
> src/libcamera/meson.build | 1 +
> 4 files changed, 351 insertions(+)
> create mode 100644 src/libcamera/byte_stream_buffer.cpp
> create mode 100644 src/libcamera/include/byte_stream_buffer.h
>
> diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp
> new file mode 100644
> index 000000000000..23d624dd0a06
> --- /dev/null
> +++ b/src/libcamera/byte_stream_buffer.cpp
> @@ -0,0 +1,286 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * byte_stream_buffer.cpp - Byte stream buffer
> + */
> +
> +#include "byte_stream_buffer.h"
> +
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "log.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Serialization);
> +
> +/**
> + * \file byte_stream_buffer.h
> + * \brief Managed memory container for serialized data
> + */
> +
> +/**
> + * \class ByteStreamBuffer
> + * \brief Wrap a memory buffer and provide sequential data read and write
> + *
> + * The ByteStreamBuffer class wraps a memory buffer and exposes sequential read
> + * and write operation with integrated boundary checks. Access beyond the end
> + * of the buffer are blocked and logged, allowing error checks to take place at
> + * the of of access operations instead of at each access. This simplifies
> + * serialization and deserialization of data.
> + *
> + * A byte stream buffer is created with a base memory pointer and a size. If the
> + * memory pointer is const, the buffer operates in read-only mode, and write
> + * operations are denied. Otherwise the buffer operates in write-only mode, and
> + * read operations are denied.
> + *
> + * Once a buffer is created, data is read or written with read() and write()
> + * respectively. Access is strictly sequential, the buffer keeps track of the
> + * current access location and advances it automatically. Reading or writing
> + * the same location multiple times is thus not possible. Bytes may also be
> + * skipped with the skip() method.
> + *
> + * The ByteStreamBuffer also supports carving out pieces of memory into other
> + * ByteStreamBuffer instances. Like a read or write operation, a carveOut()
> + * advances the internal access location, but allows the carved out memory to
> + * be accessed at a later time.
> + *
> + * All accesses beyond the end of the buffer (read, write, skip or carve out)
> + * are blocked. The first of such accesses causes a message to be logged, and
> + * the buffer being marked as having overflown. If the buffer has been carved
> + * out from a parent buffer, the parent buffer is also marked as having
> + * overflown. Any later access on an overflown buffer is blocked. The buffer
> + * overflow status can be checked with the overflow() method.
> + */
> +
> +/**
> + * \brief Construct a read ByteStreamBuffer from the memory area \a base
> + * of \a size
> + * \param[in] base The address of the memory area to wrap
> + * \param[in] size The size of the memory area to wrap
> + */
> +ByteStreamBuffer::ByteStreamBuffer(const uint8_t *base, size_t size)
> + : parent_(nullptr), base_(base), size_(size), overflow_(false),
> + read_(base), write_(nullptr)
> +{
> +}
> +
> +/**
> + * \brief Construct a write ByteStreamBuffer from the memory area \a base
> + * of \a size
> + * \param[in] base The address of the memory area to wrap
> + * \param[in] size The size of the memory area to wrap
> + */
> +ByteStreamBuffer::ByteStreamBuffer(uint8_t *base, size_t size)
> + : parent_(nullptr), base_(base), size_(size), overflow_(false),
> + read_(nullptr), write_(base)
> +{
> +}
> +
> +/**
> + * \brief Construct a ByteStreamBuffer from the contents of \a other using move
> + * semantics
> + * \param[in] other The other buffer
> + *
> + * After the move construction the \a other buffer is invalidated. Any attempt
> + * to access its contents will be considered as an overflow.
> + */
> +ByteStreamBuffer::ByteStreamBuffer(ByteStreamBuffer &&other)
> +{
> + *this = std::move(other);
> +}
> +
> +/**
> + * \brief Replace the contents of the buffer with those of \a other using move
> + * semantics
> + * \param[in] other The other buffer
> + *
> + * After the assignment the \a other buffer is invalidated. Any attempt to
> + * access its contents will be considered as an overflow.
> + */
> +ByteStreamBuffer &ByteStreamBuffer::operator=(ByteStreamBuffer &&other)
> +{
> + parent_ = other.parent_;
> + base_ = other.base_;
> + size_ = other.size_;
> + overflow_ = other.overflow_;
> + read_ = other.read_;
> + write_ = other.write_;
> +
> + other.parent_ = nullptr;
> + other.base_ = nullptr;
> + other.size_ = 0;
> + other.overflow_ = false;
> + other.read_ = nullptr;
> + other.write_ = nullptr;
> +
> + return *this;
> +}
> +
> +/**
> + * \fn ByteStreamBuffer::base()
> + * \brief Retrieve a pointer to the start location of the managed memory buffer
> + * \return A pointer to the managed memory buffer
> + */
> +
> +/**
> + * \fn ByteStreamBuffer::offset()
> + * \brief Retrieve the offset of the current access location from the base
> + * \return The offset in bytes
> + */
> +
> +/**
> + * \fn ByteStreamBuffer::size()
> + * \brief Retrieve the size of the managed memory buffer
> + * \return The size of managed memory buffer
> + */
> +
> +/**
> + * \fn ByteStreamBuffer::overflow()
> + * \brief Check if the buffer has overflown
> + * \return True if the buffer has overflow, false otherwise
> + */
> +
> +void ByteStreamBuffer::setOverflow()
> +{
> + if (parent_)
> + parent_->setOverflow();
This seems a bit more complex then it needs to be. Is it really needed
to track a parent from a carve out and set overflow if the child tries
to interact with too much data?
It is not possible to create a carv out which would create a overflow
and the overflow will be logged from the child right?
> +
> + 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list