[libcamera-devel,v3,5/8] android: camera_device: Allocate buffer pools

Message ID 20200922094738.5327-6-jacopo@jmondi.org
State Superseded
Headers show
Series
  • android: camera_device: Add support for internal buffers
Related show

Commit Message

Jacopo Mondi Sept. 22, 2020, 9:47 a.m. UTC
After the Camera has been configured, walk the list of collected
CameraStream instances and allocate memory for the ones that needs
intermediate buffers reserved by the libcamera FrameBufferAllocator.

Maintain a map between each Stream and a vector of pointers to the
associated buffers.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 36 +++++++++++++++++++++++++++++++++++
 src/android/camera_device.h   |  6 ++++++
 2 files changed, 42 insertions(+)

Comments

Kieran Bingham Sept. 23, 2020, 12:28 p.m. UTC | #1
Hi Jacopo,

On 22/09/2020 10:47, Jacopo Mondi wrote:
> After the Camera has been configured, walk the list of collected
> CameraStream instances and allocate memory for the ones that needs
> intermediate buffers reserved by the libcamera FrameBufferAllocator.
> 
> Maintain a map between each Stream and a vector of pointers to the
> associated buffers.
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 36 +++++++++++++++++++++++++++++++++++
>  src/android/camera_device.h   |  6 ++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 42fb9ea4e113..f96ea7321a67 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1189,6 +1189,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	streams_.clear();
>  	streams_.reserve(stream_list->num_streams);
>  	allocator_.clear();
> +	bufferPool_.clear();
>  
>  	/* First handle all non-MJPEG streams. */
>  	camera3_stream_t *jpegStream = nullptr;
> @@ -1336,6 +1337,25 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return ret;
>  	}
>  
> +	/*
> +	 * Now that the camera has been configured allocate buffers for
> +	 * the streams that need memory reserved by libcamera.
> +	 */
> +	for (const CameraStream &cameraStream : streams_) {
> +		const StreamConfiguration &cfg = config_->at(cameraStream.index());
> +		Stream *stream = cfg.stream();
> +
> +		if (cameraStream.type() != CameraStream::Type::Internal)
> +			continue;
> +
> +		ret = allocateBuffersPool(stream);
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to allocate buffers for stream "
> +					<< cameraStream.index();
> +			return ret;
> +		}
> +	}
> +

Some how I thought I envisaged the ownership of these buffers being 'in'
the CameraStream class ... but I think I can see that it might be
difficult to map that.

I'll try to think some more, and I'm sure things will be more clear as I
go through the other patches.


>  	return 0;
>  }
>  
> @@ -1369,6 +1389,22 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>  	return new FrameBuffer(std::move(planes));
>  }
>  
> +int CameraDevice::allocateBuffersPool(Stream *stream)

s/Buffers/Buffer/ ... Pool is already plural, so it sounds really odd to
have a Buffers Pool.

(you can have a pool of buffers, but not a buffers pool - It's just a
buffer pool).


