[libcamera-devel] [RFC PATCH 2/2] android: Send alternative stream configuration if no buffer to be sent exists

[Jacopo Mondi]

Hi Hiro,

  I'll try not to repeat what Laurent already asked

On Mon, Sep 27, 2021 at 07:48:21PM +0900, Hirokazu Honda wrote:
> A request given in processCaptureRequest() can contain only streams
> that are resolved CameraStream::Type::Mapped. In the case, no buffer
> is sent to a native camera. This fixes the issue by looking for the
> stream to be requested and alternatively sending a buffer for the
> stream to a camera.
>
> However, the substitute stream is Direct and a buffer needs to be
> provided by a HAL client. So we have to allocate a buffer in
> Android HAL adaptation layer using PlatformFrameBufferAllocator.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_device.cpp | 50 +++++++++++++++++++++++++++++------
>  src/android/camera_device.h   |  4 +++
>  src/android/camera_stream.cpp | 37 ++++++++++++++++++++++----
>  src/android/camera_stream.h   | 10 +++++--
>  4 files changed, 86 insertions(+), 15 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a693dcbe..5887e85e 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -724,10 +724,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	for (const auto &streamConfig : streamConfigs) {
>  		config->addConfiguration(streamConfig.config);
>
> +		CameraStream *mappedStream = nullptr;
>  		for (auto &stream : streamConfig.streams) {
>  			streams_.emplace_back(this, config.get(), stream.type,
> -					      stream.stream, config->size() - 1);
> +					      stream.stream, mappedStream,
> +					      config->size() - 1);
>  			stream.stream->priv = static_cast<void *>(&streams_.back());
> +			if (stream.type == CameraStream::Type::Direct)
> +				mappedStream = &streams_.back();
>  		}
>  	}
>
> @@ -969,6 +973,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>
>  	LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
>  			<< " with " << descriptor.buffers_.size() << " streams";
> +
> +	std::set<Stream *> addedStreams;
>  	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
>  		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
>  		camera3_stream *camera3Stream = camera3Buffer.stream;
> @@ -1018,6 +1024,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 * once it has been processed.
>  			 */
>  			buffer = cameraStream->getBuffer();
> +			descriptor.internalBuffers_[cameraStream] = buffer;
>  			LOG(HAL, Debug) << ss.str() << " (internal)";
>  			break;
>  		}
> @@ -1029,6 +1036,37 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>
>  		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
>  						camera3Buffer.acquire_fence);
> +		addedStreams.insert(cameraStream->stream());
> +	}
> +
> +	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
> +		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
> +		camera3_stream *camera3Stream = camera3Buffer.stream;
> +		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> +		if (cameraStream->type() != CameraStream::Type::Mapped)
> +			continue;
> +		if (addedStreams.find(cameraStream->stream()) != addedStreams.end())
> +			continue;

Can't you just iterate on addedStreams then ?

> +
> +		CameraStream *mappedStream = cameraStream->mappedStream();
> +		if (!mappedStream) {
> +			LOG(HAL, Error)
> +				<< "Could not find mappedStream for ("
> +				<< camera3Stream->width << "x"
> +				<< camera3Stream->height << ")"
> +				<< "[" << utils::hex(camera3Stream->format) << "]";
> +			return -EINVAL;
> +		}
> +
> +		ASSERT(mappedStream->type() == CameraStream::Type::Direct);
> +		FrameBuffer *mappedBuffer = mappedStream->getBuffer();
> +		if (!mappedBuffer) {
> +			LOG(HAL, Error) << "Failed getting a buffer";
> +			return -EINVAL;
> +		}
> +
> +		descriptor.internalBuffers_[mappedStream] = mappedBuffer;
> +		descriptor.request_->addBuffer(mappedStream->stream(), mappedBuffer, -1);

I don't get this. What if the 'mapped' stream was already part of the
request ? Does it get added twice ?

