[{"id":20299,"web_url":"https://patchwork.libcamera.org/comment/20299/","msgid":"<19ea7898-e59a-99b8-387e-7bc9a409eb03@ideasonboard.com>","date":"2021-10-19T11:02:50","subject":"Re: [libcamera-devel] [PATCH 09/11] android: camera_request: Don't\n\tembed full camera3_stream_buffer_t","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello,\n\nI didn't see review comments on this patch,\n\nOn 10/18/21 6:59 PM, Umang Jain wrote:\n> From: Laurent Pinchart \n>\n> The camera3_stream_buffer_t structure is meant to communicate between\n> the camera service and the HAL. They are short-live structures that\n> don't outlive the .process_capture_request() operation (when queuing\n> requests) or the .process_capture_result() callback.\n>\n> We currently store copies of the camera3_stream_buffer_t passed to\n> .process_capture_request() in Camera3RequestDescriptor::StreamBuffer to\n> store the structure members that the HAL need, and reuse them when\n> calling the .process_capture_result() callback. This is conceptually not\n> right, as the camera3_stream_buffer_t pass to the callback are not the\n> same objects as the ones received in .process_capture_request().\n>\n> Store individual fields of the camera3_stream_buffer_t in StreamBuffer\n> instead of copying the whole structure. This gives the HAL full control\n> of how data is stored, and properly decouples request queueing from\n> result reporting.\n>\n> Signed-off-by: Laurent Pinchart \n> Signed-off-by: Umang Jain \n\n\nAm i eligible to add my R-b? The patch looks sane to me:\n\nReviewed-by: Umang Jain\n\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(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 216f29c2..c493b0a4 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -830,16 +830,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> @@ -937,8 +929,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(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> @@ -973,11 +965,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> @@ -1083,12 +1075,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(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> @@ -1101,10 +1092,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> @@ -1151,33 +1141,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(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> @@ -1208,8 +1197,24 @@ void CameraDevice::sendCaptureResults()\n> \t\t\tcaptureResult.result = descriptor->resultMetadata_->get();\n> \n> \t\tstd::vector 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();\n> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> index 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 \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 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(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);\n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index 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 frameBuffer;\n> +\t\tint fence;\n> +\t\tStatus status;\n> \t};\n> \n> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 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\";","headers":{"Return-Path":"","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 CF590C324C\n\tfor ;\n\tTue, 19 Oct 2021 11:02:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F662604FF;\n\tTue, 19 Oct 2021 13:02:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48CAA604FE\n\tfor ;\n\tTue, 19 Oct 2021 13:02:55 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.98])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 64D8E12A;\n\tTue, 19 Oct 2021 13:02:54 +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=\"qavD0YVi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634641375;\n\tbh=v39PeqXw04wd6VMIlgQ0FuE7UUNRdPJNalMXFRNSOyI=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qavD0YVirD0wrLLLWeCm03c44T5LC6DqtirjsFt3O2NmMOa90dJFUUDrdYAV73OFw\n\tkqWOsLkhxXtVZcyYtDsmz+TgGlkU4U1vxnZzRuC3Ng4y3sxG+4Yj/5m+ivX0BUTUDm\n\tYuxf0oV8M0IqCixYpgPMt2bMVt2qKNTvM0P5tNsM=","To":"libcamera-devel@lists.libcamera.org","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-10-umang.jain@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<19ea7898-e59a-99b8-387e-7bc9a409eb03@ideasonboard.com>","Date":"Tue, 19 Oct 2021 16:32:50 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211018132923.476242-10-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 09/11] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20300,"web_url":"https://patchwork.libcamera.org/comment/20300/","msgid":"","date":"2021-10-19T11:15:44","subject":"Re: [libcamera-devel] [PATCH 09/11] android: camera_request: Don't\n\tembed full camera3_stream_buffer_t","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Oct 19, 2021 at 04:32:50PM +0530, Umang Jain wrote:\n> Hello,\n> \n> I didn't see review comments on this patch,\n> \n> On 10/18/21 6:59 PM, Umang Jain wrote:\n> > From: Laurent Pinchart \n> >\n> > The camera3_stream_buffer_t structure is meant to communicate between\n> > the camera service and the HAL. They are short-live structures that\n> > don't outlive the .process_capture_request() operation (when queuing\n> > requests) or the .process_capture_result() callback.\n> >\n> > We currently store copies of the camera3_stream_buffer_t passed to\n> > .process_capture_request() in Camera3RequestDescriptor::StreamBuffer to\n> > store the structure members that the HAL need, and reuse them when\n> > calling the .process_capture_result() callback. This is conceptually not\n> > right, as the camera3_stream_buffer_t pass to the callback are not the\n> > same objects as the ones received in .process_capture_request().\n> >\n> > Store individual fields of the camera3_stream_buffer_t in StreamBuffer\n> > instead of copying the whole structure. This gives the HAL full control\n> > of how data is stored, and properly decouples request queueing from\n> > result reporting.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Signed-off-by: Umang Jain \n> \n> Am i eligible to add my R-b? The patch looks sane to me:\n\nI think so, as you're not the author. We make the rules :-)\n\n> Reviewed-by: Umang Jain\n> \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(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 216f29c2..c493b0a4 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -830,16 +830,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> > @@ -937,8 +929,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(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> > @@ -973,11 +965,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> > @@ -1083,12 +1075,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(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> > @@ -1101,10 +1092,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> > @@ -1151,33 +1141,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(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> > @@ -1208,8 +1197,24 @@ void CameraDevice::sendCaptureResults()\n> > \t\t\tcaptureResult.result = descriptor->resultMetadata_->get();\n> > \n> > \t\tstd::vector 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();\n> > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> > index 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 \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 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(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);\n> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > index 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 frameBuffer;\n> > +\t\tint fence;\n> > +\t\tStatus status;\n> > \t};\n> > \n> > \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index 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\";","headers":{"Return-Path":"","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 4194AC323E\n\tfor ;\n\tTue, 19 Oct 2021 11:16:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3B2568F5B;\n\tTue, 19 Oct 2021 13:16:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80E1C604FE\n\tfor ;\n\tTue, 19 Oct 2021 13:16:03 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02ABE12A;\n\tTue, 19 Oct 2021 13:16:02 +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=\"FrNzgsIi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634642163;\n\tbh=M1wx2FTnLlz4hcv1bXV9SxD3nptG+XPomexhhQP4SzI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FrNzgsIiPDz5O/G0lzvk6B30Y/nf6BLmkRBRZ/tsKijIslDUsN79fVoxbxzc05GEj\n\tfnykwL1XCO+bS0WR+f9b/TYgLBkm3r/vvrNcXLwVrW1DlBqgZNOCKCCo1kXjk8kg5R\n\trvGQFRXhJgHfyjPLgwzytAuJh+V020+Tt8UVeOW8=","Date":"Tue, 19 Oct 2021 14:15:44 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-10-umang.jain@ideasonboard.com>\n\t<19ea7898-e59a-99b8-387e-7bc9a409eb03@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<19ea7898-e59a-99b8-387e-7bc9a409eb03@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 09/11] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]