> +{
> +	int ret = allocator_.allocate(stream);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Save a pointer to the reserved frame buffer for usage in
> +	 * the HAL.
> +	 */
> +	for (const auto &frameBuffer : allocator_.buffers(stream))
> +		bufferPool_[stream].push_back(frameBuffer.get());
> +
> +	return 0;
> +}
> +
>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
>  	if (!camera3Request->num_output_buffers) {
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 84f636f7a93c..4cef34c01a49 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -166,6 +166,9 @@ protected:
>  	std::string logPrefix() const override;
>  
>  private:
> +	using FrameBufferPool = std::map<libcamera::Stream *,
> +					 std::vector<libcamera::FrameBuffer *>>;
> +

Putting these 'in' the CameraStream would mean we don't need to keep a
mapping of it?

Hrm ... time to read further forwards...


>  	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
>  
>  	struct Camera3RequestDescriptor {
> @@ -194,6 +197,8 @@ private:
>  
>  	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> +	int allocateBuffersPool(libcamera::Stream *stream);
> +
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
>  	CameraMetadata *requestTemplatePreview();
> @@ -208,6 +213,7 @@ private:
>  	std::shared_ptr<libcamera::Camera> camera_;
>  	std::unique_ptr<libcamera::CameraConfiguration> config_;
>  	libcamera::FrameBufferAllocator allocator_;
> +	FrameBufferPool bufferPool_;
>  
>  	CameraMetadata *staticMetadata_;
>  	std::map<unsigned int, const CameraMetadata *> requestTemplates_;
>
Jacopo Mondi Sept. 23, 2020, 1:21 p.m. UTC | #2
Hi Kieran

On Wed, Sep 23, 2020 at 01:28:26PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 22/09/2020 10:47, Jacopo Mondi wrote:
> > After the Camera has been configured, walk the list of collected
> > CameraStream instances and allocate memory for the ones that needs
> > intermediate buffers reserved by the libcamera FrameBufferAllocator.
> >
> > Maintain a map between each Stream and a vector of pointers to the
> > associated buffers.
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 36 +++++++++++++++++++++++++++++++++++
> >  src/android/camera_device.h   |  6 ++++++
> >  2 files changed, 42 insertions(+)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 42fb9ea4e113..f96ea7321a67 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1189,6 +1189,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  	streams_.clear();
> >  	streams_.reserve(stream_list->num_streams);
> >  	allocator_.clear();
> > +	bufferPool_.clear();
> >
> >  	/* First handle all non-MJPEG streams. */
> >  	camera3_stream_t *jpegStream = nullptr;
> > @@ -1336,6 +1337,25 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		return ret;
> >  	}
> >
> > +	/*
> > +	 * Now that the camera has been configured allocate buffers for
> > +	 * the streams that need memory reserved by libcamera.
> > +	 */
> > +	for (const CameraStream &cameraStream : streams_) {
> > +		const StreamConfiguration &cfg = config_->at(cameraStream.index());
> > +		Stream *stream = cfg.stream();
> > +
> > +		if (cameraStream.type() != CameraStream::Type::Internal)
> > +			continue;
> > +
> > +		ret = allocateBuffersPool(stream);
> > +		if (ret) {
> > +			LOG(HAL, Error) << "Failed to allocate buffers for stream "
> > +					<< cameraStream.index();
> > +			return ret;
> > +		}
> > +	}
> > +
>
> Some how I thought I envisaged the ownership of these buffers being 'in'
> the CameraStream class ... but I think I can see that it might be
> difficult to map that.
>
> I'll try to think some more, and I'm sure things will be more clear as I
> go through the other patches.

I considered that. However, the allocator is global to the camera
device which manages its creation and celeanup, so it kind of made
sense to me to make the pool a camera device feature. I also would
have had to pass the allocator around, but that's probably not that
bad, as it is required for allocation only.

>
>
> >  	return 0;
> >  }
> >
> > @@ -1369,6 +1389,22 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
> >  	return new FrameBuffer(std::move(planes));
> >  }
> >
> > +int CameraDevice::allocateBuffersPool(Stream *stream)
>
> s/Buffers/Buffer/ ... Pool is already plural, so it sounds really odd to
> have a Buffers Pool.
>
> (you can have a pool of buffers, but not a buffers pool - It's just a
> buffer pool).
>

Ah thanks, I was actually not sure about this

