[libcamera-devel,24/31] libcamera: byte_stream_buffer: Add zero-copy read() variant
diff mbox series

Message ID 20200229164254.23604-25-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Add support for array controls
Related show

Commit Message

Laurent Pinchart Feb. 29, 2020, 4:42 p.m. UTC 
Add a read() function to ByteStreamBuffer that returns a pointer to the
data instead of copying it. Overflow check is still handled by the
class, but the caller must check the returned pointer explicitly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/byte_stream_buffer.cpp       | 39 ++++++++++++++++++++++
 src/libcamera/include/byte_stream_buffer.h |  9 +++++
 2 files changed, 48 insertions(+)

Comments

Kieran Bingham March 5, 2020, 4:26 p.m. UTC | #1 
On 29/02/2020 16:42, Laurent Pinchart wrote:
> Add a read() function to ByteStreamBuffer that returns a pointer to the
> data instead of copying it. Overflow check is still handled by the
> class, but the caller must check the returned pointer explicitly.
> 

I'm wondering why this doesn't return a span, but perhaps I'm about to
find out in a later patch if this is to implement some feature for
handling span ...

But still - this byte_stream_buffer.h/cpp knows how to deal with spans
already...

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Minors aside,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/libcamera/byte_stream_buffer.cpp       | 39 ++++++++++++++++++++++
>  src/libcamera/include/byte_stream_buffer.h |  9 +++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp
> index 40380bf0434a..576f556f54a7 100644
> --- a/src/libcamera/byte_stream_buffer.cpp
> +++ b/src/libcamera/byte_stream_buffer.cpp
> @@ -241,6 +241,19 @@ int ByteStreamBuffer::skip(size_t size)
>   * \retval -ENOSPC no more space is available in the managed memory buffer
>   */
>  
> +/**
> + * \fn template<typename T> const T *ByteStreamBuffer::read(size_t count)
> + * \brief Read data from the managed memory buffer without copy

without a copy? or "without performing a copy"

> + * \param[in] count Number of data items to read
> + *
> + * This function read \a count elements of type \a T from the buffer. Unlike


s/read/reads/

> + * the other read variants, it doesn't copy the data but returns a pointer to
> + * the first element. If data can't be read for any reason (usually due to
> + * reading more data than available), the function returns nullptr.

returns 'a' nullptr?

> + *
> + * \return A pointer to the data on success, or nullptr otherwise
> + */
> +
>  /**
>   * \fn template<typename T> int ByteStreamBuffer::write(const T *t)
>   * \brief Write \a t to the managed memory buffer
> @@ -259,6 +272,32 @@ int ByteStreamBuffer::skip(size_t size)
>   * \retval -ENOSPC no more space is available in the managed memory buffer
>   */
>  
> +const uint8_t *ByteStreamBuffer::read(size_t size, size_t count)
> +{
> +	if (!read_)
> +		return nullptr;
> +
> +	if (overflow_)
> +		return nullptr;
> +
> +	size_t bytes;
> +	if (__builtin_mul_overflow(size, count, &bytes)) {

Is this a gcc builtin? or supported in clang too? (I assume it is ...)

> +		setOverflow();
> +		return nullptr;
> +	}
> +
> +	if (read_ + bytes > base_ + size_) {
> +		LOG(Serialization, Error)
> +			<< "Unable to read " << bytes << " bytes: out of bounds";
> +		setOverflow();
> +		return nullptr;
> +	}
> +
> +	const uint8_t *data = read_;
> +	read_ += bytes;
> +	return data;
> +}
> +
>  int ByteStreamBuffer::read(uint8_t *data, size_t size)
>  {
>  	if (!read_)
> diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h
> index 17cb0146061e..b3aaa8b9fb28 100644
> --- a/src/libcamera/include/byte_stream_buffer.h
> +++ b/src/libcamera/include/byte_stream_buffer.h
> @@ -9,6 +9,7 @@
>  
>  #include <stddef.h>
>  #include <stdint.h>
> +#include <type_traits>
>  
>  #include <libcamera/span.h>
>  
> @@ -43,6 +44,13 @@ public:
>  			    data.size_bytes());
>  	}
>  
> +	template<typename T>
> +	const std::remove_reference_t<T> *read(size_t count = 1)
> +	{
> +		using return_type = const std::remove_reference_t<T> *;
> +		return reinterpret_cast<return_type>(read(sizeof(T), count));
> +	}
> +
>  	template<typename T>
>  	int write(const T *t)
>  	{
> @@ -63,6 +71,7 @@ private:
>  	void setOverflow();
>  
>  	int read(uint8_t *data, size_t size);
> +	const uint8_t *read(size_t size, size_t count);
>  	int write(const uint8_t *data, size_t size);
>  
>  	ByteStreamBuffer *parent_;
>
Laurent Pinchart March 6, 2020, 2:54 p.m. UTC | #2 
Hi Kieran,

On Thu, Mar 05, 2020 at 04:26:03PM +0000, Kieran Bingham wrote:
> On 29/02/2020 16:42, Laurent Pinchart wrote:
> > Add a read() function to ByteStreamBuffer that returns a pointer to the
> > data instead of copying it. Overflow check is still handled by the
> > class, but the caller must check the returned pointer explicitly.
> 
> I'm wondering why this doesn't return a span, but perhaps I'm about to
> find out in a later patch if this is to implement some feature for
> handling span ...

The idea is to offer a safe way to access memory in the byte stream
buffer through a typed pointer, without casting explicitly in the caller
as that would be unsafe.

> But still - this byte_stream_buffer.h/cpp knows how to deal with spans
> already...
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Minors aside,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > ---
> >  src/libcamera/byte_stream_buffer.cpp       | 39 ++++++++++++++++++++++
> >  src/libcamera/include/byte_stream_buffer.h |  9 +++++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp
> > index 40380bf0434a..576f556f54a7 100644
> > --- a/src/libcamera/byte_stream_buffer.cpp
> > +++ b/src/libcamera/byte_stream_buffer.cpp
> > @@ -241,6 +241,19 @@ int ByteStreamBuffer::skip(size_t size)
> >   * \retval -ENOSPC no more space is available in the managed memory buffer
> >   */
> >  
> > +/**
> > + * \fn template<typename T> const T *ByteStreamBuffer::read(size_t count)
> > + * \brief Read data from the managed memory buffer without copy
> 
> without a copy? or "without performing a copy"

I've used the second option.

> > + * \param[in] count Number of data items to read
> > + *
> > + * This function read \a count elements of type \a T from the buffer. Unlike
> 
> s/read/reads/
> 
> > + * the other read variants, it doesn't copy the data but returns a pointer to
> > + * the first element. If data can't be read for any reason (usually due to
> > + * reading more data than available), the function returns nullptr.
> 
> returns 'a' nullptr?

'nullptr' is a value, like NULL
(https://en.cppreference.com/w/cpp/language/nullptr), so I think this is
correct.

> 
> > + *
> > + * \return A pointer to the data on success, or nullptr otherwise
> > + */
> > +
> >  /**
> >   * \fn template<typename T> int ByteStreamBuffer::write(const T *t)
> >   * \brief Write \a t to the managed memory buffer
> > @@ -259,6 +272,32 @@ int ByteStreamBuffer::skip(size_t size)
> >   * \retval -ENOSPC no more space is available in the managed memory buffer
> >   */
> >  
> > +const uint8_t *ByteStreamBuffer::read(size_t size, size_t count)
> > +{
> > +	if (!read_)
> > +		return nullptr;
> > +
> > +	if (overflow_)
> > +		return nullptr;
> > +
> > +	size_t bytes;
> > +	if (__builtin_mul_overflow(size, count, &bytes)) {
> 
> Is this a gcc builtin? or supported in clang too? (I assume it is ...)

It's supported by both gcc and clang.

> > +		setOverflow();
> > +		return nullptr;
> > +	}
> > +
> > +	if (read_ + bytes > base_ + size_) {
> > +		LOG(Serialization, Error)
> > +			<< "Unable to read " << bytes << " bytes: out of bounds";
> > +		setOverflow();
> > +		return nullptr;
> > +	}
> > +
> > +	const uint8_t *data = read_;
> > +	read_ += bytes;
> > +	return data;
> > +}
> > +
> >  int ByteStreamBuffer::read(uint8_t *data, size_t size)
> >  {
> >  	if (!read_)
> > diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h
> > index 17cb0146061e..b3aaa8b9fb28 100644
> > --- a/src/libcamera/include/byte_stream_buffer.h
> > +++ b/src/libcamera/include/byte_stream_buffer.h
> > @@ -9,6 +9,7 @@
> >  
> >  #include <stddef.h>
> >  #include <stdint.h>
> > +#include <type_traits>
> >  
> >  #include <libcamera/span.h>
> >  
> > @@ -43,6 +44,13 @@ public:
> >  			    data.size_bytes());
> >  	}
> >  
> > +	template<typename T>
> > +	const std::remove_reference_t<T> *read(size_t count = 1)
> > +	{
> > +		using return_type = const std::remove_reference_t<T> *;
> > +		return reinterpret_cast<return_type>(read(sizeof(T), count));
> > +	}
> > +
> >  	template<typename T>
> >  	int write(const T *t)
> >  	{
> > @@ -63,6 +71,7 @@ private:
> >  	void setOverflow();
> >  
> >  	int read(uint8_t *data, size_t size);
> > +	const uint8_t *read(size_t size, size_t count);
> >  	int write(const uint8_t *data, size_t size);
> >  
> >  	ByteStreamBuffer *parent_;

Patch
diff mbox series 

diff --git a/src/libcamera/byte_stream_buffer.cpp b/src/libcamera/byte_stream_buffer.cpp
index 40380bf0434a..576f556f54a7 100644
--- a/src/libcamera/byte_stream_buffer.cpp
+++ b/src/libcamera/byte_stream_buffer.cpp
@@ -241,6 +241,19 @@  int ByteStreamBuffer::skip(size_t size)
  * \retval -ENOSPC no more space is available in the managed memory buffer
  */
 
+/**
+ * \fn template<typename T> const T *ByteStreamBuffer::read(size_t count)
+ * \brief Read data from the managed memory buffer without copy
+ * \param[in] count Number of data items to read
+ *
+ * This function read \a count elements of type \a T from the buffer. Unlike
+ * the other read variants, it doesn't copy the data but returns a pointer to
+ * the first element. If data can't be read for any reason (usually due to
+ * reading more data than available), the function returns nullptr.
+ *
+ * \return A pointer to the data on success, or nullptr otherwise
+ */
+
 /**
  * \fn template<typename T> int ByteStreamBuffer::write(const T *t)
  * \brief Write \a t to the managed memory buffer
@@ -259,6 +272,32 @@  int ByteStreamBuffer::skip(size_t size)
  * \retval -ENOSPC no more space is available in the managed memory buffer
  */
 
+const uint8_t *ByteStreamBuffer::read(size_t size, size_t count)
+{
+	if (!read_)
+		return nullptr;
+
+	if (overflow_)
+		return nullptr;
+
+	size_t bytes;
+	if (__builtin_mul_overflow(size, count, &bytes)) {
+		setOverflow();
+		return nullptr;
+	}
+
+	if (read_ + bytes > base_ + size_) {
+		LOG(Serialization, Error)
+			<< "Unable to read " << bytes << " bytes: out of bounds";
+		setOverflow();
+		return nullptr;
+	}
+
+	const uint8_t *data = read_;
+	read_ += bytes;
+	return data;
+}
+
 int ByteStreamBuffer::read(uint8_t *data, size_t size)
 {
 	if (!read_)
diff --git a/src/libcamera/include/byte_stream_buffer.h b/src/libcamera/include/byte_stream_buffer.h
index 17cb0146061e..b3aaa8b9fb28 100644
--- a/src/libcamera/include/byte_stream_buffer.h
+++ b/src/libcamera/include/byte_stream_buffer.h
@@ -9,6 +9,7 @@ 
 
 #include <stddef.h>
 #include <stdint.h>
+#include <type_traits>
 
 #include <libcamera/span.h>
 
@@ -43,6 +44,13 @@  public:
 			    data.size_bytes());
 	}
 
+	template<typename T>
+	const std::remove_reference_t<T> *read(size_t count = 1)
+	{
+		using return_type = const std::remove_reference_t<T> *;
+		return reinterpret_cast<return_type>(read(sizeof(T), count));
+	}
+
 	template<typename T>
 	int write(const T *t)
 	{
@@ -63,6 +71,7 @@  private:
 	void setOverflow();
 
 	int read(uint8_t *data, size_t size);
+	const uint8_t *read(size_t size, size_t count);
 	int write(const uint8_t *data, size_t size);
 
 	ByteStreamBuffer *parent_;