[libcamera-devel] [PATCH 09/11] android: camera_request: Don't embed full camera3_stream_buffer_t

[Umang Jain]

Hello,

I didn't see review comments on this patch,

On 10/18/21 6:59 PM, Umang Jain wrote:
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> The camera3_stream_buffer_t structure is meant to communicate between
> the camera service and the HAL. They are short-live structures that
> don't outlive the .process_capture_request() operation (when queuing
> requests) or the .process_capture_result() callback.
>
> We currently store copies of the camera3_stream_buffer_t passed to
> .process_capture_request() in Camera3RequestDescriptor::StreamBuffer to
> store the structure members that the HAL need, and reuse them when
> calling the .process_capture_result() callback. This is conceptually not
> right, as the camera3_stream_buffer_t pass to the callback are not the
> same objects as the ones received in .process_capture_request().
>
> Store individual fields of the camera3_stream_buffer_t in StreamBuffer
> instead of copying the whole structure. This gives the HAL full control
> of how data is stored, and properly decouples request queueing from
> result reporting.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>


Am i eligible to add my R-b? The patch looks sane to me:

Reviewed-by: Umang Jain<umang.jain at ideasonboard.com>

> ---
>   src/android/camera_device.cpp  | 73 ++++++++++++++++++----------------
>   src/android/camera_request.cpp | 19 +++++++--
>   src/android/camera_request.h   | 12 +++---
>   src/android/camera_stream.cpp  |  6 +--
>   4 files changed, 63 insertions(+), 47 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 216f29c2..c493b0a4 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -830,16 +830,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
>   {
>   	notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>   
> -	for (auto &buffer : descriptor->buffers_) {
> -		/*
> -		 * Signal to the framework it has to handle fences that have not
> -		 * been waited on by setting the release fence to the acquire
> -		 * fence value.
> -		 */
> -		buffer.buffer.release_fence = buffer.buffer.acquire_fence;
> -		buffer.buffer.acquire_fence = -1;
> -		buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> -	}
> +	for (auto &buffer : descriptor->buffers_)
> +		buffer.status = Camera3RequestDescriptor::Status::Error;
>   
>   	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>   }
> @@ -937,8 +929,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>   			<< " with " << descriptor->buffers_.size() << " streams";
>   
>   	for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> -		camera3_stream *camera3Stream = buffer.buffer.stream;
> -		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> +		CameraStream *cameraStream = buffer.stream;
> +		camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
>   
>   		std::stringstream ss;
>   		ss << i << " - (" << camera3Stream->width << "x"
> @@ -973,11 +965,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>   			 * lifetime management only.
>   			 */
>   			buffer.frameBuffer =
> -				createFrameBuffer(*buffer.buffer.buffer,
> +				createFrameBuffer(*buffer.camera3Buffer,
>   						  cameraStream->configuration().pixelFormat,
>   						  cameraStream->configuration().size);
>   			frameBuffer = buffer.frameBuffer.get();
> -			acquireFence = buffer.buffer.acquire_fence;
> +			acquireFence = buffer.fence;
>   			LOG(HAL, Debug) << ss.str() << " (direct)";
>   			break;
>   
> @@ -1083,12 +1075,11 @@ void CameraDevice::requestComplete(Request *request)
>   	/*
>   	 * Prepare the capture result for the Android camera stack.
>   	 *
> -	 * The buffer status is set to OK and later changed to ERROR if
> +	 * The buffer status is set to Success and later changed to Error if
>   	 * post-processing/compression fails.
>   	 */
>   	for (auto &buffer : descriptor->buffers_) {
> -		CameraStream *cameraStream =
> -			static_cast<CameraStream *>(buffer.buffer.stream->priv);
> +		CameraStream *stream = buffer.stream;
>   
>   		/*
>   		 * Streams of type Direct have been queued to the
> @@ -1101,10 +1092,9 @@ void CameraDevice::requestComplete(Request *request)
>   		 * \todo Instrument the CameraWorker to set the acquire
>   		 * fence to -1 once it has handled it and remove this check.
>   		 */
> -		if (cameraStream->type() == CameraStream::Type::Direct)
> -			buffer.buffer.acquire_fence = -1;
> -		buffer.buffer.release_fence = -1;
> -		buffer.buffer.status = CAMERA3_BUFFER_STATUS_OK;
> +		if (stream->type() == CameraStream::Type::Direct)
> +			buffer.fence = -1;
> +		buffer.status = Camera3RequestDescriptor::Status::Success;
>   	}
>   
>   	/*
> @@ -1151,33 +1141,32 @@ void CameraDevice::requestComplete(Request *request)
>   
>   	/* Handle post-processing. */
>   	for (auto &buffer : descriptor->buffers_) {
> -		CameraStream *cameraStream =
> -			static_cast<CameraStream *>(buffer.buffer.stream->priv);
> +		CameraStream *stream = buffer.stream;
>   
> -		if (cameraStream->type() == CameraStream::Type::Direct)
> +		if (stream->type() == CameraStream::Type::Direct)
>   			continue;
>   
> -		FrameBuffer *src = request->findBuffer(cameraStream->stream());
> +		FrameBuffer *src = request->findBuffer(stream->stream());
>   		if (!src) {
>   			LOG(HAL, Error) << "Failed to find a source stream buffer";
> -			buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> -			notifyError(descriptor->frameNumber_, buffer.buffer.stream,
> +			buffer.status = Camera3RequestDescriptor::Status::Error;
> +			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
>   				    CAMERA3_MSG_ERROR_BUFFER);
>   			continue;
>   		}
>   
> -		int ret = cameraStream->process(*src, buffer, descriptor);
> +		int ret = stream->process(*src, buffer, descriptor);
>   
>   		/*
>   		 * Return the FrameBuffer to the CameraStream now that we're
>   		 * done processing it.
>   		 */
> -		if (cameraStream->type() == CameraStream::Type::Internal)
> -			cameraStream->putBuffer(src);
> +		if (stream->type() == CameraStream::Type::Internal)
> +			stream->putBuffer(src);
>   
>   		if (ret) {
> -			buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> -			notifyError(descriptor->frameNumber_, buffer.buffer.stream,
> +			buffer.status = Camera3RequestDescriptor::Status::Error;
> +			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
>   				    CAMERA3_MSG_ERROR_BUFFER);
>   		}
>   	}
> @@ -1208,8 +1197,24 @@ void CameraDevice::sendCaptureResults()
>   			captureResult.result = descriptor->resultMetadata_->get();
>   
>   		std::vector<camera3_stream_buffer_t> resultBuffers;
> -		for (const auto &buffer : descriptor->buffers_)
> -			resultBuffers.emplace_back(buffer.buffer);
> +		resultBuffers.reserve(descriptor->buffers_.size());
> +
> +		for (const auto &buffer : descriptor->buffers_) {
> +			camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR;
> +
> +			if (buffer.status == Camera3RequestDescriptor::Status::Success)
> +				status = CAMERA3_BUFFER_STATUS_OK;
> +
> +			/*
> +			 * Pass the buffer fence back to the camera framework as
> +			 * a release fence. This instructs the framework to wait
> +			 * on the acquire fence in case we haven't done so
> +			 * ourselves for any reason.
> +			 */
> +			resultBuffers.push_back({ buffer.stream->camera3Stream(),
> +						  buffer.camera3Buffer, status,
> +						  -1, buffer.fence });
> +		}
>   
>   		captureResult.num_output_buffers = resultBuffers.size();
>   		captureResult.output_buffers = resultBuffers.data();
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index 614baed4..faa85ada 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -7,6 +7,8 @@
>   
>   #include "camera_request.h"
>   
> +#include <libcamera/base/span.h>
> +
>   using namespace libcamera;
>   
>   /*
> @@ -22,11 +24,20 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>   	frameNumber_ = camera3Request->frame_number;
>   
>   	/* Copy the camera3 request stream information for later access. */
> -	const uint32_t numBuffers = camera3Request->num_output_buffers;
> +	const Span<const camera3_stream_buffer_t> buffers{
> +		camera3Request->output_buffers,
> +		camera3Request->num_output_buffers
> +	};
> +
> +	buffers_.reserve(buffers.size());
> +
> +	for (const camera3_stream_buffer_t &buffer : buffers) {
> +		CameraStream *stream =
> +			static_cast<CameraStream *>(buffer.stream->priv);
>   
> -	buffers_.resize(numBuffers);
> -	for (uint32_t i = 0; i < numBuffers; i++)
> -		buffers_[i].buffer = camera3Request->output_buffers[i];
> +		buffers_.push_back({ stream, buffer.buffer, nullptr,
> +				     buffer.acquire_fence, Status::Pending });
> +	}
>   
>   	/* Clone the controls associated with the camera3 request. */
>   	settings_ = CameraMetadata(camera3Request->settings);
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index a030febf..05dabf89 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -20,6 +20,8 @@
>   #include "camera_metadata.h"
>   #include "camera_worker.h"
>   
> +class CameraStream;
> +
>   class Camera3RequestDescriptor
>   {
>   public:
> @@ -30,13 +32,11 @@ public:
>   	};
>   
>   	struct StreamBuffer {
> -		camera3_stream_buffer_t buffer;
> -		/*
> -		 * FrameBuffer instances created by wrapping a camera3 provided
> -		 * dmabuf are emplaced in this vector of unique_ptr<> for
> -		 * lifetime management.
> -		 */
> +		CameraStream *stream;
> +		buffer_handle_t *camera3Buffer;
>   		std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
> +		int fence;
> +		Status status;
>   	};
>   
>   	Camera3RequestDescriptor(libcamera::Camera *camera,
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index f3cc77e7..9b5cd0c4 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -147,11 +147,11 @@ int CameraStream::process(const FrameBuffer &source,
>   			  Camera3RequestDescriptor *request)
>   {
>   	/* Handle waiting on fences on the destination buffer. */
> -	int fence = dest.buffer.acquire_fence;
> +	int fence = dest.fence;
>   	if (fence != -1) {
>   		int ret = waitFence(fence);
>   		::close(fence);
> -		dest.buffer.acquire_fence = -1;
> +		dest.fence = -1;
>   		if (ret < 0) {
>   			LOG(HAL, Error) << "Failed waiting for fence: "
>   					<< fence << ": " << strerror(-ret);
> @@ -167,7 +167,7 @@ int CameraStream::process(const FrameBuffer &source,
>   	 * separate thread.
>   	 */
>   	const StreamConfiguration &output = configuration();
> -	CameraBuffer destBuffer(*dest.buffer.buffer, output.pixelFormat,
> +	CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,
>   				output.size, PROT_READ | PROT_WRITE);
>   	if (!destBuffer.isValid()) {
>   		LOG(HAL, Error) << "Failed to create destination buffer";