>
> > +{
> > +	int ret = allocator_.allocate(stream);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/*
> > +	 * Save a pointer to the reserved frame buffer for usage in
> > +	 * the HAL.
> > +	 */
> > +	for (const auto &frameBuffer : allocator_.buffers(stream))
> > +		bufferPool_[stream].push_back(frameBuffer.get());
> > +
> > +	return 0;
> > +}
> > +
> >  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> >  {
> >  	if (!camera3Request->num_output_buffers) {
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 84f636f7a93c..4cef34c01a49 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -166,6 +166,9 @@ protected:
> >  	std::string logPrefix() const override;
> >
> >  private:
> > +	using FrameBufferPool = std::map<libcamera::Stream *,
> > +					 std::vector<libcamera::FrameBuffer *>>;
> > +
>
> Putting these 'in' the CameraStream would mean we don't need to keep a
> mapping of it?

We would save a map and store the vectors in each CameraStream, yes.

>
> Hrm ... time to read further forwards...
>
>
> >  	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
> >
> >  	struct Camera3RequestDescriptor {
> > @@ -194,6 +197,8 @@ private:
> >
> >  	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> >  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> > +	int allocateBuffersPool(libcamera::Stream *stream);
> > +
> >  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> >  	CameraMetadata *requestTemplatePreview();
> > @@ -208,6 +213,7 @@ private:
> >  	std::shared_ptr<libcamera::Camera> camera_;
> >  	std::unique_ptr<libcamera::CameraConfiguration> config_;
> >  	libcamera::FrameBufferAllocator allocator_;
> > +	FrameBufferPool bufferPool_;
> >
> >  	CameraMetadata *staticMetadata_;
> >  	std::map<unsigned int, const CameraMetadata *> requestTemplates_;
> >
>
> --
> Regards
> --
> Kieran
Kieran Bingham Sept. 24, 2020, 4:25 p.m. UTC | #3
Hi Jacopo,

On 23/09/2020 14:21, Jacopo Mondi wrote:
> Hi Kieran
> 
> On Wed, Sep 23, 2020 at 01:28:26PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 22/09/2020 10:47, Jacopo Mondi wrote:
>>> After the Camera has been configured, walk the list of collected
>>> CameraStream instances and allocate memory for the ones that needs
>>> intermediate buffers reserved by the libcamera FrameBufferAllocator.
>>>
>>> Maintain a map between each Stream and a vector of pointers to the
>>> associated buffers.
>>>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  src/android/camera_device.cpp | 36 +++++++++++++++++++++++++++++++++++
>>>  src/android/camera_device.h   |  6 ++++++
>>>  2 files changed, 42 insertions(+)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index 42fb9ea4e113..f96ea7321a67 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -1189,6 +1189,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>>  	streams_.clear();
>>>  	streams_.reserve(stream_list->num_streams);
>>>  	allocator_.clear();
>>> +	bufferPool_.clear();
>>>
>>>  	/* First handle all non-MJPEG streams. */
>>>  	camera3_stream_t *jpegStream = nullptr;
>>> @@ -1336,6 +1337,25 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>>  		return ret;
>>>  	}
>>>
>>> +	/*
>>> +	 * Now that the camera has been configured allocate buffers for
>>> +	 * the streams that need memory reserved by libcamera.
>>> +	 */
>>> +	for (const CameraStream &cameraStream : streams_) {
>>> +		const StreamConfiguration &cfg = config_->at(cameraStream.index());
>>> +		Stream *stream = cfg.stream();
>>> +
>>> +		if (cameraStream.type() != CameraStream::Type::Internal)
>>> +			continue;
>>> +
>>> +		ret = allocateBuffersPool(stream);
>>> +		if (ret) {
>>> +			LOG(HAL, Error) << "Failed to allocate buffers for stream "
>>> +					<< cameraStream.index();
>>> +			return ret;
>>> +		}
>>> +	}
>>> +
>>
>> Some how I thought I envisaged the ownership of these buffers being 'in'
>> the CameraStream class ... but I think I can see that it might be
>> difficult to map that.
>>
>> I'll try to think some more, and I'm sure things will be more clear as I
>> go through the other patches.
> 
> I considered that. However, the allocator is global to the camera
> device which manages its creation and celeanup, so it kind of made
> sense to me to make the pool a camera device feature. I also would
> have had to pass the allocator around, but that's probably not that
> bad, as it is required for allocation only.

Not necessarily, The code here could push the buffers directly into the
CameraStream buffer pool or buffer vector or such (transferring ownership).

Or the future bufferPool could also store a pointer to the Allocator
indeed, to be able to return / free them when destroyed etc..

>>
>>>  	return 0;
>>>  }
>>>
>>> @@ -1369,6 +1389,22 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>>>  	return new FrameBuffer(std::move(planes));
>>>  }
>>>
>>> +int CameraDevice::allocateBuffersPool(Stream *stream)
>>
>> s/Buffers/Buffer/ ... Pool is already plural, so it sounds really odd to
>> have a Buffers Pool.
>>
>> (you can have a pool of buffers, but not a buffers pool - It's just a
>> buffer pool).
>>
> 
> Ah thanks, I was actually not sure about this
> 
>>
>>> +{
>>> +	int ret = allocator_.allocate(stream);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	/*
>>> +	 * Save a pointer to the reserved frame buffer for usage in
>>> +	 * the HAL.
>>> +	 */
>>> +	for (const auto &frameBuffer : allocator_.buffers(stream))
>>> +		bufferPool_[stream].push_back(frameBuffer.get());
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>>>  {
>>>  	if (!camera3Request->num_output_buffers) {
>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>> index 84f636f7a93c..4cef34c01a49 100644
>>> --- a/src/android/camera_device.h
>>> +++ b/src/android/camera_device.h
>>> @@ -166,6 +166,9 @@ protected:
>>>  	std::string logPrefix() const override;
>>>
>>>  private:
>>> +	using FrameBufferPool = std::map<libcamera::Stream *,
>>> +					 std::vector<libcamera::FrameBuffer *>>;
>>> +
>>
>> Putting these 'in' the CameraStream would mean we don't need to keep a
>> mapping of it?
> 
> We would save a map and store the vectors in each CameraStream, yes.
> 
>>
>> Hrm ... time to read further forwards...
>>
>>
>>>  	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
>>>
>>>  	struct Camera3RequestDescriptor {
>>> @@ -194,6 +197,8 @@ private:
>>>
>>>  	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
>>>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
>>> +	int allocateBuffersPool(libcamera::Stream *stream);
>>> +
>>>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>>>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
>>>  	CameraMetadata *requestTemplatePreview();
>>> @@ -208,6 +213,7 @@ private:
>>>  	std::shared_ptr<libcamera::Camera> camera_;
>>>  	std::unique_ptr<libcamera::CameraConfiguration> config_;
>>>  	libcamera::FrameBufferAllocator allocator_;
>>> +	FrameBufferPool bufferPool_;
>>>
>>>  	CameraMetadata *staticMetadata_;
>>>  	std::map<unsigned int, const CameraMetadata *> requestTemplates_;
>>>
>>
>> --
>> Regards
>> --
>> Kieran

Patch

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 42fb9ea4e113..f96ea7321a67 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1189,6 +1189,7 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 	streams_.clear();
 	streams_.reserve(stream_list->num_streams);
 	allocator_.clear();
+	bufferPool_.clear();
 
 	/* First handle all non-MJPEG streams. */
 	camera3_stream_t *jpegStream = nullptr;
@@ -1336,6 +1337,25 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 		return ret;
 	}
 
+	/*
+	 * Now that the camera has been configured allocate buffers for
+	 * the streams that need memory reserved by libcamera.
+	 */
+	for (const CameraStream &cameraStream : streams_) {
+		const StreamConfiguration &cfg = config_->at(cameraStream.index());
+		Stream *stream = cfg.stream();
+
+		if (cameraStream.type() != CameraStream::Type::Internal)
+			continue;
+
+		ret = allocateBuffersPool(stream);
+		if (ret) {
+			LOG(HAL, Error) << "Failed to allocate buffers for stream "
+					<< cameraStream.index();
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -1369,6 +1389,22 @@  FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
 	return new FrameBuffer(std::move(planes));
 }
 
+int CameraDevice::allocateBuffersPool(Stream *stream)
+{
+	int ret = allocator_.allocate(stream);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Save a pointer to the reserved frame buffer for usage in
+	 * the HAL.
+	 */
+	for (const auto &frameBuffer : allocator_.buffers(stream))
+		bufferPool_[stream].push_back(frameBuffer.get());
+
+	return 0;
+}
+
 int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
 {
 	if (!camera3Request->num_output_buffers) {
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 84f636f7a93c..4cef34c01a49 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -166,6 +166,9 @@  protected:
 	std::string logPrefix() const override;
 
 private:
+	using FrameBufferPool = std::map<libcamera::Stream *,
+					 std::vector<libcamera::FrameBuffer *>>;
+
 	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
 
 	struct Camera3RequestDescriptor {
@@ -194,6 +197,8 @@  private:
 
 	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
 	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
+	int allocateBuffersPool(libcamera::Stream *stream);
+
 	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
 	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
 	CameraMetadata *requestTemplatePreview();
@@ -208,6 +213,7 @@  private:
 	std::shared_ptr<libcamera::Camera> camera_;
 	std::unique_ptr<libcamera::CameraConfiguration> config_;
 	libcamera::FrameBufferAllocator allocator_;
+	FrameBufferPool bufferPool_;
 
 	CameraMetadata *staticMetadata_;
 	std::map<unsigned int, const CameraMetadata *> requestTemplates_;