[libcamera-devel,2/9] libcamera: stream: Add Stream memory type
diff mbox series

Message ID 20190704225334.26170-3-jacopo@jmondi.org
State Accepted
Headers show
Series
  • Add support for external bufferes
Related show

Commit Message

Jacopo Mondi July 4, 2019, 10:53 p.m. UTC 
Define the memory type a Stream uses and allow application to set it
through the associated StreamConfiguration.

A Stream can use either internal or external memory allocation methods,
depending on where the data produced by the stream is actually saved.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/stream.h |  8 ++++++++
 src/libcamera/camera.cpp   |  1 +
 src/libcamera/stream.cpp   | 30 ++++++++++++++++++++++++++++--
 3 files changed, 37 insertions(+), 2 deletions(-)

Comments

Niklas Söderlund July 6, 2019, 11:40 a.m. UTC | #1 
Hi Jacopo,

Thanks for your patch.

On 2019-07-05 00:53:27 +0200, Jacopo Mondi wrote:
> Define the memory type a Stream uses and allow application to set it
> through the associated StreamConfiguration.
> 
> A Stream can use either internal or external memory allocation methods,
> depending on where the data produced by the stream is actually saved.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/libcamera/stream.h |  8 ++++++++
>  src/libcamera/camera.cpp   |  1 +
>  src/libcamera/stream.cpp   | 30 ++++++++++++++++++++++++++++--
>  3 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index fa7d6ba4987c..796f1aff2602 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -34,6 +34,11 @@ private:
>  	std::map<unsigned int, std::vector<SizeRange>> formats_;
>  };
>  
> +enum MemoryType {
> +	InternalMemory,
> +	ExternalMemory,
> +};
> +
>  struct StreamConfiguration {
>  	StreamConfiguration();
>  	StreamConfiguration(const StreamFormats &formats);
> @@ -41,6 +46,7 @@ struct StreamConfiguration {
>  	unsigned int pixelFormat;
>  	Size size;
>  
> +	MemoryType memoryType;
>  	unsigned int bufferCount;
>  
>  	Stream *stream() const { return stream_; }
> @@ -77,6 +83,7 @@ public:
>  	std::vector<Buffer> &buffers() { return bufferPool_.buffers(); }
>  	unsigned int bufferCount() const { return bufferPool_.count(); }
>  	const StreamConfiguration &configuration() const { return configuration_; }
> +	MemoryType memoryType() const { return memoryType_; }
>  
>  protected:
>  	friend class Camera;
> @@ -86,6 +93,7 @@ protected:
>  
>  	BufferPool bufferPool_;
>  	StreamConfiguration configuration_;
> +	MemoryType memoryType_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 088a39623e36..5f756d41744a 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -683,6 +683,7 @@ int Camera::configure(CameraConfiguration *config)
>  		 * Allocate buffer objects in the pool.
>  		 * Memory will be allocated and assigned later.
>  		 */
> +		stream->memoryType_ = cfg.memoryType;
>  		stream->createBuffers(cfg.bufferCount);
>  	}
>  
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index 35197be09c26..97e0f429c9fb 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -263,6 +263,16 @@ SizeRange StreamFormats::range(unsigned int pixelformat) const
>  	return range;
>  }
>  
> +/**
> + * \enum MemoryType
> + * \brief Define the memory type used by a Stream
> + * \var MemoryType::InternalMemory
> + * The Stream uses memory allocated internally to the library and export that
> + * to applications.
> + * \var MemoryType::ExternalMemory
> + * The Stream uses buffers whose memory is allocated outside from the library.
> + */
> +
>  /**
>   * \struct StreamConfiguration
>   * \brief Configuration parameters for a stream
> @@ -276,7 +286,7 @@ SizeRange StreamFormats::range(unsigned int pixelformat) const
>   * handlers provied StreamFormats.
>   */
>  StreamConfiguration::StreamConfiguration()
> -	: stream_(nullptr)
> +	: memoryType(InternalMemory), stream_(nullptr)
>  {
>  }
>  
> @@ -284,7 +294,7 @@ StreamConfiguration::StreamConfiguration()
>   * \brief Construct a configuration with stream formats
>   */
>  StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> -	: stream_(nullptr), formats_(formats)
> +	: memoryType(InternalMemory), stream_(nullptr), formats_(formats)
>  {
>  }
>  
> @@ -301,6 +311,11 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
>   * format described in V4L2 using the V4L2_PIX_FMT_* definitions.
>   */
>  
> +/**
> + * \var StreamConfiguration::memoryType
> + * \brief The memory type the stream shall use
> + */
> +
>  /**
>   * \var StreamConfiguration::bufferCount
>   * \brief Requested number of buffers to allocate for the stream
> @@ -436,6 +451,12 @@ Stream::Stream()
>   * \return The active configuration of the stream
>   */
>  
> +/**
> + * \fn Stream::memoryType()
> + * \brief Retrieve the stream memory type
> + * \return The memory type used by the stream
> + */
> +
>  /**
>   * \brief Create buffers for the stream
>   * \param count The number of buffers to create
> @@ -476,4 +497,9 @@ void Stream::destroyBuffers()
>   * next call to Camera::configure() regardless of if it includes the stream.
>   */
>  
> +/**
> + * \var Stream::memoryType_
> + * \brief The stream memory type
> + */
> +
>  } /* namespace libcamera */
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham July 8, 2019, 10:23 a.m. UTC | #2 
Hi Jacopo,

