From patchwork Mon Oct 18 13:29:21 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 14170 X-Patchwork-Delegate: umang.jain@ideasonboard.com Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 4694EC323E for ; Mon, 18 Oct 2021 13:29:49 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id D975168F65; Mon, 18 Oct 2021 15:29:48 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="Li3eHyU5"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 15C0C68F57 for ; Mon, 18 Oct 2021 15:29:47 +0200 (CEST) Received: from perceval.ideasonboard.com (unknown [103.238.109.14]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 11AA58C6; Mon, 18 Oct 2021 15:29:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1634563786; bh=Hp1fGxT0zaO3TNOTIat4ybJaDsInBIYK3NZvP4dcObo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Li3eHyU5XEJpov57wriPeEdvHerJAwrXOZhIEjkUJV6UBFFkwn5vC7XdVM4LRIayj N4JMIreuDtv3iB1MCyBfZB4PsyhccXdsq4bxP4sn/DExxp9Ro7s17EIWGMCQ3BoG3/ nrfMVFphTSYuyZITHKKHiJfg/SzH00DlIb/iUrlM= From: Umang Jain To: libcamera-devel@lists.libcamera.org Date: Mon, 18 Oct 2021 18:59:21 +0530 Message-Id: <20211018132923.476242-10-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20211018132923.476242-1-umang.jain@ideasonboard.com> References: <20211018132923.476242-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 09/11] android: camera_request: Don't embed full camera3_stream_buffer_t X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" From: Laurent Pinchart 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 Signed-off-by: Umang Jain Reviewed-by: Umang Jain --- 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(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(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(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 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 + 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 buffers{ + camera3Request->output_buffers, + camera3Request->num_output_buffers + }; + + buffers_.reserve(buffers.size()); + + for (const camera3_stream_buffer_t &buffer : buffers) { + CameraStream *stream = + static_cast(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 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";