[{"id":16116,"web_url":"https://patchwork.libcamera.org/comment/16116/","msgid":"","date":"2021-04-03T22:21:43","subject":"Re: [libcamera-devel] [PATCH v3] android: CameraBuffer: Mark as\n\tinvalid if cros::CameraBufferManager::Register() fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Sun, Apr 04, 2021 at 01:22:47AM +0900, Hirokazu Honda wrote:\n> cros::CameraBufferManager::Register() fails if a buffer handle\n> is invalid. We should mark CameraBuffer as invalid on the failure\n> of Register().\n\nI'll add the following paragraph to describe the change in the\ndestructor:\n\nWhile the cros::CameraBufferManager Unlock() and Deregister() functions\nshould be able to handle buffers that haven't been locked and\nregistered, this isn't an API guarantee, and errors will be logged.\nAvoid this by skipping unlocking and unregistration of buffers that\nhaven't been locked or registered.\n\n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> src/android/mm/cros_camera_buffer.cpp | 22 +++++++++++++++-------\n> 1 file changed, 15 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index 7df4f47c..2a82d4b7 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -37,6 +37,7 @@ private:\n> \tbuffer_handle_t handle_;\n> \tunsigned int numPlanes_;\n> \tbool valid_;\n> +\tbool registered_;\n> \tunion {\n> \t\tvoid *addr;\n> \t\tandroid_ycbcr ycbcr;\n> @@ -49,16 +50,21 @@ private:\n> CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> \t\t\t buffer_handle_t camera3Buffer, int flags)\n> \t: Extensible::Private(cameraBuffer), handle_(camera3Buffer),\n> -\t numPlanes_(0), valid_(false)\n> +\t numPlanes_(0), valid_(false), registered(false)\n> {\n> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> \n> -\tbufferManager_->Register(camera3Buffer);\n> +\tint ret = bufferManager_->Register(camera3Buffer);\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed registering a buffer: \" << ret;\n> +\t\treturn;\n> +\t}\n> \n> +\tregistered_ = true;\n> \tnumPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> \tswitch (numPlanes_) {\n> \tcase 1: {\n> -\t\tint ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> +\t\tret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> \t\tif (ret) {\n> \t\t\tLOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> \t\t\treturn;\n> @@ -67,8 +73,8 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> \t}\n> \tcase 2:\n> \tcase 3: {\n> -\t\tint ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> -\t\t\t\t\t\t &mem.ycbcr);\n> +\t\tret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> +\t\t\t\t\t\t&mem.ycbcr);\n> \t\tif (ret) {\n> \t\t\tLOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> \t\t\treturn;\n> @@ -85,8 +91,10 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> \n> CameraBuffer::Private::~Private()\n> {\n> -\tbufferManager_->Unlock(handle_);\n> -\tbufferManager_->Deregister(handle_);\n> +\tif (valid_)\n> +\t\tbufferManager_->Unlock(handle_);\n> +\tif (registered_)\n> +\t\tbufferManager_->Deregister(handle_);\n> }\n> \n> unsigned int CameraBuffer::Private::numPlanes() const","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 49769BD695\n\tfor ;\n\tSat, 3 Apr 2021 22:22:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A72768787;\n\tSun, 4 Apr 2021 00:22:33 +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 855CC602E5\n\tfor ;\n\tSun, 4 Apr 2021 00:22:30 +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 6BEA171;\n\tSun, 4 Apr 2021 00:22:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"D8EVsgF1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617488550;\n\tbh=WcfYR1yWwKfRNQlmyavVI4/+it76nl1wUUeIWclNyso=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D8EVsgF1QXuOPxWxb58rJBMBrLXZdSiR1usp9KNIjQeNcUeRIkJO3BKmOvbhBAdEW\n\tr8iSYtcEXqRTuJeiwBXTpc7Qi2FUbyeRTLAcFU0deEi2BxA2yOcJLZDWXWKhD3eIQi\n\trx3pC8nyr6xy4S80n51LzHE0s3v31/UOUweb2bDE=","Date":"Sun, 4 Apr 2021 01:21:43 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210403162247.1618154-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210403162247.1618154-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3] android: CameraBuffer: Mark as\n\tinvalid if cros::CameraBufferManager::Register() fails","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]