On 06/07/2019 12:40, Niklas Söderlund wrote:
> Hi Jacopo,
> 
> Thanks for your patch.
> 
> On 2019-07-05 00:53:27 +0200, Jacopo Mondi wrote:
>> Define the memory type a Stream uses and allow application to set it
>> through the associated StreamConfiguration.
>>
>> A Stream can use either internal or external memory allocation methods,
>> depending on where the data produced by the stream is actually saved.

I can't get it out of my head that the external buffer pool should be
registered with the stream, so I'm commenting below with that assumption
in my head... but that is still up for debate / discussion ... so don't
take my comments as "This should be changed" - it's just discussion
points ...


>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
>> ---
>>  include/libcamera/stream.h |  8 ++++++++
>>  src/libcamera/camera.cpp   |  1 +
>>  src/libcamera/stream.cpp   | 30 ++++++++++++++++++++++++++++--
>>  3 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
>> index fa7d6ba4987c..796f1aff2602 100644
>> --- a/include/libcamera/stream.h
>> +++ b/include/libcamera/stream.h
>> @@ -34,6 +34,11 @@ private:
>>  	std::map<unsigned int, std::vector<SizeRange>> formats_;
>>  };
>>  
>> +enum MemoryType {
>> +	InternalMemory,
>> +	ExternalMemory,
>> +};
>> +
>>  struct StreamConfiguration {
>>  	StreamConfiguration();
>>  	StreamConfiguration(const StreamFormats &formats);
>> @@ -41,6 +46,7 @@ struct StreamConfiguration {
>>  	unsigned int pixelFormat;
>>  	Size size;
>>  
>> +	MemoryType memoryType;
>>  	unsigned int bufferCount;
>>  
>>  	Stream *stream() const { return stream_; }
>> @@ -77,6 +83,7 @@ public:
>>  	std::vector<Buffer> &buffers() { return bufferPool_.buffers(); }
>>  	unsigned int bufferCount() const { return bufferPool_.count(); }
>>  	const StreamConfiguration &configuration() const { return configuration_; }
>> +	MemoryType memoryType() const { return memoryType_; }
>>  
>>  protected:
>>  	friend class Camera;
>> @@ -86,6 +93,7 @@ protected:
>>  
>>  	BufferPool bufferPool_;
>>  	StreamConfiguration configuration_;
>> +	MemoryType memoryType_;
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>> index 088a39623e36..5f756d41744a 100644
>> --- a/src/libcamera/camera.cpp
>> +++ b/src/libcamera/camera.cpp
>> @@ -683,6 +683,7 @@ int Camera::configure(CameraConfiguration *config)
>>  		 * Allocate buffer objects in the pool.
>>  		 * Memory will be allocated and assigned later.
>>  		 */
>> +		stream->memoryType_ = cfg.memoryType;
>>  		stream->createBuffers(cfg.bufferCount);

So we do a stream->createBuffers() in Camera::configure. ... so that
means external BufferPool would have to be created before this. But I
don't think that's a problem ...?


This createBuffers call would of course then be dependant upon whether
we are internal or external allocation...

So if an external buffer pool were to be imported, it would have to be
/before/ Camera::configure() which might feel a bit awkward ... or
perhaps not ...



>>  	}
>>  
>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
>> index 35197be09c26..97e0f429c9fb 100644
>> --- a/src/libcamera/stream.cpp
>> +++ b/src/libcamera/stream.cpp
>> @@ -263,6 +263,16 @@ SizeRange StreamFormats::range(unsigned int pixelformat) const
>>  	return range;
>>  }
>>  
>> +/**
>> + * \enum MemoryType
>> + * \brief Define the memory type used by a Stream
>> + * \var MemoryType::InternalMemory
>> + * The Stream uses memory allocated internally to the library and export that
>> + * to applications.
>> + * \var MemoryType::ExternalMemory
>> + * The Stream uses buffers whose memory is allocated outside from the library.
>> + */
>> +
>>  /**
>>   * \struct StreamConfiguration
>>   * \brief Configuration parameters for a stream
>> @@ -276,7 +286,7 @@ SizeRange StreamFormats::range(unsigned int pixelformat) const
>>   * handlers provied StreamFormats.
>>   */
>>  StreamConfiguration::StreamConfiguration()
>> -	: stream_(nullptr)
>> +	: memoryType(InternalMemory), stream_(nullptr)
>>  {
>>  }
>>  
>> @@ -284,7 +294,7 @@ StreamConfiguration::StreamConfiguration()
>>   * \brief Construct a configuration with stream formats
>>   */
>>  StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
>> -	: stream_(nullptr), formats_(formats)
>> +	: memoryType(InternalMemory), stream_(nullptr), formats_(formats)
>>  {
>>  }
>>  
>> @@ -301,6 +311,11 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
>>   * format described in V4L2 using the V4L2_PIX_FMT_* definitions.
>>   */
>>  
>> +/**
>> + * \var StreamConfiguration::memoryType
>> + * \brief The memory type the stream shall use
>> + */
>> +
>>  /**
>>   * \var StreamConfiguration::bufferCount
>>   * \brief Requested number of buffers to allocate for the stream
>> @@ -436,6 +451,12 @@ Stream::Stream()
>>   * \return The active configuration of the stream
>>   */
>>  
>> +/**
>> + * \fn Stream::memoryType()
>> + * \brief Retrieve the stream memory type
>> + * \return The memory type used by the stream
>> + */
>> +
>>  /**
>>   * \brief Create buffers for the stream
>>   * \param count The number of buffers to create
>> @@ -476,4 +497,9 @@ void Stream::destroyBuffers()
>>   * next call to Camera::configure() regardless of if it includes the stream.
>>   */
>>  
>> +/**
>> + * \var Stream::memoryType_
>> + * \brief The stream memory type
>> + */
>> +
>>  } /* namespace libcamera */
>> -- 
>> 2.21.0
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>

Patch
diff mbox series 

diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
index fa7d6ba4987c..796f1aff2602 100644
--- a/include/libcamera/stream.h
+++ b/include/libcamera/stream.h
@@ -34,6 +34,11 @@  private:
 	std::map<unsigned int, std::vector<SizeRange>> formats_;
 };
 
+enum MemoryType {
+	InternalMemory,
+	ExternalMemory,
+};
+
 struct StreamConfiguration {
 	StreamConfiguration();
 	StreamConfiguration(const StreamFormats &formats);
@@ -41,6 +46,7 @@  struct StreamConfiguration {
 	unsigned int pixelFormat;
 	Size size;
 
+	MemoryType memoryType;
 	unsigned int bufferCount;
 
 	Stream *stream() const { return stream_; }
@@ -77,6 +83,7 @@  public:
 	std::vector<Buffer> &buffers() { return bufferPool_.buffers(); }
 	unsigned int bufferCount() const { return bufferPool_.count(); }
 	const StreamConfiguration &configuration() const { return configuration_; }
+	MemoryType memoryType() const { return memoryType_; }
 
 protected:
 	friend class Camera;
@@ -86,6 +93,7 @@  protected:
 
 	BufferPool bufferPool_;
 	StreamConfiguration configuration_;
+	MemoryType memoryType_;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 088a39623e36..5f756d41744a 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -683,6 +683,7 @@  int Camera::configure(CameraConfiguration *config)
 		 * Allocate buffer objects in the pool.
 		 * Memory will be allocated and assigned later.
 		 */
+		stream->memoryType_ = cfg.memoryType;
 		stream->createBuffers(cfg.bufferCount);
 	}
 
diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
index 35197be09c26..97e0f429c9fb 100644
--- a/src/libcamera/stream.cpp
+++ b/src/libcamera/stream.cpp
@@ -263,6 +263,16 @@  SizeRange StreamFormats::range(unsigned int pixelformat) const
 	return range;
 }
 
+/**
+ * \enum MemoryType
+ * \brief Define the memory type used by a Stream
+ * \var MemoryType::InternalMemory
+ * The Stream uses memory allocated internally to the library and export that
+ * to applications.
+ * \var MemoryType::ExternalMemory
+ * The Stream uses buffers whose memory is allocated outside from the library.
+ */
+
 /**
  * \struct StreamConfiguration
  * \brief Configuration parameters for a stream
@@ -276,7 +286,7 @@  SizeRange StreamFormats::range(unsigned int pixelformat) const
  * handlers provied StreamFormats.
  */
 StreamConfiguration::StreamConfiguration()
-	: stream_(nullptr)
+	: memoryType(InternalMemory), stream_(nullptr)
 {
 }
 
@@ -284,7 +294,7 @@  StreamConfiguration::StreamConfiguration()
  * \brief Construct a configuration with stream formats
  */
 StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
-	: stream_(nullptr), formats_(formats)
+	: memoryType(InternalMemory), stream_(nullptr), formats_(formats)
 {
 }
 
@@ -301,6 +311,11 @@  StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
  * format described in V4L2 using the V4L2_PIX_FMT_* definitions.
  */
 
+/**
+ * \var StreamConfiguration::memoryType
+ * \brief The memory type the stream shall use
+ */
+
 /**
  * \var StreamConfiguration::bufferCount
  * \brief Requested number of buffers to allocate for the stream
@@ -436,6 +451,12 @@  Stream::Stream()
  * \return The active configuration of the stream
  */
 
+/**
+ * \fn Stream::memoryType()
+ * \brief Retrieve the stream memory type
+ * \return The memory type used by the stream
+ */
+
 /**
  * \brief Create buffers for the stream
  * \param count The number of buffers to create
@@ -476,4 +497,9 @@  void Stream::destroyBuffers()
  * next call to Camera::configure() regardless of if it includes the stream.
  */
 
+/**
+ * \var Stream::memoryType_
+ * \brief The stream memory type
+ */
+
 } /* namespace libcamera */