Patch Detail
Show a patch.
GET /api/1.1/patches/14183/?format=api
{ "id": 14183, "url": "https://patchwork.libcamera.org/api/1.1/patches/14183/?format=api", "web_url": "https://patchwork.libcamera.org/patch/14183/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20211019114802.665980-10-umang.jain@ideasonboard.com>", "date": "2021-10-19T11:47:59", "name": "[libcamera-devel,v2,09/12] android: camera_request: Don't embed full camera3_stream_buffer_t", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "ad4c0803d83334e30ac783fdbd9795e59dce2dbf", "submitter": { "id": 86, "url": "https://patchwork.libcamera.org/api/1.1/people/86/?format=api", "name": "Umang Jain", "email": "umang.jain@ideasonboard.com" }, "delegate": { "id": 12, "url": "https://patchwork.libcamera.org/api/1.1/users/12/?format=api", "username": "uajain", "first_name": "Umang", "last_name": "Jain", "email": "umang.jain@ideasonboard.com" }, "mbox": "https://patchwork.libcamera.org/patch/14183/mbox/", "series": [ { "id": 2638, "url": "https://patchwork.libcamera.org/api/1.1/series/2638/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=2638", "date": "2021-10-19T11:47:50", "name": "android: Overhaul request handling", "version": 2, "mbox": "https://patchwork.libcamera.org/series/2638/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/14183/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/14183/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "X-Original-To": "parsemail@patchwork.libcamera.org", "Delivered-To": "parsemail@patchwork.libcamera.org", "Received": [ "from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 50F8FC324C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 19 Oct 2021 11:48:32 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11E5468F5D;\n\tTue, 19 Oct 2021 13:48:32 +0200 (CEST)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6268E604FF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 19 Oct 2021 13:48:30 +0200 (CEST)", "from perceval.ideasonboard.com (unknown [103.251.226.98])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C0F312A;\n\tTue, 19 Oct 2021 13:48:29 +0200 (CEST)" ], "Authentication-Results": "lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EoEPglu1\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634644110;\n\tbh=nZfi2zYf9mf27qLb1pVADiGv4Tu6Cr41QbIdQsy5KSQ=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=EoEPglu1jmIQ+hnJgYjCejzs1akcG6g2Qbt0s8gI6Ui9WAPAtzWFv2lQiSxCaEonm\n\tlyqAWNKHwD8c36rjRZ+Dly6/bioV7RUgS2rrc3+BpefIItlxnsoOD6pDQVi+zYZXZa\n\t7TCdNgcJ/wMsRYsOFa04CgM+N+RndZ8JCvcgI6Wg=", "From": "Umang Jain <umang.jain@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Tue, 19 Oct 2021 17:17:59 +0530", "Message-Id": "<20211019114802.665980-10-umang.jain@ideasonboard.com>", "X-Mailer": "git-send-email 2.31.1", "In-Reply-To": "<20211019114802.665980-1-umang.jain@ideasonboard.com>", "References": "<20211019114802.665980-1-umang.jain@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v2 09/12] android: camera_request: Don't\n\tembed full camera3_stream_buffer_t", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nThe camera3_stream_buffer_t structure is meant to communicate between\nthe camera service and the HAL. They are short-live structures that\ndon't outlive the .process_capture_request() operation (when queuing\nrequests) or the .process_capture_result() callback.\n\nWe currently store copies of the camera3_stream_buffer_t passed to\n.process_capture_request() in Camera3RequestDescriptor::StreamBuffer to\nstore the structure members that the HAL need, and reuse them when\ncalling the .process_capture_result() callback. This is conceptually not\nright, as the camera3_stream_buffer_t pass to the callback are not the\nsame objects as the ones received in .process_capture_request().\n\nStore individual fields of the camera3_stream_buffer_t in StreamBuffer\ninstead of copying the whole structure. This gives the HAL full control\nof how data is stored, and properly decouples request queueing from\nresult reporting.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\nSigned-off-by: Umang Jain <umang.jain@ideasonboard.com>\nReviewed-by: Umang Jain<umang.jain@ideasonboard.com>\n---\n src/android/camera_device.cpp | 73 ++++++++++++++++++----------------\n src/android/camera_request.cpp | 19 +++++++--\n src/android/camera_request.h | 12 +++---\n src/android/camera_stream.cpp | 6 +--\n 4 files changed, 63 insertions(+), 47 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 0bb547ae..0a2d3826 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -801,16 +801,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n {\n \tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n \n-\tfor (auto &buffer : descriptor->buffers_) {\n-\t\t/*\n-\t\t * Signal to the framework it has to handle fences that have not\n-\t\t * been waited on by setting the release fence to the acquire\n-\t\t * fence value.\n-\t\t */\n-\t\tbuffer.buffer.release_fence = buffer.buffer.acquire_fence;\n-\t\tbuffer.buffer.acquire_fence = -1;\n-\t\tbuffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n-\t}\n+\tfor (auto &buffer : descriptor->buffers_)\n+\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n \n \tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n }\n@@ -908,8 +900,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\t\t<< \" with \" << descriptor->buffers_.size() << \" streams\";\n \n \tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n-\t\tcamera3_stream *camera3Stream = buffer.buffer.stream;\n-\t\tCameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);\n+\t\tCameraStream *cameraStream = buffer.stream;\n+\t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n \n \t\tstd::stringstream ss;\n \t\tss << i << \" - (\" << camera3Stream->width << \"x\"\n@@ -944,11 +936,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\t\t * lifetime management only.\n \t\t\t */\n \t\t\tbuffer.frameBuffer =\n-\t\t\t\tcreateFrameBuffer(*buffer.buffer.buffer,\n+\t\t\t\tcreateFrameBuffer(*buffer.camera3Buffer,\n \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n \t\t\t\t\t\t cameraStream->configuration().size);\n \t\t\tframeBuffer = buffer.frameBuffer.get();\n-\t\t\tacquireFence = buffer.buffer.acquire_fence;\n+\t\t\tacquireFence = buffer.fence;\n \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n \t\t\tbreak;\n \n@@ -1055,12 +1047,11 @@ void CameraDevice::requestComplete(Request *request)\n \t/*\n \t * Prepare the capture result for the Android camera stack.\n \t *\n-\t * The buffer status is set to OK and later changed to ERROR if\n+\t * The buffer status is set to Success and later changed to Error if\n \t * post-processing/compression fails.\n \t */\n \tfor (auto &buffer : descriptor->buffers_) {\n-\t\tCameraStream *cameraStream =\n-\t\t\tstatic_cast<CameraStream *>(buffer.buffer.stream->priv);\n+\t\tCameraStream *stream = buffer.stream;\n \n \t\t/*\n \t\t * Streams of type Direct have been queued to the\n@@ -1073,10 +1064,9 @@ void CameraDevice::requestComplete(Request *request)\n \t\t * \\todo Instrument the CameraWorker to set the acquire\n \t\t * fence to -1 once it has handled it and remove this check.\n \t\t */\n-\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n-\t\t\tbuffer.buffer.acquire_fence = -1;\n-\t\tbuffer.buffer.release_fence = -1;\n-\t\tbuffer.buffer.status = CAMERA3_BUFFER_STATUS_OK;\n+\t\tif (stream->type() == CameraStream::Type::Direct)\n+\t\t\tbuffer.fence = -1;\n+\t\tbuffer.status = Camera3RequestDescriptor::Status::Success;\n \t}\n \n \t/*\n@@ -1128,33 +1118,32 @@ void CameraDevice::requestComplete(Request *request)\n \n \t/* Handle post-processing. */\n \tfor (auto &buffer : descriptor->buffers_) {\n-\t\tCameraStream *cameraStream =\n-\t\t\tstatic_cast<CameraStream *>(buffer.buffer.stream->priv);\n+\t\tCameraStream *stream = buffer.stream;\n \n-\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n+\t\tif (stream->type() == CameraStream::Type::Direct)\n \t\t\tcontinue;\n \n-\t\tFrameBuffer *src = request->findBuffer(cameraStream->stream());\n+\t\tFrameBuffer *src = request->findBuffer(stream->stream());\n \t\tif (!src) {\n \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\n-\t\t\tbuffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n-\t\t\tnotifyError(descriptor->frameNumber_, buffer.buffer.stream,\n+\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n+\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n \t\t\tcontinue;\n \t\t}\n \n-\t\tint ret = cameraStream->process(*src, buffer, descriptor);\n+\t\tint ret = stream->process(*src, buffer, descriptor);\n \n \t\t/*\n \t\t * Return the FrameBuffer to the CameraStream now that we're\n \t\t * done processing it.\n \t\t */\n-\t\tif (cameraStream->type() == CameraStream::Type::Internal)\n-\t\t\tcameraStream->putBuffer(src);\n+\t\tif (stream->type() == CameraStream::Type::Internal)\n+\t\t\tstream->putBuffer(src);\n \n \t\tif (ret) {\n-\t\t\tbuffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n-\t\t\tnotifyError(descriptor->frameNumber_, buffer.buffer.stream,\n+\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n+\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n \t\t}\n \t}\n@@ -1185,8 +1174,24 @@ void CameraDevice::sendCaptureResults()\n \t\t\tcaptureResult.result = descriptor->resultMetadata_->get();\n \n \t\tstd::vector<camera3_stream_buffer_t> resultBuffers;\n-\t\tfor (const auto &buffer : descriptor->buffers_)\n-\t\t\tresultBuffers.emplace_back(buffer.buffer);\n+\t\tresultBuffers.reserve(descriptor->buffers_.size());\n+\n+\t\tfor (const auto &buffer : descriptor->buffers_) {\n+\t\t\tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR;\n+\n+\t\t\tif (buffer.status == Camera3RequestDescriptor::Status::Success)\n+\t\t\t\tstatus = CAMERA3_BUFFER_STATUS_OK;\n+\n+\t\t\t/*\n+\t\t\t * Pass the buffer fence back to the camera framework as\n+\t\t\t * a release fence. This instructs the framework to wait\n+\t\t\t * on the acquire fence in case we haven't done so\n+\t\t\t * ourselves for any reason.\n+\t\t\t */\n+\t\t\tresultBuffers.push_back({ buffer.stream->camera3Stream(),\n+\t\t\t\t\t\t buffer.camera3Buffer, status,\n+\t\t\t\t\t\t -1, buffer.fence });\n+\t\t}\n \n \t\tcaptureResult.num_output_buffers = resultBuffers.size();\n \t\tcaptureResult.output_buffers = resultBuffers.data();\ndiff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\nindex 614baed4..faa85ada 100644\n--- a/src/android/camera_request.cpp\n+++ b/src/android/camera_request.cpp\n@@ -7,6 +7,8 @@\n \n #include \"camera_request.h\"\n \n+#include <libcamera/base/span.h>\n+\n using namespace libcamera;\n \n /*\n@@ -22,11 +24,20 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n \tframeNumber_ = camera3Request->frame_number;\n \n \t/* Copy the camera3 request stream information for later access. */\n-\tconst uint32_t numBuffers = camera3Request->num_output_buffers;\n+\tconst Span<const camera3_stream_buffer_t> buffers{\n+\t\tcamera3Request->output_buffers,\n+\t\tcamera3Request->num_output_buffers\n+\t};\n+\n+\tbuffers_.reserve(buffers.size());\n+\n+\tfor (const camera3_stream_buffer_t &buffer : buffers) {\n+\t\tCameraStream *stream =\n+\t\t\tstatic_cast<CameraStream *>(buffer.stream->priv);\n \n-\tbuffers_.resize(numBuffers);\n-\tfor (uint32_t i = 0; i < numBuffers; i++)\n-\t\tbuffers_[i].buffer = camera3Request->output_buffers[i];\n+\t\tbuffers_.push_back({ stream, buffer.buffer, nullptr,\n+\t\t\t\t buffer.acquire_fence, Status::Pending });\n+\t}\n \n \t/* Clone the controls associated with the camera3 request. */\n \tsettings_ = CameraMetadata(camera3Request->settings);\ndiff --git a/src/android/camera_request.h b/src/android/camera_request.h\nindex a030febf..05dabf89 100644\n--- a/src/android/camera_request.h\n+++ b/src/android/camera_request.h\n@@ -20,6 +20,8 @@\n #include \"camera_metadata.h\"\n #include \"camera_worker.h\"\n \n+class CameraStream;\n+\n class Camera3RequestDescriptor\n {\n public:\n@@ -30,13 +32,11 @@ public:\n \t};\n \n \tstruct StreamBuffer {\n-\t\tcamera3_stream_buffer_t buffer;\n-\t\t/*\n-\t\t * FrameBuffer instances created by wrapping a camera3 provided\n-\t\t * dmabuf are emplaced in this vector of unique_ptr<> for\n-\t\t * lifetime management.\n-\t\t */\n+\t\tCameraStream *stream;\n+\t\tbuffer_handle_t *camera3Buffer;\n \t\tstd::unique_ptr<libcamera::FrameBuffer> frameBuffer;\n+\t\tint fence;\n+\t\tStatus status;\n \t};\n \n \tCamera3RequestDescriptor(libcamera::Camera *camera,\ndiff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\nindex f3cc77e7..9b5cd0c4 100644\n--- a/src/android/camera_stream.cpp\n+++ b/src/android/camera_stream.cpp\n@@ -147,11 +147,11 @@ int CameraStream::process(const FrameBuffer &source,\n \t\t\t Camera3RequestDescriptor *request)\n {\n \t/* Handle waiting on fences on the destination buffer. */\n-\tint fence = dest.buffer.acquire_fence;\n+\tint fence = dest.fence;\n \tif (fence != -1) {\n \t\tint ret = waitFence(fence);\n \t\t::close(fence);\n-\t\tdest.buffer.acquire_fence = -1;\n+\t\tdest.fence = -1;\n \t\tif (ret < 0) {\n \t\t\tLOG(HAL, Error) << \"Failed waiting for fence: \"\n \t\t\t\t\t<< fence << \": \" << strerror(-ret);\n@@ -167,7 +167,7 @@ int CameraStream::process(const FrameBuffer &source,\n \t * separate thread.\n \t */\n \tconst StreamConfiguration &output = configuration();\n-\tCameraBuffer destBuffer(*dest.buffer.buffer, output.pixelFormat,\n+\tCameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,\n \t\t\t\toutput.size, PROT_READ | PROT_WRITE);\n \tif (!destBuffer.isValid()) {\n \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n", "prefixes": [ "libcamera-devel", "v2", "09/12" ] }