>  	}
>
>  	/*
> @@ -1180,13 +1218,6 @@ void CameraDevice::requestComplete(Request *request)
>  		int ret = cameraStream->process(*src, *buffer.buffer,
>  						descriptor.settings_,
>  						resultMetadata.get());
> -		/*
> -		 * Return the FrameBuffer to the CameraStream now that we're
> -		 * done processing it.
> -		 */
> -		if (cameraStream->type() == CameraStream::Type::Internal)
> -			cameraStream->putBuffer(src);
> -
>  		if (ret) {
>  			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>  			notifyError(descriptor.frameNumber_, buffer.stream,
> @@ -1194,6 +1225,9 @@ void CameraDevice::requestComplete(Request *request)
>  		}
>  	}
>
> +	for (auto &[stream, buffer] : descriptor.internalBuffers_)
> +		stream->putBuffer(buffer);
> +
>  	captureResult.result = resultMetadata->get();
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>  }
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 296c2f18..74d8150b 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -21,6 +21,7 @@
>
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer.h>
> #include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
>  #include <libcamera/request.h>
> @@ -84,6 +85,7 @@ private:
>  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>  		CameraMetadata settings_;
>  		std::unique_ptr<CaptureRequest> request_;
> +		std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_;
>  	};
>
>  	enum class State {
> @@ -118,6 +120,8 @@ private:
>  	std::unique_ptr<libcamera::CameraConfiguration> config_;
>  	CameraCapabilities capabilities_;
>
> +	std::unique_ptr<libcamera::FrameBufferAllocator> frame_buffer_allocator_;
> +
>  	std::map<unsigned int, std::unique_ptr<CameraMetadata>> requestTemplates_;
>  	const camera3_callback_ops_t *callbacks_;
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index e30c7ee4..fc2e82cd 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -18,6 +18,7 @@
>  #include "camera_capabilities.h"
>  #include "camera_device.h"
>  #include "camera_metadata.h"
> +#include "frame_buffer.h"
>
>  using namespace libcamera;
>
> @@ -46,10 +47,14 @@ LOG_DECLARE_CATEGORY(HAL)
>
>  CameraStream::CameraStream(CameraDevice *const cameraDevice,
>  			   CameraConfiguration *config, Type type,
> -			   camera3_stream_t *camera3Stream, unsigned int index)
> +			   camera3_stream_t *camera3Stream,
> +			   CameraStream *const mappedStream,
> +			   unsigned int index)
>  	: cameraDevice_(cameraDevice), config_(config), type_(type),
> -	  camera3Stream_(camera3Stream), index_(index)
> +	  camera3Stream_(camera3Stream), mappedStream_(mappedStream),
> +	  index_(index)
>  {
> +	ASSERT(type_ != Type::Mapped || !!mappedStream);
>  }
>
>  const StreamConfiguration &CameraStream::configuration() const
> @@ -134,8 +139,30 @@ int CameraStream::process(const FrameBuffer &source,
>
>  FrameBuffer *CameraStream::getBuffer()
>  {
> -	if (!allocator_)
> -		return nullptr;

I assume the style is due the series being an RFC...

Anyway ...

> +	if (!mutex_) {

Relying on !mutex to verify that !Internal and "first time this gets
called" is fragile. Why not handle Direct in configure() and prepare a
buffer cache there.

Anyway, I've not really gone into detail in the platform buffer
allocator but why can't this use the Camera's frame buffer allocator ?

> +		ASSERT(type_ == Type::Direct);
> +		ASSERT(!allocator_);
> +		mutex_ = std::make_unique<std::mutex>();
> +		platform_allocator_ = std::make_unique<PlatformFrameBufferAllocator>(cameraDevice_);
> +		const StreamConfiguration &config = configuration();
> +		size_t numBuffers = config.bufferCount;
> +		const int halPixelFormat = camera3Stream_->format;
> +		const uint32_t usage = camera3Stream_->usage;
> +		LOG(HAL, Error) << "getBuffer@@@@@ " << numBuffers;
> +		// numBuffers is 4 (=config.bufferCount) is not enough sadly...
> +		// Should we dynamically allocate?
> +		numBuffers = 20;
> +		const auto &buffers = platform_allocator_->allocate(
> +			halPixelFormat, config.size, usage, numBuffers);
> +		if (buffers.empty() || buffers.size() != numBuffers) {
> +			LOG(HAL, Error) << "Failed allocating FrameBuffers";
> +			return nullptr;
> +		}
> +
> +		buffers_.reserve(numBuffers);
> +		for (const auto &frameBuffer : buffers)
> +			buffers_.push_back(frameBuffer.get());
> +	}
>
>  	std::lock_guard<std::mutex> locker(*mutex_);
>
> @@ -143,7 +170,7 @@ FrameBuffer *CameraStream::getBuffer()
>  		LOG(HAL, Error) << "Buffer underrun";
>  		return nullptr;
>  	}
> -
> +	LOG(HAL, Error) << buffers_.size();
>  	FrameBuffer *buffer = buffers_.back();
>  	buffers_.pop_back();
>
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 2dab6c3a..c8eee908 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -15,10 +15,11 @@
>
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer.h>
> -#include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
>
> +#include "frame_buffer.h"
> +
>  class CameraDevice;
>  class CameraMetadata;
>  class PostProcessor;
> @@ -110,12 +111,14 @@ public:
>  	};
>  	CameraStream(CameraDevice *const cameraDevice,
>  		     libcamera::CameraConfiguration *config, Type type,
> -		     camera3_stream_t *camera3Stream, unsigned int index);
> +		     camera3_stream_t *camera3Stream,
> +		     CameraStream *const mappedStream, unsigned int index);
>
>  	Type type() const { return type_; }
>  	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
>  	const libcamera::StreamConfiguration &configuration() const;
>  	libcamera::Stream *stream() const;
> +	CameraStream *mappedStream() const { return mappedStream_; }
>
>  	int configure();
>  	int process(const libcamera::FrameBuffer &source,
> @@ -130,10 +133,13 @@ private:
>  	const libcamera::CameraConfiguration *config_;
>  	const Type type_;
>  	camera3_stream_t *camera3Stream_;
> +	CameraStream *const mappedStream_;
>  	const unsigned int index_;
>
>  	std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> +	std::unique_ptr<PlatformFrameBufferAllocator> platform_allocator_;
>  	std::vector<libcamera::FrameBuffer *> buffers_;
> +	std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>  	/*
>  	 * The class has to be MoveConstructible as instances are stored in
>  	 * an std::vector in CameraDevice.
> --
> 2.33.0.685.g46640cef36-goog
>