[{"id":28850,"web_url":"https://patchwork.libcamera.org/comment/28850/","msgid":"","date":"2024-03-05T08:26:42","subject":"Re: [PATCH] android: camera_device: Fix camera blocked issue when\n\tstopping capture","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Anle Pan, Hui Fang\n\nOn 04/03/24 3:05 pm, Anle Pan wrote:\n> The issue occurs when stopping capture soon after starting capture.\n\nCan you describe the issue/use case in more detail? It's really hard to \ninfer from one line.\n>\n> In this case, no frame get from the device, the\n> related capture request has been pushed to the\n> queue descriptors_, but the queuedRequests_ was\n> still empty due to no requests will be queue to\n> the device since the stream will be stopped soon,\n\nI don't get it how is this possible? Since processCaptureRequest() will \npush the request in the descriptors_ queue along with queuing the \nrequest to the camera.\n\nHow will the call path be interleaved /before/ processCaptureRequest() \nreturns? Possibly we need more details on what you are trying to do, \nbefore reviewing the code here. Is this related to CTS ?\n> so there will be no camera->requestComplete called\n> later, then the descriptors_ can not pop normally,\n> this will cause the pending if we want to start capture next time.\n>\n> To fix the issue, ensure the descriptors_ is\n> empty after the camera device is stopped.\n>\n> Signed-off-by: Anle Pan \n> ---\n> src/android/camera_device.cpp | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 25cedd44..d452992d 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -433,8 +433,11 @@ void CameraDevice::flush()\n> void CameraDevice::stop()\n> {\n> \tMutexLocker stateLock(stateMutex_);\n> -\tif (state_ == State::Stopped)\n> +\tif (state_ == State::Stopped) {\n> +\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> +\t\tdescriptors_ = {};\n> \t\treturn;\n> +\t}\n> \n> \tcamera_->stop();\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 070F1BD160\n\tfor ;\n\tTue, 5 Mar 2024 08:26:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4870162875;\n\tTue, 5 Mar 2024 09:26:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F196F62868\n\tfor ;\n\tTue, 5 Mar 2024 09:26:50 +0100 (CET)","from [192.168.1.102] (unknown [103.251.226.116])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BBCDBCC8;\n\tTue, 5 Mar 2024 09:26:32 +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=\"rWqjCmHW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709627194;\n\tbh=kVCFIn0xT5d24IWBqRnFNRr5YqKJ4PD2NM+BX716yqU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=rWqjCmHW8M2nqQ5jC9RqNd1H3FIUlQ9aVouZkO8ybSX/SGD4xT9Xzv7DlFcTP2JFT\n\teiRw36t5kKmHBq+9iQInkJitv+y1spFFnkeYtusZ9Hvu2eaMaFGxTyI2HRYe65707w\n\th2O7vnp19Tn8IuaG5MHUWm7mWCpvJkBuaDhbAdhY=","Message-ID":"","Date":"Tue, 5 Mar 2024 13:56:42 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] android: camera_device: Fix camera blocked issue when\n\tstopping capture","Content-Language":"en-US","To":"Anle Pan , libcamera-devel@lists.libcamera.org","References":"<20240304093539.546973-1-anle.pan@nxp.com>","From":"Umang Jain ","In-Reply-To":"<20240304093539.546973-1-anle.pan@nxp.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":"hui.fang@nxp.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28858,"web_url":"https://patchwork.libcamera.org/comment/28858/","msgid":"","date":"2024-03-05T09:28:00","subject":"RE: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":189,"url":"https://patchwork.libcamera.org/api/people/189/","name":"Anle Pan","email":"anle.pan@nxp.com"},"content":"Hi Umang Jain,\n\nthis issue was random(around 1/10), and can be reproduced by single run below cts test:\n\t./cts-tradefed run commandAndExit cts --abi arm64-v8a -m CtsMediaPlayerTestCases -t android.media.player.cts.MediaPlayerTest#testRecordedVideoPlayback90\n\n•\t3 times configuring streams in this Test:\n1) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n2) 3 streams: (0) 320x240-NV12 (1) 320x240-NV12 (2) 1280x720-YUYV: will capture on stream0 and stream2 at the same time\n3) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n\n•\twhen single run the test several times and met fail. When digging the log, can find that the current failed is caused by the abnormal complete of the previous PASS test in step3, which is the situation described in my patch.\n\nBest Regards\nAnle Pan\n\n-----Original Message-----\nFrom: Umang Jain \nSent: 2024年3月5日 16:27\nTo: Anle Pan ; libcamera-devel@lists.libcamera.org\nCc: Hui Fang \nSubject: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n\nCaution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button\n\n\nHi Anle Pan, Hui Fang\n\nOn 04/03/24 3:05 pm, Anle Pan wrote:\n> The issue occurs when stopping capture soon after starting capture.\n\nCan you describe the issue/use case in more detail? It's really hard to infer from one line.\n>\n> In this case, no frame get from the device, the related capture \n> request has been pushed to the queue descriptors_, but the \n> queuedRequests_ was still empty due to no requests will be queue to \n> the device since the stream will be stopped soon,\n\nI don't get it how is this possible? Since processCaptureRequest() will push the request in the descriptors_ queue along with queuing the request to the camera.\n\nHow will the call path be interleaved /before/ processCaptureRequest() returns? Possibly we need more details on what you are trying to do, before reviewing the code here. Is this related to CTS ?\n> so there will be no camera->requestComplete called later, then the \n> descriptors_ can not pop normally, this will cause the pending if we \n> want to start capture next time.\n>\n> To fix the issue, ensure the descriptors_ is empty after the camera \n> device is stopped.\n>\n> Signed-off-by: Anle Pan \n> ---\n> src/android/camera_device.cpp | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/android/camera_device.cpp \n> b/src/android/camera_device.cpp index 25cedd44..d452992d 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -433,8 +433,11 @@ void CameraDevice::flush()\n> void CameraDevice::stop()\n> {\n> MutexLocker stateLock(stateMutex_);\n> - if (state_ == State::Stopped)\n> + if (state_ == State::Stopped) {\n> + MutexLocker descriptorsLock(descriptorsMutex_);\n> + descriptors_ = {};\n> return;\n> + }\n>\n> camera_->stop();\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 9BEDAC326B\n\tfor ;\n\tTue, 5 Mar 2024 09:28:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71D266286C;\n\tTue, 5 Mar 2024 10:28:04 +0100 (CET)","from EUR05-VI1-obe.outbound.protection.outlook.com\n\t(mail-vi1eur05on20600.outbound.protection.outlook.com\n\t[IPv6:2a01:111:f403:2613::600])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B578D62868\n\tfor ;\n\tTue, 5 Mar 2024 10:28:02 +0100 (CET)","from DU2PR04MB8967.eurprd04.prod.outlook.com (2603:10a6:10:2e2::19)\n\tby VI1PR04MB6989.eurprd04.prod.outlook.com (2603:10a6:803:131::15)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7339.39;\n\tTue, 5 Mar 2024 09:28:01 +0000","from DU2PR04MB8967.eurprd04.prod.outlook.com\n\t([fe80::863b:1be2:4278:3470]) by\n\tDU2PR04MB8967.eurprd04.prod.outlook.com\n\t([fe80::863b:1be2:4278:3470%4]) with mapi id 15.20.7339.035;\n\tTue, 5 Mar 2024 09:28:00 +0000"],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=nxp.com header.i=@nxp.com header.b=\"Tlr1Lxmb\";\n\tdkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=nxp.com;"],"ARC-Seal":"i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;\n\tb=cd6v9jRRB8I2h4hnLQdaM2UyH7hp0+cuoU41txf7f8Y8bBfU1uPk8aG/F75PKC/c204al/nGJaJ6FAirqYYW12bcT7f2sF0KjimLjIiN342kNrhrhJOuXkhks1hwVzbo0e7r6e/z0f/eDaZmFkm7da+7EhJv6JXx4ppWMiTXbhenDrtMS19V4t80jh4zLsPqrVQI76ipMLsl+AsrGEjl8wg99ZiP9vy/gneOMU6t2j2kYPE1XzoiHWLuS8wJ1QEgqr0XUTdNqQ82SnbDWCfj9tYCRUbpaj80SMihg5e8ubBcmPum1xW8QVb7od8/z34VTJ6EYW9CtbttJnor5CVrsQ==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector9901;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=f0jLHulrL1fsEC0tT0Ltr2pIyewVmaGcwT1WYKiuM8E=;\n\tb=Ed8nGNZCscaK03+5Krb+SldVzpn7r/36KzIFArNNQGUCENnz7XpVMElVNwVO7U2K1a2ABYTpaeQhte4526HrX5UejIfyFBEoZcll9dX4sqL/jAyFKSDDYMZCd1tM8FtNoPKr+TUH4jK450G8KCVD96wpm5fBJX5kCNBaDNWGtr4+SxK2qlAZDrMSA8MdtiokWnHgYVipnia/9ipCdoN7pZCjZBRt3gYt+SmWlwFwxciyZYyWLLRqHnnjL4kLrR2tYY2/qyLbAXuBmjeFLIs6Z2eZmX5K0qRs6OPfrg+9z/hAsaJSMQVfxS+drfzMsJecdaWVmKj2PtUiDSb4s3MdOQ==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com;\n\tdkim=pass header.d=nxp.com; arc=none","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=f0jLHulrL1fsEC0tT0Ltr2pIyewVmaGcwT1WYKiuM8E=;\n\tb=Tlr1Lxmbc4WD9Xw6TmSyxVgIv62CsB0IfCZLeN5CcHqbsEuBTm/WN357GERNNq0x5WvjR4nSbU0UCxhUuQGJHY8pY5++uDyj2h5ZZFJp0uImeCv8pDnLHYNEooGJcXIILFsvgYG/mcbrldXKr8jZOb2+MlRvnTIKKbEr6o4rDs8=","From":"Anle Pan ","To":"Umang Jain ,\n\t\"libcamera-devel@lists.libcamera.org\"\n\t","Subject":"RE: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Topic":"[EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Index":"AQHabhdXqS3pG8u3kEug+OYjFSpn1rEo0YUAgAAInlA=","Date":"Tue, 5 Mar 2024 09:28:00 +0000","Message-ID":"","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t","In-Reply-To":"","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","authentication-results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=nxp.com header.i=@nxp.com header.b=\"Tlr1Lxmb\";\n\tdkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=nxp.com;"],"x-ms-publictraffictype":"Email","x-ms-traffictypediagnostic":"DU2PR04MB8967:EE_|VI1PR04MB6989:EE_","x-ms-office365-filtering-correlation-id":"aa68f27d-dc4a-48c6-513c-08dc3cf68d3c","x-ms-exchange-senderadcheck":"1","x-ms-exchange-antispam-relay":"0","x-microsoft-antispam":"BCL:0;","x-microsoft-antispam-message-info":"+DsX6fr4x/YCxPgLqxfvp2kfgegZPRsw+Ldihtaw/T7+ICJHCG5rxOD+LUtz1iv822FtKF9mQxjhVD0DZHrJ+PoZNtYRVZ9jyIg9oHllsmLS6zfAdZINO849grJBwvmU26yhSMWdZNR8+FoSPJg0RCWLWisnkhIDMnxQaGe7sR7IbQbnP42eqQHN1olMVJOplBjYx/7hTI6F5ZIwPgiKyBwxC26OSXhS17VvmbsOTgKGQH1tsFgs3U0VQUEH51Vl5ltqcCDGPBLF1b7Wa/Ml6zO0q6i9N5YcSxn6x1hz2KY4SEAAqlKF9rKgqAWhq74dRLtRhWjH0WnsR/H2s4Jx2sRDeQQqeXBhb1g9GOegymrfehpDCMJWJPhg6ggUqI4eMgaQBSVBKZdHDgL8A0VkH7E8jKmsqoWB85zqwG9qZrrtx21u91Iu/bt+mqptyEw46OKFtIIWu9aIVFfshdKNZglSOOrCBUDUau9INz+Zlylpt9D4j32ojb/ABx0qeD0+I9MTpHopdq+JPYWKs/xf6nmeYwXuiB8wbpitafT26LUpdYSDwcI1WMvD+ydVMgboswYbkNjZ8Lv2ylQ9/giIKmi3dCuY2Mmi8Ky2a6ayBpKs4LAtckHjNtR3+OMJVp77mJLlMNpKqhJnDPFYDJ1bKuMlMqwrkYhkVJVHlKzt2KQs3EwmBUYBLiqsjE9CD1/HLwVf+K2oUkLok1XDC9Gy7w==","x-forefront-antispam-report":"CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;\n\tIPV:NLI; SFV:NSPM; H:DU2PR04MB8967.eurprd04.prod.outlook.com; PTR:;\n\tCAT:NONE; \n\tSFS:(13230031)(376005)(38070700009); DIR:OUT; SFP:1101; ","x-ms-exchange-antispam-messagedata-chunkcount":"1","x-ms-exchange-antispam-messagedata-0":"=?utf-8?q?BHJd/zy8JffrQN77lxMfebsUX?=\n\t=?utf-8?q?4cdO7pxlXnO3eg3nq8VgRsUQnXXvHjWgjNKPsuxMy1UNih1HZZ/TP47Y?=\n\t=?utf-8?q?lSxwkrShK8y4AaYYp9htKw7+aeL2vCnqImyons3nP6WS7xpLdlO0qU5c?=\n\t=?utf-8?q?du30MyX4qWCOgX1rQe9MPG79uZ9mC60nzf4xpKp2svFXAn5UtFgcdNfT?=\n\t=?utf-8?q?Co73X4Y+GEj26GmMHP0uku2Zt4w7VoXyEoOtq+YHeDRiVIVOT81PuOVm?=\n\t=?utf-8?q?oq0pJH3rKUbbt2wFUTQzDjAlrsJ8wytkE8rTSfgrAVXupE+w/k03Q0YP?=\n\t=?utf-8?q?voUTHphzGILNsBIMSEXv+SGFNtDlnzvy68SHeldHbt8sQXw6k7DY5qhf?=\n\t=?utf-8?q?b4QP3Np2rKvjeZyiHWMUIz6K8/swWqfT2LXDmM5ACXpIFgtLDRRmDZpt?=\n\t=?utf-8?q?at+1Kb8mqg3x5JNyHChgRF+Jwme4sTY+uce2ItSrQ2o3jZALrh2FypUZ?=\n\t=?utf-8?q?sNaR6pz1J3wnkody6dRgomj5pfODLIyfW6DmwQ/8EfUgU5TrZp/y/42X?=\n\t=?utf-8?q?ApLy4bIZhLR/FKnXliFgzj1CXHONXKtbUWpI01h1Dp4ECAqPf+RROKEe?=\n\t=?utf-8?q?/bw4o6JS8lgtDWTRS+J+VMD/mrVkUOeHLcahQXtcnmKQZHnyWXM57See?=\n\t=?utf-8?q?z8zJQg3tl+hH2IakMhPG57qhEkZHuajezpY2e6kLv/gvd26+VbihAmGo?=\n\t=?utf-8?q?noMoKSmugje9xHo4Pr0BNxd3wmOZdzGcC4aU/dwV06GFDLcYlxTQYOzW?=\n\t=?utf-8?q?JkuHUG9ofPuNJeo2uU+OSRoec0LnhFCXbUT6pw67Mkj6dcWvV56gU5he?=\n\t=?utf-8?q?/naJqRdPX8TPnR+jS2m0QsWk7qqh2ZFHh22ZYcvyYhz1BYDdIDg/aPK8?=\n\t=?utf-8?q?x+jnp3q4kSpVzCNaLUvaNstihOz8YkpsTOYJJNaWm94ewRu1rwG/90Sa?=\n\t=?utf-8?q?YE6tOlMoxVXCqHzrJKA6WhYunoP5Xk06YBRqzhk/O8OH+Pxsy92kQs6z?=\n\t=?utf-8?q?IHKnjoAVJf7/P4cida3P8dL2WEdz+yAPY/xEeNh8829DWib4fZQU+1kh?=\n\t=?utf-8?q?imLC5uATFezax0f7XN7c9aFZCEw0aIpvspFp9+KIZsesarQAu64b34ww?=\n\t=?utf-8?q?aS838fbECDYOEl3+Bp8D1zeelnVS3tovRDwxpAPPZ0ZpWuhkCp/yLYoO?=\n\t=?utf-8?q?0l3GHFZyWVn6Y7P69zVi1lP+vYXLxkANu3ri1WRhy6WInyl99Dj/DpJG?=\n\t=?utf-8?q?LeT5Ga8EUrGXsnZRFNNzkh3CCaB8fBb//g8S0saVyd5vuaCGiKFfvagP?=\n\t=?utf-8?q?rzObEjTaJGQgrplhzQtZD3a3rccAPfZk8CpUE4qHgeam3G2yDoDMjsvq?=\n\t=?utf-8?q?ko2LEXuxnBJg+OYMtwXgDtQTtnBOpIgpalBz+Q4UXJVNTrKTednzGkAP?=\n\t=?utf-8?q?eQQ7/eKwo0Or3NmzeTLJoxS9ifcoZO45SJZnYn6P4w5VRi9kvSOUzLWl?=\n\t=?utf-8?q?iRM23+BEfDT4GOyv2dM3EeTyAwiEKmEyuNMIvLKrgPwjWnUm8Yw78qf7?=\n\t=?utf-8?q?hQ+9bakaE9NmrTb1srfu0KQI1lIJ8xeyiydWk5HjLhqeAqdxabStl3MB?=\n\t=?utf-8?q?ac5oa55BH3A4kL0zPkgQeqL60XHsFjNyzl9tyZHFrI=3D?=","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","MIME-Version":"1.0","X-OriginatorOrg":"nxp.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-AuthSource":"DU2PR04MB8967.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"aa68f27d-dc4a-48c6-513c-08dc3cf68d3c","X-MS-Exchange-CrossTenant-originalarrivaltime":"05 Mar 2024 09:28:00.8401\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635","X-MS-Exchange-CrossTenant-mailboxtype":"HOSTED","X-MS-Exchange-CrossTenant-userprincipalname":"V6sOwAVBs9tr5DMOKlynSTTXAh0n6aAzBjkWonrGVk6zdKbb6JPkZlee+McDX5vdL99B+1LeaeRbvIx3HUhqPg==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"VI1PR04MB6989","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":"Hui Fang ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29037,"web_url":"https://patchwork.libcamera.org/comment/29037/","msgid":"","date":"2024-03-22T10:36:17","subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hello\n\nOn Tue, Mar 05, 2024 at 09:28:00AM +0000, Anle Pan wrote:\n> Hi Umang Jain,\n>\n> this issue was random(around 1/10), and can be reproduced by single run below cts test:\n> \t./cts-tradefed run commandAndExit cts --abi arm64-v8a -m CtsMediaPlayerTestCases -t android.media.player.cts.MediaPlayerTest#testRecordedVideoPlayback90\n>\n> •\t3 times configuring streams in this Test:\n> 1) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n> 2) 3 streams: (0) 320x240-NV12 (1) 320x240-NV12 (2) 1280x720-YUYV: will capture on stream0 and stream2 at the same time\n> 3) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n>\n> •\twhen single run the test several times and met fail. When digging the log, can find that the current failed is caused by the abnormal complete of the previous PASS test in step3, which is the situation described in my patch.\n>\n\nThe only code path I see that could lead to descriptor_ not being\ncleared is flush() being called and then stop() called just after\npossibily as the result of a new configuration\n\nvoid CameraDevice::flush()\n{\n\t{\n\t\tMutexLocker stateLock(stateMutex_);\n\t\tif (state_ != State::Running)\n\t\t\treturn;\n\n\t\tstate_ = State::Flushing;\n\t}\n\n\tcamera_->stop();\n\n\tMutexLocker stateLock(stateMutex_);\n\tstate_ = State::Stopped;\n}\n\nvoid CameraDevice::stop()\n{\n\tMutexLocker stateLock(stateMutex_);\n\tif (state_ == State::Stopped)\n\t\treturn;\n\n\tcamera_->stop();\n\n\t{\n\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n\t\tdescriptors_ = {};\n\t}\n\n\tstreams_.clear();\n\n\tstate_ = State::Stopped;\n}\n\nint CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n{\n\t/* Before any configuration attempt, stop the camera. */\n\tstop();\n\n ...\n\n}\n\nLooking at the flush() implementation right now I wonder why we stop\nthe camera without the stateMutex_ held (the only reason I see is\nbecause it can be concurrent with processCaptureRequest() which keeps\nthe mutex held), but I certainly don't want to touch that code right\nnow without a good reason.\n\nAll in all, I think there's indeed a path that could lead to the\ndescriptor not being cleaned up, and we can't do it in flush() because\na concurrent call to processCaptureRequest() might be using the\ndescriptors to complete the in-flight requests with error state.\n\nAnle, does this match your understanding ? If so, this is what should\nbe recorded in the commit message (the cause of an issue, not the\nsymptoms)\n\n\n> Best Regards\n> Anle Pan\n>\n> -----Original Message-----\n> From: Umang Jain \n> Sent: 2024年3月5日 16:27\n> To: Anle Pan ; libcamera-devel@lists.libcamera.org\n> Cc: Hui Fang \n> Subject: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n>\n> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button\n>\n>\n> Hi Anle Pan, Hui Fang\n>\n> On 04/03/24 3:05 pm, Anle Pan wrote:\n> > The issue occurs when stopping capture soon after starting capture.\n>\n> Can you describe the issue/use case in more detail? It's really hard to infer from one line.\n> >\n> > In this case, no frame get from the device, the related capture\n> > request has been pushed to the queue descriptors_, but the\n> > queuedRequests_ was still empty due to no requests will be queue to\n> > the device since the stream will be stopped soon,\n>\n> I don't get it how is this possible? Since processCaptureRequest() will push the request in the descriptors_ queue along with queuing the request to the camera.\n>\n> How will the call path be interleaved /before/ processCaptureRequest() returns? Possibly we need more details on what you are trying to do, before reviewing the code here. Is this related to CTS ?\n> > so there will be no camera->requestComplete called later, then the\n> > descriptors_ can not pop normally, this will cause the pending if we\n> > want to start capture next time.\n> >\n> > To fix the issue, ensure the descriptors_ is empty after the camera\n> > device is stopped.\n\nDo you know precisely why a non-empty descriptors_ stalls the camera ?\nDoes it get stuck on\n\nvoid CameraDevice::sendCaptureResults()\n{\n\twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n\n?\n\n> >\n> > Signed-off-by: Anle Pan \n\nTested with CTS, no regressions detected\n\nReviewed-by: Jacopo Mondi \nTested-by: Jacopo Mondi \n\n> > ---\n> > src/android/camera_device.cpp | 5 ++++-\n> > 1 file changed, 4 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/android/camera_device.cpp\n> > b/src/android/camera_device.cpp index 25cedd44..d452992d 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -433,8 +433,11 @@ void CameraDevice::flush()\n> > void CameraDevice::stop()\n> > {\n> > MutexLocker stateLock(stateMutex_);\n> > - if (state_ == State::Stopped)\n> > + if (state_ == State::Stopped) {\n> > + MutexLocker descriptorsLock(descriptorsMutex_);\n> > + descriptors_ = {};\n> > return;\n> > + }\n> >\n> > camera_->stop();\n> >\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 B4C86C3272\n\tfor ;\n\tFri, 22 Mar 2024 10:36:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7209363055;\n\tFri, 22 Mar 2024 11:36:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47D8362CA3\n\tfor ;\n\tFri, 22 Mar 2024 11:36:20 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 611C482A;\n\tFri, 22 Mar 2024 11:35:51 +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=\"nP0bDXJ/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711103751;\n\tbh=c5uCX/kE+jCc5hUQ7vJ1RS37F31svl1xdeiNjDmj6ls=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nP0bDXJ/lnmVpdCYppFwPdxXC240LLU+VyoxjrJmenAOazjyQRP2P8c7zB+9U7CZM\n\tSrdZKEJCwJYDjBwIDClDzZfy7lGnhbi3F3v6K25PHNN3rhVTzEEcKEdNhxW0sJPTvr\n\t7hnty0RXZGGwD9tOIEdSdcbTcJAgdvHxn4ze8jD8=","Date":"Fri, 22 Mar 2024 11:36:17 +0100","From":"Jacopo Mondi ","To":"Anle Pan ","Cc":"Umang Jain , \n\t\"libcamera-devel@lists.libcamera.org\"\n\t, Hui Fang ","Subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Message-ID":"","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\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":29048,"web_url":"https://patchwork.libcamera.org/comment/29048/","msgid":"<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>","date":"2024-03-25T07:43:37","subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for testing,\n\nOn 22/03/24 4:06 pm, Jacopo Mondi wrote:\n> Hello\n>\n> On Tue, Mar 05, 2024 at 09:28:00AM +0000, Anle Pan wrote:\n>> Hi Umang Jain,\n>>\n>> this issue was random(around 1/10), and can be reproduced by single run below cts test:\n>> \t./cts-tradefed run commandAndExit cts --abi arm64-v8a -m CtsMediaPlayerTestCases -t android.media.player.cts.MediaPlayerTest#testRecordedVideoPlayback90\n>>\n>> •\t3 times configuring streams in this Test:\n>> 1) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n>> 2) 3 streams: (0) 320x240-NV12 (1) 320x240-NV12 (2) 1280x720-YUYV: will capture on stream0 and stream2 at the same time\n>> 3) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n>>\n>> •\twhen single run the test several times and met fail. When digging the log, can find that the current failed is caused by the abnormal complete of the previous PASS test in step3, which is the situation described in my patch.\n>>\n> The only code path I see that could lead to descriptor_ not being\n> cleared is flush() being called and then stop() called just after\n> possibily as the result of a new configuration\n>\n> void CameraDevice::flush()\n> {\n> \t{\n> \t\tMutexLocker stateLock(stateMutex_);\n> \t\tif (state_ != State::Running)\n> \t\t\treturn;\n>\n> \t\tstate_ = State::Flushing;\n> \t}\n>\n> \tcamera_->stop();\n>\n> \tMutexLocker stateLock(stateMutex_);\n> \tstate_ = State::Stopped;\n> }\n>\n> void CameraDevice::stop()\n> {\n> \tMutexLocker stateLock(stateMutex_);\n> \tif (state_ == State::Stopped)\n> \t\treturn;\n>\n> \tcamera_->stop();\n>\n> \t{\n> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> \t\tdescriptors_ = {};\n> \t}\n>\n> \tstreams_.clear();\n>\n> \tstate_ = State::Stopped;\n> }\n>\n> int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> {\n> \t/* Before any configuration attempt, stop the camera. */\n> \tstop();\n>\n> ...\n>\n> }\n>\n> Looking at the flush() implementation right now I wonder why we stop\n> the camera without the stateMutex_ held (the only reason I see is\n> because it can be concurrent with processCaptureRequest() which keeps\n> the mutex held), but I certainly don't want to touch that code right\n> now without a good reason.\n>\n> All in all, I think there's indeed a path that could lead to the\n> descriptor not being cleaned up, and we can't do it in flush() because\n> a concurrent call to processCaptureRequest() might be using the\n> descriptors to complete the in-flight requests with error state.\n>\n> Anle, does this match your understanding ? If so, this is what should\n> be recorded in the commit message (the cause of an issue, not the\n> symptoms)\n>\n>\n>> Best Regards\n>> Anle Pan\n>>\n>> -----Original Message-----\n>> From: Umang Jain \n>> Sent: 2024年3月5日 16:27\n>> To: Anle Pan ; libcamera-devel@lists.libcamera.org\n>> Cc: Hui Fang \n>> Subject: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n>>\n>> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button\n>>\n>>\n>> Hi Anle Pan, Hui Fang\n>>\n>> On 04/03/24 3:05 pm, Anle Pan wrote:\n>>> The issue occurs when stopping capture soon after starting capture.\n>> Can you describe the issue/use case in more detail? It's really hard to infer from one line.\n>>> In this case, no frame get from the device, the related capture\n>>> request has been pushed to the queue descriptors_, but the\n>>> queuedRequests_ was still empty due to no requests will be queue to\n>>> the device since the stream will be stopped soon,\n>> I don't get it how is this possible? Since processCaptureRequest() will push the request in the descriptors_ queue along with queuing the request to the camera.\n>>\n>> How will the call path be interleaved /before/ processCaptureRequest() returns? Possibly we need more details on what you are trying to do, before reviewing the code here. Is this related to CTS ?\n>>> so there will be no camera->requestComplete called later, then the\n>>> descriptors_ can not pop normally, this will cause the pending if we\n>>> want to start capture next time.\n>>>\n>>> To fix the issue, ensure the descriptors_ is empty after the camera\n>>> device is stopped.\n> Do you know precisely why a non-empty descriptors_ stalls the camera ?\n> Does it get stuck on\n>\n> void CameraDevice::sendCaptureResults()\n> {\n> \twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n>\n> ?\n\nWe should to formalise this as an proper issue, after getting more \ninformation.\n>>> Signed-off-by: Anle Pan \n> Tested with CTS, no regressions detected\n>\n> Reviewed-by: Jacopo Mondi \n> Tested-by: Jacopo Mondi \n>\n>>> ---\n>>> src/android/camera_device.cpp | 5 ++++-\n>>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp\n>>> b/src/android/camera_device.cpp index 25cedd44..d452992d 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -433,8 +433,11 @@ void CameraDevice::flush()\n>>> void CameraDevice::stop()\n>>> {\n>>> MutexLocker stateLock(stateMutex_);\n>>> - if (state_ == State::Stopped)\n>>> + if (state_ == State::Stopped) {\n>>> + MutexLocker descriptorsLock(descriptorsMutex_);\n>>> + descriptors_ = {};\n>>> return;\n>>> + }\n\nI am not comfortable with this patch. Consider my points below\n\n- First we don't have a exact description of the issue it is fixing\n\n- Second, if the camera state is ::Stopped already. Clears descriptors_ \ndoesn't look a good idea to me. The function which has set the camera \nstate::Stopped already, should be ideally be emptying the descriptors_...\n\n- If we are accepting this, why not just let the Camera::stop() fall \nthrough and remove\n\n             if (state_ == State::Stopped)\n                 return;\n\nas well. Then, CameraDevice::stop() will behave same as Camera::stop() - \na no-op when called even when State::Stopped and responsible to clean up \nqueues(and stuff) for the HAL layer.\n\nI should ideally back up my thoughts with some testing, but I don't have \nany kind of CTS testing available to me right now :(\n>>> camera_->stop();\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 96045C0DA4\n\tfor ;\n\tMon, 25 Mar 2024 07:43:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58A0D6308D;\n\tMon, 25 Mar 2024 08:43:48 +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 3E0FD61C37\n\tfor ;\n\tMon, 25 Mar 2024 08:43:45 +0100 (CET)","from [192.168.1.105] (unknown [103.251.226.53])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 380977E4;\n\tMon, 25 Mar 2024 08:43:11 +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=\"VEs+N/ch\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711352594;\n\tbh=1RtYaaFJZdWNA83nkWd1SI/N9FjHLSVwJx8sTW3Uhqc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=VEs+N/chketZn3jZXNhDMzQdfgTbC3+5a94HPYATaaxjvXvq5x9u4x4kRo/YaXEIu\n\tDnlBhNKYObkDOfoiSbaYPe6w/vG2WSHHBJ000SNWJn/sPb0Q8vXleD77BVbZxzIvAZ\n\t3nFEUY1iBOfmD0Ivp0SNvOXspkzTg7QpmDAEceYU=","Message-ID":"<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>","Date":"Mon, 25 Mar 2024 13:13:37 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Content-Language":"en-US","To":"Jacopo Mondi , Anle Pan ","Cc":"\"libcamera-devel@lists.libcamera.org\"\n\t, Hui Fang ","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\n\t\n\t","From":"Umang Jain ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":29049,"web_url":"https://patchwork.libcamera.org/comment/29049/","msgid":"","date":"2024-03-25T09:20:43","subject":"RE: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":189,"url":"https://patchwork.libcamera.org/api/people/189/","name":"Anle Pan","email":"anle.pan@nxp.com"},"content":"Hi Umang, Jacopo.\n\nYes, when issue happened, stuck in sendCaptureResults. The reproduce chance on my side was also not high for the cts (1/10)\n\n\t\tvoid CameraDevice::sendCaptureResults()\n\t\t{\n \twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n\nAnd Jacopo, your description is accurate, the descriptors_ has a chance to be not cleared due to the State was already 'Stopped' after flush() .\nwill ask hui to help update the commit message since the \"git send\" configuration has issue on my side.\n\nBest Regards\nAnle Pan\n\n-----Original Message-----\nFrom: Umang Jain \nSent: 2024年3月25日 15:44\nTo: Jacopo Mondi ; Anle Pan \nCc: libcamera-devel@lists.libcamera.org; Hui Fang \nSubject: Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n\nCaution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button\n\n\nHi Jacopo,\n\nThanks for testing,\n\nOn 22/03/24 4:06 pm, Jacopo Mondi wrote:\n> Hello\n>\n> On Tue, Mar 05, 2024 at 09:28:00AM +0000, Anle Pan wrote:\n>> Hi Umang Jain,\n>>\n>> this issue was random(around 1/10), and can be reproduced by single run below cts test:\n>> ./cts-tradefed run commandAndExit cts --abi arm64-v8a -m \n>> CtsMediaPlayerTestCases -t \n>> android.media.player.cts.MediaPlayerTest#testRecordedVideoPlayback90\n>>\n>> • 3 times configuring streams in this Test:\n>> 1) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n>> 2) 3 streams: (0) 320x240-NV12 (1) 320x240-NV12 (2) 1280x720-YUYV: will capture on stream0 and stream2 at the same time\n>> 3) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n>>\n>> • when single run the test several times and met fail. When digging the log, can find that the current failed is caused by the abnormal complete of the previous PASS test in step3, which is the situation described in my patch.\n>>\n> The only code path I see that could lead to descriptor_ not being \n> cleared is flush() being called and then stop() called just after \n> possibily as the result of a new configuration\n>\n> void CameraDevice::flush()\n> {\n> {\n> MutexLocker stateLock(stateMutex_);\n> if (state_ != State::Running)\n> return;\n>\n> state_ = State::Flushing;\n> }\n>\n> camera_->stop();\n>\n> MutexLocker stateLock(stateMutex_);\n> state_ = State::Stopped;\n> }\n>\n> void CameraDevice::stop()\n> {\n> MutexLocker stateLock(stateMutex_);\n> if (state_ == State::Stopped)\n> return;\n>\n> camera_->stop();\n>\n> {\n> MutexLocker descriptorsLock(descriptorsMutex_);\n> descriptors_ = {};\n> }\n>\n> streams_.clear();\n>\n> state_ = State::Stopped;\n> }\n>\n> int CameraDevice::configureStreams(camera3_stream_configuration_t \n> *stream_list) {\n> /* Before any configuration attempt, stop the camera. */\n> stop();\n>\n> ...\n>\n> }\n>\n> Looking at the flush() implementation right now I wonder why we stop \n> the camera without the stateMutex_ held (the only reason I see is \n> because it can be concurrent with processCaptureRequest() which keeps \n> the mutex held), but I certainly don't want to touch that code right \n> now without a good reason.\n>\n> All in all, I think there's indeed a path that could lead to the \n> descriptor not being cleaned up, and we can't do it in flush() because \n> a concurrent call to processCaptureRequest() might be using the \n> descriptors to complete the in-flight requests with error state.\n>\n> Anle, does this match your understanding ? If so, this is what should \n> be recorded in the commit message (the cause of an issue, not the\n> symptoms)\n>\n>\n>> Best Regards\n>> Anle Pan\n>>\n>> -----Original Message-----\n>> From: Umang Jain \n>> Sent: 2024年3月5日 16:27\n>> To: Anle Pan ; libcamera-devel@lists.libcamera.org\n>> Cc: Hui Fang \n>> Subject: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked \n>> issue when stopping capture\n>>\n>> Caution: This is an external email. Please take care when clicking \n>> links or opening attachments. When in doubt, report the message using \n>> the 'Report this email' button\n>>\n>>\n>> Hi Anle Pan, Hui Fang\n>>\n>> On 04/03/24 3:05 pm, Anle Pan wrote:\n>>> The issue occurs when stopping capture soon after starting capture.\n>> Can you describe the issue/use case in more detail? It's really hard to infer from one line.\n>>> In this case, no frame get from the device, the related capture \n>>> request has been pushed to the queue descriptors_, but the \n>>> queuedRequests_ was still empty due to no requests will be queue to \n>>> the device since the stream will be stopped soon,\n>> I don't get it how is this possible? Since processCaptureRequest() will push the request in the descriptors_ queue along with queuing the request to the camera.\n>>\n>> How will the call path be interleaved /before/ processCaptureRequest() returns? Possibly we need more details on what you are trying to do, before reviewing the code here. Is this related to CTS ?\n>>> so there will be no camera->requestComplete called later, then the \n>>> descriptors_ can not pop normally, this will cause the pending if we \n>>> want to start capture next time.\n>>>\n>>> To fix the issue, ensure the descriptors_ is empty after the camera \n>>> device is stopped.\n> Do you know precisely why a non-empty descriptors_ stalls the camera ?\n> Does it get stuck on\n>\n> void CameraDevice::sendCaptureResults()\n> {\n> while (!descriptors_.empty() && \n> !descriptors_.front()->isPending()) {\n>\n> ?\n\nWe should to formalise this as an proper issue, after getting more information.\n>>> Signed-off-by: Anle Pan \n> Tested with CTS, no regressions detected\n>\n> Reviewed-by: Jacopo Mondi \n> Tested-by: Jacopo Mondi \n>\n>>> ---\n>>> src/android/camera_device.cpp | 5 ++++-\n>>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp \n>>> b/src/android/camera_device.cpp index 25cedd44..d452992d 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -433,8 +433,11 @@ void CameraDevice::flush()\n>>> void CameraDevice::stop()\n>>> {\n>>> MutexLocker stateLock(stateMutex_);\n>>> - if (state_ == State::Stopped)\n>>> + if (state_ == State::Stopped) {\n>>> + MutexLocker descriptorsLock(descriptorsMutex_);\n>>> + descriptors_ = {};\n>>> return;\n>>> + }\n\nI am not comfortable with this patch. Consider my points below\n\n- First we don't have a exact description of the issue it is fixing\n\n- Second, if the camera state is ::Stopped already. Clears descriptors_ doesn't look a good idea to me. The function which has set the camera state::Stopped already, should be ideally be emptying the descriptors_...\n\n- If we are accepting this, why not just let the Camera::stop() fall through and remove\n\n if (state_ == State::Stopped)\n return;\n\nas well. Then, CameraDevice::stop() will behave same as Camera::stop() - a no-op when called even when State::Stopped and responsible to clean up queues(and stuff) for the HAL layer.\n\nI should ideally back up my thoughts with some testing, but I don't have any kind of CTS testing available to me right now :(\n>>> camera_->stop();\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 B67E4BD160\n\tfor ;\n\tMon, 25 Mar 2024 09:20:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71984632EA;\n\tMon, 25 Mar 2024 10:20:47 +0100 (CET)","from EUR02-AM0-obe.outbound.protection.outlook.com\n\t(mail-am0eur02on20601.outbound.protection.outlook.com\n\t[IPv6:2a01:111:f403:2606::601])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A87ED61C40\n\tfor ;\n\tMon, 25 Mar 2024 10:20:45 +0100 (CET)","from DU2PR04MB8967.eurprd04.prod.outlook.com (2603:10a6:10:2e2::19)\n\tby AM8PR04MB7235.eurprd04.prod.outlook.com (2603:10a6:20b:1d1::8)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7409.31;\n\tMon, 25 Mar 2024 09:20:44 +0000","from DU2PR04MB8967.eurprd04.prod.outlook.com\n\t([fe80::863b:1be2:4278:3470]) by\n\tDU2PR04MB8967.eurprd04.prod.outlook.com\n\t([fe80::863b:1be2:4278:3470%4]) with mapi id 15.20.7409.023;\n\tMon, 25 Mar 2024 09:20:43 +0000"],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=nxp.com header.i=@nxp.com header.b=\"ssJ3rBEw\";\n\tdkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=nxp.com;"],"ARC-Seal":"i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;\n\tb=bHUyRA5FbmgeW0fNjHDoaUhcmdbShdCDXX46FDIIg6kZJ6xJWY0U5+QAsa2gqDHKae/R/TZB2JIowtw0aFjAT4z4tk4yPWYlq2fuA1uvBnku0vuDW664lIyarZ3mWPzcCc2AaKj3MJIu99oi0DCKP1sr7JPHJDgKDLXtS4tziqaB52tdFvlnwOq+gR9/h5abguPwKx9+aRRe+ufryV519knPsPDIQ/MRgj/7padjDuarK83up9CXin9YoDxX2eXt1wWeYb6CmHOOmJccIY1S8UwKFg5iURyBmcMSsmw3rMeQvbNnA+NDbVjY7kUYl5CSBSUDfN+KmEYnaXoz4CiirQ==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector9901;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=tmQI27BDJid+bInMFd0XXw/k3OVsfJeeZSeV8xeN+3M=;\n\tb=E8nZ0cdhPIx43JT5xbzx7OBCfeUElBPavGqekTZNillTaVs6uLkJLUFFHxRI7MuNBxalLFoRcDEZ+9RTcDZC1PzmRgm357gOzyudBC9fI/iUc3m4xGnBMsKIHbCPExQ49LQVrQ5FCviGHuPeoGm9VKZylV8f54XQpXeW7pTr/xuVr1A9EjC+x17+b93ZPWI/ngLS9yt4pgjW2oAZ8sQFl3fVfbvgM8siNckZCHwwRH88/FtoUp98aVcIspjBT+ndHmt6nX60yn4V+RY2HEKe+mgpuyQxzL0nlPP488lSakgj+Hw45hHKUuF02ayuI0H92172RRh4Y7D/OC0fgEDnTA==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com;\n\tdkim=pass header.d=nxp.com; arc=none","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=tmQI27BDJid+bInMFd0XXw/k3OVsfJeeZSeV8xeN+3M=;\n\tb=ssJ3rBEwEDIde00lDNG8ceILC0pUuclOOT4Ldmrisd4IPSICwCWVL4exgXvHjIpF5ySRQ58dsnl9UTyHeq/C7NB2tvxV18vV8u2t2hCGyDG8lucFa1t3S7uO8QKdqVDgEQ4JJhw9JQjMwQNekFKp+kminneo54bPO36RgTwKdzU=","From":"Anle Pan ","To":"Umang Jain , Jacopo Mondi\n\t","CC":"\"libcamera-devel@lists.libcamera.org\"\n\t, Hui Fang ","Subject":"RE: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Topic":"[EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Index":"AQHabhdXqS3pG8u3kEug+OYjFSpn1rEo0YUAgAAInlCAGtM4gIAEhsGAgAAEMWA=","Date":"Mon, 25 Mar 2024 09:20:43 +0000","Message-ID":"","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\n\t\n\t\n\t<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>","In-Reply-To":"<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","authentication-results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=nxp.com header.i=@nxp.com header.b=\"ssJ3rBEw\";\n\tdkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=nxp.com;"],"x-ms-publictraffictype":"Email","x-ms-traffictypediagnostic":"DU2PR04MB8967:EE_|AM8PR04MB7235:EE_","x-ms-office365-filtering-correlation-id":"39c0d088-75b0-4268-1bfd-08dc4cacd90b","x-ms-exchange-senderadcheck":"1","x-ms-exchange-antispam-relay":"0","x-microsoft-antispam":"BCL:0;","x-microsoft-antispam-message-info":"8MNgeXA6R0MrOflTB8+AOibvNQD7kXbLYEKqwG+Biw2Yz5+G0E2dK418TW7GgmpNR3KKsNhuF207AinpGv05c4O4TCPieSrJ+dZS2DE4Mpi5VmMDdDh1v+mc0W3UDL/ECp7PgOKX8UrYMMk2aIoAqOb3xF85D0FRlXrB02XWqIibsWSzEF4QB8wcodDPtw1AKUPiWY0zH11DIMIi3JmAFA4Lrk8WAhhVqa4Ikulug8GZ3bKivGMlzktuJV3Yh22s3ZtAG7loXDmEJ32g561V2LZGG89dF4dOKCfK16Q2wQlIFrox+fKma4+ruDy5Ocn0iN6v6nXfgBNeHo22TzazpQ7HzOVuGfOEjsSoaxFjQtBcilStLVtfK5SB9bogcR96V8UFmZrGr5Lq7I+5IGiXxxTcf9Q9dIPgQgihZ4O71A3sGdv4XzshuB384IMYkoaHb4d8dGAPB7mIl4DjY+s1cSi0y8EZRA3BQl8waP767ywyMl3vDSzOeI0R/btAHPO2/ncaFTYkhpGBo03ZA3JVNJb6eCmuo/sMOGwqhQ/va6zP0yjfNLi9z8WqCXlW/WUnOzUe75Zh8swmyu8l2orNAykvQSC4fvV4UYRTtPBXwtTtNeots27oxcFqhHdSNfoKpBkK6FqUzdwm/c/CTu8nG/vHXmTx0Fk8c8VZOAlRlh8TXCSgXaknExYOgJJL4Bxz3klQEvET204orIDMEk/NDg==","x-forefront-antispam-report":"CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;\n\tIPV:NLI; SFV:NSPM; H:DU2PR04MB8967.eurprd04.prod.outlook.com; PTR:;\n\tCAT:NONE; \n\tSFS:(13230031)(376005)(1800799015)(366007)(38070700009); DIR:OUT;\n\tSFP:1101; ","x-ms-exchange-antispam-messagedata-chunkcount":"1","x-ms-exchange-antispam-messagedata-0":"=?utf-8?q?urgRBMXLfi28Af1BBzJKYfSMm?=\n\t=?utf-8?q?EFXS/qcMnocjgTyyo52zaOqifNglKBIF9tlCuMkntEdebG7gXp3cXCKm?=\n\t=?utf-8?q?ycN92TEiK1CapG9IgYIPqpFhQQYIxmVJK61x6XRCn757oFdd1l9pHJ2N?=\n\t=?utf-8?q?Zz53cqwWzcPauaPDuFt14lYS8977ZrHkIchJOXfrS/B8ZIHOpg1AYYmJ?=\n\t=?utf-8?q?D9f0ZyWfUgpkZ5rxq9gzx2srzHB2e1NkeBB+vlMRn7l3pgOiW6/tuRte?=\n\t=?utf-8?q?zIJbg9GACk99tOhBsQ9PtId6e2am2ZdMbpAUUvJhn0z4uIFwZfzKpE5V?=\n\t=?utf-8?q?WkrR4ICHRYUpS9fILrEVplkWXiCHtP1t0ZXCRJvYVHzZQtObgec08CPU?=\n\t=?utf-8?q?2aaqnYqzXSdlmBTPyIFTTm/5y4/+VzgePjvX/F+VNPkuyWYqzlblUwZy?=\n\t=?utf-8?q?kM4JrJzGkVsBQpRBZwFQKxrfBYy8gs7VXbi7jk8EtwcM34Rkg3BItYYS?=\n\t=?utf-8?q?4sBXvbwewTx8Z3H5aqZntoX3J3fDDQTya8SdIj0wu0csRMuKNvXGZ4h/?=\n\t=?utf-8?q?wM8n3rZAduhLMqsM/kOP0DynKrfv1zKNeKYYoiNmExfxs2tYeBdsHI/z?=\n\t=?utf-8?q?A0nrzGX0ilMKWYL8vfZD0kDpKv0SVpZxQPau9z1t0aPRYk4FBIPQP1DC?=\n\t=?utf-8?q?lIE2r11dFBwpnlgeRKwfsk7EZaugUA1RU8Ew+SZFj4AAi1GLRY10eiZ7?=\n\t=?utf-8?q?AfTMGbs/0brLLq0BiSD3fS/qZcu/goE24eNca2MzwlQ7A5FjULmbKkYa?=\n\t=?utf-8?q?SxzlPlZsPihpLg17igHLf+nlanSzBdPBKO5X4UNbxydnEqAvHk5cGg1G?=\n\t=?utf-8?q?ViwgJPq4bXXS0137BTGO6qRvonV8Kskb2LZUgsBUWzcZxdzXBF0N9JHZ?=\n\t=?utf-8?q?lb/Xx6oXwYO8MiUOb0ZKxToPTpJZcwacbXvGG/jLRYEembvBBdKwcteh?=\n\t=?utf-8?q?U6EZFRQ6RwkuxiPJqejX1nfPGxhF93O7ep6BoPTdXkl+K1QpRvNriZS2?=\n\t=?utf-8?q?H1magL/1qakKQz8OFuJ1fcyLlugz6QIefnzbcCGBDRjAtj4y2fesbhyU?=\n\t=?utf-8?q?2nDpos11AuNrQeC6xCXU8Mec8spHxtQQLnWjQnWLHLOx93LY0lif++Bc?=\n\t=?utf-8?q?nXazhHoMommAS/2tGPBK/5YWVia6XxRjz+YtqnBMAYZG8o80M+Kk3hx6?=\n\t=?utf-8?q?2MUEHhdlTR1+XZbuW9X4v8XD65wRQG8Sg5OszHEELeJ7wpgKDTXgmjSZ?=\n\t=?utf-8?q?q7nzThsc3vQv18uhhJjlOu6K6fIg7x1I5Hcx92rqwF0ST87T3cwuzjpQ?=\n\t=?utf-8?q?Wl0Hi+CZK6b/dB6ru28fQ4p5+LPPh4FVt3NvaPqAfMg94oqwpKJeV7Nj?=\n\t=?utf-8?q?vx1vniMwjq8oZsIcBV52DwbMxySO4hqqIpYcVVwcd54NWTqNSLMQJa7C?=\n\t=?utf-8?q?kwU+OCVydGoCP1tU+yqIp4vTL+gIfu2yCdwgZon1H5ZnkEbyZih5cWST?=\n\t=?utf-8?q?pXap1mu3nkT4NrqpmbdL9/tGXyx03QHVTicqeutS2zx13eYnFekxvNzh?=\n\t=?utf-8?q?h1Hl1MepEfDdxLamzpGecaGAkgYVi3M75OWWweQeuxCah5nxdkGCuK67?=\n\t=?utf-8?q?tIAxTpKNdvas8qxILd8bPKNT/7ii1H3geU0VeBhhVg=3D?=","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","MIME-Version":"1.0","X-OriginatorOrg":"nxp.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-AuthSource":"DU2PR04MB8967.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"39c0d088-75b0-4268-1bfd-08dc4cacd90b","X-MS-Exchange-CrossTenant-originalarrivaltime":"25 Mar 2024 09:20:43.8611\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635","X-MS-Exchange-CrossTenant-mailboxtype":"HOSTED","X-MS-Exchange-CrossTenant-userprincipalname":"QEQHXwxgOUwg092Ht7oagp5HtviqVta4v5K4YYkQwpk9TZcCKhB1L2xqJfJ48CCbBloupLZo0SMgn3dNOZ1PKA==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"AM8PR04MB7235","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":29057,"web_url":"https://patchwork.libcamera.org/comment/29057/","msgid":"","date":"2024-03-26T03:13:05","subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":186,"url":"https://patchwork.libcamera.org/api/people/186/","name":"Fang Hui","email":"hui.fang@nxp.com"},"content":"@Anle Pan No need \"git send-email\" again, will new another topic.\n\nJust reply the refined patch.\n\nAuthor: Anle Pan \nDate: Fri Mar 1 17:32:54 2024 +0000\n\n android: camera_device: Fix the stuck issue in sendCaptureResults.\n\n When flush() is being called and then a new stream configuration is set,\n descriptor_ may has a chance to be not cleared, which was skipped due to\n the Stopped State. This will cause a stuck in sendCaptureResults.\n\n To fix the issue, ensure the descriptors_ is always empty when the camera\n State is Stopped.\n\n Signed-off-by: Anle Pan \n\ndiff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex d2679a97..7fda6ce6 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -433,8 +433,11 @@ void CameraDevice::flush()\n void CameraDevice::stop()\n {\n MutexLocker stateLock(stateMutex_);\n- if (state_ == State::Stopped)\n+ if (state_ == State::Stopped) {\n+ MutexLocker descriptorsLock(descriptorsMutex_);\n+ descriptors_ = {};\n return;\n+ }\n\n camera_->stop();","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 5897BBD160\n\tfor ;\n\tTue, 26 Mar 2024 03:13:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC1CB632EA;\n\tTue, 26 Mar 2024 04:13:10 +0100 (CET)","from EUR01-DB5-obe.outbound.protection.outlook.com\n\t(mail-db5eur01on070c.outbound.protection.outlook.com\n\t[IPv6:2a01:111:f400:fe02::70c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8AAD61C35\n\tfor ;\n\tTue, 26 Mar 2024 04:13:08 +0100 (CET)","from DB9PR04MB9284.eurprd04.prod.outlook.com (2603:10a6:10:36c::8)\n\tby AM9PR04MB8323.eurprd04.prod.outlook.com (2603:10a6:20b:3e5::21)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7409.31;\n\tTue, 26 Mar 2024 03:13:07 +0000","from DB9PR04MB9284.eurprd04.prod.outlook.com\n\t([fe80::f658:f649:f731:17cb]) by\n\tDB9PR04MB9284.eurprd04.prod.outlook.com\n\t([fe80::f658:f649:f731:17cb%7]) with mapi id 15.20.7409.031;\n\tTue, 26 Mar 2024 03:13:05 +0000"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=nxp.com header.i=@nxp.com header.b=\"CWGZovNT\";\n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;\n\tb=RMyTGEQnkC0+pl6F8wsFbxRux3nEnR/w0rHFuV1jb93BL4yazFktTjF1wbgH4ZjO7mFUtkEBGpwyGq7ctgXqkMqu6Q4cLEj83MnKTCtwa86I0EfNUly+r5hjWIB57oR7xLxSYMqa8oSkYaFd4OhSyfl2kICUrPLUlN3CC4EBU3g4aCK00qebttrWP/rrz1oLYNG78uxrRsTUjG1Tr02pxtU8/OL9ENLpeDmvsatrD6mrVFSsKSrwYxkYbEpM2fLp1EPOKmKebBmRzE42BfDTMZOneVxoZgQWWUmfn73ucN/WFDGBeIiGBiJrYIkvsxu5Gn95HbRBoAIXVAOGm1YSYA==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector9901;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=bVCIYMzItFeaQoU/qDJoBAp3qgFWfiNtdQhyqggSZQg=;\n\tb=jRZ6VufKNy9q1QOp4zEH5taTAf4vOwFvWuJSWt0slr32j9mZyZz8iQAD20bAyoxQ0dSd2utdLwyZsGd0YnYGNRrUp0JFnPPUi0OgGXdQrUOMu/ZeGHT7iIoZHS+ftxZyrO0y041b1tB/oY11T8GTTP7dQxjzum/i+KOlQtRXe9p06XP+IQPNjBJ2SyQzc7DO5NNr+gnWMoyu80VP/e+/Bxv6mX5k/XKiIr925N1WAcRtysAepPfxGyoxuHQ8e2nbKnTWzZzOviXerkGLIk9LHjRWi8cGmI5DAQ282tjABaRZ2tuJMA+5OFqq8gOCg1JA7mcpWtvzB0Mqh7tE6rHdXg==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com;\n\tdkim=pass header.d=nxp.com; arc=none","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=bVCIYMzItFeaQoU/qDJoBAp3qgFWfiNtdQhyqggSZQg=;\n\tb=CWGZovNTB+3ixQeQiZV1cWdn6ptv7Fqo6M5v9LDVU8OL8MYwpI+8DZ8IxUVps7cDlx7V3HgNos38hDm9rm3wz1ekWyZBmAMI+aRz1FHNmsJoSlp6+I1vL/NX+uSZA962weFM35hUh4oe5lIvZF0g6PIBiFkEnwUadD2m7PEflfY=","From":"Hui Fang ","To":"Anle Pan , Umang Jain ,\n\tJacopo Mondi ","CC":"\"libcamera-devel@lists.libcamera.org\"\n\t, Hui Fang ","Subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Topic":"[EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Index":"AQHabhdYilzqS/KnJ0GZgDK8wRbHfLEo0YUAgAARIACAGsq2gIAEhsGAgAAbIYCAASrKjg==","Date":"Tue, 26 Mar 2024 03:13:05 +0000","Message-ID":"","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\n\t\n\t\n\t<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>\n\t","In-Reply-To":"","Accept-Language":"en-US","Content-Language":"en-US","X-Mentions":"anle.pan@nxp.com","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","msip_labels":"","x-ms-publictraffictype":"Email","x-ms-traffictypediagnostic":"DB9PR04MB9284:EE_|AM9PR04MB8323:EE_","x-ms-exchange-senderadcheck":"1","x-ms-exchange-antispam-relay":"0","x-microsoft-antispam":"BCL:0;","x-microsoft-antispam-message-info":"/dtvwvAbr/FcJHr5D1XOTG7CZ+LmJfV4NAcZBUv6BDkC7VOLTif6f+jviJvtyg5k/BAD0gtT/Z8pW5YPfqFwdRrc0M6B9Q7y+GRcUrkc1wHLzJnCxMwDwr9zX0ZvZv78/DNVq1heMEl7s4EpNGluXAwQmA/24J/WdXvWNkhyDFQFtibC6JjGAHWpbLs7EwR42YE3gqbalrSAc/yCo8sfld/7kPBefNshgCg8yfPMe2GyPjPa8U5bs+Hg8Z7KGbU3XT1+cFfUD3kuv0LlrxcaUjJp3stx1n4EyIFdbywegXDaoEor1nRRcWvblfn+Wy1Dnn25l8lZEgp55v48sa6tS/CHILtaJBjDin6CPfb9s+2NDo9yl6sPWEaIQi9DXBhtyiSFhoePDcK3sBwVAM0jdF8H7LOsZ9NLZYxfh45dxdQ/6XoO0PSiuio1SfwXtgiaXBC6tNsPYToT5yByoP0xcnxuREyQX4i63RyLnI2fazA9oD7fXPaYc5wkqODpKwuIsFrCyN8wdVIWcCChMx1aVBQaPAd5gkaodDPfZ8N5VLRPYtRHIAVJy93s4PtQJdreHgeRRFw5deoUmoV3GH77djZ9catG6U450pvVC8oXyI2whhQA0cOWhcwaAGua8+VLITUGxSfoOhmEefHd2iEV0hb7LfBZ/Z3bkz8z7NVcT7w=","x-forefront-antispam-report":"CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;\n\tIPV:NLI; SFV:NSPM; H:DB9PR04MB9284.eurprd04.prod.outlook.com; PTR:;\n\tCAT:NONE; \n\tSFS:(13230031)(376005)(366007)(1800799015); DIR:OUT; SFP:1102; ","x-ms-exchange-antispam-messagedata-chunkcount":"1","x-ms-exchange-antispam-messagedata-0":"=?utf-8?q?sdjznndAax2HHWnNmauPbz/vk?=\n\t=?utf-8?q?AKZsQH/mzAZzY9d8E8AQs5Jao2ZCyEZDg0lO2kXBa7ruuB627Hpg5FgG?=\n\t=?utf-8?q?vE+eVc4ox0BZtnZ3Hnuf+fs8G0lzVj14MW9Rt6OrP1oEbIfW2eXvG1WP?=\n\t=?utf-8?q?oJFdV2r26Qm+Zk80SKWStphLXCGQkcuF16xJ0wYyO1XabjO6rBWF3/5u?=\n\t=?utf-8?q?lNcHm4gq4oGZWXDEGNYh6bz8pORBXe3snp9PtTqZX0l3OVA2GSbjdRWN?=\n\t=?utf-8?q?OivqI8v7NuZKUTpi/OfFHXgONCZnDzU6+m2FG/x7BKk5wUOamUJZIX3F?=\n\t=?utf-8?q?zl9allONOBhsFcsVwNlZEDtYnsy3mOAJHb8AOOUuUdnY9bzmo2ZdXjKR?=\n\t=?utf-8?q?VGJuJgwycxsxakloZ+Jr+sXQoo6lByeLjeWzYdsryHw2aBavC5G5/rPU?=\n\t=?utf-8?q?HuSz8YoItHftL04+KWEUf13PuQw5aqR0H7JGCT935+jd9/AMsNgpTwe6?=\n\t=?utf-8?q?JE1ANt5qvFQkhuLRYgV3JIlgSrZmS0Zhzyeoxup0y9mKXsyGSeUfwaaS?=\n\t=?utf-8?q?o4B7rUQyhQlhvfboiT/oB5B9XSgLgL+uIEjl/fuQPvWmSPQEcRFrlNtt?=\n\t=?utf-8?q?u6bvQqOrU1oZtWFMWyHanVakjZaJKqFjX1E2MzChTWAKGL8uuLhgZo7H?=\n\t=?utf-8?q?lbfer49tVXVfKwC1TNGaxcfO74mz8NRLZ6H9fB/ekUdneOWWjbCK8R+7?=\n\t=?utf-8?q?oLvBaSaet75r8+bjDlU8rgCP1z1BUqtjYdOiPWcgwTzLNvUQRVJF+Gie?=\n\t=?utf-8?q?s+5jIFwck0WErJ5+Lfwz2dstm1C+JZKOl86zd0kpYDjbVqgo2v9LKXtt?=\n\t=?utf-8?q?Y5ehfe56IdMi/JsT5tXe5YjM31LFTg27RB8rQppK9fEAAdF5onLQjrSA?=\n\t=?utf-8?q?rPWm/OS5p1v9Hw+QcQ8BSD16nfEIogoPDn61YbeWtFGX6h1sHIE93Sxf?=\n\t=?utf-8?q?8euhEoMf6Q5bq+lTjOPTf9ch4k4ghCncHA1jrjhdRCThX4ZZewt+Ua9r?=\n\t=?utf-8?q?Uj3Y39qzu6qrfM3mjOe3aCT6lo9U7RLz5YA+7qRDRGifjs1a9Taa2rpY?=\n\t=?utf-8?q?RSx0lTXgfH6LFZr9zsOKMzsLamQV2ir1u8OMjD3iXubh2GWjJnASa5Z0?=\n\t=?utf-8?q?YjZEB+aPWAWUPz7r9gCxrQZdikIZ9Mzr2O4Rnx4p+zh6ZFsnawkLAr2P?=\n\t=?utf-8?q?KmRmRlspgTkFJ+CQ7NecT6vMv+vZKLGtEuA7AMJJYirmwE1c+E5Xvlcu?=\n\t=?utf-8?q?PpHNk1pWze6+ZRohhy8dUN3P27DqXd4GIxn/lpNN4ThjCXt0ifOdz+i8?=\n\t=?utf-8?q?kQxKG6NH5KryUSEJyauaeIM/8kgJZn28HD1y8b+6odVd/4JjCFitRAk1?=\n\t=?utf-8?q?O084DC/rRIIokD3XMSBlgVhi8nY/P5JQc3EzbS6PvcuYz78z3xpBu4Un?=\n\t=?utf-8?q?gWqY9FGwK/JnrXkfD830nSG9EGCTeD060nFYPnu/qpzSjv4GQ/mc5Azj?=\n\t=?utf-8?q?+F94rNJ0efSuG4gs8eGOBlzkcWIPpKxVHe2qz9RQZqTUN7g7onEbeDIw?=\n\t=?utf-8?q?oOGl0rSMRuPg0+HSs+E4HYYjHPvkba5bkTXAplA+Mk/ReOSzYL5McOc3?=\n\t=?utf-8?q?ZAJXpLmZVxOv29Z4O+LRJuFq1vMAN8idSAyJjGIPNY=3D?=","Content-Type":"multipart/alternative;\n\tboundary=\"_000_DB9PR04MB92841C0A9357C9610DA2358F87352DB9PR04MB9284eurp_\"","MIME-Version":"1.0","X-OriginatorOrg":"nxp.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-AuthSource":"DB9PR04MB9284.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"761e71dd-4ca9-49ac-fd89-08dc4d42a784","X-MS-Exchange-CrossTenant-originalarrivaltime":"26 Mar 2024 03:13:05.3222\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635","X-MS-Exchange-CrossTenant-mailboxtype":"HOSTED","X-MS-Exchange-CrossTenant-userprincipalname":"oamNNL+fsyp5LDbzxOQfL3YvECxzc/XRmkAJn5ZhCiqBTmNq5tVwTMLnubFIGxfXmcBFOq7ktapvvUmnz4ZYoA==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"AM9PR04MB8323","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":29058,"web_url":"https://patchwork.libcamera.org/comment/29058/","msgid":"","date":"2024-03-26T08:08:48","subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi,\n\nOn Tue, Mar 26, 2024 at 03:13:05AM +0000, Hui Fang wrote:\n> @Anle Pan No need \"git send-email\" again, will new another topic.\n>\n> Just reply the refined patch.\n\nnot a problem for this patch, but as a general rule, do not reply to a\npatch with a new version of the same patch, just send a new one. It's\neasier to track and I think it is also required for patchwork to\nidentify it correctly.\n\n>\n> Author: Anle Pan \n> Date: Fri Mar 1 17:32:54 2024 +0000\n>\n> android: camera_device: Fix the stuck issue in sendCaptureResults.\n\nNo '.' at the end of the commit message.\n\nThe actual subject should be something like\n\nandroid: camera_device: Always clear descriptors_ in stop()\n\n>\n> When flush() is being called and then a new stream configuration is set,\n> descriptor_ may has a chance to be not cleared, which was skipped due to\n\ndescriptors_\n\n> the Stopped State. This will cause a stuck in sendCaptureResults.\n\ndue to the Camera being in Stopped state.\n>\n> To fix the issue, ensure the descriptors_ is always empty when the camera\n> State is Stopped.\n>\n> Signed-off-by: Anle Pan \n\nReviewed-by: Jacopo Mondi \nTested-by: Jacopo Mondi \n\nWith the few fixes above, I'll merge.\n\nAlso, as you have sent this new version, I'll add\n\nSigned-off-by: Fang Hui \n\nThanks\n j\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index d2679a97..7fda6ce6 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -433,8 +433,11 @@ void CameraDevice::flush()\n> void CameraDevice::stop()\n> {\n> MutexLocker stateLock(stateMutex_);\n> - if (state_ == State::Stopped)\n> + if (state_ == State::Stopped) {\n> + MutexLocker descriptorsLock(descriptorsMutex_);\n> + descriptors_ = {};\n> return;\n> + }\n>\n> camera_->stop();\n> ________________________________\n> From: Anle Pan \n> Sent: Monday, March 25, 2024 5:20 PM\n> To: Umang Jain ; Jacopo Mondi \n> Cc: libcamera-devel@lists.libcamera.org ; Hui Fang \n> Subject: RE: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n>\n> Hi Umang, Jacopo.\n>\n> Yes, when issue happened, stuck in sendCaptureResults. The reproduce chance on my side was also not high for the cts (1/10)\n>\n> void CameraDevice::sendCaptureResults()\n> {\n> while (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n>\n> And Jacopo, your description is accurate, the descriptors_ has a chance to be not cleared due to the State was already 'Stopped' after flush() .\n> will ask hui to help update the commit message since the \"git send\" configuration has issue on my side.\n>\n> Best Regards\n> Anle Pan\n>\n> -----Original Message-----\n> From: Umang Jain \n> Sent: 2024年3月25日 15:44\n> To: Jacopo Mondi ; Anle Pan \n> Cc: libcamera-devel@lists.libcamera.org; Hui Fang \n> Subject: Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n>\n> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button\n>\n>\n> Hi Jacopo,\n>\n> Thanks for testing,\n>\n> On 22/03/24 4:06 pm, Jacopo Mondi wrote:\n> > Hello\n> >\n> > On Tue, Mar 05, 2024 at 09:28:00AM +0000, Anle Pan wrote:\n> >> Hi Umang Jain,\n> >>\n> >> this issue was random(around 1/10), and can be reproduced by single run below cts test:\n> >> ./cts-tradefed run commandAndExit cts --abi arm64-v8a -m\n> >> CtsMediaPlayerTestCases -t\n> >> android.media.player.cts.MediaPlayerTest#testRecordedVideoPlayback90\n> >>\n> >> • 3 times configuring streams in this Test:\n> >> 1) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n> >> 2) 3 streams: (0) 320x240-NV12 (1) 320x240-NV12 (2) 1280x720-YUYV: will capture on stream0 and stream2 at the same time\n> >> 3) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n> >>\n> >> • when single run the test several times and met fail. When digging the log, can find that the current failed is caused by the abnormal complete of the previous PASS test in step3, which is the situation described in my patch.\n> >>\n> > The only code path I see that could lead to descriptor_ not being\n> > cleared is flush() being called and then stop() called just after\n> > possibily as the result of a new configuration\n> >\n> > void CameraDevice::flush()\n> > {\n> > {\n> > MutexLocker stateLock(stateMutex_);\n> > if (state_ != State::Running)\n> > return;\n> >\n> > state_ = State::Flushing;\n> > }\n> >\n> > camera_->stop();\n> >\n> > MutexLocker stateLock(stateMutex_);\n> > state_ = State::Stopped;\n> > }\n> >\n> > void CameraDevice::stop()\n> > {\n> > MutexLocker stateLock(stateMutex_);\n> > if (state_ == State::Stopped)\n> > return;\n> >\n> > camera_->stop();\n> >\n> > {\n> > MutexLocker descriptorsLock(descriptorsMutex_);\n> > descriptors_ = {};\n> > }\n> >\n> > streams_.clear();\n> >\n> > state_ = State::Stopped;\n> > }\n> >\n> > int CameraDevice::configureStreams(camera3_stream_configuration_t\n> > *stream_list) {\n> > /* Before any configuration attempt, stop the camera. */\n> > stop();\n> >\n> > ...\n> >\n> > }\n> >\n> > Looking at the flush() implementation right now I wonder why we stop\n> > the camera without the stateMutex_ held (the only reason I see is\n> > because it can be concurrent with processCaptureRequest() which keeps\n> > the mutex held), but I certainly don't want to touch that code right\n> > now without a good reason.\n> >\n> > All in all, I think there's indeed a path that could lead to the\n> > descriptor not being cleaned up, and we can't do it in flush() because\n> > a concurrent call to processCaptureRequest() might be using the\n> > descriptors to complete the in-flight requests with error state.\n> >\n> > Anle, does this match your understanding ? If so, this is what should\n> > be recorded in the commit message (the cause of an issue, not the\n> > symptoms)\n> >\n> >\n> >> Best Regards\n> >> Anle Pan\n> >>\n> >> -----Original Message-----\n> >> From: Umang Jain \n> >> Sent: 2024年3月5日 16:27\n> >> To: Anle Pan ; libcamera-devel@lists.libcamera.org\n> >> Cc: Hui Fang \n> >> Subject: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n> >> issue when stopping capture\n> >>\n> >> Caution: This is an external email. Please take care when clicking\n> >> links or opening attachments. When in doubt, report the message using\n> >> the 'Report this email' button\n> >>\n> >>\n> >> Hi Anle Pan, Hui Fang\n> >>\n> >> On 04/03/24 3:05 pm, Anle Pan wrote:\n> >>> The issue occurs when stopping capture soon after starting capture.\n> >> Can you describe the issue/use case in more detail? It's really hard to infer from one line.\n> >>> In this case, no frame get from the device, the related capture\n> >>> request has been pushed to the queue descriptors_, but the\n> >>> queuedRequests_ was still empty due to no requests will be queue to\n> >>> the device since the stream will be stopped soon,\n> >> I don't get it how is this possible? Since processCaptureRequest() will push the request in the descriptors_ queue along with queuing the request to the camera.\n> >>\n> >> How will the call path be interleaved /before/ processCaptureRequest() returns? Possibly we need more details on what you are trying to do, before reviewing the code here. Is this related to CTS ?\n> >>> so there will be no camera->requestComplete called later, then the\n> >>> descriptors_ can not pop normally, this will cause the pending if we\n> >>> want to start capture next time.\n> >>>\n> >>> To fix the issue, ensure the descriptors_ is empty after the camera\n> >>> device is stopped.\n> > Do you know precisely why a non-empty descriptors_ stalls the camera ?\n> > Does it get stuck on\n> >\n> > void CameraDevice::sendCaptureResults()\n> > {\n> > while (!descriptors_.empty() &&\n> > !descriptors_.front()->isPending()) {\n> >\n> > ?\n>\n> We should to formalise this as an proper issue, after getting more information.\n> >>> Signed-off-by: Anle Pan \n> > Tested with CTS, no regressions detected\n> >\n> > Reviewed-by: Jacopo Mondi \n> > Tested-by: Jacopo Mondi \n> >\n> >>> ---\n> >>> src/android/camera_device.cpp | 5 ++++-\n> >>> 1 file changed, 4 insertions(+), 1 deletion(-)\n> >>>\n> >>> diff --git a/src/android/camera_device.cpp\n> >>> b/src/android/camera_device.cpp index 25cedd44..d452992d 100644\n> >>> --- a/src/android/camera_device.cpp\n> >>> +++ b/src/android/camera_device.cpp\n> >>> @@ -433,8 +433,11 @@ void CameraDevice::flush()\n> >>> void CameraDevice::stop()\n> >>> {\n> >>> MutexLocker stateLock(stateMutex_);\n> >>> - if (state_ == State::Stopped)\n> >>> + if (state_ == State::Stopped) {\n> >>> + MutexLocker descriptorsLock(descriptorsMutex_);\n> >>> + descriptors_ = {};\n> >>> return;\n> >>> + }\n>\n> I am not comfortable with this patch. Consider my points below\n>\n> - First we don't have a exact description of the issue it is fixing\n>\n> - Second, if the camera state is ::Stopped already. Clears descriptors_ doesn't look a good idea to me. The function which has set the camera state::Stopped already, should be ideally be emptying the descriptors_...\n>\n> - If we are accepting this, why not just let the Camera::stop() fall through and remove\n>\n> if (state_ == State::Stopped)\n> return;\n>\n> as well. Then, CameraDevice::stop() will behave same as Camera::stop() - a no-op when called even when State::Stopped and responsible to clean up queues(and stuff) for the HAL layer.\n>\n> I should ideally back up my thoughts with some testing, but I don't have any kind of CTS testing available to me right now :(\n> >>> camera_->stop();\n> >>>\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 A368FC0DA4\n\tfor ;\n\tTue, 26 Mar 2024 08:08:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1DBD76330D;\n\tTue, 26 Mar 2024 09:08:53 +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 C7A236303D\n\tfor ;\n\tTue, 26 Mar 2024 09:08:51 +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 22CEE63B;\n\tTue, 26 Mar 2024 09:08:20 +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=\"HAy9xyYj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711440500;\n\tbh=PKTP3ONfgH2cwg+KeJeBEcKRDnu4rSvykFPhnlvfLRg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HAy9xyYjQxOWGFg8lGIZnbIio8yOc/PEeN/59mL1XZISavyyj01tGwRaYacfMu0PG\n\t+qeIdJT+8P/1UKl57CpnpADud3j9FKNPZG2cOaRr6n9qqNSmBuziqm1sMQDHE6ddV0\n\t6kEaYDZJVuTKy3dTmZXpJH2jJNdoRBA/3JwkXc+g=","Date":"Tue, 26 Mar 2024 09:08:48 +0100","From":"Jacopo Mondi ","To":"Hui Fang ","Cc":"Anle Pan , Umang Jain , \n\tJacopo Mondi , \n\t\"libcamera-devel@lists.libcamera.org\"\n\t","Subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Message-ID":"","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\n\t\n\t\n\t<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>\n\t\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":29059,"web_url":"https://patchwork.libcamera.org/comment/29059/","msgid":"","date":"2024-03-26T08:20:31","subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":186,"url":"https://patchwork.libcamera.org/api/people/186/","name":"Fang Hui","email":"hui.fang@nxp.com"},"content":"Hi, Jacopo\n For refined patch, always use \"git send-email\", Even the subject changed?\n\nBRs,\nFang Hui","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 E8A8CBD160\n\tfor ;\n\tTue, 26 Mar 2024 08:20:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF9866330D;\n\tTue, 26 Mar 2024 09:20:35 +0100 (CET)","from EUR03-AM7-obe.outbound.protection.outlook.com\n\t(mail-am7eur03on20701.outbound.protection.outlook.com\n\t[IPv6:2a01:111:f403:260e::701])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 35E0463037\n\tfor ;\n\tTue, 26 Mar 2024 09:20:34 +0100 (CET)","from DB9PR04MB9284.eurprd04.prod.outlook.com (2603:10a6:10:36c::8)\n\tby DUZPR04MB9821.eurprd04.prod.outlook.com (2603:10a6:10:4b1::11)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7409.31;\n\tTue, 26 Mar 2024 08:20:31 +0000","from DB9PR04MB9284.eurprd04.prod.outlook.com\n\t([fe80::f658:f649:f731:17cb]) by\n\tDB9PR04MB9284.eurprd04.prod.outlook.com\n\t([fe80::f658:f649:f731:17cb%7]) with mapi id 15.20.7409.031;\n\tTue, 26 Mar 2024 08:20:31 +0000"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=nxp.com header.i=@nxp.com header.b=\"naG7UMJp\";\n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;\n\tb=aIY7qBkAVoIYhUaQtmMsWjJjEwzjcuB2Cnf9RiQ8WiWjnzcvB1yeLVT1tFkghCwcXtbhUpqFpz9FiXMsUNsD69k9N0WaK3TuQhWJoLUHXBKOPLdmYaZAxJf9PTAprzdgy6+8rHlMAvmciZ8aNfBb5lyZrCc6nQmgJ0Gi++qtxGoc7hgg71NsiZ49O6X3JF2cvgmmG6gASMnUPtk9iABLkwJod3D2ayafnAbmnpRbAqIQyVepB2gsm0OJs1q1yCIEaz26Fj+/hzi8OdmDMdMGHzbBM/d2Q/RecDaDd2v1u0AkWO/IHtS7ICIcFV0zPAGOPVY3jEoZRmuVCC+apUncag==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector9901;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=a2mV97nGbUNh8RJCq5tqAc9ccxd45oPTX7Ls595wiFY=;\n\tb=j1bRbBjd10LVJiSFIkvftC61ooqVicOsuk3w7nvWPL2fnCcxpjxIDnZImnkH7sPfVuZ3rG2LTUtfym3oBxXenYJub6cur6fErlTok7AAf02tqUsrHkAS78VIyVQLF1msHuy9V22nHPtfXZlhdg5dK6UZHKXstwdrEaa8xTh3cnA8m8jgXyZcI7Y3eLSUf4szmFOlnJ0rECxuSZoTuKZQAabL2A8k0yJKpOJzjLqsSxLoJXI0DAuqSQIKO385zSSOa4LhSIGdqQbWVD/lxdu5ZztC3Wz5hmbvgOToVJEnO2WsB8LJ9+iNsRhaUfS9AbUMnfZ+I+5keh56Lk+YY9DEjQ==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com;\n\tdkim=pass header.d=nxp.com; arc=none","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=a2mV97nGbUNh8RJCq5tqAc9ccxd45oPTX7Ls595wiFY=;\n\tb=naG7UMJpRCfAxo68Sst16FPovhaLhQCJBUj82JzQr+PibymuJRkxbq/63r9ApytfTCn7cyKv6Dfz8Yhf8Z++MTm4dqMkjW91P0MIPLU8oJRmpcGwkGgaFnGhWIDXrKO3aNuVnW79R0lUapGEM6Ls1qiqs6u2M37r5mZWwbGHQL8=","From":"Hui Fang ","To":"Jacopo Mondi ","CC":"Anle Pan , Umang Jain ,\n\t\"libcamera-devel@lists.libcamera.org\"\n\t","Subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Topic":"[EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Index":"AQHabhdYilzqS/KnJ0GZgDK8wRbHfLEo0YUAgAARIACAGsq2gIAEhsGAgAAbIYCAASrKjoAAU3MAgAACw6w=","Date":"Tue, 26 Mar 2024 08:20:31 +0000","Message-ID":"","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\n\t\n\t\n\t<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","msip_labels":"","x-ms-publictraffictype":"Email","x-ms-traffictypediagnostic":"DB9PR04MB9284:EE_|DUZPR04MB9821:EE_","x-ms-exchange-senderadcheck":"1","x-ms-exchange-antispam-relay":"0","x-microsoft-antispam":"BCL:0;","x-microsoft-antispam-message-info":"RmUmbXdFm195+YSyGp//fzywuEhyeJG+bV2t2bFT6HvDxtvvrieShpvf8aMD26fJ5yib25Z1bPuw+JeXI1bhEy8IRt6W2Q+fuWC8JDo9hjZsCWb/hACMBQgvh1wX1jUR0yIcU0nh3kShGJIwW+fYOLePElgB/h5paWWVZuXza1rSEKA0qDyAcNCbdpgFJfNwPcMoWk6VoAGSDTgFeyIh/KZTJzjc2gt5CWzQNqS4n5aDX9XW9kaBYzqDffvwKPMocvvz+G9HeN9YMTekzP/8T558WLV93mQDUvEJacwuQImLefqAa+1XXNHSQeyZAiBxKY4RaA4Z5Jfgv+M/g4q+RkiavH1RbLzPBEdNbkxO3NLF4x6qGKCiHzqujJjxJ3eWBxxM97edqV9tG4lysPwm3kZqBrzxoN8/1WJzJUnr48V0wjSqpvzruseMMP6IcsBCmb553Y85uKUXnl5UL+npjDNm+zqUrRp8IBDlf0qQweFLf3q2UaRv0QpxuUDawmtv1hVFlGBKeaWMp2gKJU7uWYPVMKKyKAUcUhAGdOqYognHVd1M/SI9geC+0UcmwPi9j72ELZrCDHK48DvopequFgMAaiaI0LUDzz6OgAAmTg2J7F3H9rKoFAUPV9V4pomh5/pcE1OhQXRiwVUT2DaKL1wAwuihEqUglXltPiIPqvY=","x-forefront-antispam-report":"CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;\n\tIPV:NLI; SFV:NSPM; H:DB9PR04MB9284.eurprd04.prod.outlook.com; PTR:;\n\tCAT:NONE; \n\tSFS:(13230031)(366007)(376005)(1800799015); DIR:OUT; SFP:1102; ","x-ms-exchange-antispam-messagedata-chunkcount":"1","x-ms-exchange-antispam-messagedata-0":"=?utf-8?q?HT/UyGlfOHe3HdzzlYVIs/Pcg?=\n\t=?utf-8?q?7IF694FnWCCrR2LkPXZngpmzcbTE+ycRiA2G3ZPgF5n1uGTumFtWo9zy?=\n\t=?utf-8?q?4XJEV0Kaxii1hyqCzCYkMX/9OqjqcGNP3MeGBGmPVkAZkJKIsw3s7Hfm?=\n\t=?utf-8?q?cYvfped+MKXlDKETiknPUelhfIfVi73xRwu8Ol2R5Vpt1FftW+RfkYvf?=\n\t=?utf-8?q?3eHdHGo3gnjq0zDWzloLkGvAqLH51wJh7+l2f6LTDd7sxn/ZHHyVUE1y?=\n\t=?utf-8?q?VdJmWcLWoZI/iWbnvL1zAD9IUwtcjMy3JxA/23B9c83CANyBBlOSly9D?=\n\t=?utf-8?q?OmHLJbqs14txH/4hDr1cYlNgDiUdkReLwggC6mbWbhcEsuvxB8UZEmBj?=\n\t=?utf-8?q?6kaSFihD9rj1gLQO2Ppva3Iz4ozLlQAzi5beJlgiNqsgTMeA0GyD/10O?=\n\t=?utf-8?q?5mCgZSwTNPoMcilLsXR3s+TxdtlbNK7fbhBXOyIT3q6hqnz1/xYbVGgF?=\n\t=?utf-8?q?Jhk+UwX9xFo/Tl3Gju6ZDBCA4cf0oveW4fKAh4eUIQUJ6pjQZe49NzGS?=\n\t=?utf-8?q?9MkwcFAz+Vwgz/0O6IlzqpN07iVc8hd3cVtYLH1AWo2l4BE6Jg849fzR?=\n\t=?utf-8?q?YMBiw9PXpMm0SAdiu1veoynLyh/tqjOvCePC7a8J3gzCX9yCIqYokicm?=\n\t=?utf-8?q?91AGmAUMDw4mgHzhPxtOxxxpqN2hrdGGbD1aJeb/8b/Q/KnCnNeV3N97?=\n\t=?utf-8?q?G48iW+IV/gz59KfSACZFjqrbCyVMDmV7LIpW3zEnaqbhafpnlxXB1YA9?=\n\t=?utf-8?q?SE4w1M4J0ljVcyHEPHcUk7ep1Pv6WIacvMVWvpIfHlDPbF/2C+hsJcjr?=\n\t=?utf-8?q?rinLH+8ietFujaVgNA2fJ3E4VhexfFRtcIb/bUI8xvmdJhghZkS4aoe1?=\n\t=?utf-8?q?biLjzsz/uNmQAm6Ou9vjuzpZfVC00C3s2N9JldFJoy1EhUmpQQ8B6+zQ?=\n\t=?utf-8?q?63b1PmJC2zAWZxNjhj0w50BMYfRSQu477qOGGq5TVkrmxNCoM1R4XW98?=\n\t=?utf-8?q?SK1S0DWIxzFD32jyecveTz6tvB4KapuoXSY15Kib9Tc/FCylpHwIjJHF?=\n\t=?utf-8?q?Q3f7FW9/+tzR6GX//cLSsOhxWr/8tTjxqPyxFAjw6/PK2mWgazywZ1Hg?=\n\t=?utf-8?q?5D27ya+C1HwxZkoiMY61Tcuc8G/son0Eo8LKWuJlrpT5c6EW3xAbPo7U?=\n\t=?utf-8?q?cx9NRaA9J8MfyU+QZ4Wb3cDId4Yi3uh1TB0+QdOlv8JCvBRi6IOlvbc+?=\n\t=?utf-8?q?Lt2n0QU/qjEgTuqRLhYQrd9HjDaCsPg9ImJ4x4os/q3k3K4u97LibCRW?=\n\t=?utf-8?q?nCAnKDIfu1mlrkx+jn6tflpcxT+GvBFacl1ZW4x6qD36lS/6s0hOlgE8?=\n\t=?utf-8?q?rrj7V6OndfMehEWMXZwyZ8Th8cuK5xARMn3GnrI9ynNGq2gJ5JAQbPjr?=\n\t=?utf-8?q?7vGqtA7med39PP41YK4k1zdn4HbC0vTC/1RJSHtb0U46/yW9ES639fXu?=\n\t=?utf-8?q?lfhrz7rGT33uAfzZh/YEzdH5sFP6HbdnE/RDFwNvbST2NaJ4RY1VcRaA?=\n\t=?utf-8?q?qRvr/CyWSXLADjGXw895+PAtlaF6MnhDMBO/7wD4eMbhELCKHEUGoikZ?=\n\t=?utf-8?q?JkkMfPirXTEeiin2YpAPH0ddVP3NQASnQd7w+ix5TY=3D?=","Content-Type":"multipart/alternative;\n\tboundary=\"_000_DB9PR04MB92845BDEFF3CCBB00795081B87352DB9PR04MB9284eurp_\"","MIME-Version":"1.0","X-OriginatorOrg":"nxp.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-AuthSource":"DB9PR04MB9284.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"1a704536-4963-4f0d-4225-08dc4d6d9a43","X-MS-Exchange-CrossTenant-originalarrivaltime":"26 Mar 2024 08:20:31.4342\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635","X-MS-Exchange-CrossTenant-mailboxtype":"HOSTED","X-MS-Exchange-CrossTenant-userprincipalname":"25J1kL5anrhuOy2KyMi0uUF4ipE/vK6GFstU0HeyJrI08vAloJKBue8UJxa0T6GTT9uFOY0rAYjXcUKUw/EqBg==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"DUZPR04MB9821","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":29060,"web_url":"https://patchwork.libcamera.org/comment/29060/","msgid":"","date":"2024-03-26T08:22:50","subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"No worries, no need to re-send. Small things can be fixed (if you're\nfine with what I proposed) when I'll apply the patch.\n\nOn Tue, Mar 26, 2024 at 08:20:31AM +0000, Hui Fang wrote:\n> Hi, Jacopo\n> For refined patch, always use \"git send-email\", Even the subject changed?\n>\n> BRs,\n> Fang Hui\n> ________________________________\n> From: Jacopo Mondi \n> Sent: Tuesday, March 26, 2024 4:08 PM\n> To: Hui Fang \n> Cc: Anle Pan ; Umang Jain ; Jacopo Mondi ; libcamera-devel@lists.libcamera.org \n> Subject: Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n>\n> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button\n>\n>\n> Hi,\n>\n> On Tue, Mar 26, 2024 at 03:13:05AM +0000, Hui Fang wrote:\n> > @Anle Pan No need \"git send-email\" again, will new another topic.\n> >\n> > Just reply the refined patch.\n>\n> not a problem for this patch, but as a general rule, do not reply to a\n> patch with a new version of the same patch, just send a new one. It's\n> easier to track and I think it is also required for patchwork to\n> identify it correctly.\n>\n> >\n> > Author: Anle Pan \n> > Date: Fri Mar 1 17:32:54 2024 +0000\n> >\n> > android: camera_device: Fix the stuck issue in sendCaptureResults.\n>\n> No '.' at the end of the commit message.\n>\n> The actual subject should be something like\n>\n> android: camera_device: Always clear descriptors_ in stop()\n>\n> >\n> > When flush() is being called and then a new stream configuration is set,\n> > descriptor_ may has a chance to be not cleared, which was skipped due to\n>\n> descriptors_\n>\n> > the Stopped State. This will cause a stuck in sendCaptureResults.\n>\n> due to the Camera being in Stopped state.\n> >\n> > To fix the issue, ensure the descriptors_ is always empty when the camera\n> > State is Stopped.\n> >\n> > Signed-off-by: Anle Pan \n>\n> Reviewed-by: Jacopo Mondi \n> Tested-by: Jacopo Mondi \n>\n> With the few fixes above, I'll merge.\n>\n> Also, as you have sent this new version, I'll add\n>\n> Signed-off-by: Fang Hui \n>\n> Thanks\n> j\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index d2679a97..7fda6ce6 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -433,8 +433,11 @@ void CameraDevice::flush()\n> > void CameraDevice::stop()\n> > {\n> > MutexLocker stateLock(stateMutex_);\n> > - if (state_ == State::Stopped)\n> > + if (state_ == State::Stopped) {\n> > + MutexLocker descriptorsLock(descriptorsMutex_);\n> > + descriptors_ = {};\n> > return;\n> > + }\n> >\n> > camera_->stop();\n> > ________________________________\n> > From: Anle Pan \n> > Sent: Monday, March 25, 2024 5:20 PM\n> > To: Umang Jain ; Jacopo Mondi \n> > Cc: libcamera-devel@lists.libcamera.org ; Hui Fang \n> > Subject: RE: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n> >\n> > Hi Umang, Jacopo.\n> >\n> > Yes, when issue happened, stuck in sendCaptureResults. The reproduce chance on my side was also not high for the cts (1/10)\n> >\n> > void CameraDevice::sendCaptureResults()\n> > {\n> > while (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n> >\n> > And Jacopo, your description is accurate, the descriptors_ has a chance to be not cleared due to the State was already 'Stopped' after flush() .\n> > will ask hui to help update the commit message since the \"git send\" configuration has issue on my side.\n> >\n> > Best Regards\n> > Anle Pan\n> >\n> > -----Original Message-----\n> > From: Umang Jain \n> > Sent: 2024年3月25日 15:44\n> > To: Jacopo Mondi ; Anle Pan \n> > Cc: libcamera-devel@lists.libcamera.org; Hui Fang \n> > Subject: Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n> >\n> > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button\n> >\n> >\n> > Hi Jacopo,\n> >\n> > Thanks for testing,\n> >\n> > On 22/03/24 4:06 pm, Jacopo Mondi wrote:\n> > > Hello\n> > >\n> > > On Tue, Mar 05, 2024 at 09:28:00AM +0000, Anle Pan wrote:\n> > >> Hi Umang Jain,\n> > >>\n> > >> this issue was random(around 1/10), and can be reproduced by single run below cts test:\n> > >> ./cts-tradefed run commandAndExit cts --abi arm64-v8a -m\n> > >> CtsMediaPlayerTestCases -t\n> > >> android.media.player.cts.MediaPlayerTest#testRecordedVideoPlayback90\n> > >>\n> > >> • 3 times configuring streams in this Test:\n> > >> 1) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n> > >> 2) 3 streams: (0) 320x240-NV12 (1) 320x240-NV12 (2) 1280x720-YUYV: will capture on stream0 and stream2 at the same time\n> > >> 3) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n> > >>\n> > >> • when single run the test several times and met fail. When digging the log, can find that the current failed is caused by the abnormal complete of the previous PASS test in step3, which is the situation described in my patch.\n> > >>\n> > > The only code path I see that could lead to descriptor_ not being\n> > > cleared is flush() being called and then stop() called just after\n> > > possibily as the result of a new configuration\n> > >\n> > > void CameraDevice::flush()\n> > > {\n> > > {\n> > > MutexLocker stateLock(stateMutex_);\n> > > if (state_ != State::Running)\n> > > return;\n> > >\n> > > state_ = State::Flushing;\n> > > }\n> > >\n> > > camera_->stop();\n> > >\n> > > MutexLocker stateLock(stateMutex_);\n> > > state_ = State::Stopped;\n> > > }\n> > >\n> > > void CameraDevice::stop()\n> > > {\n> > > MutexLocker stateLock(stateMutex_);\n> > > if (state_ == State::Stopped)\n> > > return;\n> > >\n> > > camera_->stop();\n> > >\n> > > {\n> > > MutexLocker descriptorsLock(descriptorsMutex_);\n> > > descriptors_ = {};\n> > > }\n> > >\n> > > streams_.clear();\n> > >\n> > > state_ = State::Stopped;\n> > > }\n> > >\n> > > int CameraDevice::configureStreams(camera3_stream_configuration_t\n> > > *stream_list) {\n> > > /* Before any configuration attempt, stop the camera. */\n> > > stop();\n> > >\n> > > ...\n> > >\n> > > }\n> > >\n> > > Looking at the flush() implementation right now I wonder why we stop\n> > > the camera without the stateMutex_ held (the only reason I see is\n> > > because it can be concurrent with processCaptureRequest() which keeps\n> > > the mutex held), but I certainly don't want to touch that code right\n> > > now without a good reason.\n> > >\n> > > All in all, I think there's indeed a path that could lead to the\n> > > descriptor not being cleaned up, and we can't do it in flush() because\n> > > a concurrent call to processCaptureRequest() might be using the\n> > > descriptors to complete the in-flight requests with error state.\n> > >\n> > > Anle, does this match your understanding ? If so, this is what should\n> > > be recorded in the commit message (the cause of an issue, not the\n> > > symptoms)\n> > >\n> > >\n> > >> Best Regards\n> > >> Anle Pan\n> > >>\n> > >> -----Original Message-----\n> > >> From: Umang Jain \n> > >> Sent: 2024年3月5日 16:27\n> > >> To: Anle Pan ; libcamera-devel@lists.libcamera.org\n> > >> Cc: Hui Fang \n> > >> Subject: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n> > >> issue when stopping capture\n> > >>\n> > >> Caution: This is an external email. Please take care when clicking\n> > >> links or opening attachments. When in doubt, report the message using\n> > >> the 'Report this email' button\n> > >>\n> > >>\n> > >> Hi Anle Pan, Hui Fang\n> > >>\n> > >> On 04/03/24 3:05 pm, Anle Pan wrote:\n> > >>> The issue occurs when stopping capture soon after starting capture.\n> > >> Can you describe the issue/use case in more detail? It's really hard to infer from one line.\n> > >>> In this case, no frame get from the device, the related capture\n> > >>> request has been pushed to the queue descriptors_, but the\n> > >>> queuedRequests_ was still empty due to no requests will be queue to\n> > >>> the device since the stream will be stopped soon,\n> > >> I don't get it how is this possible? Since processCaptureRequest() will push the request in the descriptors_ queue along with queuing the request to the camera.\n> > >>\n> > >> How will the call path be interleaved /before/ processCaptureRequest() returns? Possibly we need more details on what you are trying to do, before reviewing the code here. Is this related to CTS ?\n> > >>> so there will be no camera->requestComplete called later, then the\n> > >>> descriptors_ can not pop normally, this will cause the pending if we\n> > >>> want to start capture next time.\n> > >>>\n> > >>> To fix the issue, ensure the descriptors_ is empty after the camera\n> > >>> device is stopped.\n> > > Do you know precisely why a non-empty descriptors_ stalls the camera ?\n> > > Does it get stuck on\n> > >\n> > > void CameraDevice::sendCaptureResults()\n> > > {\n> > > while (!descriptors_.empty() &&\n> > > !descriptors_.front()->isPending()) {\n> > >\n> > > ?\n> >\n> > We should to formalise this as an proper issue, after getting more information.\n> > >>> Signed-off-by: Anle Pan \n> > > Tested with CTS, no regressions detected\n> > >\n> > > Reviewed-by: Jacopo Mondi \n> > > Tested-by: Jacopo Mondi \n> > >\n> > >>> ---\n> > >>> src/android/camera_device.cpp | 5 ++++-\n> > >>> 1 file changed, 4 insertions(+), 1 deletion(-)\n> > >>>\n> > >>> diff --git a/src/android/camera_device.cpp\n> > >>> b/src/android/camera_device.cpp index 25cedd44..d452992d 100644\n> > >>> --- a/src/android/camera_device.cpp\n> > >>> +++ b/src/android/camera_device.cpp\n> > >>> @@ -433,8 +433,11 @@ void CameraDevice::flush()\n> > >>> void CameraDevice::stop()\n> > >>> {\n> > >>> MutexLocker stateLock(stateMutex_);\n> > >>> - if (state_ == State::Stopped)\n> > >>> + if (state_ == State::Stopped) {\n> > >>> + MutexLocker descriptorsLock(descriptorsMutex_);\n> > >>> + descriptors_ = {};\n> > >>> return;\n> > >>> + }\n> >\n> > I am not comfortable with this patch. Consider my points below\n> >\n> > - First we don't have a exact description of the issue it is fixing\n> >\n> > - Second, if the camera state is ::Stopped already. Clears descriptors_ doesn't look a good idea to me. The function which has set the camera state::Stopped already, should be ideally be emptying the descriptors_...\n> >\n> > - If we are accepting this, why not just let the Camera::stop() fall through and remove\n> >\n> > if (state_ == State::Stopped)\n> > return;\n> >\n> > as well. Then, CameraDevice::stop() will behave same as Camera::stop() - a no-op when called even when State::Stopped and responsible to clean up queues(and stuff) for the HAL layer.\n> >\n> > I should ideally back up my thoughts with some testing, but I don't have any kind of CTS testing available to me right now :(\n> > >>> camera_->stop();\n> > >>>\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 55D17BD160\n\tfor ;\n\tTue, 26 Mar 2024 08:22:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6FA14632EA;\n\tTue, 26 Mar 2024 09:22:56 +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 6582763037\n\tfor ;\n\tTue, 26 Mar 2024 09:22:54 +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 B810963B;\n\tTue, 26 Mar 2024 09:22:22 +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=\"YnQDE8Fk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711441342;\n\tbh=wCLC6VNDJgzPEwzhw2JPzAMOjeqlDc4FAHc3bxNj8ag=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YnQDE8FkU0KkykNk4seGRgv6yCu5tdaulZ9kDmJNWW6oHi104z+ME94n3Ur7KGbIx\n\t+A9ZhWE4EemugQ4MrjyZ34F1q6YQWcSuJgx9PxFlved6NQcV1dqfbBBf4ULV3SJ6JL\n\tM9vlY/qbrRf2Ea8cIh0/2q0lxshLcGjK3bUdhqXI=","Date":"Tue, 26 Mar 2024 09:22:50 +0100","From":"Jacopo Mondi ","To":"Hui Fang ","Cc":"Jacopo Mondi , \n\tAnle Pan , Umang Jain ,\n\t\"libcamera-devel@lists.libcamera.org\"\n\t","Subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Message-ID":"","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\n\t\n\t\n\t<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>\n\t\n\t\n\t\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":29061,"web_url":"https://patchwork.libcamera.org/comment/29061/","msgid":"","date":"2024-03-26T08:51:28","subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":186,"url":"https://patchwork.libcamera.org/api/people/186/","name":"Fang Hui","email":"hui.fang@nxp.com"},"content":"Hi, Jacopo\n We (Anle and me) are ok with your proposition, please help refine and apply, thanks!\n\nBRs,\nFang Hui","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 03C11BD160\n\tfor ;\n\tTue, 26 Mar 2024 08:51:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D1F816330D;\n\tTue, 26 Mar 2024 09:51:34 +0100 (CET)","from EUR04-VI1-obe.outbound.protection.outlook.com\n\t(mail-vi1eur04on20701.outbound.protection.outlook.com\n\t[IPv6:2a01:111:f403:2611::701])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E63D63037\n\tfor ;\n\tTue, 26 Mar 2024 09:51:32 +0100 (CET)","from DB9PR04MB9284.eurprd04.prod.outlook.com (2603:10a6:10:36c::8)\n\tby AM0PR04MB7028.eurprd04.prod.outlook.com (2603:10a6:208:19a::7)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7409.31;\n\tTue, 26 Mar 2024 08:51:28 +0000","from DB9PR04MB9284.eurprd04.prod.outlook.com\n\t([fe80::f658:f649:f731:17cb]) by\n\tDB9PR04MB9284.eurprd04.prod.outlook.com\n\t([fe80::f658:f649:f731:17cb%7]) with mapi id 15.20.7409.031;\n\tTue, 26 Mar 2024 08:51:28 +0000"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=nxp.com header.i=@nxp.com header.b=\"mqeA4kCa\";\n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;\n\tb=PVQyusiTGvTN+1z+cjqPSBVkx8tN7MrU9HxzR4By1R3Ls3VFi4yuEygOKRUumRUdcVZpbM3GIrViFoj0F9KxEMSrU+Cq0joka/TTyZZtxqUYOuuxYnPHpPxXsBdhsu75nXyui9bTuA5cq9U5JK8JvH23agPnqkmf8BSdVQtRpKGew3tnypdHbeWiBFjcnXytc/0C4Ue9ePZiO5acfC24VoQRS5NBc4PaosH1NHcLZfs28R2vWyefjIxJADqEe/cT86xcGKYr6U4FXXQvkdk1K7S9N7nRyUFDEvpRQh3V0vkYjPBH5bTsQA5benVrViFUhn07WgLV973oaM9xeq0oNw==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector9901;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=L82pOTOFm9IoMpRZdamQ/244pYcbj5BBH1jpEIavoWU=;\n\tb=fRLUGWd/1qQYmjRIYgqN785uS2ET19ox8eVX6eU1O0Zve3zjW39tjek678CqxD2fGkHtnMgu10i7UPNB8/yG9JCG11DEIK+Hpc4tqinCC8LbYg/B5eYFZ7LQBWOq0taTOUgIS2o3BE4KfkMmEqi/CQqZvJHSjiPgzvPNEakG58PvK0omSQNaEZp+xjENiB3IvNlfOV+cyGgW/swWohxxuSl81EYDkmgxrUJYtxuHKPCdSWDqrMm/6du5i/bx/cuxM9cKNC9sHnZP2q2GPvRVbOyFJE8F1HpF/9/Kdvz8a2vlPLoEAg2oG2/ThdI0rux8gzYhQxE/su7Z5A+4cTWv+A==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com;\n\tdkim=pass header.d=nxp.com; arc=none","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=L82pOTOFm9IoMpRZdamQ/244pYcbj5BBH1jpEIavoWU=;\n\tb=mqeA4kCarSEq9ImzH4VAZP8lPvWUwlZ+J4237Xs0IMjHMeG5htWi6Uc/dpWB8nGQU5R9wPbSlhGOkdoahIfwvLoqzWZOHD1wlpDIASe2wVrg0JrSV63VGo7dmjQMIeMGb0ocm07S8XX4g/Y6RPlxWzKdh+LT40Yxik/8xIDGB5Y=","From":"Hui Fang ","To":"Jacopo Mondi ","CC":"Anle Pan , Umang Jain ,\n\t\"libcamera-devel@lists.libcamera.org\"\n\t","Subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Topic":"[EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Thread-Index":"AQHabhdYilzqS/KnJ0GZgDK8wRbHfLEo0YUAgAARIACAGsq2gIAEhsGAgAAbIYCAASrKjoAAU3MAgAACw6yAAAEpAIAAB4+t","Date":"Tue, 26 Mar 2024 08:51:28 +0000","Message-ID":"","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\n\t\n\t\n\t<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","msip_labels":"","x-ms-publictraffictype":"Email","x-ms-traffictypediagnostic":"DB9PR04MB9284:EE_|AM0PR04MB7028:EE_","x-ms-exchange-senderadcheck":"1","x-ms-exchange-antispam-relay":"0","x-microsoft-antispam":"BCL:0;","x-microsoft-antispam-message-info":"Nz5RyedWNlM38O+yO0vEoAJpipoU1ubxWtqgedLMK4SgeOGL66zTnLvAeRJT1+SG9Gm6FkZDHo98q2EIjHX9nhZHii/t4i+EvJC0Agaa2n33zMVWfmza7oTnblbOkcazDeopsjGA62RKzkL8TboIh46CABkjZ2pnlAfoD0Ojue3k04SPbvMDt/agISM5DBJlwCYgoaZ/YQd5vOjYb2NJFSN/qc93g65ljZYeTRhSov3avNcVYtfeKbAe/AQSks0IabWYE1zdn7lPmxnGXy1HKtsVJ9S7AXqAi4iAbpoYYVh7qTHZG0tuygoUJ2wsvbguLZqABpmYSxjSaf/TtO9dPIqVsS96+75Wqjnx2TKtouTiBswwlMmjALt9Ln4h/p+UIxbGIgxyNSVaNgCSdnwAjhX10ppFq2Mvotq7iSgCouDLtWK4QwcPYJZ2dum9OhBB7255q0KQmQqub0buFKGIfL8NldjeyIpMoatMZI/8PF3sBJIZ6giRHaxoxAZy2t9vL8iVJhNJuXsv+Ws3ZshcCaH/y9ieTLaZyhiyLQ2JU2nmQqa5m3tZ7zEJSFUKVmQ2rb7RVZHlhAPWsA+ZHpg9ezS4AtjwPES+hN2MN81oRza4MsC+rYhexH21dADnX4dP5IpczLRlAKU8BnWxFejN6EhpoL6/M/vyFLLgzg810YE=","x-forefront-antispam-report":"CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;\n\tIPV:NLI; SFV:NSPM; H:DB9PR04MB9284.eurprd04.prod.outlook.com; PTR:;\n\tCAT:NONE; \n\tSFS:(13230031)(366007)(376005)(1800799015); DIR:OUT; SFP:1102; ","x-ms-exchange-antispam-messagedata-chunkcount":"1","x-ms-exchange-antispam-messagedata-0":"=?utf-8?q?/0OyoJJrJ98EwRur9hMuxrkMP?=\n\t=?utf-8?q?GGkvwyn1jAI3R5kAqpLIknE6ktqyIhfEuV/7DCn1YdCLSKapGzIc95Py?=\n\t=?utf-8?q?LxahHUzMtWF3aFnuwPrDQxt933DwbapY26YKCHPf7Vtk3GSPmVJbrGUT?=\n\t=?utf-8?q?rsdPPFMCkjzbFHzcY03aKMRTQcOOYZj/MT/eeTWXpvh3zDxWWiCG0mwZ?=\n\t=?utf-8?q?jLvIwA5VciA0V6heavVuFc+8/FjnO26JlwcL3NQZ+VTPDcj+x0cgn0HM?=\n\t=?utf-8?q?IvpDF1nyhA6i0LrjpeeoTm+wFDIPAgEf20x5ceSQu7snF2IqaYR7vSAc?=\n\t=?utf-8?q?TDMm8fjWcxIAjz8ip4CgwaJC997YAJvw/SLbIb3QmmB2V7N6U5cn1kZV?=\n\t=?utf-8?q?Hoa7kJ5eaBdUP+VpzeOLx5SXxeflQ3kIieCjHSdOCl88aaLXOG5tu7mU?=\n\t=?utf-8?q?b3nLIk1rGwzgvN6PkwZzL/3N5K5BItTa0BCU9qt/SXclgAs7Yis+N2eL?=\n\t=?utf-8?q?prAk9/mUtx5ZETeCFih4rGR1j1uIMSYdeEYLG51UjW68RvZAMhQskHZc?=\n\t=?utf-8?q?2+dhutELlkDxlunqeqjsK5+I0wG/4zA3Br+RZQcPOyIHQ6mMYXyIJmT4?=\n\t=?utf-8?q?RvUt8Ys6HPYheixEf4vRNIDj7FWSYgBOBJ9gR7wER8A/xc1HDnGHLLJC?=\n\t=?utf-8?q?MacV4t9MBI+z7Ba86t+g9aySm/y5kYhukFXjbaqx5cUa6+jMJTF8lJFA?=\n\t=?utf-8?q?EaKMpfdVM5F5MRyDDEO7gyoWmGTDRftQTrBR6D2XNnso2zUSk+lLlvwN?=\n\t=?utf-8?q?0Ns/w5GcBLCzlKcf/BM5ihplZbTHkGvrWXCJEnAA2IQ6e44H5o/pBtOh?=\n\t=?utf-8?q?K+0qZ3xckLh7watmb+yNJe4SvvsjUUFDNUg1v0uimQJN7oMOPfs0XWyR?=\n\t=?utf-8?q?MZgeJ39GJMoHxpqVFcFBhohmQtG/uCPZD5WuBYMbagPVhiDcEocRyMkC?=\n\t=?utf-8?q?ZMf9u11pEIevB0P1woTJjAdd5qpOJ448JW4CEHKEHLgRY2lI3AUD3ZGl?=\n\t=?utf-8?q?7oazJrMfeZhv0fbqT4jssBVA3fpIEVaIZpgc/NPP7bc5ndTHpqCpfWFn?=\n\t=?utf-8?q?hjVcM8+uc9EKFsWenAAjE8XsMN2KKUDDBJMoq3MIqb1r/LBUtwxyICT3?=\n\t=?utf-8?q?Vzqi0ftsid7TJk2/8PJ4eRliS2gubCZ3lOzD/SAyqZ634KMXoIe+AvHj?=\n\t=?utf-8?q?ezEUYJWTHcNebT+w2I5G5HIFg9fICvbqB4e69XgmdlK+fx340XDzNRJY?=\n\t=?utf-8?q?reJ7f7bRSwZ1tU4MRMlO7J2BYU8I5w6K75WlNsTCTDOgO2+8ZYHNBkpb?=\n\t=?utf-8?q?Vz2/R80SQyvygg+aVoPyR4hXjTnTByUGqvfG1PPPDnNOZZrcn7Naj4dh?=\n\t=?utf-8?q?ssycp/WabdxAZxKz1APQTwmtZT0x72TuUdxMsRLpNYxLZloT7BRwqJks?=\n\t=?utf-8?q?ts9j4jV0ka2QufNl9NwCEGMqTrb2Sq6JETR+boEq6A8pe5e46Iplv2fo?=\n\t=?utf-8?q?eSsYMDOK59j53OFl6uzotw/BFoyMvVlP2shqEmF70qn5AwJWf3Rry8hL?=\n\t=?utf-8?q?9bvvfb9n8EH0Qv0JZdiZLNEZHG944dL1ISD7YG6oVY7ljzZhJbdvKr6q?=\n\t=?utf-8?q?qf/TB7MCEAFBhPE8WwOazbrka+cRdzt373Q6yhHfDk=3D?=","Content-Type":"multipart/alternative;\n\tboundary=\"_000_DB9PR04MB9284409268AD47DB94F8645487352DB9PR04MB9284eurp_\"","MIME-Version":"1.0","X-OriginatorOrg":"nxp.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-AuthSource":"DB9PR04MB9284.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"af8166f3-6cac-46a5-f62e-08dc4d71ed02","X-MS-Exchange-CrossTenant-originalarrivaltime":"26 Mar 2024 08:51:28.2298\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635","X-MS-Exchange-CrossTenant-mailboxtype":"HOSTED","X-MS-Exchange-CrossTenant-userprincipalname":"r225IwXWXgOoW6ePsKipNvgBJXJgODDqlhqIw3/10WFIgWwwtD91SIEyHuBcu2AjNI7529BZAtWjazGN8Oa56w==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"AM0PR04MB7028","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":29062,"web_url":"https://patchwork.libcamera.org/comment/29062/","msgid":"","date":"2024-03-26T08:56:17","subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang,\n sorry, I missed your reply\n\nOn Mon, Mar 25, 2024 at 01:13:37PM +0530, Umang Jain wrote:\n> Hi Jacopo,\n>\n> Thanks for testing,\n>\n> On 22/03/24 4:06 pm, Jacopo Mondi wrote:\n> > Hello\n> >\n> > On Tue, Mar 05, 2024 at 09:28:00AM +0000, Anle Pan wrote:\n> > > Hi Umang Jain,\n> > >\n> > > this issue was random(around 1/10), and can be reproduced by single run below cts test:\n> > > \t./cts-tradefed run commandAndExit cts --abi arm64-v8a -m CtsMediaPlayerTestCases -t android.media.player.cts.MediaPlayerTest#testRecordedVideoPlayback90\n> > >\n> > > •\t3 times configuring streams in this Test:\n> > > 1) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n> > > 2) 3 streams: (0) 320x240-NV12 (1) 320x240-NV12 (2) 1280x720-YUYV: will capture on stream0 and stream2 at the same time\n> > > 3) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n> > >\n> > > •\twhen single run the test several times and met fail. When digging the log, can find that the current failed is caused by the abnormal complete of the previous PASS test in step3, which is the situation described in my patch.\n> > >\n> > The only code path I see that could lead to descriptor_ not being\n> > cleared is flush() being called and then stop() called just after\n> > possibily as the result of a new configuration\n> >\n> > void CameraDevice::flush()\n> > {\n> > \t{\n> > \t\tMutexLocker stateLock(stateMutex_);\n> > \t\tif (state_ != State::Running)\n> > \t\t\treturn;\n> >\n> > \t\tstate_ = State::Flushing;\n> > \t}\n> >\n> > \tcamera_->stop();\n> >\n> > \tMutexLocker stateLock(stateMutex_);\n> > \tstate_ = State::Stopped;\n> > }\n> >\n> > void CameraDevice::stop()\n> > {\n> > \tMutexLocker stateLock(stateMutex_);\n> > \tif (state_ == State::Stopped)\n> > \t\treturn;\n> >\n> > \tcamera_->stop();\n> >\n> > \t{\n> > \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > \t\tdescriptors_ = {};\n> > \t}\n> >\n> > \tstreams_.clear();\n> >\n> > \tstate_ = State::Stopped;\n> > }\n> >\n> > int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > {\n> > \t/* Before any configuration attempt, stop the camera. */\n> > \tstop();\n> >\n> > ...\n> >\n> > }\n> >\n> > Looking at the flush() implementation right now I wonder why we stop\n> > the camera without the stateMutex_ held (the only reason I see is\n> > because it can be concurrent with processCaptureRequest() which keeps\n> > the mutex held), but I certainly don't want to touch that code right\n> > now without a good reason.\n> >\n> > All in all, I think there's indeed a path that could lead to the\n> > descriptor not being cleaned up, and we can't do it in flush() because\n> > a concurrent call to processCaptureRequest() might be using the\n> > descriptors to complete the in-flight requests with error state.\n> >\n> > Anle, does this match your understanding ? If so, this is what should\n> > be recorded in the commit message (the cause of an issue, not the\n> > symptoms)\n> >\n> >\n> > > Best Regards\n> > > Anle Pan\n> > >\n> > > -----Original Message-----\n> > > From: Umang Jain \n> > > Sent: 2024年3月5日 16:27\n> > > To: Anle Pan ; libcamera-devel@lists.libcamera.org\n> > > Cc: Hui Fang \n> > > Subject: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n> > >\n> > > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button\n> > >\n> > >\n> > > Hi Anle Pan, Hui Fang\n> > >\n> > > On 04/03/24 3:05 pm, Anle Pan wrote:\n> > > > The issue occurs when stopping capture soon after starting capture.\n> > > Can you describe the issue/use case in more detail? It's really hard to infer from one line.\n> > > > In this case, no frame get from the device, the related capture\n> > > > request has been pushed to the queue descriptors_, but the\n> > > > queuedRequests_ was still empty due to no requests will be queue to\n> > > > the device since the stream will be stopped soon,\n> > > I don't get it how is this possible? Since processCaptureRequest() will push the request in the descriptors_ queue along with queuing the request to the camera.\n> > >\n> > > How will the call path be interleaved /before/ processCaptureRequest() returns? Possibly we need more details on what you are trying to do, before reviewing the code here. Is this related to CTS ?\n> > > > so there will be no camera->requestComplete called later, then the\n> > > > descriptors_ can not pop normally, this will cause the pending if we\n> > > > want to start capture next time.\n> > > >\n> > > > To fix the issue, ensure the descriptors_ is empty after the camera\n> > > > device is stopped.\n> > Do you know precisely why a non-empty descriptors_ stalls the camera ?\n> > Does it get stuck on\n> >\n> > void CameraDevice::sendCaptureResults()\n> > {\n> > \twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n> >\n> > ?\n>\n> We should to formalise this as an proper issue, after getting more\n> information.\n> > > > Signed-off-by: Anle Pan \n> > Tested with CTS, no regressions detected\n> >\n> > Reviewed-by: Jacopo Mondi \n> > Tested-by: Jacopo Mondi \n> >\n> > > > ---\n> > > > src/android/camera_device.cpp | 5 ++++-\n> > > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp\n> > > > b/src/android/camera_device.cpp index 25cedd44..d452992d 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -433,8 +433,11 @@ void CameraDevice::flush()\n> > > > void CameraDevice::stop()\n> > > > {\n> > > > MutexLocker stateLock(stateMutex_);\n> > > > - if (state_ == State::Stopped)\n> > > > + if (state_ == State::Stopped) {\n> > > > + MutexLocker descriptorsLock(descriptorsMutex_);\n> > > > + descriptors_ = {};\n> > > > return;\n> > > > + }\n>\n> I am not comfortable with this patch. Consider my points below\n>\n> - First we don't have a exact description of the issue it is fixing\n\nNo we do, don't we ?\n\n>\n> - Second, if the camera state is ::Stopped already. Clears descriptors_\n> doesn't look a good idea to me. The function which has set the camera\n> state::Stopped already, should be ideally be emptying the descriptors_...\n>\n\nWe can't clear descriptors_ in flush() because a concurrent\nprocessCaptureRequests() might be dealing with descriptors_ (and\ncomplete it if state == Stopped, so no need to forcefully clear it)\n\n> - If we are accepting this, why not just let the Camera::stop() fall through\n> and remove\n>\n>             if (state_ == State::Stopped)\n>                 return;\n>\n> as well. Then, CameraDevice::stop() will behave same as Camera::stop() - a\n> no-op when called even when State::Stopped and responsible to clean up\n> queues(and stuff) for the HAL layer.\n\nThis might be worth experimenting with indeed!\n\nCameraDevice::stop() should then look like:\n\n void CameraDevice::stop()\n {\n MutexLocker stateLock(stateMutex_);\n\n camera_->stop();\n\n {\n MutexLocker descriptorsLock(descriptorsMutex_);\n descriptors_ = {};\n }\n\n streams_.clear();\n\n state_ = State::Stopped;\n }\n\nBut before testing it further, do we agree the issue to fix is real ?\n\n>\n> I should ideally back up my thoughts with some testing, but I don't have any\n> kind of CTS testing available to me right now :(\n> > > > camera_->stop();\n> > > >\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 3ABFCC0DA4\n\tfor ;\n\tTue, 26 Mar 2024 08:56:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC5A5632EA;\n\tTue, 26 Mar 2024 09:56:21 +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 2A62363037\n\tfor ;\n\tTue, 26 Mar 2024 09:56:20 +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 7982363B;\n\tTue, 26 Mar 2024 09:55:48 +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=\"CODEdjvl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711443348;\n\tbh=/+3LziMFMJlPn0dhzJ8kNKChh3G1J4bs0jPbNwPTtFE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CODEdjvlgdSUijY9WGhLqxbq+0DgqGTJ6DjQmG+eyQtnP1GIj4VdCS5GCNLxQEFbT\n\tLks0RosY0sqNGOcMUc6257EHShVdD6pVniYvTFHANjqvCgEzMW9SjmOplODy1GBCcC\n\tGceKMI37T802XYr8bzfkVa+dVejgkpsyAWV3IM5U=","Date":"Tue, 26 Mar 2024 09:56:17 +0100","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"Jacopo Mondi , \n\tAnle Pan , \"libcamera-devel@lists.libcamera.org\"\n\t, Hui Fang ","Subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","Message-ID":"","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\n\t\n\t\n\t<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>","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":29071,"web_url":"https://patchwork.libcamera.org/comment/29071/","msgid":"","date":"2024-03-26T12:53:24","subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 26/03/24 2:26 pm, Jacopo Mondi wrote:\n> Hi Umang,\n> sorry, I missed your reply\n>\n> On Mon, Mar 25, 2024 at 01:13:37PM +0530, Umang Jain wrote:\n>> Hi Jacopo,\n>>\n>> Thanks for testing,\n>>\n>> On 22/03/24 4:06 pm, Jacopo Mondi wrote:\n>>> Hello\n>>>\n>>> On Tue, Mar 05, 2024 at 09:28:00AM +0000, Anle Pan wrote:\n>>>> Hi Umang Jain,\n>>>>\n>>>> this issue was random(around 1/10), and can be reproduced by single run below cts test:\n>>>> \t./cts-tradefed run commandAndExit cts --abi arm64-v8a -m CtsMediaPlayerTestCases -t android.media.player.cts.MediaPlayerTest#testRecordedVideoPlayback90\n>>>>\n>>>> •\t3 times configuring streams in this Test:\n>>>> 1) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n>>>> 2) 3 streams: (0) 320x240-NV12 (1) 320x240-NV12 (2) 1280x720-YUYV: will capture on stream0 and stream2 at the same time\n>>>> 3) 2 streams: (0) 320x240-NV12 (1) 1280x720-YUYV: capture several frames on stream1\n>>>>\n>>>> •\twhen single run the test several times and met fail. When digging the log, can find that the current failed is caused by the abnormal complete of the previous PASS test in step3, which is the situation described in my patch.\n>>>>\n>>> The only code path I see that could lead to descriptor_ not being\n>>> cleared is flush() being called and then stop() called just after\n>>> possibily as the result of a new configuration\n>>>\n>>> void CameraDevice::flush()\n>>> {\n>>> \t{\n>>> \t\tMutexLocker stateLock(stateMutex_);\n>>> \t\tif (state_ != State::Running)\n>>> \t\t\treturn;\n>>>\n>>> \t\tstate_ = State::Flushing;\n>>> \t}\n>>>\n>>> \tcamera_->stop();\n>>>\n>>> \tMutexLocker stateLock(stateMutex_);\n>>> \tstate_ = State::Stopped;\n>>> }\n>>>\n>>> void CameraDevice::stop()\n>>> {\n>>> \tMutexLocker stateLock(stateMutex_);\n>>> \tif (state_ == State::Stopped)\n>>> \t\treturn;\n>>>\n>>> \tcamera_->stop();\n>>>\n>>> \t{\n>>> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>>> \t\tdescriptors_ = {};\n>>> \t}\n>>>\n>>> \tstreams_.clear();\n>>>\n>>> \tstate_ = State::Stopped;\n>>> }\n>>>\n>>> int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>>> {\n>>> \t/* Before any configuration attempt, stop the camera. */\n>>> \tstop();\n>>>\n>>> ...\n>>>\n>>> }\n>>>\n>>> Looking at the flush() implementation right now I wonder why we stop\n>>> the camera without the stateMutex_ held (the only reason I see is\n>>> because it can be concurrent with processCaptureRequest() which keeps\n>>> the mutex held), but I certainly don't want to touch that code right\n>>> now without a good reason.\n>>>\n>>> All in all, I think there's indeed a path that could lead to the\n>>> descriptor not being cleaned up, and we can't do it in flush() because\n>>> a concurrent call to processCaptureRequest() might be using the\n>>> descriptors to complete the in-flight requests with error state.\n>>>\n>>> Anle, does this match your understanding ? If so, this is what should\n>>> be recorded in the commit message (the cause of an issue, not the\n>>> symptoms)\n>>>\n>>>\n>>>> Best Regards\n>>>> Anle Pan\n>>>>\n>>>> -----Original Message-----\n>>>> From: Umang Jain \n>>>> Sent: 2024年3月5日 16:27\n>>>> To: Anle Pan ; libcamera-devel@lists.libcamera.org\n>>>> Cc: Hui Fang \n>>>> Subject: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked issue when stopping capture\n>>>>\n>>>> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button\n>>>>\n>>>>\n>>>> Hi Anle Pan, Hui Fang\n>>>>\n>>>> On 04/03/24 3:05 pm, Anle Pan wrote:\n>>>>> The issue occurs when stopping capture soon after starting capture.\n>>>> Can you describe the issue/use case in more detail? It's really hard to infer from one line.\n>>>>> In this case, no frame get from the device, the related capture\n>>>>> request has been pushed to the queue descriptors_, but the\n>>>>> queuedRequests_ was still empty due to no requests will be queue to\n>>>>> the device since the stream will be stopped soon,\n>>>> I don't get it how is this possible? Since processCaptureRequest() will push the request in the descriptors_ queue along with queuing the request to the camera.\n>>>>\n>>>> How will the call path be interleaved /before/ processCaptureRequest() returns? Possibly we need more details on what you are trying to do, before reviewing the code here. Is this related to CTS ?\n>>>>> so there will be no camera->requestComplete called later, then the\n>>>>> descriptors_ can not pop normally, this will cause the pending if we\n>>>>> want to start capture next time.\n>>>>>\n>>>>> To fix the issue, ensure the descriptors_ is empty after the camera\n>>>>> device is stopped.\n>>> Do you know precisely why a non-empty descriptors_ stalls the camera ?\n>>> Does it get stuck on\n>>>\n>>> void CameraDevice::sendCaptureResults()\n>>> {\n>>> \twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n>>>\n>>> ?\n>> We should to formalise this as an proper issue, after getting more\n>> information.\n>>>>> Signed-off-by: Anle Pan \n>>> Tested with CTS, no regressions detected\n>>>\n>>> Reviewed-by: Jacopo Mondi \n>>> Tested-by: Jacopo Mondi \n>>>\n>>>>> ---\n>>>>> src/android/camera_device.cpp | 5 ++++-\n>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>>>>>\n>>>>> diff --git a/src/android/camera_device.cpp\n>>>>> b/src/android/camera_device.cpp index 25cedd44..d452992d 100644\n>>>>> --- a/src/android/camera_device.cpp\n>>>>> +++ b/src/android/camera_device.cpp\n>>>>> @@ -433,8 +433,11 @@ void CameraDevice::flush()\n>>>>> void CameraDevice::stop()\n>>>>> {\n>>>>> MutexLocker stateLock(stateMutex_);\n>>>>> - if (state_ == State::Stopped)\n>>>>> + if (state_ == State::Stopped) {\n>>>>> + MutexLocker descriptorsLock(descriptorsMutex_);\n>>>>> + descriptors_ = {};\n>>>>> return;\n>>>>> + }\n>> I am not comfortable with this patch. Consider my points below\n>>\n>> - First we don't have a exact description of the issue it is fixing\n> No we do, don't we ?\n\nack, i re-read ...\n>\n>> - Second, if the camera state is ::Stopped already. Clears descriptors_\n>> doesn't look a good idea to me. The function which has set the camera\n>> state::Stopped already, should be ideally be emptying the descriptors_...\n>>\n> We can't clear descriptors_ in flush() because a concurrent\n> processCaptureRequests() might be dealing with descriptors_ (and\n> complete it if state == Stopped, so no need to forcefully clear it)\n\nYea, I get the idea that we can't clear the descriptors_ there.\n>\n>> - If we are accepting this, why not just let the Camera::stop() fall through\n>> and remove\n>>\n>>             if (state_ == State::Stopped)\n>>                 return;\n>>\n>> as well. Then, CameraDevice::stop() will behave same as Camera::stop() - a\n>> no-op when called even when State::Stopped and responsible to clean up\n>> queues(and stuff) for the HAL layer.\n> This might be worth experimenting with indeed!\n\nI would like myself to test the implementation of flush() and stop() at \nthe same time probably.. because currently flush is calling the \ncamera_->stop() directly.\n>\n> CameraDevice::stop() should then look like:\n>\n> void CameraDevice::stop()\n> {\n> MutexLocker stateLock(stateMutex_);\n>\n> camera_->stop();\n>\n> {\n> MutexLocker descriptorsLock(descriptorsMutex_);\n> descriptors_ = {};\n> }\n>\n> streams_.clear();\n>\n> state_ = State::Stopped;\n> }\n>\n> But before testing it further, do we agree the issue to fix is real ?\n\nAgreed.\nThanks for handling this!\n\n>\n>> I should ideally back up my thoughts with some testing, but I don't have any\n>> kind of CTS testing available to me right now :(\n>>>>> camera_->stop();\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 25852C0DA4\n\tfor ;\n\tTue, 26 Mar 2024 12:53:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6B0263326;\n\tTue, 26 Mar 2024 13:53:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDC1463037\n\tfor ;\n\tTue, 26 Mar 2024 13:53:32 +0100 (CET)","from [192.168.1.105] (unknown [103.251.226.53])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BB46263B;\n\tTue, 26 Mar 2024 13:52:59 +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=\"eVXRd/HC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711457581;\n\tbh=bq00LUFVKnwFX7q6+fySAndZw8Zba0jZMABhIFVa63M=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=eVXRd/HCf5uFNmWYBivr4cCIR82DUwDQ5FfovavViYaP0U6xWNLiHIZ1JXCQB2YXt\n\tpg/+TKZNWYdRgvRA1GrDuqg2H+E/OMQ/7GDY8L/ndLSDQ/1MY0LYDgGi1kx19YD6hy\n\tXvn8ftC5nJPQsbS2d18pFu258RxcVpWdquEw1RU8=","Message-ID":"","Date":"Tue, 26 Mar 2024 18:23:24 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [EXT] Re: [PATCH] android: camera_device: Fix camera blocked\n\tissue when stopping capture","To":"Jacopo Mondi ","Cc":"Anle Pan , \"libcamera-devel@lists.libcamera.org\"\n\t, Hui Fang ","References":"<20240304093539.546973-1-anle.pan@nxp.com>\n\t\n\t\n\t\n\t<979571eb-904f-4cf0-81c6-9d0cfcdbabea@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]