[PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to Camera3RequestDescriptor

[Jacopo Mondi]

Hi Harevy

On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote:
> Previously there's a potential issue in StreamBuffer::internalBuffer's
> lifetime, when more than one streams depend on the same internal buffer.

s/.//

lifetime, when more than one streams depend on the same internal buffer
for post-processing, the buffer is returned to the CameraStream pool
as soon as the first post-processing request has completed.

>
> This patch fixes the issue by returning the buffer when the whole
> Camera3RequestDescriptor is destructed.

Actually it does more I guess, it allows to map multiple streams of
type ::Mapped to the same libcamera::Stream, doesn't it ?

>
> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  src/android/camera_device.cpp  | 63 +++++++++++++++++++---------------
>  src/android/camera_request.cpp | 13 ++++---
>  src/android/camera_request.h   |  3 +-
>  3 files changed, 46 insertions(+), 33 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 11613fac9..62f724041 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * to a libcamera stream. Streams of type Mapped will be handled later.
>  	 *
>  	 * Collect the CameraStream associated to each requested capture stream.
> -	 * Since requestedStreams is an std:map<>, no duplications can happen.
> +	 * Since requestedDirectBuffers is an std:map<>, no duplications can
> +	 * happen.
>  	 */
> -	std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers;
> +	std::map<CameraStream *, libcamera::FrameBuffer *> requestedDirectBuffers;

Do you actually need to rename ?

>  	for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
>  		CameraStream *cameraStream = buffer.stream;
>  		camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
> @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			frameBuffer = buffer.frameBuffer.get();
>  			acquireFence = std::move(buffer.fence);
>
> -			requestedBuffers[cameraStream] = frameBuffer;
> +			requestedDirectBuffers[cameraStream] = frameBuffer;
>  			LOG(HAL, Debug) << ss.str() << " (direct)";
>  			break;
>
> @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 * Get the frame buffer from the CameraStream internal
>  			 * buffer pool.
>  			 *
> -			 * The buffer has to be returned to the CameraStream
> -			 * once it has been processed.
> +			 * The buffer will be returned to the CameraStream when
> +			 * the request is destructed.

or 'destroyed'

>  			 */
>  			frameBuffer = cameraStream->getBuffer();
> -			buffer.internalBuffer = frameBuffer;
>  			buffer.srcBuffer = frameBuffer;
>
> -			requestedBuffers[cameraStream] = frameBuffer;
> +			/*
> +			 * Track the allocated internal buffers, which will be
> +			 * recycled when the descriptor destroyed.
> +			 * */

Stray * */

> +			descriptor->internalBuffers_[cameraStream] = frameBuffer;
>  			LOG(HAL, Debug) << ss.str() << " (internal)";
>
>  			descriptor->pendingStreamsToProcess_.insert(
> @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		 * post-processing. No need to recycle the buffer since it's
>  		 * owned by Android.
>  		 */
> -		auto iterDirectBuffer = requestedBuffers.find(sourceStream);
> -		if (iterDirectBuffer != requestedBuffers.end()) {
> +		auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream);
> +		if (iterDirectBuffer != requestedDirectBuffers.end()) {
>  			buffer.srcBuffer = iterDirectBuffer->second;
>  			continue;
>  		}
>
>  		/*
> -		 * If that's not the case, we need to add a buffer to the request
> -		 * for this stream.
> +		 * If that's not the case, we use an internal buffer allocated
> +		 * from the source stream.
> +		 *
> +		 * If an internal buffer has been requested for the source
> +		 * stream before, we should reuse it.
> +		 */
> +		auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);
> +		if (iterInternalBuffer != descriptor->internalBuffers_.end()) {
> +			buffer.srcBuffer = iterInternalBuffer->second;
> +			continue;
> +		}
> +
> +		/*
> +		 * Otherwise, we need to create an internal buffer to the
> +		 * request for the source stream. Get the frame buffer from the
> +		 * source stream's internal buffer pool.
> +		 *
> +		 * The buffer will be returned to the CameraStream when the
> +		 * request is destructed.
>  		 */
> -		FrameBuffer *frameBuffer = cameraStream->getBuffer();
> -		buffer.internalBuffer = frameBuffer;
> +		FrameBuffer *frameBuffer = sourceStream->getBuffer();

Here I don't see why we're using the sourceStream's pool instead
of the cameraStream's pool

However, I think this goes in the direction of making the HAL capable
of more things, and I'm looking at the diff of this and the previous
patch combined and I think you should squash the two and make a commit
about "Enabling multiple processed streams to map to the same internal
buffer".

Looking at the combined diff, it feels to me you could populate
buffer.src for the Direct case in the switch() and keep using an
std::set for requestedDirectBuffers (or requestedStreams). This would
make the diff smaller probably.


>  		buffer.srcBuffer = frameBuffer;
>
>  		descriptor->request_->addBuffer(sourceStream->stream(),
>  						frameBuffer, nullptr);
>
> -		requestedBuffers[sourceStream] = frameBuffer;
> +		/* Track the allocated internal buffer. */
> +		descriptor->internalBuffers_[sourceStream] = frameBuffer;
>  	}
>
>  	/*
> @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request)
>  		if (ret) {
>  			setBufferStatus(*buffer, StreamBuffer::Status::Error);
>  			descriptor->pendingStreamsToProcess_.erase(stream);
> -
> -			/*
> -			 * If the framebuffer is internal to CameraStream return
> -			 * it back now that we're done processing it.
> -			 */
> -			if (buffer->internalBuffer)
> -				stream->putBuffer(buffer->internalBuffer);
>  		}
>  	}
>
> @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer,
>  {
>  	setBufferStatus(*streamBuffer, status);
>
> -	/*
> -	 * If the framebuffer is internal to CameraStream return it back now
> -	 * that we're done processing it.
> -	 */
> -	if (streamBuffer->internalBuffer)
> -		streamBuffer->stream->putBuffer(streamBuffer->internalBuffer);
> -
>  	Camera3RequestDescriptor *request = streamBuffer->request;
>
>  	{
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index 52a3ac1f7..a9240a83c 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -10,6 +10,7 @@
>  #include <libcamera/base/span.h>
>
>  #include "camera_buffer.h"
> +#include "camera_stream.h"
>
>  using namespace libcamera;
>
> @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>  	request_ = camera->createRequest(reinterpret_cast<uint64_t>(this));
>  }
>
> -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> +Camera3RequestDescriptor::~Camera3RequestDescriptor()
> +{
> +	/*
> +	 * Recycle the allocated internal buffer back to its source stream.
> +	 */

Fits in one line most probably

> +	for (auto &[sourceStream, frameBuffer] : internalBuffers_)
> +		sourceStream->putBuffer(frameBuffer);
> +}
>
>  /**
>   * \class StreamBuffer
> @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
>   * \var StreamBuffer::status
>   * \brief Track the status of the buffer
>   *
> - * \var StreamBuffer::internalBuffer
> - * \brief Pointer to a buffer internally handled by CameraStream (if any)
> - *
>   * \var StreamBuffer::srcBuffer
>   * \brief Pointer to the source frame buffer used for post-processing
>   *
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index 335f1985d..6b2a00795 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -49,7 +49,6 @@ public:
>  	std::unique_ptr<HALFrameBuffer> frameBuffer;
>  	libcamera::UniqueFD fence;
>  	Status status = Status::Success;
> -	libcamera::FrameBuffer *internalBuffer = nullptr;
>  	const libcamera::FrameBuffer *srcBuffer = nullptr;
>  	std::unique_ptr<CameraBuffer> dstBuffer;
>  	Camera3RequestDescriptor *request;
> @@ -85,6 +84,8 @@ public:
>  	std::unique_ptr<libcamera::Request> request_;
>  	std::unique_ptr<CameraMetadata> resultMetadata_;
>
> +	std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_;
> +
>  	bool complete_ = false;
>  	Status status_ = Status::Success;
>
> --
> 2.47.0.338.g60cca15819-goog
>