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

[Kieran Bingham]

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 at ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo at 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_;
> 

-- 
Regards
--
Kieran