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