[{"id":20305,"web_url":"https://patchwork.libcamera.org/comment/20305/","msgid":"<20211019121208.jef4ahbayckmaikh@uno.localdomain>","date":"2021-10-19T12:12:08","subject":"Re: [libcamera-devel] [PATCH v2 09/12] android: camera_request:\n\tDon't embed full camera3_stream_buffer_t","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi\n\nOn Tue, Oct 19, 2021 at 05:17:59PM +0530, 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> Reviewed-by: Umang Jain\n\nI'm not sure I get what is functionally different, we were copying in\nthe received camera3_stream_buffer_t, so there was no dependency with\nthe ones passed in by the service.\n\nAnyway, it shortne names, so I think it's good :)\nReviewed-by: Jacopo Mondi \n\nThanks\n j\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 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(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(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(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 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\";\n> --\n> 2.31.0\n>","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 58F26C323E\n\tfor ;\n\tTue, 19 Oct 2021 12:11:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ACDB868F5B;\n\tTue, 19 Oct 2021 14:11:20 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A646604FE\n\tfor ;\n\tTue, 19 Oct 2021 14:11:19 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id B5B40FF814;\n\tTue, 19 Oct 2021 12:11:18 +0000 (UTC)"],"Date":"Tue, 19 Oct 2021 14:12:08 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20211019121208.jef4ahbayckmaikh@uno.localdomain>","References":"<20211019114802.665980-1-umang.jain@ideasonboard.com>\n\t<20211019114802.665980-10-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211019114802.665980-10-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 09/12] android: camera_request:\n\tDon'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":",\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\" "}},{"id":20308,"web_url":"https://patchwork.libcamera.org/comment/20308/","msgid":"","date":"2021-10-19T12:16:26","subject":"Re: [libcamera-devel] [PATCH v2 09/12] android: camera_request:\n\tDon't embed 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":"Hi Jacopo,\n\nOn Tue, Oct 19, 2021 at 02:12:08PM +0200, Jacopo Mondi wrote:\n> On Tue, Oct 19, 2021 at 05:17:59PM +0530, 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> > Reviewed-by: Umang Jain\n> \n> I'm not sure I get what is functionally different, we were copying in\n> the received camera3_stream_buffer_t, so there was no dependency with\n> the ones passed in by the service.\n\nWe don't use the same instances as we copy them, but we reuse the value,\nwhich I don't think is conceptually correct. camera3_stream_buffer_t is\nmeant to pass buffer information in the API, it's not a buffer object in\nitself, so I don't think it's the right level of abstraction (and we\nhave to extend it with additional fields anyway).\n\n> Anyway, it shortne names, so I think it's good :)\n> Reviewed-by: Jacopo Mondi \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 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(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(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(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 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 26C56BF415\n\tfor ;\n\tTue, 19 Oct 2021 12:16:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92EC2604FE;\n\tTue, 19 Oct 2021 14:16:46 +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 3A75F604FE\n\tfor ;\n\tTue, 19 Oct 2021 14:16:45 +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 874A412A;\n\tTue, 19 Oct 2021 14:16:44 +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=\"kWs6gKNJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634645804;\n\tbh=HcDGsWDaanqbPPUkZVcMmr+hiQ2526l3yWFuF/5kegg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kWs6gKNJkMkNrL0WX0GpmkLAbCgh5DNFRakKWgjInxBmo0XksQy2Mfx7MLw2r2NRP\n\tCIL/l1eE20lMLxzvqrxHFTsuBBAithbj+MTHlKb37rbUWQrUic4ng8Sdd+kEQbjITJ\n\tcgMNZT8hfkOOKbG8tDu/WCelvxnDbM8xHY9YmMKc=","Date":"Tue, 19 Oct 2021 15:16:26 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211019114802.665980-1-umang.jain@ideasonboard.com>\n\t<20211019114802.665980-10-umang.jain@ideasonboard.com>\n\t<20211019121208.jef4ahbayckmaikh@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211019121208.jef4ahbayckmaikh@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 09/12] android: camera_request:\n\tDon'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":",\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\" "}},{"id":20310,"web_url":"https://patchwork.libcamera.org/comment/20310/","msgid":"","date":"2021-10-20T02:13:30","subject":"Re: [libcamera-devel] [PATCH v2 09/12] android: camera_request:\n\tDon't embed full camera3_stream_buffer_t","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Tue, Oct 19, 2021 at 9:16 PM Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> On Tue, Oct 19, 2021 at 02:12:08PM +0200, Jacopo Mondi wrote:\n> > On Tue, Oct 19, 2021 at 05:17:59PM +0530, 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> > > Reviewed-by: Umang Jain\n> >\n> > I'm not sure I get what is functionally different, we were copying in\n> > the received camera3_stream_buffer_t, so there was no dependency with\n> > the ones passed in by the service.\n>\n> We don't use the same instances as we copy them, but we reuse the value,\n> which I don't think is conceptually correct. camera3_stream_buffer_t is\n> meant to pass buffer information in the API, it's not a buffer object in\n> itself, so I don't think it's the right level of abstraction (and we\n> have to extend it with additional fields anyway).\n>\n> > Anyway, it shortne names, so I think it's good :)\n> > Reviewed-by: Jacopo Mondi \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 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> > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > >\n> > > - for (auto &buffer : descriptor->buffers_) {\n> > > - /*\n> > > - * Signal to the framework it has to handle fences that have not\n> > > - * been waited on by setting the release fence to the acquire\n> > > - * fence value.\n> > > - */\n> > > - buffer.buffer.release_fence = buffer.buffer.acquire_fence;\n> > > - buffer.buffer.acquire_fence = -1;\n> > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > - }\n> > > + for (auto &buffer : descriptor->buffers_)\n> > > + buffer.status = Camera3RequestDescriptor::Status::Error;\n> > >\n> > > descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > }\n> > > @@ -908,8 +900,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > << \" with \" << descriptor->buffers_.size() << \" streams\";\n> > >\n> > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> > > - camera3_stream *camera3Stream = buffer.buffer.stream;\n> > > - CameraStream *cameraStream = static_cast(camera3Stream->priv);\n> > > + CameraStream *cameraStream = buffer.stream;\n> > > + camera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> > >\n> > > std::stringstream ss;\n> > > ss << i << \" - (\" << camera3Stream->width << \"x\"\n> > > @@ -944,11 +936,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > * lifetime management only.\n> > > */\n> > > buffer.frameBuffer =\n> > > - createFrameBuffer(*buffer.buffer.buffer,\n> > > + createFrameBuffer(*buffer.camera3Buffer,\n> > > cameraStream->configuration().pixelFormat,\n> > > cameraStream->configuration().size);\n> > > frameBuffer = buffer.frameBuffer.get();\n> > > - acquireFence = buffer.buffer.acquire_fence;\n> > > + acquireFence = buffer.fence;\n> > > LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > > break;\n> > >\n> > > @@ -1055,12 +1047,11 @@ void CameraDevice::requestComplete(Request *request)\n> > > /*\n> > > * Prepare the capture result for the Android camera stack.\n> > > *\n> > > - * The buffer status is set to OK and later changed to ERROR if\n> > > + * The buffer status is set to Success and later changed to Error if\n> > > * post-processing/compression fails.\n> > > */\n> > > for (auto &buffer : descriptor->buffers_) {\n> > > - CameraStream *cameraStream =\n> > > - static_cast(buffer.buffer.stream->priv);\n> > > + CameraStream *stream = buffer.stream;\n> > >\n> > > /*\n> > > * Streams of type Direct have been queued to the\n> > > @@ -1073,10 +1064,9 @@ void CameraDevice::requestComplete(Request *request)\n> > > * \\todo Instrument the CameraWorker to set the acquire\n> > > * fence to -1 once it has handled it and remove this check.\n> > > */\n> > > - if (cameraStream->type() == CameraStream::Type::Direct)\n> > > - buffer.buffer.acquire_fence = -1;\n> > > - buffer.buffer.release_fence = -1;\n> > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > > + if (stream->type() == CameraStream::Type::Direct)\n> > > + buffer.fence = -1;\n> > > + buffer.status = Camera3RequestDescriptor::Status::Success;\n> > > }\n> > >\n> > > /*\n> > > @@ -1128,33 +1118,32 @@ void CameraDevice::requestComplete(Request *request)\n> > >\n> > > /* Handle post-processing. */\n> > > for (auto &buffer : descriptor->buffers_) {\n> > > - CameraStream *cameraStream =\n> > > - static_cast(buffer.buffer.stream->priv);\n> > > + CameraStream *stream = buffer.stream;\n> > >\n> > > - if (cameraStream->type() == CameraStream::Type::Direct)\n> > > + if (stream->type() == CameraStream::Type::Direct)\n> > > continue;\n> > >\n> > > - FrameBuffer *src = request->findBuffer(cameraStream->stream());\n> > > + FrameBuffer *src = request->findBuffer(stream->stream());\n> > > if (!src) {\n> > > LOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > - notifyError(descriptor->frameNumber_, buffer.buffer.stream,\n> > > + buffer.status = Camera3RequestDescriptor::Status::Error;\n> > > + notifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> > > CAMERA3_MSG_ERROR_BUFFER);\n> > > continue;\n> > > }\n> > >\n> > > - int ret = cameraStream->process(*src, buffer, descriptor);\n> > > + int ret = stream->process(*src, buffer, descriptor);\n> > >\n> > > /*\n> > > * Return the FrameBuffer to the CameraStream now that we're\n> > > * done processing it.\n> > > */\n> > > - if (cameraStream->type() == CameraStream::Type::Internal)\n> > > - cameraStream->putBuffer(src);\n> > > + if (stream->type() == CameraStream::Type::Internal)\n> > > + stream->putBuffer(src);\n> > >\n> > > if (ret) {\n> > > - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > - notifyError(descriptor->frameNumber_, buffer.buffer.stream,\n> > > + buffer.status = Camera3RequestDescriptor::Status::Error;\n> > > + notifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> > > CAMERA3_MSG_ERROR_BUFFER);\n> > > }\n> > > }\n> > > @@ -1185,8 +1174,24 @@ void CameraDevice::sendCaptureResults()\n> > > captureResult.result = descriptor->resultMetadata_->get();\n> > >\n> > > std::vector resultBuffers;\n> > > - for (const auto &buffer : descriptor->buffers_)\n> > > - resultBuffers.emplace_back(buffer.buffer);\n> > > + resultBuffers.reserve(descriptor->buffers_.size());\n> > > +\n\nIf I read the change of this patch series correctly, when\nabortRequest() is called, this for-loop is not reachable?\n\n> > > + for (const auto &buffer : descriptor->buffers_) {\n> > > + camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > +\n> > > + if (buffer.status == Camera3RequestDescriptor::Status::Success)\n> > > + status = CAMERA3_BUFFER_STATUS_OK;\n> > > +\n> > > + /*\n> > > + * Pass the buffer fence back to the camera framework as\n> > > + * a release fence. This instructs the framework to wait\n> > > + * on the acquire fence in case we haven't done so\n> > > + * ourselves for any reason.\n> > > + */\n> > > + resultBuffers.push_back({ buffer.stream->camera3Stream(),\n> > > + buffer.camera3Buffer, status,\n> > > + -1, buffer.fence });\n> > > + }\n> > >\n> > > captureResult.num_output_buffers = resultBuffers.size();\n> > > captureResult.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> > > frameNumber_ = camera3Request->frame_number;\n> > >\n> > > /* Copy the camera3 request stream information for later access. */\n> > > - const uint32_t numBuffers = camera3Request->num_output_buffers;\n> > > + const Span buffers{\n> > > + camera3Request->output_buffers,\n> > > + camera3Request->num_output_buffers\n> > > + };\n> > > +\n> > > + buffers_.reserve(buffers.size());\n> > > +\n> > > + for (const camera3_stream_buffer_t &buffer : buffers) {\n> > > + CameraStream *stream =\n> > > + static_cast(buffer.stream->priv);\n> > >\n> > > - buffers_.resize(numBuffers);\n> > > - for (uint32_t i = 0; i < numBuffers; i++)\n> > > - buffers_[i].buffer = camera3Request->output_buffers[i];\n> > > + buffers_.push_back({ stream, buffer.buffer, nullptr,\n> > > + buffer.acquire_fence, Status::Pending });\n> > > + }\n> > >\n> > > /* Clone the controls associated with the camera3 request. */\n> > > settings_ = 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> > > };\n> > >\n> > > struct StreamBuffer {\n> > > - camera3_stream_buffer_t buffer;\n> > > - /*\n> > > - * FrameBuffer instances created by wrapping a camera3 provided\n> > > - * dmabuf are emplaced in this vector of unique_ptr<> for\n> > > - * lifetime management.\n> > > - */\n\n\nWould you mind commenting the ownership of these variables?\n\nReviewed-by: Hirokazu Honda \n\nThanks,\n-Hiro\n> > > + CameraStream *stream;\n> > > + buffer_handle_t *camera3Buffer;\n> > > std::unique_ptr frameBuffer;\n> > > + int fence;\n> > > + Status status;\n> > > };\n> > >\n> > > Camera3RequestDescriptor(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> > > Camera3RequestDescriptor *request)\n> > > {\n> > > /* Handle waiting on fences on the destination buffer. */\n> > > - int fence = dest.buffer.acquire_fence;\n> > > + int fence = dest.fence;\n> > > if (fence != -1) {\n> > > int ret = waitFence(fence);\n> > > ::close(fence);\n> > > - dest.buffer.acquire_fence = -1;\n> > > + dest.fence = -1;\n> > > if (ret < 0) {\n> > > LOG(HAL, Error) << \"Failed waiting for fence: \"\n> > > << fence << \": \" << strerror(-ret);\n> > > @@ -167,7 +167,7 @@ int CameraStream::process(const FrameBuffer &source,\n> > > * separate thread.\n> > > */\n> > > const StreamConfiguration &output = configuration();\n> > > - CameraBuffer destBuffer(*dest.buffer.buffer, output.pixelFormat,\n> > > + CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,\n> > > output.size, PROT_READ | PROT_WRITE);\n> > > if (!destBuffer.isValid()) {\n> > > LOG(HAL, Error) << \"Failed to create destination buffer\";\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 E5F66BF415\n\tfor ;\n\tWed, 20 Oct 2021 02:13:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D0C268F5B;\n\tWed, 20 Oct 2021 04:13:42 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ABD2860125\n\tfor ;\n\tWed, 20 Oct 2021 04:13:40 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id r18so20378449edv.12\n\tfor ;\n\tTue, 19 Oct 2021 19:13:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"deV8EVWJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=mA9vhNCzZSuP+zKaux+av/QaIJVxZc+BET6Nda7U9zw=;\n\tb=deV8EVWJJx19glfeU9Qx86wmunN4AUQd075eDnrW6/7M8suvFcgt5OtwqMt1v+R8DK\n\to6jaX6Pl2+ect7UmibhTTpEfm2+Dq/GRGiILKolU9umolNRyoSGlGpbbcLyl1f2ts9N3\n\tN6YVm1z7Bc2vteggIzKcRE2kCZxH1V4M2Ufmk=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=mA9vhNCzZSuP+zKaux+av/QaIJVxZc+BET6Nda7U9zw=;\n\tb=25CZlxuN7VcBnc94KmfhXqLP4kQz5nZOdfDyjOlNq1nHE+ENzwGoY69n8WkXwKbx70\n\t0D8TH1NgJ9iYcN00OJls7AsFFxUd54+XAs7ZVki0bT9L14GwTWh2TIKwZWz0yHdSpK83\n\tnktMEbcGj1shhxMVXD0gyTFJueG68k6lM2qzQ8MkNy5Z/UwlPCAi5XrKWlia0eK+x/4F\n\tqkpDY75K/h2EVWFQjc/l4VFJuUOgVwcRVGBrSLWA/IitZzAFrALjr8wmDTRMAUhSqQn2\n\t5yyy+q0dfy/bLh1ye2PmhkY6SecpZOHmkAOufJYTF42CXCEVl4dQp5GBTI6twMl7aDVn\n\tqx8w==","X-Gm-Message-State":"AOAM532PXZUn1FsJqLz+jx+WD2kU/dhESQSdS9cnlRnxECID+UFlVZDQ\n\tK+PEUAYepf2hf1DL/CniAsuPs8rhhmPAPXyk7tEtD013OnOj9w==","X-Google-Smtp-Source":"ABdhPJyPjo411MJeKnogLerkBfHcujuAtUmy+D76z0N5BvWhuZd/heWIIpShgW0sRczevWyPo6+oP6EFKThFjmeeXug=","X-Received":"by 2002:a05:6402:3554:: with SMTP id\n\tf20mr58909518edd.354.1634696020127; \n\tTue, 19 Oct 2021 19:13:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20211019114802.665980-1-umang.jain@ideasonboard.com>\n\t<20211019114802.665980-10-umang.jain@ideasonboard.com>\n\t<20211019121208.jef4ahbayckmaikh@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 20 Oct 2021 11:13:30 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 09/12] android: camera_request:\n\tDon'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":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]