[libcamera-devel,12/15] android: camera_stream: Add methods to get/put buffers
diff mbox series

Message ID 20201005104649.10812-13-laurent.pinchart@ideasonboard.com
State Rejected
Delegated to: Laurent Pinchart
Headers show
Series
  • CameraStream refactoring
Related show

Commit Message

Laurent Pinchart Oct. 5, 2020, 10:46 a.m. UTC 
From: Jacopo Mondi <jacopo@jmondi.org>

Add two methods to the CameraStream class to get and put FrameBuffer
pointers from the pool of allocated buffers.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_stream.cpp | 35 +++++++++++++++++++++++++++++++++--
 src/android/camera_stream.h   |  4 ++++
 2 files changed, 37 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Oct. 5, 2020, 12:32 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:46PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Add two methods to the CameraStream class to get and put FrameBuffer
> pointers from the pool of allocated buffers.

You may want to explain in the commit message that you're actually
creating a pool of buffers, and what it is used for.

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_stream.cpp | 35 +++++++++++++++++++++++++++++++++--
>  src/android/camera_stream.h   |  4 ++++
>  2 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index dbde1e786300..c4b727eee63e 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -19,21 +19,24 @@ LOG_DECLARE_CATEGORY(HAL);
>  CameraStream::CameraStream(CameraDevice *cameraDev, Type type,
>  			   camera3_stream_t *androidStream, unsigned int index)
>  	: cameraDevice_(cameraDev), type_(type), androidStream_(androidStream),
> -	  index_(index), encoder_(nullptr), allocator_(nullptr)
> +	  index_(index), encoder_(nullptr), allocator_(nullptr), mutex_(nullptr)
>  {
>  	config_ = cameraDevice_->cameraConfiguration();
>  
>  	if (type_ == Type::Internal || type_ == Type::Mapped)
>  		encoder_ = new EncoderLibJpeg();
>  
> -	if (type == Type::Internal)
> +	if (type == Type::Internal) {
>  		allocator_ = new FrameBufferAllocator(cameraDevice_->camera());
> +		mutex_ = new std::mutex();

I'd embed this in the class.

> +	}
>  }
>  
>  CameraStream::~CameraStream()
>  {
>  	delete encoder_;
>  	delete allocator_;
> +	delete mutex_;
>  }
>  
>  const StreamConfiguration &CameraStream::streamConfiguration() const
> @@ -135,3 +138,31 @@ int CameraStream::process(libcamera::FrameBuffer *source, MappedCamera3Buffer *d
>  
>  	return 0;
>  }
> +
> +FrameBuffer *CameraStream::getBuffer()
> +{
> +	if (!allocator_)
> +		return nullptr;
> +
> +	std::lock_guard<std::mutex> locker(*mutex_);
> +
> +	if (buffers_.empty()) {
> +		LOG(HAL, Error) << "Buffer underrun";
> +		return nullptr;
> +	}
> +
> +	FrameBuffer *buffer = buffers_.back();
> +	buffers_.pop_back();
> +
> +	return buffer;
> +}
> +
> +void CameraStream::putBuffer(libcamera::FrameBuffer *buffer)
> +{
> +	if (!allocator_)
> +		return;
> +
> +	std::lock_guard<std::mutex> locker(*mutex_);
> +
> +	buffers_.push_back(buffer);
> +}
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index c6ff79230b7e..b79a97606c60 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -8,6 +8,7 @@
>  #define __ANDROID_CAMERA_STREAM_H__
>  
>  #include <memory>
> +#include <mutex>
>  #include <vector>
>  
>  #include <hardware/camera3.h>
> @@ -127,6 +128,8 @@ public:
>  	int process(libcamera::FrameBuffer *source,
>  		    MappedCamera3Buffer *dest,
>  		    CameraMetadata *metadata);
> +	libcamera::FrameBuffer *getBuffer();
> +	void putBuffer(libcamera::FrameBuffer *buffer);
>  
>  private:
>  	CameraDevice *cameraDevice_;
> @@ -143,6 +146,7 @@ private:
>  	Encoder *encoder_;
>  	libcamera::FrameBufferAllocator *allocator_;
>  	std::vector<libcamera::FrameBuffer *> buffers_;
> +	std::mutex *mutex_;
>  };
>  
>  #endif /* __ANDROID_CAMERA_STREAM__ */
Kieran Bingham Oct. 6, 2020, 12:45 p.m. UTC | #2 
Hi Jacopo,

