[libcamera-devel,09/11] android: camera_request: Don't embed full camera3_stream_buffer_t
diff mbox series

Message ID 20211018132923.476242-10-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • android: Overhaul request handling
Related show

Commit Message

Umang Jain Oct. 18, 2021, 1:29 p.m. UTC
From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@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(-)

Comments

Umang Jain Oct. 19, 2021, 11:02 a.m. UTC | #1
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@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@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>


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

Reviewed-by: Umang Jain<umang.jain@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";
Laurent Pinchart Oct. 19, 2021, 11:15 a.m. UTC | #2
On Tue, Oct 19, 2021 at 04:32:50PM +0530, Umang Jain wrote:
> 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@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@ideasonboard.com>
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> 
> Am i eligible to add my R-b? The patch looks sane to me:

I think so, as you're not the author. We make the rules :-)

> Reviewed-by: Umang Jain<umang.jain@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";

Patch
diff mbox series

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";