[libcamera-devel] [PATCH 12/15] android: camera_stream: Add methods to get/put buffers

[Kieran Bingham]

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 at 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 at 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 at 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__ */
> 

-- 
Regards
--
Kieran