[libcamera-devel] [PATCH v4 6/9] android: camera_device: Allocate buffer pools

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Sep 30, 2020 at 03:27:04PM +0200, 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 8692771a4538..07041dd916d5 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1191,6 +1191,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	streams_.clear();
>  	streams_.reserve(stream_list->num_streams);
>  	allocator_.freeAll();
> +	bufferPool_.clear();
>  
>  	/* First handle all non-MJPEG streams. */
>  	camera3_stream_t *jpegStream = nullptr;
> @@ -1338,6 +1339,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.

I would write "that are internal and will thus not received buffers from
the camera service.".

> +	 */
> +	for (const CameraStream &cameraStream : streams_) {
> +		const StreamConfiguration &cfg = config_->at(cameraStream.index());
> +		Stream *stream = cfg.stream();
> +
> +		if (cameraStream.type() != CameraStream::Type::Internal)
> +			continue;
> +
> +		ret = allocateBufferPool(stream);
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to allocate buffers for stream "
> +					<< cameraStream.index();
> +			return ret;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1371,6 +1391,22 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>  	return new FrameBuffer(std::move(planes));
>  }
>  
> +int CameraDevice::allocateBufferPool(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 75e3305089d9..c46610725e12 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 *>>;

Do we need this type ? I agree that the right-hand side is long and not
nice to write, but the type is only used once, below. Having an alias
requires looking it up when reading the code. Up to you.

> +
>  	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 allocateBufferPool(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_;

Maybe bufferPools_ as there's more than one pool ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  
>  	CameraMetadata *staticMetadata_;
>  	std::map<unsigned int, const CameraMetadata *> requestTemplates_;

-- 
Regards,

Laurent Pinchart