On 05/10/2020 13:32, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Oct 05, 2020 at 01:46:46PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo@jmondi.org>
>>
>> Add two methods to the CameraStream class to get and put FrameBuffer
>> pointers from the pool of allocated buffers.
> 
> You may want to explain in the commit message that you're actually
> creating a pool of buffers, and what it is used for.
> 
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  src/android/camera_stream.cpp | 35 +++++++++++++++++++++++++++++++++--
>>  src/android/camera_stream.h   |  4 ++++
>>  2 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index dbde1e786300..c4b727eee63e 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -19,21 +19,24 @@ LOG_DECLARE_CATEGORY(HAL);
>>  CameraStream::CameraStream(CameraDevice *cameraDev, Type type,
>>  			   camera3_stream_t *androidStream, unsigned int index)
>>  	: cameraDevice_(cameraDev), type_(type), androidStream_(androidStream),
>> -	  index_(index), encoder_(nullptr), allocator_(nullptr)
>> +	  index_(index), encoder_(nullptr), allocator_(nullptr), mutex_(nullptr)
>>  {
>>  	config_ = cameraDevice_->cameraConfiguration();
>>  
>>  	if (type_ == Type::Internal || type_ == Type::Mapped)
>>  		encoder_ = new EncoderLibJpeg();
>>  
>> -	if (type == Type::Internal)
>> +	if (type == Type::Internal) {
>>  		allocator_ = new FrameBufferAllocator(cameraDevice_->camera());
>> +		mutex_ = new std::mutex();
> 
> I'd embed this in the class.

Agreed.

Or - as it only protects the BufferPool - we could bring back a class
BufferPool(), and have it in there with the allocate/get/put interface ...

</me misses his BufferPool :D >

I digress - A full BufferPool object/interface isn't needed right now ;-)

Anyway, with the mutex as part of the class instead of allocated.

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


> 
>> +	}
>>  }
>>  
>>  CameraStream::~CameraStream()
>>  {
>>  	delete encoder_;
>>  	delete allocator_;
>> +	delete mutex_;
>>  }
>>  
>>  const StreamConfiguration &CameraStream::streamConfiguration() const
>> @@ -135,3 +138,31 @@ int CameraStream::process(libcamera::FrameBuffer *source, MappedCamera3Buffer *d
>>  
>>  	return 0;
>>  }
>> +
>> +FrameBuffer *CameraStream::getBuffer()
>> +{
>> +	if (!allocator_)
>> +		return nullptr;
>> +
>> +	std::lock_guard<std::mutex> locker(*mutex_);
>> +
>> +	if (buffers_.empty()) {
>> +		LOG(HAL, Error) << "Buffer underrun";
>> +		return nullptr;
>> +	}
>> +
>> +	FrameBuffer *buffer = buffers_.back();
>> +	buffers_.pop_back();
>> +
>> +	return buffer;
>> +}
>> +
>> +void CameraStream::putBuffer(libcamera::FrameBuffer *buffer)
>> +{
>> +	if (!allocator_)
>> +		return;
>> +
>> +	std::lock_guard<std::mutex> locker(*mutex_);
>> +
>> +	buffers_.push_back(buffer);
>> +}
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index c6ff79230b7e..b79a97606c60 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -8,6 +8,7 @@
>>  #define __ANDROID_CAMERA_STREAM_H__
>>  
>>  #include <memory>
>> +#include <mutex>
>>  #include <vector>
>>  
>>  #include <hardware/camera3.h>
>> @@ -127,6 +128,8 @@ public:
>>  	int process(libcamera::FrameBuffer *source,
>>  		    MappedCamera3Buffer *dest,
>>  		    CameraMetadata *metadata);
>> +	libcamera::FrameBuffer *getBuffer();
>> +	void putBuffer(libcamera::FrameBuffer *buffer);
>>  
>>  private:
>>  	CameraDevice *cameraDevice_;
>> @@ -143,6 +146,7 @@ private:
>>  	Encoder *encoder_;
>>  	libcamera::FrameBufferAllocator *allocator_;
>>  	std::vector<libcamera::FrameBuffer *> buffers_;
>> +	std::mutex *mutex_;
>>  };
>>  
>>  #endif /* __ANDROID_CAMERA_STREAM__ */
>

