[libcamera-devel,2/2] android: camera_stream: Use PlatformFrameBufferAllocator

14896 diff mbox series

CameraStream originally creates FrameBuffers by
FrameBufferAllocator and thus buffers are allocated in V4L2 API.
This replaces the allocator in CameraStream with
PlatformFrameBufferAllocator. It allocates a buffer in a platform
dependent graphic buffer allocation API.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_stream.cpp | 24 +++++++++++++-----------
 src/android/camera_stream.h   |  5 +++--
 2 files changed, 16 insertions(+), 13 deletions(-)

Hi Hiro

On Tue, Nov 30, 2021 at 09:44:28PM +0900, Hirokazu Honda wrote:
> CameraStream originally creates FrameBuffers by
> FrameBufferAllocator and thus buffers are allocated in V4L2 API.
> This replaces the allocator in CameraStream with
> PlatformFrameBufferAllocator. It allocates a buffer in a platform
> dependent graphic buffer allocation API.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_stream.cpp | 24 +++++++++++++-----------
>  src/android/camera_stream.h   |  5 +++--
>  2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 9023c13c..d22dd6b3 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -22,6 +22,7 @@
>  #include "camera_capabilities.h"
>  #include "camera_device.h"
>  #include "camera_metadata.h"
> +#include "frame_buffer_allocator.h"
>  #include "post_processor.h"
>
>  using namespace libcamera;
> @@ -118,16 +119,8 @@ int CameraStream::configure()
>  	}
>
>  	if (type_ == Type::Internal) {
> -		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> +		allocator_ = std::make_unique<PlatformFrameBufferAllocator>(cameraDevice_);
>  		mutex_ = std::make_unique<std::mutex>();
> -
> -		int ret = allocator_->allocate(stream());
> -		if (ret < 0)
> -			return ret;
> -
> -		/* Save a pointer to the reserved frame buffers */
> -		for (const auto &frameBuffer : allocator_->buffers(stream()))
> -			buffers_.push_back(frameBuffer.get());
>  	}
>
>  	camera3Stream_->max_buffers = configuration().bufferCount;
> @@ -211,8 +204,17 @@ FrameBuffer *CameraStream::getBuffer()
>  	std::lock_guard<std::mutex> locker(*mutex_);
>
>  	if (buffers_.empty()) {
> -		LOG(HAL, Error) << "Buffer underrun";
> -		return nullptr;
> +		const int frameFormat =
> +			cameraDevice_->capabilities()->toAndroidFormat(
> +				configuration().pixelFormat);
> +		if (frameFormat == -1)
> +			return nullptr;

I would be tempted to ASSERT to catch a development error earlier

> +
> +		auto frameBuffer = allocator_->allocate(frameFormat,
> +							configuration().size,
> +							camera3Stream_->usage);

should you check for !framebuffer ?

> +		allocatedBuffers_.push_back(std::move(frameBuffer));
> +		buffers_.emplace_back(allocatedBuffers_.back().get());

Oh nice, so this allocates buffers on demand. I guess the max number
of allocated buffers is equal to the max number of requests in flight,
so a rather small one

I wonder, and I honestly don't know the answer or what the usual usage
pattern is: do we need to ever keep a cache of allocated buffers ? Is
allocation/deallocation expensive and should be minimized, or is it
better to allocate and release every time ? I suspect reusing the same
framebuffers probably maximizes the cache hits in the v4l2 device
dmabuf mappings but I'm not sure what the usual usage pattern is. How
is this commonly used in cros ?

>  	}
>
>  	FrameBuffer *buffer = buffers_.back();
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 0c402deb..190ac6c0 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -19,7 +19,6 @@
>
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer.h>
> -#include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
>
> @@ -27,6 +26,7 @@
>  #include "post_processor.h"
>
>  class CameraDevice;
> +class PlatformFrameBufferAllocator;
>
>  class CameraStream
>  {
> @@ -168,7 +168,8 @@ private:
>  	camera3_stream_t *camera3Stream_;
>  	const unsigned int index_;
>
> -	std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> +	std::unique_ptr<PlatformFrameBufferAllocator> allocator_;
> +	std::vector<std::unique_ptr<libcamera::FrameBuffer>> allocatedBuffers_;

If I recall correctly one of the two backends had as requirement that
framebuffers should be deleted before the allocator. If that's the
case, should we manually clear the vector then release the allocator_
in the destructor ?

>  	std::vector<libcamera::FrameBuffer *> buffers_;
>  	/*
>  	 * The class has to be MoveConstructible as instances are stored in
> --
> 2.34.0.rc2.393.gf8c9666880-goog
>