[{"id":32434,"web_url":"https://patchwork.libcamera.org/comment/32434/","msgid":"","date":"2024-11-28T14:24:44","subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harevy\n\nOn Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote:\n> Previously there's a potential issue in StreamBuffer::internalBuffer's\n> lifetime, when more than one streams depend on the same internal buffer.\n\ns/.//\n\nlifetime, when more than one streams depend on the same internal buffer\nfor post-processing, the buffer is returned to the CameraStream pool\nas soon as the first post-processing request has completed.\n\n>\n> This patch fixes the issue by returning the buffer when the whole\n> Camera3RequestDescriptor is destructed.\n\nActually it does more I guess, it allows to map multiple streams of\ntype ::Mapped to the same libcamera::Stream, doesn't it ?\n\n>\n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n> ---\n> src/android/camera_device.cpp | 63 +++++++++++++++++++---------------\n> src/android/camera_request.cpp | 13 ++++---\n> src/android/camera_request.h | 3 +-\n> 3 files changed, 46 insertions(+), 33 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 11613fac9..62f724041 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t * to a libcamera stream. Streams of type Mapped will be handled later.\n> \t *\n> \t * Collect the CameraStream associated to each requested capture stream.\n> -\t * Since requestedStreams is an std:map<>, no duplications can happen.\n> +\t * Since requestedDirectBuffers is an std:map<>, no duplications can\n> +\t * happen.\n> \t */\n> -\tstd::map requestedBuffers;\n> +\tstd::map requestedDirectBuffers;\n\nDo you actually need to rename ?\n\n> \tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> \t\tCameraStream *cameraStream = buffer.stream;\n> \t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\tframeBuffer = buffer.frameBuffer.get();\n> \t\t\tacquireFence = std::move(buffer.fence);\n>\n> -\t\t\trequestedBuffers[cameraStream] = frameBuffer;\n> +\t\t\trequestedDirectBuffers[cameraStream] = frameBuffer;\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> \t\t\tbreak;\n>\n> @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t * Get the frame buffer from the CameraStream internal\n> \t\t\t * buffer pool.\n> \t\t\t *\n> -\t\t\t * The buffer has to be returned to the CameraStream\n> -\t\t\t * once it has been processed.\n> +\t\t\t * The buffer will be returned to the CameraStream when\n> +\t\t\t * the request is destructed.\n\nor 'destroyed'\n\n> \t\t\t */\n> \t\t\tframeBuffer = cameraStream->getBuffer();\n> -\t\t\tbuffer.internalBuffer = frameBuffer;\n> \t\t\tbuffer.srcBuffer = frameBuffer;\n>\n> -\t\t\trequestedBuffers[cameraStream] = frameBuffer;\n> +\t\t\t/*\n> +\t\t\t * Track the allocated internal buffers, which will be\n> +\t\t\t * recycled when the descriptor destroyed.\n> +\t\t\t * */\n\nStray * */\n\n> +\t\t\tdescriptor->internalBuffers_[cameraStream] = frameBuffer;\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n>\n> \t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t * post-processing. No need to recycle the buffer since it's\n> \t\t * owned by Android.\n> \t\t */\n> -\t\tauto iterDirectBuffer = requestedBuffers.find(sourceStream);\n> -\t\tif (iterDirectBuffer != requestedBuffers.end()) {\n> +\t\tauto iterDirectBuffer = requestedDirectBuffers.find(sourceStream);\n> +\t\tif (iterDirectBuffer != requestedDirectBuffers.end()) {\n> \t\t\tbuffer.srcBuffer = iterDirectBuffer->second;\n> \t\t\tcontinue;\n> \t\t}\n>\n> \t\t/*\n> -\t\t * If that's not the case, we need to add a buffer to the request\n> -\t\t * for this stream.\n> +\t\t * If that's not the case, we use an internal buffer allocated\n> +\t\t * from the source stream.\n> +\t\t *\n> +\t\t * If an internal buffer has been requested for the source\n> +\t\t * stream before, we should reuse it.\n> +\t\t */\n> +\t\tauto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);\n> +\t\tif (iterInternalBuffer != descriptor->internalBuffers_.end()) {\n> +\t\t\tbuffer.srcBuffer = iterInternalBuffer->second;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Otherwise, we need to create an internal buffer to the\n> +\t\t * request for the source stream. Get the frame buffer from the\n> +\t\t * source stream's internal buffer pool.\n> +\t\t *\n> +\t\t * The buffer will be returned to the CameraStream when the\n> +\t\t * request is destructed.\n> \t\t */\n> -\t\tFrameBuffer *frameBuffer = cameraStream->getBuffer();\n> -\t\tbuffer.internalBuffer = frameBuffer;\n> +\t\tFrameBuffer *frameBuffer = sourceStream->getBuffer();\n\nHere I don't see why we're using the sourceStream's pool instead\nof the cameraStream's pool\n\nHowever, I think this goes in the direction of making the HAL capable\nof more things, and I'm looking at the diff of this and the previous\npatch combined and I think you should squash the two and make a commit\nabout \"Enabling multiple processed streams to map to the same internal\nbuffer\".\n\nLooking at the combined diff, it feels to me you could populate\nbuffer.src for the Direct case in the switch() and keep using an\nstd::set for requestedDirectBuffers (or requestedStreams). This would\nmake the diff smaller probably.\n\n\n> \t\tbuffer.srcBuffer = frameBuffer;\n>\n> \t\tdescriptor->request_->addBuffer(sourceStream->stream(),\n> \t\t\t\t\t\tframeBuffer, nullptr);\n>\n> -\t\trequestedBuffers[sourceStream] = frameBuffer;\n> +\t\t/* Track the allocated internal buffer. */\n> +\t\tdescriptor->internalBuffers_[sourceStream] = frameBuffer;\n> \t}\n>\n> \t/*\n> @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tif (ret) {\n> \t\t\tsetBufferStatus(*buffer, StreamBuffer::Status::Error);\n> \t\t\tdescriptor->pendingStreamsToProcess_.erase(stream);\n> -\n> -\t\t\t/*\n> -\t\t\t * If the framebuffer is internal to CameraStream return\n> -\t\t\t * it back now that we're done processing it.\n> -\t\t\t */\n> -\t\t\tif (buffer->internalBuffer)\n> -\t\t\t\tstream->putBuffer(buffer->internalBuffer);\n> \t\t}\n> \t}\n>\n> @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer,\n> {\n> \tsetBufferStatus(*streamBuffer, status);\n>\n> -\t/*\n> -\t * If the framebuffer is internal to CameraStream return it back now\n> -\t * that we're done processing it.\n> -\t */\n> -\tif (streamBuffer->internalBuffer)\n> -\t\tstreamBuffer->stream->putBuffer(streamBuffer->internalBuffer);\n> -\n> \tCamera3RequestDescriptor *request = streamBuffer->request;\n>\n> \t{\n> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> index 52a3ac1f7..a9240a83c 100644\n> --- a/src/android/camera_request.cpp\n> +++ b/src/android/camera_request.cpp\n> @@ -10,6 +10,7 @@\n> #include \n>\n> #include \"camera_buffer.h\"\n> +#include \"camera_stream.h\"\n>\n> using namespace libcamera;\n>\n> @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \trequest_ = camera->createRequest(reinterpret_cast(this));\n> }\n>\n> -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> +Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> +{\n> +\t/*\n> +\t * Recycle the allocated internal buffer back to its source stream.\n> +\t */\n\nFits in one line most probably\n\n> +\tfor (auto &[sourceStream, frameBuffer] : internalBuffers_)\n> +\t\tsourceStream->putBuffer(frameBuffer);\n> +}\n>\n> /**\n> * \\class StreamBuffer\n> @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> * \\var StreamBuffer::status\n> * \\brief Track the status of the buffer\n> *\n> - * \\var StreamBuffer::internalBuffer\n> - * \\brief Pointer to a buffer internally handled by CameraStream (if any)\n> - *\n> * \\var StreamBuffer::srcBuffer\n> * \\brief Pointer to the source frame buffer used for post-processing\n> *\n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index 335f1985d..6b2a00795 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -49,7 +49,6 @@ public:\n> \tstd::unique_ptr frameBuffer;\n> \tlibcamera::UniqueFD fence;\n> \tStatus status = Status::Success;\n> -\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> \tconst libcamera::FrameBuffer *srcBuffer = nullptr;\n> \tstd::unique_ptr dstBuffer;\n> \tCamera3RequestDescriptor *request;\n> @@ -85,6 +84,8 @@ public:\n> \tstd::unique_ptr request_;\n> \tstd::unique_ptr resultMetadata_;\n>\n> +\tstd::map internalBuffers_;\n> +\n> \tbool complete_ = false;\n> \tStatus status_ = Status::Success;\n>\n> --\n> 2.47.0.338.g60cca15819-goog\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 51564BD78E\n\tfor ;\n\tThu, 28 Nov 2024 14:24:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39AD465FA9;\n\tThu, 28 Nov 2024 15:24:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D2DB565898\n\tfor ;\n\tThu, 28 Nov 2024 15:24:47 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E3EE959D;\n\tThu, 28 Nov 2024 15:24:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QJvFAU5C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732803864;\n\tbh=EqEN+/B/2DkFhdvrOV6XUJ01WjBZcpiw5ejBksznlF4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QJvFAU5CwaLYSjiudwtcDDokkAM3DT1fnBWKMJWwnqKNfTjBnN7yqvHAVo5utvrly\n\t2I3DBuf4i/wrft4wpk/vibeS/gPJZeWGsxwzC+p7yNEdJy7U6vCly+PsIeQY/LDoqN\n\tP/EWXz6TFH6y52SKqnSmYy8YUP7rQRnhXbZ/jZbM=","Date":"Thu, 28 Nov 2024 15:24:44 +0100","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","Message-ID":"","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-5-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241127092632.3145984-5-chenghaoyang@chromium.org>","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":32435,"web_url":"https://patchwork.libcamera.org/comment/32435/","msgid":"<20241128143242.GA29106@pendragon.ideasonboard.com>","date":"2024-11-28T14:32:42","subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Nov 28, 2024 at 03:24:44PM +0100, Jacopo Mondi wrote:\n> Hi Harevy\n> \n> On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote:\n> > Previously there's a potential issue in StreamBuffer::internalBuffer's\n> > lifetime, when more than one streams depend on the same internal buffer.\n> \n> s/.//\n> \n> lifetime, when more than one streams depend on the same internal buffer\n> for post-processing, the buffer is returned to the CameraStream pool\n> as soon as the first post-processing request has completed.\n> \n> >\n> > This patch fixes the issue by returning the buffer when the whole\n> > Camera3RequestDescriptor is destructed.\n> \n> Actually it does more I guess, it allows to map multiple streams of\n> type ::Mapped to the same libcamera::Stream, doesn't it ?\n> \n> >\n> > Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > Signed-off-by: Harvey Yang \n> > ---\n> > src/android/camera_device.cpp | 63 +++++++++++++++++++---------------\n> > src/android/camera_request.cpp | 13 ++++---\n> > src/android/camera_request.h | 3 +-\n> > 3 files changed, 46 insertions(+), 33 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 11613fac9..62f724041 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t * to a libcamera stream. Streams of type Mapped will be handled later.\n> > \t *\n> > \t * Collect the CameraStream associated to each requested capture stream.\n> > -\t * Since requestedStreams is an std:map<>, no duplications can happen.\n> > +\t * Since requestedDirectBuffers is an std:map<>, no duplications can\n> > +\t * happen.\n> > \t */\n> > -\tstd::map requestedBuffers;\n> > +\tstd::map requestedDirectBuffers;\n> \n> Do you actually need to rename ?\n> \n> > \tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> > \t\tCameraStream *cameraStream = buffer.stream;\n> > \t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t\tframeBuffer = buffer.frameBuffer.get();\n> > \t\t\tacquireFence = std::move(buffer.fence);\n> >\n> > -\t\t\trequestedBuffers[cameraStream] = frameBuffer;\n> > +\t\t\trequestedDirectBuffers[cameraStream] = frameBuffer;\n> > \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > \t\t\tbreak;\n> >\n> > @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t\t * Get the frame buffer from the CameraStream internal\n> > \t\t\t * buffer pool.\n> > \t\t\t *\n> > -\t\t\t * The buffer has to be returned to the CameraStream\n> > -\t\t\t * once it has been processed.\n> > +\t\t\t * The buffer will be returned to the CameraStream when\n> > +\t\t\t * the request is destructed.\n> \n> or 'destroyed'\n> \n> > \t\t\t */\n> > \t\t\tframeBuffer = cameraStream->getBuffer();\n> > -\t\t\tbuffer.internalBuffer = frameBuffer;\n> > \t\t\tbuffer.srcBuffer = frameBuffer;\n> >\n> > -\t\t\trequestedBuffers[cameraStream] = frameBuffer;\n> > +\t\t\t/*\n> > +\t\t\t * Track the allocated internal buffers, which will be\n> > +\t\t\t * recycled when the descriptor destroyed.\n> > +\t\t\t * */\n> \n> Stray * */\n> \n> > +\t\t\tdescriptor->internalBuffers_[cameraStream] = frameBuffer;\n> > \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> >\n> > \t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> > @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t * post-processing. No need to recycle the buffer since it's\n> > \t\t * owned by Android.\n> > \t\t */\n> > -\t\tauto iterDirectBuffer = requestedBuffers.find(sourceStream);\n> > -\t\tif (iterDirectBuffer != requestedBuffers.end()) {\n> > +\t\tauto iterDirectBuffer = requestedDirectBuffers.find(sourceStream);\n> > +\t\tif (iterDirectBuffer != requestedDirectBuffers.end()) {\n> > \t\t\tbuffer.srcBuffer = iterDirectBuffer->second;\n> > \t\t\tcontinue;\n> > \t\t}\n> >\n> > \t\t/*\n> > -\t\t * If that's not the case, we need to add a buffer to the request\n> > -\t\t * for this stream.\n> > +\t\t * If that's not the case, we use an internal buffer allocated\n> > +\t\t * from the source stream.\n> > +\t\t *\n> > +\t\t * If an internal buffer has been requested for the source\n> > +\t\t * stream before, we should reuse it.\n> > +\t\t */\n> > +\t\tauto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);\n> > +\t\tif (iterInternalBuffer != descriptor->internalBuffers_.end()) {\n> > +\t\t\tbuffer.srcBuffer = iterInternalBuffer->second;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Otherwise, we need to create an internal buffer to the\n> > +\t\t * request for the source stream. Get the frame buffer from the\n> > +\t\t * source stream's internal buffer pool.\n> > +\t\t *\n> > +\t\t * The buffer will be returned to the CameraStream when the\n> > +\t\t * request is destructed.\n> > \t\t */\n> > -\t\tFrameBuffer *frameBuffer = cameraStream->getBuffer();\n> > -\t\tbuffer.internalBuffer = frameBuffer;\n> > +\t\tFrameBuffer *frameBuffer = sourceStream->getBuffer();\n> \n> Here I don't see why we're using the sourceStream's pool instead\n> of the cameraStream's pool\n> \n> However, I think this goes in the direction of making the HAL capable\n> of more things, and I'm looking at the diff of this and the previous\n> patch combined and I think you should squash the two and make a commit\n> about \"Enabling multiple processed streams to map to the same internal\n> buffer\".\n\nProcessed streams are expensive, is there enough CPU power to make that\npossible ? I suppose it wouldn't be an issue when producing, for\ninstance, a processed JPEG stream with a hardware encoder and a scaled\nYUV stream with libyuv from the same stream.\n\nThis is interesting, but isn't mentioned at all in the cover letter, and\ngoes well beyond the subject of the series. Is is an intentional change\n?\n\n> Looking at the combined diff, it feels to me you could populate\n> buffer.src for the Direct case in the switch() and keep using an\n> std::set for requestedDirectBuffers (or requestedStreams). This would\n> make the diff smaller probably.\n> \n> \n> > \t\tbuffer.srcBuffer = frameBuffer;\n> >\n> > \t\tdescriptor->request_->addBuffer(sourceStream->stream(),\n> > \t\t\t\t\t\tframeBuffer, nullptr);\n> >\n> > -\t\trequestedBuffers[sourceStream] = frameBuffer;\n> > +\t\t/* Track the allocated internal buffer. */\n> > +\t\tdescriptor->internalBuffers_[sourceStream] = frameBuffer;\n> > \t}\n> >\n> > \t/*\n> > @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\tif (ret) {\n> > \t\t\tsetBufferStatus(*buffer, StreamBuffer::Status::Error);\n> > \t\t\tdescriptor->pendingStreamsToProcess_.erase(stream);\n> > -\n> > -\t\t\t/*\n> > -\t\t\t * If the framebuffer is internal to CameraStream return\n> > -\t\t\t * it back now that we're done processing it.\n> > -\t\t\t */\n> > -\t\t\tif (buffer->internalBuffer)\n> > -\t\t\t\tstream->putBuffer(buffer->internalBuffer);\n> > \t\t}\n> > \t}\n> >\n> > @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer,\n> > {\n> > \tsetBufferStatus(*streamBuffer, status);\n> >\n> > -\t/*\n> > -\t * If the framebuffer is internal to CameraStream return it back now\n> > -\t * that we're done processing it.\n> > -\t */\n> > -\tif (streamBuffer->internalBuffer)\n> > -\t\tstreamBuffer->stream->putBuffer(streamBuffer->internalBuffer);\n> > -\n> > \tCamera3RequestDescriptor *request = streamBuffer->request;\n> >\n> > \t{\n> > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> > index 52a3ac1f7..a9240a83c 100644\n> > --- a/src/android/camera_request.cpp\n> > +++ b/src/android/camera_request.cpp\n> > @@ -10,6 +10,7 @@\n> > #include \n> >\n> > #include \"camera_buffer.h\"\n> > +#include \"camera_stream.h\"\n> >\n> > using namespace libcamera;\n> >\n> > @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > \trequest_ = camera->createRequest(reinterpret_cast(this));\n> > }\n> >\n> > -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > +Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > +{\n> > +\t/*\n> > +\t * Recycle the allocated internal buffer back to its source stream.\n> > +\t */\n> \n> Fits in one line most probably\n> \n> > +\tfor (auto &[sourceStream, frameBuffer] : internalBuffers_)\n> > +\t\tsourceStream->putBuffer(frameBuffer);\n> > +}\n> >\n> > /**\n> > * \\class StreamBuffer\n> > @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > * \\var StreamBuffer::status\n> > * \\brief Track the status of the buffer\n> > *\n> > - * \\var StreamBuffer::internalBuffer\n> > - * \\brief Pointer to a buffer internally handled by CameraStream (if any)\n> > - *\n> > * \\var StreamBuffer::srcBuffer\n> > * \\brief Pointer to the source frame buffer used for post-processing\n> > *\n> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > index 335f1985d..6b2a00795 100644\n> > --- a/src/android/camera_request.h\n> > +++ b/src/android/camera_request.h\n> > @@ -49,7 +49,6 @@ public:\n> > \tstd::unique_ptr frameBuffer;\n> > \tlibcamera::UniqueFD fence;\n> > \tStatus status = Status::Success;\n> > -\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> > \tconst libcamera::FrameBuffer *srcBuffer = nullptr;\n> > \tstd::unique_ptr dstBuffer;\n> > \tCamera3RequestDescriptor *request;\n> > @@ -85,6 +84,8 @@ public:\n> > \tstd::unique_ptr request_;\n> > \tstd::unique_ptr resultMetadata_;\n> >\n> > +\tstd::map internalBuffers_;\n> > +\n> > \tbool complete_ = false;\n> > \tStatus status_ = Status::Success;\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 DBDECBDE6B\n\tfor ;\n\tThu, 28 Nov 2024 14:32:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B28065FA9;\n\tThu, 28 Nov 2024 15:32:54 +0100 (CET)","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 DDBDE65898\n\tfor ;\n\tThu, 28 Nov 2024 15:32:52 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C05CA59D;\n\tThu, 28 Nov 2024 15:32:28 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"C7s+BGbr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732804349;\n\tbh=o1LFUClIs9NS6+3Fr8/9W2gDKqYUvftyv6+5r7GdiqM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C7s+BGbrv4vGCfQkrDVM+Uq3EsfKdmOvr2LHO3WNqe35zlyfoFZOY8bvhu6e2s5Ts\n\tEYP/bwG/RSTVfJY2f1fMKnNjb1zf3bYAyQDCtch3jaVv/ByJDPo6RGAk2YcifVbCuu\n\tGGx0c+z+QfclPMENK5QfZmMWvn+dP1fiT59OK2N8=","Date":"Thu, 28 Nov 2024 16:32:42 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","Message-ID":"<20241128143242.GA29106@pendragon.ideasonboard.com>","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-5-chenghaoyang@chromium.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":32449,"web_url":"https://patchwork.libcamera.org/comment/32449/","msgid":"","date":"2024-11-29T07:51:39","subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo and Laurent,\n\nOn Thu, Nov 28, 2024 at 10:32 PM Laurent Pinchart\n wrote:\n>\n> On Thu, Nov 28, 2024 at 03:24:44PM +0100, Jacopo Mondi wrote:\n> > Hi Harevy\n> >\n> > On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote:\n> > > Previously there's a potential issue in StreamBuffer::internalBuffer's\n> > > lifetime, when more than one streams depend on the same internal buffer.\n> >\n> > s/.//\n> >\n> > lifetime, when more than one streams depend on the same internal buffer\n> > for post-processing, the buffer is returned to the CameraStream pool\n> > as soon as the first post-processing request has completed.\n\nUpdated.\n\n> >\n> > >\n> > > This patch fixes the issue by returning the buffer when the whole\n> > > Camera3RequestDescriptor is destructed.\n> >\n> > Actually it does more I guess, it allows to map multiple streams of\n> > type ::Mapped to the same libcamera::Stream, doesn't it ?\n\nAccording to the previous implementation, I'd say that's already\nallowed, just not correctly implemented yet.\nLooking into [1], the condition doesn't care if the sourceStream\nhas been mapped by another mapped stream or not.\n\nUnless you mean that because there's such a bug, it's not allowed...\nI'd say that we should have a FATAL/ERROR log at least, or even\nblock such a configuration.\n\n[1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1071\n\n\n> >\n> > >\n> > > Signed-off-by: Han-Lin Chen \n> > > Co-developed-by: Harvey Yang \n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > src/android/camera_device.cpp | 63 +++++++++++++++++++---------------\n> > > src/android/camera_request.cpp | 13 ++++---\n> > > src/android/camera_request.h | 3 +-\n> > > 3 files changed, 46 insertions(+), 33 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 11613fac9..62f724041 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > * to a libcamera stream. Streams of type Mapped will be handled later.\n> > > *\n> > > * Collect the CameraStream associated to each requested capture stream.\n> > > - * Since requestedStreams is an std:map<>, no duplications can happen.\n> > > + * Since requestedDirectBuffers is an std:map<>, no duplications can\n> > > + * happen.\n> > > */\n> > > - std::map requestedBuffers;\n> > > + std::map requestedDirectBuffers;\n> >\n> > Do you actually need to rename ?\n\nIn this patch, this map only includes buffers from direct type.\nThe internal type buffers are included by\n`Camera3RequestDescriptor::internalBuffers_` instead.\n\nIf we don't exclude the internal ones in this map, we don't\nneed to rename it as well. WDYT?\n\n> >\n> > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> > > CameraStream *cameraStream = buffer.stream;\n> > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> > > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > frameBuffer = buffer.frameBuffer.get();\n> > > acquireFence = std::move(buffer.fence);\n> > >\n> > > - requestedBuffers[cameraStream] = frameBuffer;\n> > > + requestedDirectBuffers[cameraStream] = frameBuffer;\n> > > LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > > break;\n> > >\n> > > @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > * Get the frame buffer from the CameraStream internal\n> > > * buffer pool.\n> > > *\n> > > - * The buffer has to be returned to the CameraStream\n> > > - * once it has been processed.\n> > > + * The buffer will be returned to the CameraStream when\n> > > + * the request is destructed.\n> >\n> > or 'destroyed'\n\nDone\n\n> >\n> > > */\n> > > frameBuffer = cameraStream->getBuffer();\n> > > - buffer.internalBuffer = frameBuffer;\n> > > buffer.srcBuffer = frameBuffer;\n> > >\n> > > - requestedBuffers[cameraStream] = frameBuffer;\n> > > + /*\n> > > + * Track the allocated internal buffers, which will be\n> > > + * recycled when the descriptor destroyed.\n> > > + * */\n> >\n> > Stray * */\n\nUpdated\n\n> >\n> > > + descriptor->internalBuffers_[cameraStream] = frameBuffer;\n> > > LOG(HAL, Debug) << ss.str() << \" (internal)\";\n> > >\n> > > descriptor->pendingStreamsToProcess_.insert(\n> > > @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > * post-processing. No need to recycle the buffer since it's\n> > > * owned by Android.\n> > > */\n> > > - auto iterDirectBuffer = requestedBuffers.find(sourceStream);\n> > > - if (iterDirectBuffer != requestedBuffers.end()) {\n> > > + auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream);\n> > > + if (iterDirectBuffer != requestedDirectBuffers.end()) {\n> > > buffer.srcBuffer = iterDirectBuffer->second;\n> > > continue;\n> > > }\n> > >\n> > > /*\n> > > - * If that's not the case, we need to add a buffer to the request\n> > > - * for this stream.\n> > > + * If that's not the case, we use an internal buffer allocated\n> > > + * from the source stream.\n> > > + *\n> > > + * If an internal buffer has been requested for the source\n> > > + * stream before, we should reuse it.\n> > > + */\n> > > + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);\n> > > + if (iterInternalBuffer != descriptor->internalBuffers_.end()) {\n> > > + buffer.srcBuffer = iterInternalBuffer->second;\n> > > + continue;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Otherwise, we need to create an internal buffer to the\n> > > + * request for the source stream. Get the frame buffer from the\n> > > + * source stream's internal buffer pool.\n> > > + *\n> > > + * The buffer will be returned to the CameraStream when the\n> > > + * request is destructed.\n> > > */\n> > > - FrameBuffer *frameBuffer = cameraStream->getBuffer();\n> > > - buffer.internalBuffer = frameBuffer;\n> > > + FrameBuffer *frameBuffer = sourceStream->getBuffer();\n> >\n> > Here I don't see why we're using the sourceStream's pool instead\n> > of the cameraStream's pool\n\nBecause the buffer recycle process is different. In this patch, it'll be\nreturned to the stream set to `internalBuffers_`. sourceStream is the\ncorrect one after this patch.\n\n> >\n> > However, I think this goes in the direction of making the HAL capable\n> > of more things, and I'm looking at the diff of this and the previous\n> > patch combined and I think you should squash the two and make a commit\n> > about \"Enabling multiple processed streams to map to the same internal\n> > buffer\".\n\nHmm, as I stated above. I think multiple processed streams to map to the same\ninternal buffer is already allowed, just that it's not handled properly.\nDo you still think that we should squash the two into one?\n\n>\n> Processed streams are expensive, is there enough CPU power to make that\n> possible ? I suppose it wouldn't be an issue when producing, for\n> instance, a processed JPEG stream with a hardware encoder and a scaled\n> YUV stream with libyuv from the same stream.\n>\n> This is interesting, but isn't mentioned at all in the cover letter, and\n> goes well beyond the subject of the series. Is is an intentional change\n> ?\n\nTherefore, I took this as a fix of an issue, instead of adding a feature.\n\nLet me know if you prefer to merge this as a separate series.\n\n>\n> > Looking at the combined diff, it feels to me you could populate\n> > buffer.src for the Direct case in the switch() and keep using an\n\nSorry, I don't get it. Mapped streams need to be handled the last,\nso that other streams and `requestedDirectBuffers` and\n`internalBuffers_` are setup, IIUC.\n\n> > std::set for requestedDirectBuffers (or requestedStreams). This would\n> > make the diff smaller probably.\n> >\n> >\n> > > buffer.srcBuffer = frameBuffer;\n> > >\n> > > descriptor->request_->addBuffer(sourceStream->stream(),\n> > > frameBuffer, nullptr);\n> > >\n> > > - requestedBuffers[sourceStream] = frameBuffer;\n> > > + /* Track the allocated internal buffer. */\n> > > + descriptor->internalBuffers_[sourceStream] = frameBuffer;\n> > > }\n> > >\n> > > /*\n> > > @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request)\n> > > if (ret) {\n> > > setBufferStatus(*buffer, StreamBuffer::Status::Error);\n> > > descriptor->pendingStreamsToProcess_.erase(stream);\n> > > -\n> > > - /*\n> > > - * If the framebuffer is internal to CameraStream return\n> > > - * it back now that we're done processing it.\n> > > - */\n> > > - if (buffer->internalBuffer)\n> > > - stream->putBuffer(buffer->internalBuffer);\n> > > }\n> > > }\n> > >\n> > > @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer,\n> > > {\n> > > setBufferStatus(*streamBuffer, status);\n> > >\n> > > - /*\n> > > - * If the framebuffer is internal to CameraStream return it back now\n> > > - * that we're done processing it.\n> > > - */\n> > > - if (streamBuffer->internalBuffer)\n> > > - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer);\n> > > -\n> > > Camera3RequestDescriptor *request = streamBuffer->request;\n> > >\n> > > {\n> > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> > > index 52a3ac1f7..a9240a83c 100644\n> > > --- a/src/android/camera_request.cpp\n> > > +++ b/src/android/camera_request.cpp\n> > > @@ -10,6 +10,7 @@\n> > > #include \n> > >\n> > > #include \"camera_buffer.h\"\n> > > +#include \"camera_stream.h\"\n> > >\n> > > using namespace libcamera;\n> > >\n> > > @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > > request_ = camera->createRequest(reinterpret_cast(this));\n> > > }\n> > >\n> > > -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > +Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > > +{\n> > > + /*\n> > > + * Recycle the allocated internal buffer back to its source stream.\n> > > + */\n> >\n> > Fits in one line most probably\n\nDone\n\nBR,\nHarvey\n\n> >\n> > > + for (auto &[sourceStream, frameBuffer] : internalBuffers_)\n> > > + sourceStream->putBuffer(frameBuffer);\n> > > +}\n> > >\n> > > /**\n> > > * \\class StreamBuffer\n> > > @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > * \\var StreamBuffer::status\n> > > * \\brief Track the status of the buffer\n> > > *\n> > > - * \\var StreamBuffer::internalBuffer\n> > > - * \\brief Pointer to a buffer internally handled by CameraStream (if any)\n> > > - *\n> > > * \\var StreamBuffer::srcBuffer\n> > > * \\brief Pointer to the source frame buffer used for post-processing\n> > > *\n> > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > > index 335f1985d..6b2a00795 100644\n> > > --- a/src/android/camera_request.h\n> > > +++ b/src/android/camera_request.h\n> > > @@ -49,7 +49,6 @@ public:\n> > > std::unique_ptr frameBuffer;\n> > > libcamera::UniqueFD fence;\n> > > Status status = Status::Success;\n> > > - libcamera::FrameBuffer *internalBuffer = nullptr;\n> > > const libcamera::FrameBuffer *srcBuffer = nullptr;\n> > > std::unique_ptr dstBuffer;\n> > > Camera3RequestDescriptor *request;\n> > > @@ -85,6 +84,8 @@ public:\n> > > std::unique_ptr request_;\n> > > std::unique_ptr resultMetadata_;\n> > >\n> > > + std::map internalBuffers_;\n> > > +\n> > > bool complete_ = false;\n> > > Status status_ = Status::Success;\n> > >\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 C2D2BC31E9\n\tfor ;\n\tFri, 29 Nov 2024 07:51:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A61FE65FF6;\n\tFri, 29 Nov 2024 08:51:52 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1CB8A60CE6\n\tfor ;\n\tFri, 29 Nov 2024 08:51:51 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id\n\t38308e7fff4ca-2ffced84ba8so15353221fa.2\n\tfor ;\n\tThu, 28 Nov 2024 23:51:51 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"WXqYmieX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1732866710; x=1733471510;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=1QRafxIVZRlPLbFTuvurm/Byac/vwBQPYCP0PvsMoAc=;\n\tb=WXqYmieXaGGVR1n/pweKCXFJj7LbHUYtbfLl00CQYOWSd4i+Fh0eTRpWdS67ff6UNb\n\tV+4yWKnOjxeIpO6fA9CZiLGFG5ZULelEGbOQr7U6aJ7JmuqddvKvgyXuCcRRuzA53vIV\n\tFIN0kVvG4+NhWHz9ULw+YsH5QWKQo/LcgtMaQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732866710; x=1733471510;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=1QRafxIVZRlPLbFTuvurm/Byac/vwBQPYCP0PvsMoAc=;\n\tb=cVjKDXI/0JAJj2JTW+uoCoFG3ED5ej0yJvroj/u10bYZJjL2ewXuQ9cfQUACPwSamZ\n\tCnf6LKYtskx1TFYVOCTotGtbLgzbwr8+qAu7QQpOE6tS6t0bRexdAfyzii+wUFunNdq2\n\ti+N9MlUTjgqLbplxTA0TS7ZPzImu7APbCL7gGGVtQrdVeIsGbr5TX17P9GqwbEtxdhq3\n\tOYtJ2oWe1UlwJrlTWruc8LA74cHzQXNPWjkmFgy+ErpTITiNm+NHNusd4wi68oxHfeVl\n\tJiSEdXWWJ+sZOnk3QNfDa2pWDZ0BYPJ+zryqPS3GKa4+UWhP6FrgCATMFfy26cqT2H4B\n\t4V2w==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUdzhqdifBqulHckqDr/1TXgKaxJphuG7I2JqOZSGw5Ow6s0S7uxBs4J0NM67JaZLdDOHSgSwnyDXZD4xoeOTY=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxWWL0Z7cllhjcxNwGclbqdQgTfnmHavRn+eZs1iaZiret3IjnY\n\tAVdJLUEnZXkTXeoPWCDuDQra1bEM1SQ3/nuSk+AHfoPSfIQKPjZgnCvfyUY4RROb4nZASm6ai+e\n\tPVS7QoYltx7mgIem4JJiyhRa4CLIYBE4bNgMHVu7L8+OPBc1qQw==","X-Gm-Gg":"ASbGncvN8V+FAOLab0eoLkACqPO5ULiMagCy53NIJPvV18vAxcgeJzUE9mXsX1a3ytz\n\tk1ZvTAiBb9sgmEkhx2Gvazcxvr05FIZ2VNfBKZGVHVETlYh4vCxKVhCYUTbk=","X-Google-Smtp-Source":"AGHT+IGhUA1SIlEgUsTmpldsh+DwQyg6+TscYiLhCND4+5jbXD1iLqsgYjh2yP5e6yT+R1eqiSi2mLmJxGzMNaLIpQo=","X-Received":"by 2002:a2e:9a0a:0:b0:2ff:c422:c52 with SMTP id\n\t38308e7fff4ca-2ffd60ecdf5mr56387671fa.39.1732866710212;\n\tThu, 28 Nov 2024 23:51:50 -0800 (PST)","MIME-Version":"1.0","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-5-chenghaoyang@chromium.org>\n\t\n\t<20241128143242.GA29106@pendragon.ideasonboard.com>","In-Reply-To":"<20241128143242.GA29106@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Fri, 29 Nov 2024 15:51:39 +0800","Message-ID":"","Subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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":32485,"web_url":"https://patchwork.libcamera.org/comment/32485/","msgid":"","date":"2024-12-02T15:39:51","subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Fri, Nov 29, 2024 at 03:51:39PM +0800, Cheng-Hao Yang wrote:\n> Hi Jacopo and Laurent,\n>\n> On Thu, Nov 28, 2024 at 10:32 PM Laurent Pinchart\n> wrote:\n> >\n> > On Thu, Nov 28, 2024 at 03:24:44PM +0100, Jacopo Mondi wrote:\n> > > Hi Harevy\n> > >\n> > > On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote:\n> > > > Previously there's a potential issue in StreamBuffer::internalBuffer's\n> > > > lifetime, when more than one streams depend on the same internal buffer.\n> > >\n> > > s/.//\n> > >\n> > > lifetime, when more than one streams depend on the same internal buffer\n> > > for post-processing, the buffer is returned to the CameraStream pool\n> > > as soon as the first post-processing request has completed.\n>\n> Updated.\n>\n> > >\n> > > >\n> > > > This patch fixes the issue by returning the buffer when the whole\n> > > > Camera3RequestDescriptor is destructed.\n> > >\n> > > Actually it does more I guess, it allows to map multiple streams of\n> > > type ::Mapped to the same libcamera::Stream, doesn't it ?\n>\n> According to the previous implementation, I'd say that's already\n> allowed, just not correctly implemented yet.\n> Looking into [1], the condition doesn't care if the sourceStream\n> has been mapped by another mapped stream or not.\n\nNo but as you can see it is not supported as we immediately return the\nsource stream buffer once post-processing is done.\n\nHowever, something tells me this was by design as we allow a single\n(Mapped) JPEG stream\n\n\t\t/* Defer handling of MJPEG streams until all others are known. */\n\t\tif (stream->format == HAL_PIXEL_FORMAT_BLOB) {\n\t\t\tif (jpegStream) {\n\t\t\t\tLOG(HAL, Error)\n\t\t\t\t\t<< \"Multiple JPEG streams are not supported\";\n\t\t\t\treturn -EINVAL;\n\t\t\t}\n\n\t\t\tjpegStream = stream;\n\t\t\tcontinue;\n\t\t}\n\nWhat is the use case you have for mapping multiple Mapped streams to\nthe same Direct|Internal one ? Multiple streams produced using libyuv\nconversion ? (afaict that's the only other post-processing case we\nsupport)\n\n>\n> Unless you mean that because there's such a bug, it's not allowed...\n> I'd say that we should have a FATAL/ERROR log at least, or even\n> block such a configuration.\n>\n\nWell, this patch fixes it, doesn't it ?\n\n> [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1071\n>\n>\n> > >\n> > > >\n> > > > Signed-off-by: Han-Lin Chen \n> > > > Co-developed-by: Harvey Yang \n> > > > Signed-off-by: Harvey Yang \n> > > > ---\n> > > > src/android/camera_device.cpp | 63 +++++++++++++++++++---------------\n> > > > src/android/camera_request.cpp | 13 ++++---\n> > > > src/android/camera_request.h | 3 +-\n> > > > 3 files changed, 46 insertions(+), 33 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 11613fac9..62f724041 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > * to a libcamera stream. Streams of type Mapped will be handled later.\n> > > > *\n> > > > * Collect the CameraStream associated to each requested capture stream.\n> > > > - * Since requestedStreams is an std:map<>, no duplications can happen.\n> > > > + * Since requestedDirectBuffers is an std:map<>, no duplications can\n> > > > + * happen.\n> > > > */\n> > > > - std::map requestedBuffers;\n> > > > + std::map requestedDirectBuffers;\n> > >\n> > > Do you actually need to rename ?\n>\n> In this patch, this map only includes buffers from direct type.\n> The internal type buffers are included by\n> `Camera3RequestDescriptor::internalBuffers_` instead.\n>\n\nlet's call it directBuffers as a middle ground\n\n> If we don't exclude the internal ones in this map, we don't\n> need to rename it as well. WDYT?\n>\n> > >\n> > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> > > > CameraStream *cameraStream = buffer.stream;\n> > > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> > > > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > frameBuffer = buffer.frameBuffer.get();\n> > > > acquireFence = std::move(buffer.fence);\n> > > >\n> > > > - requestedBuffers[cameraStream] = frameBuffer;\n> > > > + requestedDirectBuffers[cameraStream] = frameBuffer;\n> > > > LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > > > break;\n> > > >\n> > > > @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > * Get the frame buffer from the CameraStream internal\n> > > > * buffer pool.\n> > > > *\n> > > > - * The buffer has to be returned to the CameraStream\n> > > > - * once it has been processed.\n> > > > + * The buffer will be returned to the CameraStream when\n> > > > + * the request is destructed.\n> > >\n> > > or 'destroyed'\n>\n> Done\n>\n> > >\n> > > > */\n> > > > frameBuffer = cameraStream->getBuffer();\n> > > > - buffer.internalBuffer = frameBuffer;\n> > > > buffer.srcBuffer = frameBuffer;\n> > > >\n> > > > - requestedBuffers[cameraStream] = frameBuffer;\n> > > > + /*\n> > > > + * Track the allocated internal buffers, which will be\n> > > > + * recycled when the descriptor destroyed.\n> > > > + * */\n> > >\n> > > Stray * */\n>\n> Updated\n>\n> > >\n> > > > + descriptor->internalBuffers_[cameraStream] = frameBuffer;\n> > > > LOG(HAL, Debug) << ss.str() << \" (internal)\";\n> > > >\n> > > > descriptor->pendingStreamsToProcess_.insert(\n> > > > @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > * post-processing. No need to recycle the buffer since it's\n> > > > * owned by Android.\n> > > > */\n> > > > - auto iterDirectBuffer = requestedBuffers.find(sourceStream);\n> > > > - if (iterDirectBuffer != requestedBuffers.end()) {\n> > > > + auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream);\n> > > > + if (iterDirectBuffer != requestedDirectBuffers.end()) {\n> > > > buffer.srcBuffer = iterDirectBuffer->second;\n> > > > continue;\n> > > > }\n> > > >\n> > > > /*\n> > > > - * If that's not the case, we need to add a buffer to the request\n> > > > - * for this stream.\n> > > > + * If that's not the case, we use an internal buffer allocated\n> > > > + * from the source stream.\n> > > > + *\n\nCould you break this comment in two ? The first part apply to the rest\nof the for() loop, the second part only to the following if()\n\n\t\t/*\n\t\t * If that's not the case, we use an internal buffer allocated\n\t\t * from the source stream.\n\t\t */\n\n\t\t/*\n * If an internal buffer has been requested for the source\n\t\t * stream before, we should reuse it.\n\t\t */\n\t\tauto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);\n\t\tif (iterInternalBuffer != descriptor->internalBuffers_.end()) {\n\t\t\tbuffer.srcBuffer = iterInternalBuffer->second;\n\t\t\tcontinue;\n\t\t}\n\n\t\t/*\n\t\t * Otherwise, we need to create an internal buffer to the\n\t\t * request for the source stream. Get the frame buffer from the\n\t\t * source stream's internal buffer pool.\n\t\t *\n\t\t * The buffer will be returned to the CameraStream when the\n\t\t * request is destructed.\n\t\t */\n\t\tFrameBuffer *frameBuffer = sourceStream->getBuffer();\n\t\tbuffer.srcBuffer = frameBuffer;\n\n\t\tdescriptor->request_->addBuffer(sourceStream->stream(),\n\t\t\t\t\t\tframeBuffer, nullptr);\n\n> > > > + * If an internal buffer has been requested for the source\n> > > > + * stream before, we should reuse it.\n> > > > + */\n> > > > + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);\n> > > > + if (iterInternalBuffer != descriptor->internalBuffers_.end()) {\n> > > > + buffer.srcBuffer = iterInternalBuffer->second;\n> > > > + continue;\n> > > > + }\n> > > > +\n> > > > + /*\n> > > > + * Otherwise, we need to create an internal buffer to the\n> > > > + * request for the source stream. Get the frame buffer from the\n> > > > + * source stream's internal buffer pool.\n> > > > + *\n> > > > + * The buffer will be returned to the CameraStream when the\n> > > > + * request is destructed.\n> > > > */\n> > > > - FrameBuffer *frameBuffer = cameraStream->getBuffer();\n> > > > - buffer.internalBuffer = frameBuffer;\n> > > > + FrameBuffer *frameBuffer = sourceStream->getBuffer();\n> > >\n> > > Here I don't see why we're using the sourceStream's pool instead\n> > > of the cameraStream's pool\n>\n> Because the buffer recycle process is different. In this patch, it'll be\n> returned to the stream set to `internalBuffers_`. sourceStream is the\n> correct one after this patch.\n>\n\nOk. As I understand this it is because, now that we allow multiple\nMapped stream to be ... mapped ... on a Direct stream. If we fall here\nit's because the Direct stream its not part of the capture_request, so\nwe have to create a buffer from a pool and map multiple processed\nstreams on it. So yes the pool to use is the sourceStream one.\n\n> > >\n> > > However, I think this goes in the direction of making the HAL capable\n> > > of more things, and I'm looking at the diff of this and the previous\n> > > patch combined and I think you should squash the two and make a commit\n> > > about \"Enabling multiple processed streams to map to the same internal\n> > > buffer\".\n>\n> Hmm, as I stated above. I think multiple processed streams to map to the same\n> internal buffer is already allowed, just that it's not handled properly.\n> Do you still think that we should squash the two into one?\n>\n\nLooking at the combined diff it seems easier to make a single commit\nalong the lines of\n\n-------------------------------------------------------------------------------\nandroid: Correctly support multiple Mapped streams\n\nThe Android camera HAL supports creating streams for the Android\nframework by post-processing streams produced by libcamera.\n\nHowever at the moment a single Mapped stream can be associated with a\nDirect stream because, after the first post-processing, the data from\nthe internal stream are returned preventing further post-processing\npasses.\n\nFix this by storing the list of Direct stream buffers used as the\npost-processing source in an Camera3RequestDescriptor::internalBuffers_\nmap to replace the single internalBuffer_ pointer and return the\ninternal buffers when the capture request is deleted.\n-------------------------------------------------------------------------------\n\nI'm sure this can be written a 1000 times better than this, but\nat least, this is the level of detail and description a commit message\nshould provide\n\n> >\n> > Processed streams are expensive, is there enough CPU power to make that\n> > possible ? I suppose it wouldn't be an issue when producing, for\n> > instance, a processed JPEG stream with a hardware encoder and a scaled\n> > YUV stream with libyuv from the same stream.\n> >\n> > This is interesting, but isn't mentioned at all in the cover letter, and\n> > goes well beyond the subject of the series. Is is an intentional change\n> > ?\n>\n> Therefore, I took this as a fix of an issue, instead of adding a feature.\n>\n> Let me know if you prefer to merge this as a separate series.\n>\n> >\n> > > Looking at the combined diff, it feels to me you could populate\n> > > buffer.src for the Direct case in the switch() and keep using an\n>\n> Sorry, I don't get it. Mapped streams need to be handled the last,\n> so that other streams and `requestedDirectBuffers` and\n> `internalBuffers_` are setup, IIUC.\n>\n\nNo you're right, there's no buffer.srcBuffer to populate for Direct\n\nThanks\n j\n\n> > > std::set for requestedDirectBuffers (or requestedStreams). This would\n> > > make the diff smaller probably.\n> > >\n> > >\n> > > > buffer.srcBuffer = frameBuffer;\n> > > >\n> > > > descriptor->request_->addBuffer(sourceStream->stream(),\n> > > > frameBuffer, nullptr);\n> > > >\n> > > > - requestedBuffers[sourceStream] = frameBuffer;\n> > > > + /* Track the allocated internal buffer. */\n> > > > + descriptor->internalBuffers_[sourceStream] = frameBuffer;\n> > > > }\n> > > >\n> > > > /*\n> > > > @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request)\n> > > > if (ret) {\n> > > > setBufferStatus(*buffer, StreamBuffer::Status::Error);\n> > > > descriptor->pendingStreamsToProcess_.erase(stream);\n> > > > -\n> > > > - /*\n> > > > - * If the framebuffer is internal to CameraStream return\n> > > > - * it back now that we're done processing it.\n> > > > - */\n> > > > - if (buffer->internalBuffer)\n> > > > - stream->putBuffer(buffer->internalBuffer);\n> > > > }\n> > > > }\n> > > >\n> > > > @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer,\n> > > > {\n> > > > setBufferStatus(*streamBuffer, status);\n> > > >\n> > > > - /*\n> > > > - * If the framebuffer is internal to CameraStream return it back now\n> > > > - * that we're done processing it.\n> > > > - */\n> > > > - if (streamBuffer->internalBuffer)\n> > > > - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer);\n> > > > -\n> > > > Camera3RequestDescriptor *request = streamBuffer->request;\n> > > >\n> > > > {\n> > > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> > > > index 52a3ac1f7..a9240a83c 100644\n> > > > --- a/src/android/camera_request.cpp\n> > > > +++ b/src/android/camera_request.cpp\n> > > > @@ -10,6 +10,7 @@\n> > > > #include \n> > > >\n> > > > #include \"camera_buffer.h\"\n> > > > +#include \"camera_stream.h\"\n> > > >\n> > > > using namespace libcamera;\n> > > >\n> > > > @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > > > request_ = camera->createRequest(reinterpret_cast(this));\n> > > > }\n> > > >\n> > > > -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > > +Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > > > +{\n> > > > + /*\n> > > > + * Recycle the allocated internal buffer back to its source stream.\n> > > > + */\n> > >\n> > > Fits in one line most probably\n>\n> Done\n>\n> BR,\n> Harvey\n>\n> > >\n> > > > + for (auto &[sourceStream, frameBuffer] : internalBuffers_)\n> > > > + sourceStream->putBuffer(frameBuffer);\n> > > > +}\n> > > >\n> > > > /**\n> > > > * \\class StreamBuffer\n> > > > @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > > * \\var StreamBuffer::status\n> > > > * \\brief Track the status of the buffer\n> > > > *\n> > > > - * \\var StreamBuffer::internalBuffer\n> > > > - * \\brief Pointer to a buffer internally handled by CameraStream (if any)\n> > > > - *\n> > > > * \\var StreamBuffer::srcBuffer\n> > > > * \\brief Pointer to the source frame buffer used for post-processing\n> > > > *\n> > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > > > index 335f1985d..6b2a00795 100644\n> > > > --- a/src/android/camera_request.h\n> > > > +++ b/src/android/camera_request.h\n> > > > @@ -49,7 +49,6 @@ public:\n> > > > std::unique_ptr frameBuffer;\n> > > > libcamera::UniqueFD fence;\n> > > > Status status = Status::Success;\n> > > > - libcamera::FrameBuffer *internalBuffer = nullptr;\n> > > > const libcamera::FrameBuffer *srcBuffer = nullptr;\n> > > > std::unique_ptr dstBuffer;\n> > > > Camera3RequestDescriptor *request;\n> > > > @@ -85,6 +84,8 @@ public:\n> > > > std::unique_ptr request_;\n> > > > std::unique_ptr resultMetadata_;\n> > > >\n> > > > + std::map internalBuffers_;\n> > > > +\n> > > > bool complete_ = false;\n> > > > Status status_ = Status::Success;\n> > > >\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 6F5F6BDCBF\n\tfor ;\n\tMon, 2 Dec 2024 15:39:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9885B6606B;\n\tMon, 2 Dec 2024 16:39:58 +0100 (CET)","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 7CC5D6605F\n\tfor ;\n\tMon, 2 Dec 2024 16:39:56 +0100 (CET)","from ideasonboard.com (mob-5-90-236-68.net.vodafone.it\n\t[5.90.236.68])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1DC7B514;\n\tMon, 2 Dec 2024 16:39:29 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EHdGfuTv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733153969;\n\tbh=laCxXTHtpBuwWz029VqZzVtf1tThNQZYqMbI3HTJvII=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EHdGfuTvzSJXO7jcWrDyN/IPnXKs3rpnmZ8G9imHu+unu8Kirv2ct2W01YjqclVV+\n\tXwKdtdVqlXJHsI2Kp4K6Sd4ccvxl/fI5beGz9VAYQn0pjM/lBXw7Pl3Zy2CDoVbuYa\n\toEkqPnnju1kbTvna+bWvro5vkT3uKAxrG2srnnF0=","Date":"Mon, 2 Dec 2024 16:39:51 +0100","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Laurent Pinchart , \n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","Message-ID":"","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-5-chenghaoyang@chromium.org>\n\t\n\t<20241128143242.GA29106@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":32489,"web_url":"https://patchwork.libcamera.org/comment/32489/","msgid":"","date":"2024-12-03T07:43:01","subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Mon, Dec 2, 2024 at 11:39 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Fri, Nov 29, 2024 at 03:51:39PM +0800, Cheng-Hao Yang wrote:\n> > Hi Jacopo and Laurent,\n> >\n> > On Thu, Nov 28, 2024 at 10:32 PM Laurent Pinchart\n> > wrote:\n> > >\n> > > On Thu, Nov 28, 2024 at 03:24:44PM +0100, Jacopo Mondi wrote:\n> > > > Hi Harevy\n> > > >\n> > > > On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote:\n> > > > > Previously there's a potential issue in StreamBuffer::internalBuffer's\n> > > > > lifetime, when more than one streams depend on the same internal buffer.\n> > > >\n> > > > s/.//\n> > > >\n> > > > lifetime, when more than one streams depend on the same internal buffer\n> > > > for post-processing, the buffer is returned to the CameraStream pool\n> > > > as soon as the first post-processing request has completed.\n> >\n> > Updated.\n> >\n> > > >\n> > > > >\n> > > > > This patch fixes the issue by returning the buffer when the whole\n> > > > > Camera3RequestDescriptor is destructed.\n> > > >\n> > > > Actually it does more I guess, it allows to map multiple streams of\n> > > > type ::Mapped to the same libcamera::Stream, doesn't it ?\n> >\n> > According to the previous implementation, I'd say that's already\n> > allowed, just not correctly implemented yet.\n> > Looking into [1], the condition doesn't care if the sourceStream\n> > has been mapped by another mapped stream or not.\n>\n> No but as you can see it is not supported as we immediately return the\n> source stream buffer once post-processing is done.\n>\n> However, something tells me this was by design as we allow a single\n> (Mapped) JPEG stream\n>\n> /* Defer handling of MJPEG streams until all others are known. */\n> if (stream->format == HAL_PIXEL_FORMAT_BLOB) {\n> if (jpegStream) {\n> LOG(HAL, Error)\n> << \"Multiple JPEG streams are not supported\";\n> return -EINVAL;\n> }\n>\n> jpegStream = stream;\n> continue;\n> }\n>\n> What is the use case you have for mapping multiple Mapped streams to\n> the same Direct|Internal one ? Multiple streams produced using libyuv\n> conversion ? (afaict that's the only other post-processing case we\n> support)\n\nYes, and yes, we would expect at most one Internal stream and multiple\nMapped streams with libyuv conversion I think. It's still allowed (to\nbe configured) in the tot version, and has the issue of\ninternalBuffer_ lifetime, right? That's why I think this patch is a\npure issue fix.\n\n>\n> >\n> > Unless you mean that because there's such a bug, it's not allowed...\n> > I'd say that we should have a FATAL/ERROR log at least, or even\n> > block such a configuration.\n> >\n>\n> Well, this patch fixes it, doesn't it ?\n\nYes :)\n\n>\n> > [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1071\n> >\n> >\n> > > >\n> > > > >\n> > > > > Signed-off-by: Han-Lin Chen \n> > > > > Co-developed-by: Harvey Yang \n> > > > > Signed-off-by: Harvey Yang \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 63 +++++++++++++++++++---------------\n> > > > > src/android/camera_request.cpp | 13 ++++---\n> > > > > src/android/camera_request.h | 3 +-\n> > > > > 3 files changed, 46 insertions(+), 33 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index 11613fac9..62f724041 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > > * to a libcamera stream. Streams of type Mapped will be handled later.\n> > > > > *\n> > > > > * Collect the CameraStream associated to each requested capture stream.\n> > > > > - * Since requestedStreams is an std:map<>, no duplications can happen.\n> > > > > + * Since requestedDirectBuffers is an std:map<>, no duplications can\n> > > > > + * happen.\n> > > > > */\n> > > > > - std::map requestedBuffers;\n> > > > > + std::map requestedDirectBuffers;\n> > > >\n> > > > Do you actually need to rename ?\n> >\n> > In this patch, this map only includes buffers from direct type.\n> > The internal type buffers are included by\n> > `Camera3RequestDescriptor::internalBuffers_` instead.\n> >\n>\n> let's call it directBuffers as a middle ground\n\nSure, done.\n\n>\n> > If we don't exclude the internal ones in this map, we don't\n> > need to rename it as well. WDYT?\n> >\n> > > >\n> > > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> > > > > CameraStream *cameraStream = buffer.stream;\n> > > > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> > > > > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > > frameBuffer = buffer.frameBuffer.get();\n> > > > > acquireFence = std::move(buffer.fence);\n> > > > >\n> > > > > - requestedBuffers[cameraStream] = frameBuffer;\n> > > > > + requestedDirectBuffers[cameraStream] = frameBuffer;\n> > > > > LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > > > > break;\n> > > > >\n> > > > > @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > > * Get the frame buffer from the CameraStream internal\n> > > > > * buffer pool.\n> > > > > *\n> > > > > - * The buffer has to be returned to the CameraStream\n> > > > > - * once it has been processed.\n> > > > > + * The buffer will be returned to the CameraStream when\n> > > > > + * the request is destructed.\n> > > >\n> > > > or 'destroyed'\n> >\n> > Done\n> >\n> > > >\n> > > > > */\n> > > > > frameBuffer = cameraStream->getBuffer();\n> > > > > - buffer.internalBuffer = frameBuffer;\n> > > > > buffer.srcBuffer = frameBuffer;\n> > > > >\n> > > > > - requestedBuffers[cameraStream] = frameBuffer;\n> > > > > + /*\n> > > > > + * Track the allocated internal buffers, which will be\n> > > > > + * recycled when the descriptor destroyed.\n> > > > > + * */\n> > > >\n> > > > Stray * */\n> >\n> > Updated\n> >\n> > > >\n> > > > > + descriptor->internalBuffers_[cameraStream] = frameBuffer;\n> > > > > LOG(HAL, Debug) << ss.str() << \" (internal)\";\n> > > > >\n> > > > > descriptor->pendingStreamsToProcess_.insert(\n> > > > > @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > > * post-processing. No need to recycle the buffer since it's\n> > > > > * owned by Android.\n> > > > > */\n> > > > > - auto iterDirectBuffer = requestedBuffers.find(sourceStream);\n> > > > > - if (iterDirectBuffer != requestedBuffers.end()) {\n> > > > > + auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream);\n> > > > > + if (iterDirectBuffer != requestedDirectBuffers.end()) {\n> > > > > buffer.srcBuffer = iterDirectBuffer->second;\n> > > > > continue;\n> > > > > }\n> > > > >\n> > > > > /*\n> > > > > - * If that's not the case, we need to add a buffer to the request\n> > > > > - * for this stream.\n> > > > > + * If that's not the case, we use an internal buffer allocated\n> > > > > + * from the source stream.\n> > > > > + *\n>\n> Could you break this comment in two ? The first part apply to the rest\n> of the for() loop, the second part only to the following if()\n>\n> /*\n> * If that's not the case, we use an internal buffer allocated\n> * from the source stream.\n> */\n>\n> /*\n> * If an internal buffer has been requested for the source\n> * stream before, we should reuse it.\n> */\n> auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);\n> if (iterInternalBuffer != descriptor->internalBuffers_.end()) {\n> buffer.srcBuffer = iterInternalBuffer->second;\n> continue;\n> }\n>\n> /*\n> * Otherwis`e, we need to create an internal buffer to the\n> * request for the source stream. Get the frame buffer from the\n> * source stream's internal buffer pool.\n> *\n> * The buffer will be returned to the CameraStream when the\n> * request is destructed.\n> */\n> FrameBuffer *frameBuffer = sourceStream->getBuffer();\n> buffer.srcBuffer = frameBuffer;\n>\n> descriptor->request_->addBuffer(sourceStream->stream(),\n> frameBuffer, nullptr);\n>\n\nSure, done.\n\n> > > > > + * If an internal buffer has been requested for the source\n> > > > > + * stream before, we should reuse it.\n> > > > > + */\n> > > > > + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);\n> > > > > + if (iterInternalBuffer != descriptor->internalBuffers_.end()) {\n> > > > > + buffer.srcBuffer = iterInternalBuffer->second;\n> > > > > + continue;\n> > > > > + }\n> > > > > +\n> > > > > + /*\n> > > > > + * Otherwise, we need to create an internal buffer to the\n> > > > > + * request for the source stream. Get the frame buffer from the\n> > > > > + * source stream's internal buffer pool.\n> > > > > + *\n> > > > > + * The buffer will be returned to the CameraStream when the\n> > > > > + * request is destructed.\n> > > > > */\n> > > > > - FrameBuffer *frameBuffer = cameraStream->getBuffer();\n> > > > > - buffer.internalBuffer = frameBuffer;\n> > > > > + FrameBuffer *frameBuffer = sourceStream->getBuffer();\n> > > >\n> > > > Here I don't see why we're using the sourceStream's pool instead\n> > > > of the cameraStream's pool\n> >\n> > Because the buffer recycle process is different. In this patch, it'll be\n> > returned to the stream set to `internalBuffers_`. sourceStream is the\n> > correct one after this patch.\n> >\n>\n> Ok. As I understand this it is because, now that we allow multiple\n> Mapped stream to be ... mapped ... on a Direct stream. If we fall here\n> it's because the Direct stream its not part of the capture_request, so\n> we have to create a buffer from a pool and map multiple processed\n> streams on it. So yes the pool to use is the sourceStream one.\n\nExactly :)\n\n>\n> > > >\n> > > > However, I think this goes in the direction of making the HAL capable\n> > > > of more things, and I'm looking at the diff of this and the previous\n> > > > patch combined and I think you should squash the two and make a commit\n> > > > about \"Enabling multiple processed streams to map to the same internal\n> > > > buffer\".\n> >\n> > Hmm, as I stated above. I think multiple processed streams to map to the same\n> > internal buffer is already allowed, just that it's not handled properly.\n> > Do you still think that we should squash the two into one?\n> >\n>\n> Looking at the combined diff it seems easier to make a single commit\n> along the lines of\n>\n> -------------------------------------------------------------------------------\n> android: Correctly support multiple Mapped streams\n>\n> The Android camera HAL supports creating streams for the Android\n> framework by post-processing streams produced by libcamera.\n>\n> However at the moment a single Mapped stream can be associated with a\n> Direct stream because, after the first post-processing, the data from\n> the internal stream are returned preventing further post-processing\n> passes.\n>\n> Fix this by storing the list of Direct stream buffers used as the\n> post-processing source in an Camera3RequestDescriptor::internalBuffers_\n> map to replace the single internalBuffer_ pointer and return the\n> internal buffers when the capture request is deleted.\n> -------------------------------------------------------------------------------\n>\n> I'm sure this can be written a 1000 times better than this, but\n> at least, this is the level of detail and description a commit message\n> should provide\n\nThanks, adopted. I'll try to provide more details next time...\n\nBR,\nHarvey\n\n>\n> > >\n> > > Processed streams are expensive, is there enough CPU power to make that\n> > > possible ? I suppose it wouldn't be an issue when producing, for\n> > > instance, a processed JPEG stream with a hardware encoder and a scaled\n> > > YUV stream with libyuv from the same stream.\n> > >\n> > > This is interesting, but isn't mentioned at all in the cover letter, and\n> > > goes well beyond the subject of the series. Is is an intentional change\n> > > ?\n> >\n> > Therefore, I took this as a fix of an issue, instead of adding a feature.\n> >\n> > Let me know if you prefer to merge this as a separate series.\n> >\n> > >\n> > > > Looking at the combined diff, it feels to me you could populate\n> > > > buffer.src for the Direct case in the switch() and keep using an\n> >\n> > Sorry, I don't get it. Mapped streams need to be handled the last,\n> > so that other streams and `requestedDirectBuffers` and\n> > `internalBuffers_` are setup, IIUC.\n> >\n>\n> No you're right, there's no buffer.srcBuffer to populate for Direct\n>\n> Thanks\n> j\n>\n> > > > std::set for requestedDirectBuffers (or requestedStreams). This would\n> > > > make the diff smaller probably.\n> > > >\n> > > >\n> > > > > buffer.srcBuffer = frameBuffer;\n> > > > >\n> > > > > descriptor->request_->addBuffer(sourceStream->stream(),\n> > > > > frameBuffer, nullptr);\n> > > > >\n> > > > > - requestedBuffers[sourceStream] = frameBuffer;\n> > > > > + /* Track the allocated internal buffer. */\n> > > > > + descriptor->internalBuffers_[sourceStream] = frameBuffer;\n> > > > > }\n> > > > >\n> > > > > /*\n> > > > > @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request)\n> > > > > if (ret) {\n> > > > > setBufferStatus(*buffer, StreamBuffer::Status::Error);\n> > > > > descriptor->pendingStreamsToProcess_.erase(stream);\n> > > > > -\n> > > > > - /*\n> > > > > - * If the framebuffer is internal to CameraStream return\n> > > > > - * it back now that we're done processing it.\n> > > > > - */\n> > > > > - if (buffer->internalBuffer)\n> > > > > - stream->putBuffer(buffer->internalBuffer);\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer,\n> > > > > {\n> > > > > setBufferStatus(*streamBuffer, status);\n> > > > >\n> > > > > - /*\n> > > > > - * If the framebuffer is internal to CameraStream return it back now\n> > > > > - * that we're done processing it.\n> > > > > - */\n> > > > > - if (streamBuffer->internalBuffer)\n> > > > > - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer);\n> > > > > -\n> > > > > Camera3RequestDescriptor *request = streamBuffer->request;\n> > > > >\n> > > > > {\n> > > > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> > > > > index 52a3ac1f7..a9240a83c 100644\n> > > > > --- a/src/android/camera_request.cpp\n> > > > > +++ b/src/android/camera_request.cpp\n> > > > > @@ -10,6 +10,7 @@\n> > > > > #include \n> > > > >\n> > > > > #include \"camera_buffer.h\"\n> > > > > +#include \"camera_stream.h\"\n> > > > >\n> > > > > using namespace libcamera;\n> > > > >\n> > > > > @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > > > > request_ = camera->createRequest(reinterpret_cast(this));\n> > > > > }\n> > > > >\n> > > > > -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > > > +Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > > > > +{\n> > > > > + /*\n> > > > > + * Recycle the allocated internal buffer back to its source stream.\n> > > > > + */\n> > > >\n> > > > Fits in one line most probably\n> >\n> > Done\n> >\n> > BR,\n> > Harvey\n> >\n> > > >\n> > > > > + for (auto &[sourceStream, frameBuffer] : internalBuffers_)\n> > > > > + sourceStream->putBuffer(frameBuffer);\n> > > > > +}\n> > > > >\n> > > > > /**\n> > > > > * \\class StreamBuffer\n> > > > > @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > > > * \\var StreamBuffer::status\n> > > > > * \\brief Track the status of the buffer\n> > > > > *\n> > > > > - * \\var StreamBuffer::internalBuffer\n> > > > > - * \\brief Pointer to a buffer internally handled by CameraStream (if any)\n> > > > > - *\n> > > > > * \\var StreamBuffer::srcBuffer\n> > > > > * \\brief Pointer to the source frame buffer used for post-processing\n> > > > > *\n> > > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > > > > index 335f1985d..6b2a00795 100644\n> > > > > --- a/src/android/camera_request.h\n> > > > > +++ b/src/android/camera_request.h\n> > > > > @@ -49,7 +49,6 @@ public:\n> > > > > std::unique_ptr frameBuffer;\n> > > > > libcamera::UniqueFD fence;\n> > > > > Status status = Status::Success;\n> > > > > - libcamera::FrameBuffer *internalBuffer = nullptr;\n> > > > > const libcamera::FrameBuffer *srcBuffer = nullptr;\n> > > > > std::unique_ptr dstBuffer;\n> > > > > Camera3RequestDescriptor *request;\n> > > > > @@ -85,6 +84,8 @@ public:\n> > > > > std::unique_ptr request_;\n> > > > > std::unique_ptr resultMetadata_;\n> > > > >\n> > > > > + std::map internalBuffers_;\n> > > > > +\n> > > > > bool complete_ = false;\n> > > > > Status status_ = Status::Success;\n> > > > >\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 3B26ABDCBF\n\tfor ;\n\tTue, 3 Dec 2024 07:43:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 665D066078;\n\tTue, 3 Dec 2024 08:43:16 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DBBD2618B1\n\tfor ;\n\tTue, 3 Dec 2024 08:43:13 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id\n\t38308e7fff4ca-2ffbf4580cbso53348171fa.2\n\tfor ;\n\tMon, 02 Dec 2024 23:43:13 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"QUdwJIf3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733211793; x=1733816593;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=QN2/xi8oBz1Eq5+FPRTe8+y7NGf9DcGPtxCVzspqH3M=;\n\tb=QUdwJIf3sC6Iq0X0KXnFYM9JPQ4wgK5dl2EQDF2NA8CyEFAbpRNepY+1unkpn6O+zv\n\tm2xJl83OBlUH3w74FWBWQkEH4Yc+b2h8sZzz2x85SfeHE7MK01lAXdDdbkLzjd3m/QSi\n\tRwbtXRV7u4RjCG28Za+Aov0uQbfdGnnNRCATM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733211793; x=1733816593;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=QN2/xi8oBz1Eq5+FPRTe8+y7NGf9DcGPtxCVzspqH3M=;\n\tb=Ae4t4NJ5JpDLJxWtJ6YYnLvyFfX9dwmDZ0o59Al8HxWAnj0DKBHqPUNNUj1xv5LLIo\n\tV30qJs0RMIaKtux4TKkrnMgtkXjvpGYc6IDt1MBLBkJ6JBKyTnEzsNaKlswHl9sFlufp\n\tjMbOwgw3dtIh4ObMcZhFWSg2873D2pshp4aGnkY828SdrKTP95xAAJYO3636TDS8M3n9\n\tEZGbhxQonTyM8dNB+jFsSnTwMzR+da4s2/SinegDWWmAXGlaUAWmm8KZe/+qyrHqlBLt\n\tgckXlhKiSyWTmquNUmnbuQSy2zq2XSePNI/nsya+1gqNA+1o26iHYoWPtC/qiXH0Leyi\n\t7tGQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVAcIRN9spwWyk+jaThTwHcUIU2FrEGfDRtbh+wAIkTdfYrhbpJxWw7WWt5t7R6TQ9XeYVnZa3obDsOpVzfYPU=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YwZPXkZx7/NxSVMt5yM0WbxTRq/fUsmi1TLyVF+U8B+m7SwAdwe\n\tISQ0jaQ4eSExU07hi1cV0F4T+3K0XDYJegDjwtlBlC/x0IsVI9ssNcBN/fHvhckyS+hKZupmIod\n\tTKL73jDJDr2Neujhr4qAH8nAFIEe+0pFCXszi","X-Gm-Gg":"ASbGncvlbUF1Xd/RIVm/IhOxbYXcShYw59x4dzOjShWFgbbR3wG5OUpuF2fCkdzF+7t\n\tW+UfABEIXeydnd+AtGRwrgisS4t2amHWqOYLlRRYkNQgvz7ChFT3rgPiv7kA=","X-Google-Smtp-Source":"AGHT+IGPHeAVadZ2lueRjcnyvxfYH2fdhrhlXrPDR7a724ZmJXQmOps1S9GD7Kv0P6ehzRmU6rRwnnfj01fmhYoOGj0=","X-Received":"by 2002:a2e:bc81:0:b0:2fc:a347:6d90 with SMTP id\n\t38308e7fff4ca-30009d07f68mr9977561fa.27.1733211792240;\n\tMon, 02 Dec 2024 23:43:12 -0800 (PST)","MIME-Version":"1.0","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-5-chenghaoyang@chromium.org>\n\t\n\t<20241128143242.GA29106@pendragon.ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Tue, 3 Dec 2024 15:43:01 +0800","Message-ID":"","Subject":"Re: [PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to\n\tCamera3RequestDescriptor","To":"Jacopo Mondi ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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\" "}}]