[{"id":19628,"web_url":"https://patchwork.libcamera.org/comment/19628/","msgid":"","date":"2021-09-13T01:01:23","subject":"Re: [libcamera-devel] [PATCH v2 7/9] android: camera_device: Move\n\tbuffer mapping for post processing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Sep 10, 2021 at 12:36:36PM +0530, Umang Jain wrote:\n> Buffer mapping for post processing currently happens in\n> CameraStream::process(). However, it can be easily be moved to\n> CameraDevice just before the call to CameraStream::process(). The\n> reason to do so is that, we can hold the mapped destination buffer\n> pointer as a part of Camera3RequestDescriptor. Camera3RequestDescriptor\n> already hold other post-processing related context to enable async post\n> processing in subsequent commits.\n\nWhy is it better to map the buffer in CameraDevice::requestComplete() ?\nAs mapping can be a costly operation, it seems better to me to move it\nto the post-processor thread instead.\n\n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 14 +++++++++++++-\n> src/android/camera_device.h | 1 +\n> src/android/camera_stream.cpp | 16 ++--------------\n> src/android/camera_stream.h | 2 +-\n> 4 files changed, 17 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 7f04d225..988d4232 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1163,13 +1163,25 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> \t\tdescriptor.captureResult_ = captureResult;\n> \n> +\t\t/* \\todo Buffer mapping should be moved to a separate thread? */\n> +\t\tconst StreamConfiguration &output = cameraStream->configuration();\n> +\t\tdescriptor.destBuffer_ = std::make_unique(\n> +\t\t\t*buffer.buffer, formats::MJPEG, output.size,\n> +\t\t\tPROT_READ | PROT_WRITE);\n> +\n> +\t\tif (!descriptor.destBuffer_->isValid()) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> +\t\t\treturn;\n> +\t\t}\n> +\n> \t\tstd::unique_ptr reqDescriptor =\n> \t\t\tstd::make_unique();\n> \t\t*reqDescriptor = std::move(descriptor);\n> \t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> \n> \t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> -\t\tint ret = cameraStream->process(src, *buffer.buffer,\n> +\t\tint ret = cameraStream->process(src,\n> +\t\t\t\t\t\tcurrentDescriptor->destBuffer_.get(),\n> \t\t\t\t\t\tcurrentDescriptor->settings_,\n> \t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n> \t\t\t\t\t\tcurrentDescriptor);\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index e7318358..b62d373c 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -58,6 +58,7 @@ struct Camera3RequestDescriptor {\n> \t\tFailed,\n> \t};\n> \tstd::unique_ptr resultMetadata_;\n> +\tstd::unique_ptr destBuffer_;\n> \tcamera3_capture_result_t captureResult_;\n> \tlibcamera::FrameBuffer *internalBuffer_;\n> \tcompletionStatus status_;\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index c7d874b2..5fd04bbf 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -102,7 +102,7 @@ int CameraStream::configure()\n> }\n> \n> int CameraStream::process(const FrameBuffer *source,\n> -\t\t\t buffer_handle_t camera3Dest,\n> +\t\t\t CameraBuffer *destBuffer,\n> \t\t\t const CameraMetadata &requestMetadata,\n> \t\t\t CameraMetadata *resultMetadata,\n> \t\t\t const Camera3RequestDescriptor *context)\n> @@ -110,19 +110,7 @@ int CameraStream::process(const FrameBuffer *source,\n> \tif (!postProcessor_)\n> \t\treturn 0;\n> \n> -\t/*\n> -\t * \\todo Buffer mapping and processing should be moved to a\n> -\t * separate thread.\n> -\t */\n> -\tconst StreamConfiguration &output = configuration();\n> -\tCameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> -\t\t\t PROT_READ | PROT_WRITE);\n> -\tif (!dest.isValid()) {\n> -\t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\treturn postProcessor_->process(source, &dest, requestMetadata,\n> +\treturn postProcessor_->process(source, destBuffer, requestMetadata,\n> \t\t\t\t resultMetadata, context);\n> }\n> \n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index be076995..8097ddbc 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -124,7 +124,7 @@ public:\n> \n> \tint configure();\n> \tint process(const libcamera::FrameBuffer *source,\n> -\t\t buffer_handle_t camera3Dest,\n> +\t\t CameraBuffer *destBuffer,\n> \t\t const CameraMetadata &requestMetadata,\n> \t\t CameraMetadata *resultMetadata,\n> \t\t const Camera3RequestDescriptor *context);","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 1D101BDC71\n\tfor ;\n\tMon, 13 Sep 2021 01:01:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9CED60250;\n\tMon, 13 Sep 2021 03:01:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3500060250\n\tfor ;\n\tMon, 13 Sep 2021 03:01:48 +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 B6DEA9E;\n\tMon, 13 Sep 2021 03:01:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"frn5/3Yq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631494907;\n\tbh=J81iz2qfboyFObsNwnwS5HydYxvstYxoGEkkvlLIuYw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=frn5/3YqtLjHITwXFkRntrB7RiRpQJKNv7GhZzrKE3+JpfAcIhTuGQbDe5vyp9Qdn\n\t2foTA+uD50tnGxkAG23HJbMXyCnvWgZ3q5pNld0rfeqdoevgJPm9QYVa1m1TDqVYSw\n\t+q0NuF9Xgkjnzi4tUh6kqkLvisjBZAXFfi77Qm4Y=","Date":"Mon, 13 Sep 2021 04:01:23 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-8-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210910070638.467294-8-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 7/9] android: camera_device: Move\n\tbuffer mapping for post processing","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":19635,"web_url":"https://patchwork.libcamera.org/comment/19635/","msgid":"<582bf525-0059-0246-7531-a35d26b00a83@ideasonboard.com>","date":"2021-09-13T07:05:56","subject":"Re: [libcamera-devel] [PATCH v2 7/9] android: camera_device: Move\n\tbuffer mapping for post processing","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"On 9/13/21 6:31 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 10, 2021 at 12:36:36PM +0530, Umang Jain wrote:\n>> Buffer mapping for post processing currently happens in\n>> CameraStream::process(). However, it can be easily be moved to\n>> CameraDevice just before the call to CameraStream::process(). The\n>> reason to do so is that, we can hold the mapped destination buffer\n>> pointer as a part of Camera3RequestDescriptor. Camera3RequestDescriptor\n>> already hold other post-processing related context to enable async post\n>> processing in subsequent commits.\n> Why is it better to map the buffer in CameraDevice::requestComplete() ?\n> As mapping can be a costly operation, it seems better to me to move it\n> to the post-processor thread instead.\n\n\nWe can save a pointer in the context(aka Camera3RequestDescriptor) to \ndestBuffer if I move it to CameraDevice.\n\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 14 +++++++++++++-\n>> src/android/camera_device.h | 1 +\n>> src/android/camera_stream.cpp | 16 ++--------------\n>> src/android/camera_stream.h | 2 +-\n>> 4 files changed, 17 insertions(+), 16 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 7f04d225..988d4232 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1163,13 +1163,25 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>> \t\tdescriptor.captureResult_ = captureResult;\n>> \n>> +\t\t/* \\todo Buffer mapping should be moved to a separate thread? */\n>> +\t\tconst StreamConfiguration &output = cameraStream->configuration();\n>> +\t\tdescriptor.destBuffer_ = std::make_unique(\n>> +\t\t\t*buffer.buffer, formats::MJPEG, output.size,\n>> +\t\t\tPROT_READ | PROT_WRITE);\n>> +\n>> +\t\tif (!descriptor.destBuffer_->isValid()) {\n>> +\t\t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n>> +\t\t\treturn;\n>> +\t\t}\n>> +\n>> \t\tstd::unique_ptr reqDescriptor =\n>> \t\t\tstd::make_unique();\n>> \t\t*reqDescriptor = std::move(descriptor);\n>> \t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>> \n>> \t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n>> -\t\tint ret = cameraStream->process(src, *buffer.buffer,\n>> +\t\tint ret = cameraStream->process(src,\n>> +\t\t\t\t\t\tcurrentDescriptor->destBuffer_.get(),\n>> \t\t\t\t\t\tcurrentDescriptor->settings_,\n>> \t\t\t\t\t\tcurrentDescriptor->resultMetadata_.get(),\n>> \t\t\t\t\t\tcurrentDescriptor);\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index e7318358..b62d373c 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -58,6 +58,7 @@ struct Camera3RequestDescriptor {\n>> \t\tFailed,\n>> \t};\n>> \tstd::unique_ptr resultMetadata_;\n>> +\tstd::unique_ptr destBuffer_;\n>> \tcamera3_capture_result_t captureResult_;\n>> \tlibcamera::FrameBuffer *internalBuffer_;\n>> \tcompletionStatus status_;\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index c7d874b2..5fd04bbf 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -102,7 +102,7 @@ int CameraStream::configure()\n>> }\n>> \n>> int CameraStream::process(const FrameBuffer *source,\n>> -\t\t\t buffer_handle_t camera3Dest,\n>> +\t\t\t CameraBuffer *destBuffer,\n>> \t\t\t const CameraMetadata &requestMetadata,\n>> \t\t\t CameraMetadata *resultMetadata,\n>> \t\t\t const Camera3RequestDescriptor *context)\n>> @@ -110,19 +110,7 @@ int CameraStream::process(const FrameBuffer *source,\n>> \tif (!postProcessor_)\n>> \t\treturn 0;\n>> \n>> -\t/*\n>> -\t * \\todo Buffer mapping and processing should be moved to a\n>> -\t * separate thread.\n>> -\t */\n>> -\tconst StreamConfiguration &output = configuration();\n>> -\tCameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n>> -\t\t\t PROT_READ | PROT_WRITE);\n>> -\tif (!dest.isValid()) {\n>> -\t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n>> -\t\treturn -EINVAL;\n>> -\t}\n>> -\n>> -\treturn postProcessor_->process(source, &dest, requestMetadata,\n>> +\treturn postProcessor_->process(source, destBuffer, requestMetadata,\n>> \t\t\t\t resultMetadata, context);\n>> }\n>> \n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index be076995..8097ddbc 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -124,7 +124,7 @@ public:\n>> \n>> \tint configure();\n>> \tint process(const libcamera::FrameBuffer *source,\n>> -\t\t buffer_handle_t camera3Dest,\n>> +\t\t CameraBuffer *destBuffer,\n>> \t\t const CameraMetadata &requestMetadata,\n>> \t\t CameraMetadata *resultMetadata,\n>> \t\t const Camera3RequestDescriptor *context);","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 17949BDC71\n\tfor ;\n\tMon, 13 Sep 2021 07:06:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91F1069182;\n\tMon, 13 Sep 2021 09:06:02 +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 C2E4269181\n\tfor ;\n\tMon, 13 Sep 2021 09:06:00 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.152])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D65DA9E;\n\tMon, 13 Sep 2021 09:05:59 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"P66xuKrf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631516760;\n\tbh=6HCZIXWlmu66AKgySCrBVsZq8CamjyWED6xZ2Uusx3I=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=P66xuKrfBLh+AEx0mbADhSh/FR4vuR5Ol152Px101Y3V6iMDuuaxT97qAAwO85/MW\n\ttcKfYPRvMLbLfhf5CdsQcdcr4gWQZIPlWu9wNfY4YRW2D4CEkBX+sNPJWQq/cVcvaw\n\tfIo/ZV367dL6G8rqj4Lqi/8Sg/nrY+YpQicalKpg=","To":"Laurent Pinchart ","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-8-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<582bf525-0059-0246-7531-a35d26b00a83@ideasonboard.com>","Date":"Mon, 13 Sep 2021 12:35:56 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 7/9] android: camera_device: Move\n\tbuffer mapping for post processing","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\" "}}]