Patch
diff mbox series 

diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index dbde1e786300..c4b727eee63e 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -19,21 +19,24 @@  LOG_DECLARE_CATEGORY(HAL);
 CameraStream::CameraStream(CameraDevice *cameraDev, Type type,
 			   camera3_stream_t *androidStream, unsigned int index)
 	: cameraDevice_(cameraDev), type_(type), androidStream_(androidStream),
-	  index_(index), encoder_(nullptr), allocator_(nullptr)
+	  index_(index), encoder_(nullptr), allocator_(nullptr), mutex_(nullptr)
 {
 	config_ = cameraDevice_->cameraConfiguration();
 
 	if (type_ == Type::Internal || type_ == Type::Mapped)
 		encoder_ = new EncoderLibJpeg();
 
-	if (type == Type::Internal)
+	if (type == Type::Internal) {
 		allocator_ = new FrameBufferAllocator(cameraDevice_->camera());
+		mutex_ = new std::mutex();
+	}
 }
 
 CameraStream::~CameraStream()
 {
 	delete encoder_;
 	delete allocator_;
+	delete mutex_;
 }
 
 const StreamConfiguration &CameraStream::streamConfiguration() const
@@ -135,3 +138,31 @@  int CameraStream::process(libcamera::FrameBuffer *source, MappedCamera3Buffer *d
 
 	return 0;
 }
+
+FrameBuffer *CameraStream::getBuffer()
+{
+	if (!allocator_)
+		return nullptr;
+
+	std::lock_guard<std::mutex> locker(*mutex_);
+
+	if (buffers_.empty()) {
+		LOG(HAL, Error) << "Buffer underrun";
+		return nullptr;
+	}
+
+	FrameBuffer *buffer = buffers_.back();
+	buffers_.pop_back();
+
+	return buffer;
+}
+
+void CameraStream::putBuffer(libcamera::FrameBuffer *buffer)
+{
+	if (!allocator_)
+		return;
+
+	std::lock_guard<std::mutex> locker(*mutex_);
+
+	buffers_.push_back(buffer);
+}
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index c6ff79230b7e..b79a97606c60 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -8,6 +8,7 @@ 
 #define __ANDROID_CAMERA_STREAM_H__
 
 #include <memory>
+#include <mutex>
 #include <vector>
 
 #include <hardware/camera3.h>
@@ -127,6 +128,8 @@  public:
 	int process(libcamera::FrameBuffer *source,
 		    MappedCamera3Buffer *dest,
 		    CameraMetadata *metadata);
+	libcamera::FrameBuffer *getBuffer();
+	void putBuffer(libcamera::FrameBuffer *buffer);
 
 private:
 	CameraDevice *cameraDevice_;
@@ -143,6 +146,7 @@  private:
 	Encoder *encoder_;
 	libcamera::FrameBufferAllocator *allocator_;
 	std::vector<libcamera::FrameBuffer *> buffers_;
+	std::mutex *mutex_;
 };
 
 #endif /* __ANDROID_CAMERA_STREAM__ */