[libcamera-devel] [PATCH 2/9] libcamera: stream: Add Stream memory type

[Kieran Bingham]

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 at jmondi.org>
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran