[{"id":19232,"web_url":"https://patchwork.libcamera.org/comment/19232/","msgid":"","date":"2021-08-31T21:04:19","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> The problem is happening because we seem to add a CameraStream\n> associated buffer(depending on the CameraStream::Type) to the Request,\n\ns/(/ (/\n\n> in CameraDevice::processCaptureRequest().\n> \n> However, when the camera stops, all the current buffers are marked with\n> FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> associated with the CameraStream (that was previously added to the\n> request) has now been cleared out with a part of streams_.clear(), even\n> before the camera stop() has been invoked. Any access to those request\n> buffers after they have been cleared, shall result in a crash.\n\ns/shall/will/\n\n> Signed-off-by: Hirokazu Honda \n\nI can fix the above when pushing.\n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/android/camera_device.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 8ca76719..fda77db4 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> \n> void CameraDevice::close()\n> {\n> -\tstreams_.clear();\n> -\n> \tstop();\n> \n> \tcamera_->release();\n> @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> \tcamera_->stop();\n> \n> \tdescriptors_.clear();\n> +\tstreams_.clear();\n> +\n> \tstate_ = State::Stopped;\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 0C480BD87D\n\tfor ;\n\tTue, 31 Aug 2021 21:04:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BABA26916A;\n\tTue, 31 Aug 2021 23:04:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A6B6368890\n\tfor ;\n\tTue, 31 Aug 2021 23:04:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3D12424F;\n\tTue, 31 Aug 2021 23:04:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MjAb/4J9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630443874;\n\tbh=LnqdES2adibOR82wXIGO95vR5Gjiw6xL54/dhsVYi28=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MjAb/4J9r3kkRp7jKmR0HzUx4uiPoVx27fgDQjEQ1DBWJexNW4Nz+s4dl8zjAD3F0\n\tUPWuGdelpOAjp2eZs+3Yj62NmgmVpAOBg/zCiTJ0Ats+Mzr12Ug2nWUdPJdn0JRHXa\n\t3fMilb+MfKFSm5AwcdEtmey52ZMSveXCrxkruKwM=","Date":"Wed, 1 Sep 2021 00:04:19 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210831183739.901729-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210831183739.901729-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19237,"web_url":"https://patchwork.libcamera.org/comment/19237/","msgid":"<7c8769d9-890c-5e12-6128-6968a523e977@ideasonboard.com>","date":"2021-09-01T05:49:27","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello,\n\nOn 9/1/21 2:34 AM, Laurent Pinchart wrote:\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n>> The problem is happening because we seem to add a CameraStream\n>> associated buffer(depending on the CameraStream::Type) to the Request,\n> s/(/ (/\n>\n>> in CameraDevice::processCaptureRequest().\n>>\n>> However, when the camera stops, all the current buffers are marked with\n>> FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n>> associated with the CameraStream (that was previously added to the\n>> request) has now been cleared out with a part of streams_.clear(), even\n>> before the camera stop() has been invoked. Any access to those request\n>> buffers after they have been cleared, shall result in a crash.\n> s/shall/will/\n>\n>> Signed-off-by: Hirokazu Honda \n> I can fix the above when pushing.\n>\n> Reviewed-by: Laurent Pinchart \n\n\nReviewed-by: Umang Jain \n\n>\n>> ---\n>> src/android/camera_device.cpp | 4 ++--\n>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 8ca76719..fda77db4 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n>> \n>> void CameraDevice::close()\n>> {\n>> -\tstreams_.clear();\n>> -\n>> \tstop();\n>> \n>> \tcamera_->release();\n>> @@ -457,6 +455,8 @@ void CameraDevice::stop()\n>> \tcamera_->stop();\n>> \n>> \tdescriptors_.clear();\n>> +\tstreams_.clear();\n>> +\n>> \tstate_ = State::Stopped;\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 4E8D2BD87D\n\tfor ;\n\tWed, 1 Sep 2021 05:49:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98CE26916A;\n\tWed, 1 Sep 2021 07:49:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6552360252\n\tfor ;\n\tWed, 1 Sep 2021 07:49:33 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 22D605A7;\n\tWed, 1 Sep 2021 07:49:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FWUMtve3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630475372;\n\tbh=IJkppAhAJhfTiD7MpeMqaMpulk0pvg/r7qk0LCrR6wU=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=FWUMtve3wKKADpl4Ob3teh2mTGbkHNH6jDFPHXXkTgWwk9eYaRbjeDaZiLTaEoGmU\n\tXD1pBW01MrU6kXifQ3uL/dMSQUd3CUNsW0+FBsC2An4HqNrsuqfXFb3rdF4QxnMd7N\n\tk6HrLMIA7Rqi9jHNAAK6p5yT4PA75s011fvxhH4I=","To":"Laurent Pinchart ,\n\tHirokazu Honda ","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t","From":"Umang Jain ","Message-ID":"<7c8769d9-890c-5e12-6128-6968a523e977@ideasonboard.com>","Date":"Wed, 1 Sep 2021 11:19:27 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19262,"web_url":"https://patchwork.libcamera.org/comment/19262/","msgid":"<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>","date":"2021-09-01T15:47:08","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> The problem is happening because we seem to add a CameraStream\n> associated buffer(depending on the CameraStream::Type) to the Request,\n> in CameraDevice::processCaptureRequest().\n>\n> However, when the camera stops, all the current buffers are marked with\n> FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> associated with the CameraStream (that was previously added to the\n> request) has now been cleared out with a part of streams_.clear(), even\n> before the camera stop() has been invoked. Any access to those request\n> buffers after they have been cleared, shall result in a crash.\n\nI recall it was painful at the time when we had to deal with flush()\nto get the right ordering, and I had done the same. However, since\nstop() is only called by configureStreams() I removed the\nstreams_.clear() there and had issues, so I gave up.\n\nBut I have this patch gone through cros_camera_test and also CTS seems\nfine. Also looking at the code it seems an harmless change so I wonder\nwhat was wrong at the time.\n\nAs far as I can tell:\n\nTested-by: Jacopo Mondi \nAcked-by: Jacopo Mondi \n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 8ca76719..fda77db4 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n>\n> void CameraDevice::close()\n> {\n> -\tstreams_.clear();\n> -\n> \tstop();\n>\n> \tcamera_->release();\n> @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> \tcamera_->stop();\n>\n> \tdescriptors_.clear();\n> +\tstreams_.clear();\n> +\n> \tstate_ = State::Stopped;\n> }\n>\n> --\n> 2.33.0.259.gc128427fd7-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6D1E7BD87D\n\tfor ;\n\tWed, 1 Sep 2021 15:46:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D20386916A;\n\tWed, 1 Sep 2021 17:46:20 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4301060288\n\tfor ;\n\tWed, 1 Sep 2021 17:46:19 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id BF843C000A;\n\tWed, 1 Sep 2021 15:46:18 +0000 (UTC)"],"Date":"Wed, 1 Sep 2021 17:47:08 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>","References":"<20210831183739.901729-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210831183739.901729-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19265,"web_url":"https://patchwork.libcamera.org/comment/19265/","msgid":"","date":"2021-09-02T03:11:40","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 01, 2021 at 05:47:08PM +0200, Jacopo Mondi wrote:\n> On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> > The problem is happening because we seem to add a CameraStream\n> > associated buffer(depending on the CameraStream::Type) to the Request,\n> > in CameraDevice::processCaptureRequest().\n> >\n> > However, when the camera stops, all the current buffers are marked with\n> > FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> > associated with the CameraStream (that was previously added to the\n> > request) has now been cleared out with a part of streams_.clear(), even\n> > before the camera stop() has been invoked. Any access to those request\n> > buffers after they have been cleared, shall result in a crash.\n> \n> I recall it was painful at the time when we had to deal with flush()\n> to get the right ordering, and I had done the same. However, since\n> stop() is only called by configureStreams() I removed the\n> streams_.clear() there and had issues, so I gave up.\n\nDo you mean you have tried removing streams_.clear() from\nconfigureStreams() while testing this patch and ran into issues, or that\nyou tried that when implementing support for flush() ?\n\n> But I have this patch gone through cros_camera_test and also CTS seems\n> fine. Also looking at the code it seems an harmless change so I wonder\n> what was wrong at the time.\n> \n> As far as I can tell:\n> \n> Tested-by: Jacopo Mondi \n> Acked-by: Jacopo Mondi \n> \n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 8ca76719..fda77db4 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> >\n> > void CameraDevice::close()\n> > {\n> > -\tstreams_.clear();\n> > -\n> > \tstop();\n> >\n> > \tcamera_->release();\n> > @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> > \tcamera_->stop();\n> >\n> > \tdescriptors_.clear();\n> > +\tstreams_.clear();\n> > +\n> > \tstate_ = State::Stopped;\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 CDA29BD87D\n\tfor ;\n\tThu, 2 Sep 2021 03:12:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 039DA6916A;\n\tThu, 2 Sep 2021 05:12:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 614EA68890\n\tfor ;\n\tThu, 2 Sep 2021 05:11:57 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B1DF945E;\n\tThu, 2 Sep 2021 05:11:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Zy1wnOdy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630552316;\n\tbh=RQGyIsmCnNMhj5beGRSbh9t+4sHcyDPYh3ZUoJvm2T4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zy1wnOdy6AkfolAW8Cyw6CYCrDEpbJ78FGkdXkrQtcRg23vVNJAr0xZGKFSsERTjq\n\t89wHzdyoPwmmtlEXC4gpcGzmBr0XwMze64mARSZHMxOjsCQrw9+GjkDFYXZ4ZofwfF\n\t9kQEShJvo33y5i7OYrCBSKQBagryK6RzzXHiFv2g=","Date":"Thu, 2 Sep 2021 06:11:40 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19267,"web_url":"https://patchwork.libcamera.org/comment/19267/","msgid":"<20210902065934.iuso7ylvynkzyhse@uno.localdomain>","date":"2021-09-02T06:59:34","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Sep 02, 2021 at 06:11:40AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wed, Sep 01, 2021 at 05:47:08PM +0200, Jacopo Mondi wrote:\n> > On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> > > The problem is happening because we seem to add a CameraStream\n> > > associated buffer(depending on the CameraStream::Type) to the Request,\n> > > in CameraDevice::processCaptureRequest().\n> > >\n> > > However, when the camera stops, all the current buffers are marked with\n> > > FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> > > associated with the CameraStream (that was previously added to the\n> > > request) has now been cleared out with a part of streams_.clear(), even\n> > > before the camera stop() has been invoked. Any access to those request\n> > > buffers after they have been cleared, shall result in a crash.\n> >\n> > I recall it was painful at the time when we had to deal with flush()\n> > to get the right ordering, and I had done the same. However, since\n> > stop() is only called by configureStreams() I removed the\n> > streams_.clear() there and had issues, so I gave up.\n>\n> Do you mean you have tried removing streams_.clear() from\n> configureStreams() while testing this patch and ran into issues, or that\n> you tried that when implementing support for flush() ?\n>\n\nI meant when developing flush().\nI haven't tried to do the same on top of this patch, but it might be\ngood to do so ?\n\nThanks\n j\n\n> > But I have this patch gone through cros_camera_test and also CTS seems\n> > fine. Also looking at the code it seems an harmless change so I wonder\n> > what was wrong at the time.\n> >\n> > As far as I can tell:\n> >\n> > Tested-by: Jacopo Mondi \n> > Acked-by: Jacopo Mondi \n> >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 4 ++--\n> > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 8ca76719..fda77db4 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> > >\n> > > void CameraDevice::close()\n> > > {\n> > > -\tstreams_.clear();\n> > > -\n> > > \tstop();\n> > >\n> > > \tcamera_->release();\n> > > @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> > > \tcamera_->stop();\n> > >\n> > > \tdescriptors_.clear();\n> > > +\tstreams_.clear();\n> > > +\n> > > \tstate_ = State::Stopped;\n> > > }\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0C463BD87D\n\tfor ;\n\tThu, 2 Sep 2021 06:58:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E4646916A;\n\tThu, 2 Sep 2021 08:58:48 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C85B560503\n\tfor ;\n\tThu, 2 Sep 2021 08:58:46 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 120E360002;\n\tThu, 2 Sep 2021 06:58:45 +0000 (UTC)"],"Date":"Thu, 2 Sep 2021 08:59:34 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210902065934.iuso7ylvynkzyhse@uno.localdomain>","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19268,"web_url":"https://patchwork.libcamera.org/comment/19268/","msgid":"","date":"2021-09-02T07:40:39","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Sep 02, 2021 at 08:59:34AM +0200, Jacopo Mondi wrote:\n> On Thu, Sep 02, 2021 at 06:11:40AM +0300, Laurent Pinchart wrote:\n> > On Wed, Sep 01, 2021 at 05:47:08PM +0200, Jacopo Mondi wrote:\n> > > On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> > > > The problem is happening because we seem to add a CameraStream\n> > > > associated buffer(depending on the CameraStream::Type) to the Request,\n> > > > in CameraDevice::processCaptureRequest().\n> > > >\n> > > > However, when the camera stops, all the current buffers are marked with\n> > > > FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> > > > associated with the CameraStream (that was previously added to the\n> > > > request) has now been cleared out with a part of streams_.clear(), even\n> > > > before the camera stop() has been invoked. Any access to those request\n> > > > buffers after they have been cleared, shall result in a crash.\n> > >\n> > > I recall it was painful at the time when we had to deal with flush()\n> > > to get the right ordering, and I had done the same. However, since\n> > > stop() is only called by configureStreams() I removed the\n> > > streams_.clear() there and had issues, so I gave up.\n> >\n> > Do you mean you have tried removing streams_.clear() from\n> > configureStreams() while testing this patch and ran into issues, or that\n> > you tried that when implementing support for flush() ?\n> \n> I meant when developing flush().\n> I haven't tried to do the same on top of this patch, but it might be\n> good to do so ?\n\nI think so, because if it works, it's a good cleanup (possibly as part\nof this patch actually, not on top), and if it doesn't, there's\nsomething fishy :-) Would you be able to test it ?\n\n> > > But I have this patch gone through cros_camera_test and also CTS seems\n> > > fine. Also looking at the code it seems an harmless change so I wonder\n> > > what was wrong at the time.\n> > >\n> > > As far as I can tell:\n> > >\n> > > Tested-by: Jacopo Mondi \n> > > Acked-by: Jacopo Mondi \n> > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/camera_device.cpp | 4 ++--\n> > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 8ca76719..fda77db4 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> > > >\n> > > > void CameraDevice::close()\n> > > > {\n> > > > -\tstreams_.clear();\n> > > > -\n> > > > \tstop();\n> > > >\n> > > > \tcamera_->release();\n> > > > @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> > > > \tcamera_->stop();\n> > > >\n> > > > \tdescriptors_.clear();\n> > > > +\tstreams_.clear();\n> > > > +\n> > > > \tstate_ = State::Stopped;\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 8FFFEBDC71\n\tfor ;\n\tThu, 2 Sep 2021 07:40:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E21E46916A;\n\tThu, 2 Sep 2021 09:40:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2591660254\n\tfor ;\n\tThu, 2 Sep 2021 09:40:57 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8863145E;\n\tThu, 2 Sep 2021 09:40:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fXtGvYQD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630568456;\n\tbh=ADOIRzV+p5CzFrIPLT40n7B0Op4ijq3iCfM2ynxDd1A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fXtGvYQDXAGrcr5x/CFzcyH4/EMhTRGBfLvBY8xVPE1Kx/SoAEQPBnGNwdGu+v31g\n\tcAqyWId6Cm7nv9j8XDGXoRzcGh8OlCk7ibf7IUcSnXLwInZ85E3eTgHTGjtp0Bbf/u\n\tz9Vmilw6M4Y5D7WKkqaEhdmio6vJ80ML9scaKfTo=","Date":"Thu, 2 Sep 2021 10:40:39 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>\n\t\n\t<20210902065934.iuso7ylvynkzyhse@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210902065934.iuso7ylvynkzyhse@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19270,"web_url":"https://patchwork.libcamera.org/comment/19270/","msgid":"<20210902082559.hhrzbg6kpzljgctb@uno.localdomain>","date":"2021-09-02T08:25:59","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Thu, Sep 02, 2021 at 10:40:39AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Thu, Sep 02, 2021 at 08:59:34AM +0200, Jacopo Mondi wrote:\n> > On Thu, Sep 02, 2021 at 06:11:40AM +0300, Laurent Pinchart wrote:\n> > > On Wed, Sep 01, 2021 at 05:47:08PM +0200, Jacopo Mondi wrote:\n> > > > On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> > > > > The problem is happening because we seem to add a CameraStream\n> > > > > associated buffer(depending on the CameraStream::Type) to the Request,\n> > > > > in CameraDevice::processCaptureRequest().\n> > > > >\n> > > > > However, when the camera stops, all the current buffers are marked with\n> > > > > FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> > > > > associated with the CameraStream (that was previously added to the\n> > > > > request) has now been cleared out with a part of streams_.clear(), even\n> > > > > before the camera stop() has been invoked. Any access to those request\n> > > > > buffers after they have been cleared, shall result in a crash.\n> > > >\n> > > > I recall it was painful at the time when we had to deal with flush()\n> > > > to get the right ordering, and I had done the same. However, since\n> > > > stop() is only called by configureStreams() I removed the\n> > > > streams_.clear() there and had issues, so I gave up.\n> > >\n> > > Do you mean you have tried removing streams_.clear() from\n> > > configureStreams() while testing this patch and ran into issues, or that\n> > > you tried that when implementing support for flush() ?\n> >\n> > I meant when developing flush().\n> > I haven't tried to do the same on top of this patch, but it might be\n> > good to do so ?\n>\n> I think so, because if it works, it's a good cleanup (possibly as part\n> of this patch actually, not on top), and if it doesn't, there's\n> something fishy :-) Would you be able to test it ?\n>\n\nWith this applied\n\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -544,6 +544,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n LOG(HAL, Error) << \"No streams in configuration\";\n return -EINVAL;\n }\n+ streams_.reserve(stream_list->num_streams);\n\n #if defined(OS_CHROMEOS)\n if (!validateCropRotate(*stream_list))\n@@ -560,14 +561,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n return -EINVAL;\n }\n\n- /*\n- * Clear and remove any existing configuration from previous calls, and\n- * ensure the required entries are available without further\n- * reallocation.\n- */\n- streams_.clear();\n- streams_.reserve(stream_list->num_streams);\n-\n std::vector streamConfigs;\n streamConfigs.reserve(stream_list->num_streams);\n\n I can easily get a segfault running CTS.\n I analyzed the coredump and the stack trace doesn't help much\n\n(soraka-libcamera-gdb) bt\n#0 0x000000043983e000 in ?? ()\n#1 0x00007eee396ee5b5 in base::OnceCallback::Run() && (this=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/callback.h:102\n#2 base::TaskAnnotator::RunTask (this=, trace_event_name=, pending_task=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/common/task_annotator.cc:163\n#3 0x00007eee396ff465 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl (this=this@entry=0x7eee2c035ed0, continuation_lazy_now=continuation_lazy_now@entry=0x7eee34bd0910)\n at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:332\n#4 0x00007eee396ff27c in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork (this=0x7eee2c035ed0)\n at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:252\n#5 0x00007eee396a322d in base::MessagePumpDefault::Run (this=0x7eee1c00cc70, delegate=) at ../../../libchrome-0.0.1/platform2/libchrome/base/message_loop/message_pump_default.cc:39\n#6 0x00007eee396ff730 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run (this=, application_tasks_allowed=, timeout=...)\n at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:446\n#7 0x00007eee396d0325 in base::RunLoop::Run (this=0x7eee34bd0aa0) at ../../../libchrome-0.0.1/platform2/libchrome/base/run_loop.cc:124\n#8 0x00007eee3971d04e in base::Thread::ThreadMain (this=0x7eee2c034090) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:382\n#9 0x00007eee39718f3d in base::(anonymous namespace)::ThreadFunc (params=0x7eee2c033c50) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/platform_thread_posix.cc:87\n#10 0x00007eee395adfbf in start_thread (arg=0x7eee34bd1640) at pthread_create.c:463\n#11 0x00007eee393f735f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95\n(soraka-libcamera-gdb) quit\n\nThe last ?? in #0 makes me think I might not have symbols for the\ncamera service, which I cannot easily recompile due to the cros-debug\nUSE flag issue. I need to rebuild all the packages and I've now kicked\nit.\n\nOne day I'll learn to shut up and ack patches without asking for\nreasons to fall into rabbit holes like this one.\n\n\n> > > > But I have this patch gone through cros_camera_test and also CTS seems\n> > > > fine. Also looking at the code it seems an harmless change so I wonder\n> > > > what was wrong at the time.\n> > > >\n> > > > As far as I can tell:\n> > > >\n> > > > Tested-by: Jacopo Mondi \n> > > > Acked-by: Jacopo Mondi \n> > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 4 ++--\n> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index 8ca76719..fda77db4 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> > > > >\n> > > > > void CameraDevice::close()\n> > > > > {\n> > > > > -\tstreams_.clear();\n> > > > > -\n> > > > > \tstop();\n> > > > >\n> > > > > \tcamera_->release();\n> > > > > @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> > > > > \tcamera_->stop();\n> > > > >\n> > > > > \tdescriptors_.clear();\n> > > > > +\tstreams_.clear();\n> > > > > +\n> > > > > \tstate_ = State::Stopped;\n> > > > > }\n> > > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E8861BDC71\n\tfor ;\n\tThu, 2 Sep 2021 08:25:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6013769166;\n\tThu, 2 Sep 2021 10:25:12 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0A2160254\n\tfor ;\n\tThu, 2 Sep 2021 10:25:10 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 257F0100007;\n\tThu, 2 Sep 2021 08:25:09 +0000 (UTC)"],"Date":"Thu, 2 Sep 2021 10:25:59 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210902082559.hhrzbg6kpzljgctb@uno.localdomain>","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>\n\t\n\t<20210902065934.iuso7ylvynkzyhse@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19274,"web_url":"https://patchwork.libcamera.org/comment/19274/","msgid":"","date":"2021-09-02T08:51:02","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Sep 02, 2021 at 10:25:59AM +0200, Jacopo Mondi wrote:\n> On Thu, Sep 02, 2021 at 10:40:39AM +0300, Laurent Pinchart wrote:\n> > On Thu, Sep 02, 2021 at 08:59:34AM +0200, Jacopo Mondi wrote:\n> > > On Thu, Sep 02, 2021 at 06:11:40AM +0300, Laurent Pinchart wrote:\n> > > > On Wed, Sep 01, 2021 at 05:47:08PM +0200, Jacopo Mondi wrote:\n> > > > > On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> > > > > > The problem is happening because we seem to add a CameraStream\n> > > > > > associated buffer(depending on the CameraStream::Type) to the Request,\n> > > > > > in CameraDevice::processCaptureRequest().\n> > > > > >\n> > > > > > However, when the camera stops, all the current buffers are marked with\n> > > > > > FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> > > > > > associated with the CameraStream (that was previously added to the\n> > > > > > request) has now been cleared out with a part of streams_.clear(), even\n> > > > > > before the camera stop() has been invoked. Any access to those request\n> > > > > > buffers after they have been cleared, shall result in a crash.\n> > > > >\n> > > > > I recall it was painful at the time when we had to deal with flush()\n> > > > > to get the right ordering, and I had done the same. However, since\n> > > > > stop() is only called by configureStreams() I removed the\n> > > > > streams_.clear() there and had issues, so I gave up.\n> > > >\n> > > > Do you mean you have tried removing streams_.clear() from\n> > > > configureStreams() while testing this patch and ran into issues, or that\n> > > > you tried that when implementing support for flush() ?\n> > >\n> > > I meant when developing flush().\n> > > I haven't tried to do the same on top of this patch, but it might be\n> > > good to do so ?\n> >\n> > I think so, because if it works, it's a good cleanup (possibly as part\n> > of this patch actually, not on top), and if it doesn't, there's\n> > something fishy :-) Would you be able to test it ?\n> \n> With this applied\n> \n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -544,6 +544,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> LOG(HAL, Error) << \"No streams in configuration\";\n> return -EINVAL;\n> }\n> + streams_.reserve(stream_list->num_streams);\n> \n> #if defined(OS_CHROMEOS)\n> if (!validateCropRotate(*stream_list))\n> @@ -560,14 +561,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> return -EINVAL;\n> }\n> \n> - /*\n> - * Clear and remove any existing configuration from previous calls, and\n> - * ensure the required entries are available without further\n> - * reallocation.\n> - */\n> - streams_.clear();\n> - streams_.reserve(stream_list->num_streams);\n> -\n> std::vector streamConfigs;\n> streamConfigs.reserve(stream_list->num_streams);\n> \n> I can easily get a segfault running CTS.\n> I analyzed the coredump and the stack trace doesn't help much\n> \n> (soraka-libcamera-gdb) bt\n> #0 0x000000043983e000 in ?? ()\n> #1 0x00007eee396ee5b5 in base::OnceCallback::Run() && (this=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/callback.h:102\n> #2 base::TaskAnnotator::RunTask (this=, trace_event_name=, pending_task=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/common/task_annotator.cc:163\n> #3 0x00007eee396ff465 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl (this=this@entry=0x7eee2c035ed0, continuation_lazy_now=continuation_lazy_now@entry=0x7eee34bd0910)\n> at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:332\n> #4 0x00007eee396ff27c in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork (this=0x7eee2c035ed0)\n> at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:252\n> #5 0x00007eee396a322d in base::MessagePumpDefault::Run (this=0x7eee1c00cc70, delegate=) at ../../../libchrome-0.0.1/platform2/libchrome/base/message_loop/message_pump_default.cc:39\n> #6 0x00007eee396ff730 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run (this=, application_tasks_allowed=, timeout=...)\n> at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:446\n> #7 0x00007eee396d0325 in base::RunLoop::Run (this=0x7eee34bd0aa0) at ../../../libchrome-0.0.1/platform2/libchrome/base/run_loop.cc:124\n> #8 0x00007eee3971d04e in base::Thread::ThreadMain (this=0x7eee2c034090) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:382\n> #9 0x00007eee39718f3d in base::(anonymous namespace)::ThreadFunc (params=0x7eee2c033c50) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/platform_thread_posix.cc:87\n> #10 0x00007eee395adfbf in start_thread (arg=0x7eee34bd1640) at pthread_create.c:463\n> #11 0x00007eee393f735f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95\n> (soraka-libcamera-gdb) quit\n> \n> The last ?? in #0 makes me think I might not have symbols for the\n> camera service, which I cannot easily recompile due to the cros-debug\n> USE flag issue. I need to rebuild all the packages and I've now kicked\n> it.\n> \n> One day I'll learn to shut up and ack patches without asking for\n> reasons to fall into rabbit holes like this one.\n\nThanks for the quick test.\n\nHiro, is this something you can look at ?\n\n> > > > > But I have this patch gone through cros_camera_test and also CTS seems\n> > > > > fine. Also looking at the code it seems an harmless change so I wonder\n> > > > > what was wrong at the time.\n> > > > >\n> > > > > As far as I can tell:\n> > > > >\n> > > > > Tested-by: Jacopo Mondi \n> > > > > Acked-by: Jacopo Mondi \n> > > > >\n> > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > ---\n> > > > > > src/android/camera_device.cpp | 4 ++--\n> > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > index 8ca76719..fda77db4 100644\n> > > > > > --- a/src/android/camera_device.cpp\n> > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> > > > > >\n> > > > > > void CameraDevice::close()\n> > > > > > {\n> > > > > > -\tstreams_.clear();\n> > > > > > -\n> > > > > > \tstop();\n> > > > > >\n> > > > > > \tcamera_->release();\n> > > > > > @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> > > > > > \tcamera_->stop();\n> > > > > >\n> > > > > > \tdescriptors_.clear();\n> > > > > > +\tstreams_.clear();\n> > > > > > +\n> > > > > > \tstate_ = State::Stopped;\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 201FBBD87D\n\tfor ;\n\tThu, 2 Sep 2021 08:51:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D815B6916D;\n\tThu, 2 Sep 2021 10:51:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16A8760254\n\tfor ;\n\tThu, 2 Sep 2021 10:51:19 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8FD36532;\n\tThu, 2 Sep 2021 10:51:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ssEq8Ur5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630572678;\n\tbh=Hg0sq04naRs004yEHrWdS8f3YGMpyrNO9foeo19OzG8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ssEq8Ur5RlRyoh/tFvz/t7SEiewwaNlr10TkUPNqMf8W0wS/aqlCEh63TeytvI2WG\n\t2VjnJgSmtPK2Mw4AvfQELI8bzm8GQj8uYUoDOsDB8cxSqtO/dTjxMSYqk18biqbdzH\n\tCHSl1Hu60YELnEn6HgSJeI8nrRHSlWKGbMgcZRys=","Date":"Thu, 2 Sep 2021 11:51:02 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>\n\t\n\t<20210902065934.iuso7ylvynkzyhse@uno.localdomain>\n\t\n\t<20210902082559.hhrzbg6kpzljgctb@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210902082559.hhrzbg6kpzljgctb@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19279,"web_url":"https://patchwork.libcamera.org/comment/19279/","msgid":"<20210902095631.mywctgus4utkbcz5@uno.localdomain>","date":"2021-09-02T09:56:31","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Thu, Sep 02, 2021 at 11:51:02AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Thu, Sep 02, 2021 at 10:25:59AM +0200, Jacopo Mondi wrote:\n> > On Thu, Sep 02, 2021 at 10:40:39AM +0300, Laurent Pinchart wrote:\n> > > On Thu, Sep 02, 2021 at 08:59:34AM +0200, Jacopo Mondi wrote:\n> > > > On Thu, Sep 02, 2021 at 06:11:40AM +0300, Laurent Pinchart wrote:\n> > > > > On Wed, Sep 01, 2021 at 05:47:08PM +0200, Jacopo Mondi wrote:\n> > > > > > On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> > > > > > > The problem is happening because we seem to add a CameraStream\n> > > > > > > associated buffer(depending on the CameraStream::Type) to the Request,\n> > > > > > > in CameraDevice::processCaptureRequest().\n> > > > > > >\n> > > > > > > However, when the camera stops, all the current buffers are marked with\n> > > > > > > FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> > > > > > > associated with the CameraStream (that was previously added to the\n> > > > > > > request) has now been cleared out with a part of streams_.clear(), even\n> > > > > > > before the camera stop() has been invoked. Any access to those request\n> > > > > > > buffers after they have been cleared, shall result in a crash.\n> > > > > >\n> > > > > > I recall it was painful at the time when we had to deal with flush()\n> > > > > > to get the right ordering, and I had done the same. However, since\n> > > > > > stop() is only called by configureStreams() I removed the\n> > > > > > streams_.clear() there and had issues, so I gave up.\n> > > > >\n> > > > > Do you mean you have tried removing streams_.clear() from\n> > > > > configureStreams() while testing this patch and ran into issues, or that\n> > > > > you tried that when implementing support for flush() ?\n> > > >\n> > > > I meant when developing flush().\n> > > > I haven't tried to do the same on top of this patch, but it might be\n> > > > good to do so ?\n> > >\n> > > I think so, because if it works, it's a good cleanup (possibly as part\n> > > of this patch actually, not on top), and if it doesn't, there's\n> > > something fishy :-) Would you be able to test it ?\n> >\n> > With this applied\n> >\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -544,6 +544,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > LOG(HAL, Error) << \"No streams in configuration\";\n> > return -EINVAL;\n> > }\n> > + streams_.reserve(stream_list->num_streams);\n> >\n> > #if defined(OS_CHROMEOS)\n> > if (!validateCropRotate(*stream_list))\n> > @@ -560,14 +561,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > return -EINVAL;\n> > }\n> >\n> > - /*\n> > - * Clear and remove any existing configuration from previous calls, and\n> > - * ensure the required entries are available without further\n> > - * reallocation.\n> > - */\n> > - streams_.clear();\n> > - streams_.reserve(stream_list->num_streams);\n> > -\n> > std::vector streamConfigs;\n> > streamConfigs.reserve(stream_list->num_streams);\n> >\n> > I can easily get a segfault running CTS.\n> > I analyzed the coredump and the stack trace doesn't help much\n> >\n> > (soraka-libcamera-gdb) bt\n> > #0 0x000000043983e000 in ?? ()\n> > #1 0x00007eee396ee5b5 in base::OnceCallback::Run() && (this=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/callback.h:102\n> > #2 base::TaskAnnotator::RunTask (this=, trace_event_name=, pending_task=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/common/task_annotator.cc:163\n> > #3 0x00007eee396ff465 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl (this=this@entry=0x7eee2c035ed0, continuation_lazy_now=continuation_lazy_now@entry=0x7eee34bd0910)\n> > at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:332\n> > #4 0x00007eee396ff27c in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork (this=0x7eee2c035ed0)\n> > at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:252\n> > #5 0x00007eee396a322d in base::MessagePumpDefault::Run (this=0x7eee1c00cc70, delegate=) at ../../../libchrome-0.0.1/platform2/libchrome/base/message_loop/message_pump_default.cc:39\n> > #6 0x00007eee396ff730 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run (this=, application_tasks_allowed=, timeout=...)\n> > at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:446\n> > #7 0x00007eee396d0325 in base::RunLoop::Run (this=0x7eee34bd0aa0) at ../../../libchrome-0.0.1/platform2/libchrome/base/run_loop.cc:124\n> > #8 0x00007eee3971d04e in base::Thread::ThreadMain (this=0x7eee2c034090) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:382\n> > #9 0x00007eee39718f3d in base::(anonymous namespace)::ThreadFunc (params=0x7eee2c033c50) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/platform_thread_posix.cc:87\n> > #10 0x00007eee395adfbf in start_thread (arg=0x7eee34bd1640) at pthread_create.c:463\n> > #11 0x00007eee393f735f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95\n> > (soraka-libcamera-gdb) quit\n> >\n> > The last ?? in #0 makes me think I might not have symbols for the\n> > camera service, which I cannot easily recompile due to the cros-debug\n> > USE flag issue. I need to rebuild all the packages and I've now kicked\n> > it.\n> >\n> > One day I'll learn to shut up and ack patches without asking for\n> > reasons to fall into rabbit holes like this one.\n>\n> Thanks for the quick test.\n\nI can confirm removing streams_.clear() from configureStreams() easily\ntriggers a segfault when running CTS. That's indeed the same issue I\nwas facing when developing flush().\n\nI really cannot figure out why it happens looking at the code. I got\na few more stack traces with debuga symbols in the camera service from the\ncoredumps but they do not seem that helpful at a first look.\n\n[Current thread is 1 (Thread 0x7c372ffff640 (LWP 8265))]\n(soraka-libcamera-gdb) bt\n#0 std::__1::__cxx_atomic_fetch_sub (__a=0x400000000, __delta=1, __order=std::__1::memory_order_acq_rel) at /usr/bin/../include/c++/v1/atomic:1072\n#1 std::__1::__atomic_base::fetch_sub (this=0x400000000, __op=1, __m=std::__1::memory_order_acq_rel) at /usr/bin/../include/c++/v1/atomic:1719\n#2 base::AtomicRefCount::Decrement (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/atomic_ref_count.h:39\n#3 base::subtle::RefCountedThreadSafeBase::ReleaseImpl (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/ref_counted.h:218\n#4 base::subtle::RefCountedThreadSafeBase::Release (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/ref_counted.h:170\n#5 base::RefCountedThreadSafe::Release (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/ref_counted.h:398\n#6 scoped_refptr::Release (ptr=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/scoped_refptr.h:322\n#7 scoped_refptr::~scoped_refptr (this=) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/scoped_refptr.h:224\n#8 base::internal::CallbackBase::~CallbackBase (this=) at ../../../libchrome-0.0.1/platform2/libchrome/base/callback_internal.cc:85\n#9 0x00007c37561eb82b in base::sequence_manager::internal::AtomicFlagSet::Group::~Group (this=0x7c373000ef80) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/atomic_flag_set.cc:132\n#10 std::__1::default_delete::operator() (this=, __ptr=0x7c373000ef80) at /usr/bin/../include/c++/v1/memory:1335\n#11 0x00007c37561eaede in base::sequence_manager::internal::AtomicFlagSet::RemoveFromAllocList (group=0x7c373000ef80, this=) at /usr/bin/../include/c++/v1/memory:1595\n#12 base::sequence_manager::internal::AtomicFlagSet::AtomicFlag::ReleaseAtomicFlag (this=0x7c3730013b90) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/atomic_flag_set.cc:77\n#13 0x00007c37561f3cec in base::sequence_manager::internal::TaskQueueImpl::UnregisterTaskQueue (this=0x7c37300139e0) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/task_queue_impl.cc:201\n#14 0x00007c37561ec1df in base::sequence_manager::internal::SequenceManagerImpl::~SequenceManagerImpl (this=0x7c37300008d0) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/sequence_manager_impl.cc:234\n#15 0x00007c37561ec52e in base::sequence_manager::internal::SequenceManagerImpl::~SequenceManagerImpl (this=0x7c37300008d0) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/sequence_manager_impl.cc:212\n#16 0x00007c3756218317 in std::__1::default_delete::operator() (this=0x7c3730014848, __ptr=0x400000000) at /usr/bin/../include/c++/v1/memory:1335\n#17 std::__1::unique_ptr >::reset (this=0x7c3730014848, __p=0x0)\n at /usr/bin/../include/c++/v1/memory:1596\n#18 std::__1::unique_ptr >::~unique_ptr (this=0x7c3730014848)\n at /usr/bin/../include/c++/v1/memory:1550\n#19 base::(anonymous namespace)::SequenceManagerThreadDelegate::~SequenceManagerThreadDelegate (this=0x7c3730014840) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:67\n#20 base::(anonymous namespace)::SequenceManagerThreadDelegate::~SequenceManagerThreadDelegate (this=0x7c3730014840) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:67\n#21 0x00007c3756218098 in std::__1::default_delete::operator() (this=0x7c3744039bc0, __ptr=0x400000000) at /usr/bin/../include/c++/v1/memory:1335\n#22 std::__1::unique_ptr >::reset (this=0x7c3744039bc0, __p=0x0) at /usr/bin/../include/c++/v1/memory:1596\n#23 base::Thread::ThreadMain (this=0x7c3744039b40) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:400\n#24 0x00007c3756213f3d in base::(anonymous namespace)::ThreadFunc (params=0x7c3730011840) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/platform_thread_posix.cc:87\n#25 0x00007c37560a8fbf in start_thread (arg=0x7c372ffff640) at pthread_create.c:463\n#26 0x00007c3755ef235f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95\n\n>\n> Hiro, is this something you can look at ?\n>\n> > > > > > But I have this patch gone through cros_camera_test and also CTS seems\n> > > > > > fine. Also looking at the code it seems an harmless change so I wonder\n> > > > > > what was wrong at the time.\n> > > > > >\n> > > > > > As far as I can tell:\n> > > > > >\n> > > > > > Tested-by: Jacopo Mondi \n> > > > > > Acked-by: Jacopo Mondi \n> > > > > >\n> > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > ---\n> > > > > > > src/android/camera_device.cpp | 4 ++--\n> > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > index 8ca76719..fda77db4 100644\n> > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> > > > > > >\n> > > > > > > void CameraDevice::close()\n> > > > > > > {\n> > > > > > > -\tstreams_.clear();\n> > > > > > > -\n> > > > > > > \tstop();\n> > > > > > >\n> > > > > > > \tcamera_->release();\n> > > > > > > @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> > > > > > > \tcamera_->stop();\n> > > > > > >\n> > > > > > > \tdescriptors_.clear();\n> > > > > > > +\tstreams_.clear();\n> > > > > > > +\n> > > > > > > \tstate_ = State::Stopped;\n> > > > > > > }\n> > > > > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6B59EBDC71\n\tfor ;\n\tThu, 2 Sep 2021 09:55:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4538E6916A;\n\tThu, 2 Sep 2021 11:55:45 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71C8B60254\n\tfor ;\n\tThu, 2 Sep 2021 11:55:43 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id C4F421BF20E;\n\tThu, 2 Sep 2021 09:55:42 +0000 (UTC)"],"Date":"Thu, 2 Sep 2021 11:56:31 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210902095631.mywctgus4utkbcz5@uno.localdomain>","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>\n\t\n\t<20210902065934.iuso7ylvynkzyhse@uno.localdomain>\n\t\n\t<20210902082559.hhrzbg6kpzljgctb@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19321,"web_url":"https://patchwork.libcamera.org/comment/19321/","msgid":"","date":"2021-09-03T08:26:41","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, Sep 2, 2021 at 6:55 PM Jacopo Mondi wrote:\n>\n> Hello,\n>\n> On Thu, Sep 02, 2021 at 11:51:02AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > On Thu, Sep 02, 2021 at 10:25:59AM +0200, Jacopo Mondi wrote:\n> > > On Thu, Sep 02, 2021 at 10:40:39AM +0300, Laurent Pinchart wrote:\n> > > > On Thu, Sep 02, 2021 at 08:59:34AM +0200, Jacopo Mondi wrote:\n> > > > > On Thu, Sep 02, 2021 at 06:11:40AM +0300, Laurent Pinchart wrote:\n> > > > > > On Wed, Sep 01, 2021 at 05:47:08PM +0200, Jacopo Mondi wrote:\n> > > > > > > On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> > > > > > > > The problem is happening because we seem to add a CameraStream\n> > > > > > > > associated buffer(depending on the CameraStream::Type) to the Request,\n> > > > > > > > in CameraDevice::processCaptureRequest().\n> > > > > > > >\n> > > > > > > > However, when the camera stops, all the current buffers are marked with\n> > > > > > > > FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> > > > > > > > associated with the CameraStream (that was previously added to the\n> > > > > > > > request) has now been cleared out with a part of streams_.clear(), even\n> > > > > > > > before the camera stop() has been invoked. Any access to those request\n> > > > > > > > buffers after they have been cleared, shall result in a crash.\n> > > > > > >\n> > > > > > > I recall it was painful at the time when we had to deal with flush()\n> > > > > > > to get the right ordering, and I had done the same. However, since\n> > > > > > > stop() is only called by configureStreams() I removed the\n> > > > > > > streams_.clear() there and had issues, so I gave up.\n> > > > > >\n> > > > > > Do you mean you have tried removing streams_.clear() from\n> > > > > > configureStreams() while testing this patch and ran into issues, or that\n> > > > > > you tried that when implementing support for flush() ?\n> > > > >\n> > > > > I meant when developing flush().\n> > > > > I haven't tried to do the same on top of this patch, but it might be\n> > > > > good to do so ?\n> > > >\n> > > > I think so, because if it works, it's a good cleanup (possibly as part\n> > > > of this patch actually, not on top), and if it doesn't, there's\n> > > > something fishy :-) Would you be able to test it ?\n> > >\n> > > With this applied\n> > >\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -544,6 +544,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > LOG(HAL, Error) << \"No streams in configuration\";\n> > > return -EINVAL;\n> > > }\n> > > + streams_.reserve(stream_list->num_streams);\n> > >\n> > > #if defined(OS_CHROMEOS)\n> > > if (!validateCropRotate(*stream_list))\n> > > @@ -560,14 +561,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > return -EINVAL;\n> > > }\n> > >\n> > > - /*\n> > > - * Clear and remove any existing configuration from previous calls, and\n> > > - * ensure the required entries are available without further\n> > > - * reallocation.\n> > > - */\n> > > - streams_.clear();\n> > > - streams_.reserve(stream_list->num_streams);\n> > > -\n> > > std::vector streamConfigs;\n> > > streamConfigs.reserve(stream_list->num_streams);\n> > >\n> > > I can easily get a segfault running CTS.\n> > > I analyzed the coredump and the stack trace doesn't help much\n> > >\n> > > (soraka-libcamera-gdb) bt\n> > > #0 0x000000043983e000 in ?? ()\n> > > #1 0x00007eee396ee5b5 in base::OnceCallback::Run() && (this=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/callback.h:102\n> > > #2 base::TaskAnnotator::RunTask (this=, trace_event_name=, pending_task=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/common/task_annotator.cc:163\n> > > #3 0x00007eee396ff465 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl (this=this@entry=0x7eee2c035ed0, continuation_lazy_now=continuation_lazy_now@entry=0x7eee34bd0910)\n> > > at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:332\n> > > #4 0x00007eee396ff27c in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork (this=0x7eee2c035ed0)\n> > > at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:252\n> > > #5 0x00007eee396a322d in base::MessagePumpDefault::Run (this=0x7eee1c00cc70, delegate=) at ../../../libchrome-0.0.1/platform2/libchrome/base/message_loop/message_pump_default.cc:39\n> > > #6 0x00007eee396ff730 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run (this=, application_tasks_allowed=, timeout=...)\n> > > at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:446\n> > > #7 0x00007eee396d0325 in base::RunLoop::Run (this=0x7eee34bd0aa0) at ../../../libchrome-0.0.1/platform2/libchrome/base/run_loop.cc:124\n> > > #8 0x00007eee3971d04e in base::Thread::ThreadMain (this=0x7eee2c034090) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:382\n> > > #9 0x00007eee39718f3d in base::(anonymous namespace)::ThreadFunc (params=0x7eee2c033c50) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/platform_thread_posix.cc:87\n> > > #10 0x00007eee395adfbf in start_thread (arg=0x7eee34bd1640) at pthread_create.c:463\n> > > #11 0x00007eee393f735f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95\n> > > (soraka-libcamera-gdb) quit\n> > >\n> > > The last ?? in #0 makes me think I might not have symbols for the\n> > > camera service, which I cannot easily recompile due to the cros-debug\n> > > USE flag issue. I need to rebuild all the packages and I've now kicked\n> > > it.\n> > >\n> > > One day I'll learn to shut up and ack patches without asking for\n> > > reasons to fall into rabbit holes like this one.\n> >\n> > Thanks for the quick test.\n>\n> I can confirm removing streams_.clear() from configureStreams() easily\n> triggers a segfault when running CTS. That's indeed the same issue I\n> was facing when developing flush().\n>\n> I really cannot figure out why it happens looking at the code. I got\n> a few more stack traces with debuga symbols in the camera service from the\n> coredumps but they do not seem that helpful at a first look.\n>\n> [Current thread is 1 (Thread 0x7c372ffff640 (LWP 8265))]\n> (soraka-libcamera-gdb) bt\n> #0 std::__1::__cxx_atomic_fetch_sub (__a=0x400000000, __delta=1, __order=std::__1::memory_order_acq_rel) at /usr/bin/../include/c++/v1/atomic:1072\n> #1 std::__1::__atomic_base::fetch_sub (this=0x400000000, __op=1, __m=std::__1::memory_order_acq_rel) at /usr/bin/../include/c++/v1/atomic:1719\n> #2 base::AtomicRefCount::Decrement (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/atomic_ref_count.h:39\n> #3 base::subtle::RefCountedThreadSafeBase::ReleaseImpl (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/ref_counted.h:218\n> #4 base::subtle::RefCountedThreadSafeBase::Release (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/ref_counted.h:170\n> #5 base::RefCountedThreadSafe::Release (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/ref_counted.h:398\n> #6 scoped_refptr::Release (ptr=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/scoped_refptr.h:322\n> #7 scoped_refptr::~scoped_refptr (this=) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/scoped_refptr.h:224\n> #8 base::internal::CallbackBase::~CallbackBase (this=) at ../../../libchrome-0.0.1/platform2/libchrome/base/callback_internal.cc:85\n> #9 0x00007c37561eb82b in base::sequence_manager::internal::AtomicFlagSet::Group::~Group (this=0x7c373000ef80) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/atomic_flag_set.cc:132\n> #10 std::__1::default_delete::operator() (this=, __ptr=0x7c373000ef80) at /usr/bin/../include/c++/v1/memory:1335\n> #11 0x00007c37561eaede in base::sequence_manager::internal::AtomicFlagSet::RemoveFromAllocList (group=0x7c373000ef80, this=) at /usr/bin/../include/c++/v1/memory:1595\n> #12 base::sequence_manager::internal::AtomicFlagSet::AtomicFlag::ReleaseAtomicFlag (this=0x7c3730013b90) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/atomic_flag_set.cc:77\n> #13 0x00007c37561f3cec in base::sequence_manager::internal::TaskQueueImpl::UnregisterTaskQueue (this=0x7c37300139e0) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/task_queue_impl.cc:201\n> #14 0x00007c37561ec1df in base::sequence_manager::internal::SequenceManagerImpl::~SequenceManagerImpl (this=0x7c37300008d0) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/sequence_manager_impl.cc:234\n> #15 0x00007c37561ec52e in base::sequence_manager::internal::SequenceManagerImpl::~SequenceManagerImpl (this=0x7c37300008d0) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/sequence_manager_impl.cc:212\n> #16 0x00007c3756218317 in std::__1::default_delete::operator() (this=0x7c3730014848, __ptr=0x400000000) at /usr/bin/../include/c++/v1/memory:1335\n> #17 std::__1::unique_ptr >::reset (this=0x7c3730014848, __p=0x0)\n> at /usr/bin/../include/c++/v1/memory:1596\n> #18 std::__1::unique_ptr >::~unique_ptr (this=0x7c3730014848)\n> at /usr/bin/../include/c++/v1/memory:1550\n> #19 base::(anonymous namespace)::SequenceManagerThreadDelegate::~SequenceManagerThreadDelegate (this=0x7c3730014840) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:67\n> #20 base::(anonymous namespace)::SequenceManagerThreadDelegate::~SequenceManagerThreadDelegate (this=0x7c3730014840) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:67\n> #21 0x00007c3756218098 in std::__1::default_delete::operator() (this=0x7c3744039bc0, __ptr=0x400000000) at /usr/bin/../include/c++/v1/memory:1335\n> #22 std::__1::unique_ptr >::reset (this=0x7c3744039bc0, __p=0x0) at /usr/bin/../include/c++/v1/memory:1596\n> #23 base::Thread::ThreadMain (this=0x7c3744039b40) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:400\n> #24 0x00007c3756213f3d in base::(anonymous namespace)::ThreadFunc (params=0x7c3730011840) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/platform_thread_posix.cc:87\n> #25 0x00007c37560a8fbf in start_thread (arg=0x7c372ffff640) at pthread_create.c:463\n> #26 0x00007c3755ef235f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95\n>\n> >\n> > Hiro, is this something you can look at ?\n\nI guess this is because streams_ is not cleared when state_ is Stopped.\nCould you try again by modifying as follows?\n\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -448,8 +448,10 @@ void CameraDevice::flush()\n void CameraDevice::stop()\n {\n MutexLocker stateLock(stateMutex_);\n- if (state_ == State::Stopped)\n+ if (state_ == State::Stopped) {\n+ streams_.clear();\n return;\n+ }\n\nThanks,\n-Hiro\n> >\n> > > > > > > But I have this patch gone through cros_camera_test and also CTS seems\n> > > > > > > fine. Also looking at the code it seems an harmless change so I wonder\n> > > > > > > what was wrong at the time.\n> > > > > > >\n> > > > > > > As far as I can tell:\n> > > > > > >\n> > > > > > > Tested-by: Jacopo Mondi \n> > > > > > > Acked-by: Jacopo Mondi \n> > > > > > >\n> > > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > > ---\n> > > > > > > > src/android/camera_device.cpp | 4 ++--\n> > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > > > >\n> > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > > index 8ca76719..fda77db4 100644\n> > > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> > > > > > > >\n> > > > > > > > void CameraDevice::close()\n> > > > > > > > {\n> > > > > > > > - streams_.clear();\n> > > > > > > > -\n> > > > > > > > stop();\n> > > > > > > >\n> > > > > > > > camera_->release();\n> > > > > > > > @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> > > > > > > > camera_->stop();\n> > > > > > > >\n> > > > > > > > descriptors_.clear();\n> > > > > > > > + streams_.clear();\n> > > > > > > > +\n> > > > > > > > state_ = State::Stopped;\n> > > > > > > > }\n> > > > > > > >\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9BC39BDC71\n\tfor ;\n\tFri, 3 Sep 2021 08:26:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 557C56916A;\n\tFri, 3 Sep 2021 10:26:54 +0200 (CEST)","from mail-ej1-x634.google.com (mail-ej1-x634.google.com\n\t[IPv6:2a00:1450:4864:20::634])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E7EBA60251\n\tfor ;\n\tFri, 3 Sep 2021 10:26:52 +0200 (CEST)","by mail-ej1-x634.google.com with SMTP id e21so10433670ejz.12\n\tfor ;\n\tFri, 03 Sep 2021 01:26:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"WoG0ZZLm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=miKwjnWssvSfk1wH6iuKtxzVSCeGFxoOsMPf+UtnrFE=;\n\tb=WoG0ZZLmKCJNcshnIaSiIQhbNE0vWvVpHulH5J09n7m17XLOQ2sD39OcRYYTPw3p3f\n\ta+/BL3WDn86+qTqWvS+tjtNGhB8oCNNAElmJV5dYznk/v5zk3RocjOGpfXHKCu5IMTJW\n\tQ5qAKR+CWqBQ6m9ToPsLx7WqerwVkZIGSLDKE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=miKwjnWssvSfk1wH6iuKtxzVSCeGFxoOsMPf+UtnrFE=;\n\tb=UNkfHmno/swQs1BB2R7hAYv3zcic+hYd+vAA16HykIINgmq8PlQrcHtu5MteoH8Kjf\n\ttL/TWfOJamY65u5uIlUAWsYjqEPKHld53wzwli+Bb4/rWqjpeSDIIobNFka21RSQtr3H\n\twf2Wz+rGDcNH7M3fx5JzcwPi14cEM7k4gpkhRV6ZSkOrv769L7oDmC0PKyQIhXq/Jn6Z\n\tpTWAe0fPAbJrkLNELtAVrl5CrtNGAvDwp5Aao5MdgN8p9bOOhHUESsgCkUL5Hx2ZOnjq\n\tdA082Ri/Ycqs/B2yRcLnJ0HckEDleYb4mZ/MNboumx5B4NRcZkaX2NzpfhlqJ2hA+MqB\n\tbJBQ==","X-Gm-Message-State":"AOAM533HBskHdDEYajul2bQ4eR+KdKj5A2SivojXSBtmPpSw0yoFHJ25\n\tQ0qoeAb595n5cQfE0WdrDY+3hmIjLlueLS5soyNJfdk+cmcr/g==","X-Google-Smtp-Source":"ABdhPJwwOUDhPOJO33+jBcngiC69bnunSxn05P84tuVa/Zq2UcPX/pXgPcmKXbN6XePlMD78dxSUVd/uYcVaVbU4Ygs=","X-Received":"by 2002:a17:906:e104:: with SMTP id\n\tgj4mr2825936ejb.306.1630657612418; \n\tFri, 03 Sep 2021 01:26:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>\n\t\n\t<20210902065934.iuso7ylvynkzyhse@uno.localdomain>\n\t\n\t<20210902082559.hhrzbg6kpzljgctb@uno.localdomain>\n\t\n\t<20210902095631.mywctgus4utkbcz5@uno.localdomain>","In-Reply-To":"<20210902095631.mywctgus4utkbcz5@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 3 Sep 2021 17:26:41 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19382,"web_url":"https://patchwork.libcamera.org/comment/19382/","msgid":"<20210903170625.csuv2ps3utqxufzs@uno.localdomain>","date":"2021-09-03T17:06:25","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Fri, Sep 03, 2021 at 05:26:41PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo,\n>\n> On Thu, Sep 2, 2021 at 6:55 PM Jacopo Mondi wrote:\n> >\n> > Hello,\n> >\n> > On Thu, Sep 02, 2021 at 11:51:02AM +0300, Laurent Pinchart wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Thu, Sep 02, 2021 at 10:25:59AM +0200, Jacopo Mondi wrote:\n> > > > On Thu, Sep 02, 2021 at 10:40:39AM +0300, Laurent Pinchart wrote:\n> > > > > On Thu, Sep 02, 2021 at 08:59:34AM +0200, Jacopo Mondi wrote:\n> > > > > > On Thu, Sep 02, 2021 at 06:11:40AM +0300, Laurent Pinchart wrote:\n> > > > > > > On Wed, Sep 01, 2021 at 05:47:08PM +0200, Jacopo Mondi wrote:\n> > > > > > > > On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> > > > > > > > > The problem is happening because we seem to add a CameraStream\n> > > > > > > > > associated buffer(depending on the CameraStream::Type) to the Request,\n> > > > > > > > > in CameraDevice::processCaptureRequest().\n> > > > > > > > >\n> > > > > > > > > However, when the camera stops, all the current buffers are marked with\n> > > > > > > > > FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> > > > > > > > > associated with the CameraStream (that was previously added to the\n> > > > > > > > > request) has now been cleared out with a part of streams_.clear(), even\n> > > > > > > > > before the camera stop() has been invoked. Any access to those request\n> > > > > > > > > buffers after they have been cleared, shall result in a crash.\n> > > > > > > >\n> > > > > > > > I recall it was painful at the time when we had to deal with flush()\n> > > > > > > > to get the right ordering, and I had done the same. However, since\n> > > > > > > > stop() is only called by configureStreams() I removed the\n> > > > > > > > streams_.clear() there and had issues, so I gave up.\n> > > > > > >\n> > > > > > > Do you mean you have tried removing streams_.clear() from\n> > > > > > > configureStreams() while testing this patch and ran into issues, or that\n> > > > > > > you tried that when implementing support for flush() ?\n> > > > > >\n> > > > > > I meant when developing flush().\n> > > > > > I haven't tried to do the same on top of this patch, but it might be\n> > > > > > good to do so ?\n> > > > >\n> > > > > I think so, because if it works, it's a good cleanup (possibly as part\n> > > > > of this patch actually, not on top), and if it doesn't, there's\n> > > > > something fishy :-) Would you be able to test it ?\n> > > >\n> > > > With this applied\n> > > >\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -544,6 +544,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > LOG(HAL, Error) << \"No streams in configuration\";\n> > > > return -EINVAL;\n> > > > }\n> > > > + streams_.reserve(stream_list->num_streams);\n> > > >\n> > > > #if defined(OS_CHROMEOS)\n> > > > if (!validateCropRotate(*stream_list))\n> > > > @@ -560,14 +561,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > return -EINVAL;\n> > > > }\n> > > >\n> > > > - /*\n> > > > - * Clear and remove any existing configuration from previous calls, and\n> > > > - * ensure the required entries are available without further\n> > > > - * reallocation.\n> > > > - */\n> > > > - streams_.clear();\n> > > > - streams_.reserve(stream_list->num_streams);\n> > > > -\n> > > > std::vector streamConfigs;\n> > > > streamConfigs.reserve(stream_list->num_streams);\n> > > >\n> > > > I can easily get a segfault running CTS.\n> > > > I analyzed the coredump and the stack trace doesn't help much\n> > > >\n> > > > (soraka-libcamera-gdb) bt\n> > > > #0 0x000000043983e000 in ?? ()\n> > > > #1 0x00007eee396ee5b5 in base::OnceCallback::Run() && (this=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/callback.h:102\n> > > > #2 base::TaskAnnotator::RunTask (this=, trace_event_name=, pending_task=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/common/task_annotator.cc:163\n> > > > #3 0x00007eee396ff465 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl (this=this@entry=0x7eee2c035ed0, continuation_lazy_now=continuation_lazy_now@entry=0x7eee34bd0910)\n> > > > at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:332\n> > > > #4 0x00007eee396ff27c in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork (this=0x7eee2c035ed0)\n> > > > at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:252\n> > > > #5 0x00007eee396a322d in base::MessagePumpDefault::Run (this=0x7eee1c00cc70, delegate=) at ../../../libchrome-0.0.1/platform2/libchrome/base/message_loop/message_pump_default.cc:39\n> > > > #6 0x00007eee396ff730 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run (this=, application_tasks_allowed=, timeout=...)\n> > > > at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:446\n> > > > #7 0x00007eee396d0325 in base::RunLoop::Run (this=0x7eee34bd0aa0) at ../../../libchrome-0.0.1/platform2/libchrome/base/run_loop.cc:124\n> > > > #8 0x00007eee3971d04e in base::Thread::ThreadMain (this=0x7eee2c034090) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:382\n> > > > #9 0x00007eee39718f3d in base::(anonymous namespace)::ThreadFunc (params=0x7eee2c033c50) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/platform_thread_posix.cc:87\n> > > > #10 0x00007eee395adfbf in start_thread (arg=0x7eee34bd1640) at pthread_create.c:463\n> > > > #11 0x00007eee393f735f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95\n> > > > (soraka-libcamera-gdb) quit\n> > > >\n> > > > The last ?? in #0 makes me think I might not have symbols for the\n> > > > camera service, which I cannot easily recompile due to the cros-debug\n> > > > USE flag issue. I need to rebuild all the packages and I've now kicked\n> > > > it.\n> > > >\n> > > > One day I'll learn to shut up and ack patches without asking for\n> > > > reasons to fall into rabbit holes like this one.\n> > >\n> > > Thanks for the quick test.\n> >\n> > I can confirm removing streams_.clear() from configureStreams() easily\n> > triggers a segfault when running CTS. That's indeed the same issue I\n> > was facing when developing flush().\n> >\n> > I really cannot figure out why it happens looking at the code. I got\n> > a few more stack traces with debuga symbols in the camera service from the\n> > coredumps but they do not seem that helpful at a first look.\n> >\n> > [Current thread is 1 (Thread 0x7c372ffff640 (LWP 8265))]\n> > (soraka-libcamera-gdb) bt\n> > #0 std::__1::__cxx_atomic_fetch_sub (__a=0x400000000, __delta=1, __order=std::__1::memory_order_acq_rel) at /usr/bin/../include/c++/v1/atomic:1072\n> > #1 std::__1::__atomic_base::fetch_sub (this=0x400000000, __op=1, __m=std::__1::memory_order_acq_rel) at /usr/bin/../include/c++/v1/atomic:1719\n> > #2 base::AtomicRefCount::Decrement (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/atomic_ref_count.h:39\n> > #3 base::subtle::RefCountedThreadSafeBase::ReleaseImpl (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/ref_counted.h:218\n> > #4 base::subtle::RefCountedThreadSafeBase::Release (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/ref_counted.h:170\n> > #5 base::RefCountedThreadSafe::Release (this=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/ref_counted.h:398\n> > #6 scoped_refptr::Release (ptr=0x400000000) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/scoped_refptr.h:322\n> > #7 scoped_refptr::~scoped_refptr (this=) at ../../../libchrome-0.0.1/platform2/libchrome/base/memory/scoped_refptr.h:224\n> > #8 base::internal::CallbackBase::~CallbackBase (this=) at ../../../libchrome-0.0.1/platform2/libchrome/base/callback_internal.cc:85\n> > #9 0x00007c37561eb82b in base::sequence_manager::internal::AtomicFlagSet::Group::~Group (this=0x7c373000ef80) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/atomic_flag_set.cc:132\n> > #10 std::__1::default_delete::operator() (this=, __ptr=0x7c373000ef80) at /usr/bin/../include/c++/v1/memory:1335\n> > #11 0x00007c37561eaede in base::sequence_manager::internal::AtomicFlagSet::RemoveFromAllocList (group=0x7c373000ef80, this=) at /usr/bin/../include/c++/v1/memory:1595\n> > #12 base::sequence_manager::internal::AtomicFlagSet::AtomicFlag::ReleaseAtomicFlag (this=0x7c3730013b90) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/atomic_flag_set.cc:77\n> > #13 0x00007c37561f3cec in base::sequence_manager::internal::TaskQueueImpl::UnregisterTaskQueue (this=0x7c37300139e0) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/task_queue_impl.cc:201\n> > #14 0x00007c37561ec1df in base::sequence_manager::internal::SequenceManagerImpl::~SequenceManagerImpl (this=0x7c37300008d0) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/sequence_manager_impl.cc:234\n> > #15 0x00007c37561ec52e in base::sequence_manager::internal::SequenceManagerImpl::~SequenceManagerImpl (this=0x7c37300008d0) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/sequence_manager_impl.cc:212\n> > #16 0x00007c3756218317 in std::__1::default_delete::operator() (this=0x7c3730014848, __ptr=0x400000000) at /usr/bin/../include/c++/v1/memory:1335\n> > #17 std::__1::unique_ptr >::reset (this=0x7c3730014848, __p=0x0)\n> > at /usr/bin/../include/c++/v1/memory:1596\n> > #18 std::__1::unique_ptr >::~unique_ptr (this=0x7c3730014848)\n> > at /usr/bin/../include/c++/v1/memory:1550\n> > #19 base::(anonymous namespace)::SequenceManagerThreadDelegate::~SequenceManagerThreadDelegate (this=0x7c3730014840) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:67\n> > #20 base::(anonymous namespace)::SequenceManagerThreadDelegate::~SequenceManagerThreadDelegate (this=0x7c3730014840) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:67\n> > #21 0x00007c3756218098 in std::__1::default_delete::operator() (this=0x7c3744039bc0, __ptr=0x400000000) at /usr/bin/../include/c++/v1/memory:1335\n> > #22 std::__1::unique_ptr >::reset (this=0x7c3744039bc0, __p=0x0) at /usr/bin/../include/c++/v1/memory:1596\n> > #23 base::Thread::ThreadMain (this=0x7c3744039b40) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:400\n> > #24 0x00007c3756213f3d in base::(anonymous namespace)::ThreadFunc (params=0x7c3730011840) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/platform_thread_posix.cc:87\n> > #25 0x00007c37560a8fbf in start_thread (arg=0x7c372ffff640) at pthread_create.c:463\n> > #26 0x00007c3755ef235f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95\n> >\n> > >\n> > > Hiro, is this something you can look at ?\n>\n> I guess this is because streams_ is not cleared when state_ is Stopped.\n> Could you try again by modifying as follows?\n>\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -448,8 +448,10 @@ void CameraDevice::flush()\n> void CameraDevice::stop()\n> {\n> MutexLocker stateLock(stateMutex_);\n> - if (state_ == State::Stopped)\n> + if (state_ == State::Stopped) {\n> + streams_.clear();\n> return;\n> + }\n\nOh I see your reasoning!\nIt seems right, if we have a flush, it will stop the camera, and the\nnext call to stop() won't clear streams_.\n\nHowever, I've not been able to run a single meaningful test the whole\nday due to spurious crashes in the FrameBuffer class.\n\nI'll wait for the dust to settle and re-test this change.\n>\n> Thanks,\n> -Hiro\n> > >\n> > > > > > > > But I have this patch gone through cros_camera_test and also CTS seems\n> > > > > > > > fine. Also looking at the code it seems an harmless change so I wonder\n> > > > > > > > what was wrong at the time.\n> > > > > > > >\n> > > > > > > > As far as I can tell:\n> > > > > > > >\n> > > > > > > > Tested-by: Jacopo Mondi \n> > > > > > > > Acked-by: Jacopo Mondi \n> > > > > > > >\n> > > > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > > > ---\n> > > > > > > > > src/android/camera_device.cpp | 4 ++--\n> > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > > > > >\n> > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > > > index 8ca76719..fda77db4 100644\n> > > > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > > > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> > > > > > > > >\n> > > > > > > > > void CameraDevice::close()\n> > > > > > > > > {\n> > > > > > > > > - streams_.clear();\n> > > > > > > > > -\n> > > > > > > > > stop();\n> > > > > > > > >\n> > > > > > > > > camera_->release();\n> > > > > > > > > @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> > > > > > > > > camera_->stop();\n> > > > > > > > >\n> > > > > > > > > descriptors_.clear();\n> > > > > > > > > + streams_.clear();\n> > > > > > > > > +\n> > > > > > > > > state_ = State::Stopped;\n> > > > > > > > > }\n> > > > > > > > >\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 971E1BD87D\n\tfor ;\n\tFri, 3 Sep 2021 17:05:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE7E86916A;\n\tFri, 3 Sep 2021 19:05:39 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F05F269167\n\tfor ;\n\tFri, 3 Sep 2021 19:05:37 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 4017E60002;\n\tFri, 3 Sep 2021 17:05:37 +0000 (UTC)"],"Date":"Fri, 3 Sep 2021 19:06:25 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210903170625.csuv2ps3utqxufzs@uno.localdomain>","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t<20210901154708.5j5cufgw5vrc23r7@uno.localdomain>\n\t\n\t<20210902065934.iuso7ylvynkzyhse@uno.localdomain>\n\t\n\t<20210902082559.hhrzbg6kpzljgctb@uno.localdomain>\n\t\n\t<20210902095631.mywctgus4utkbcz5@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19449,"web_url":"https://patchwork.libcamera.org/comment/19449/","msgid":"<20210906132707.7xhihht62gdl5wi7@uno.localdomain>","date":"2021-09-06T13:27:07","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> The problem is happening because we seem to add a CameraStream\n> associated buffer(depending on the CameraStream::Type) to the Request,\n> in CameraDevice::processCaptureRequest().\n>\n> However, when the camera stops, all the current buffers are marked with\n> FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> associated with the CameraStream (that was previously added to the\n> request) has now been cleared out with a part of streams_.clear(), even\n> before the camera stop() has been invoked. Any access to those request\n> buffers after they have been cleared, shall result in a crash.\n>\n> Signed-off-by: Hirokazu Honda \n\nI have re-tested today this patch, and piled some other on top.\n\nI have fixed the commit message and collected tags, can I resend this\nas part of a larger series ?\n\nThanks\n j\n\n> ---\n> src/android/camera_device.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 8ca76719..fda77db4 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n>\n> void CameraDevice::close()\n> {\n> -\tstreams_.clear();\n> -\n> \tstop();\n>\n> \tcamera_->release();\n> @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> \tcamera_->stop();\n>\n> \tdescriptors_.clear();\n> +\tstreams_.clear();\n> +\n> \tstate_ = State::Stopped;\n> }\n>\n> --\n> 2.33.0.259.gc128427fd7-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7CD32BD87D\n\tfor ;\n\tMon, 6 Sep 2021 13:26:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEE346916C;\n\tMon, 6 Sep 2021 15:26:20 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C444960137\n\tfor ;\n\tMon, 6 Sep 2021 15:26:19 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 437C61BF215;\n\tMon, 6 Sep 2021 13:26:18 +0000 (UTC)"],"Date":"Mon, 6 Sep 2021 15:27:07 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210906132707.7xhihht62gdl5wi7@uno.localdomain>","References":"<20210831183739.901729-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210831183739.901729-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19451,"web_url":"https://patchwork.libcamera.org/comment/19451/","msgid":"","date":"2021-09-06T13:32:31","subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Mon, Sep 6, 2021 at 10:26 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:\n> > The problem is happening because we seem to add a CameraStream\n> > associated buffer(depending on the CameraStream::Type) to the Request,\n> > in CameraDevice::processCaptureRequest().\n> >\n> > However, when the camera stops, all the current buffers are marked with\n> > FrameMetadata::FrameCancelled and proceed to completion. But the buffer\n> > associated with the CameraStream (that was previously added to the\n> > request) has now been cleared out with a part of streams_.clear(), even\n> > before the camera stop() has been invoked. Any access to those request\n> > buffers after they have been cleared, shall result in a crash.\n> >\n> > Signed-off-by: Hirokazu Honda \n>\n> I have re-tested today this patch, and piled some other on top.\n>\n> I have fixed the commit message and collected tags, can I resend this\n> as part of a larger series ?\n>\n\nSure. If you think it is necessary, please do so.\n\n-Hiro\n> Thanks\n> j\n>\n> > ---\n> > src/android/camera_device.cpp | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 8ca76719..fda77db4 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> >\n> > void CameraDevice::close()\n> > {\n> > - streams_.clear();\n> > -\n> > stop();\n> >\n> > camera_->release();\n> > @@ -457,6 +455,8 @@ void CameraDevice::stop()\n> > camera_->stop();\n> >\n> > descriptors_.clear();\n> > + streams_.clear();\n> > +\n> > state_ = State::Stopped;\n> > }\n> >\n> > --\n> > 2.33.0.259.gc128427fd7-goog\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0325FBDC71\n\tfor ;\n\tMon, 6 Sep 2021 13:32:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 686566916A;\n\tMon, 6 Sep 2021 15:32:44 +0200 (CEST)","from mail-ej1-x630.google.com (mail-ej1-x630.google.com\n\t[IPv6:2a00:1450:4864:20::630])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF18860137\n\tfor ;\n\tMon, 6 Sep 2021 15:32:42 +0200 (CEST)","by mail-ej1-x630.google.com with SMTP id u14so13513207ejf.13\n\tfor ;\n\tMon, 06 Sep 2021 06:32:42 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"cJq2DBs7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=3dO4qwRGt4A1Uwjmwni+7Exvs3LTkyPWgFnP/Y48xw4=;\n\tb=cJq2DBs7wIdmKJuFinWHD/iZ+uXUBB+hvH37NNcTyiUHNR+gd7tOmiAGeJHRjX9wz0\n\tIIzZnW7nyCLDk6uEQnpOa3HqfjH97pcRDIBpsFBUMtNrcm7jlHdfuxwyMYX8cmjYgfHs\n\tC749ClsRHqIL1sMRBqwPn1FY0ovHJzbu9U+KQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=3dO4qwRGt4A1Uwjmwni+7Exvs3LTkyPWgFnP/Y48xw4=;\n\tb=PdFYn3VRpMU8wgaLdjqU2cn46xFrXNCfr86jfZ7M2jIkOT1PKNPD+/narMvDMoi1jT\n\tlRLf3AgtRqgebBZ8vYwTrgnJUgZZoP4pkAybBHQjjHPP6lYylDUjJY95MQ3uEe6bjYaF\n\tyoCD4HRSy3HbIz2ZrEfKWdYCP5L3myvt6J0AtO9mtT1bmGbOpINXxeswgYSgKr5AoT3c\n\tgMYYvQLDDWlzie6nFQl2YeyTdjZtNnSgD0EIhVVHWmixGpDBSq9DAR1EbpknI+sE4eN8\n\t2P2RhxFLw0CL7ITjYhZw5pturAhktSsSzrMCmWckSFq+Ulc1yMoy8blduUjwaGBgevd5\n\tyVgA==","X-Gm-Message-State":"AOAM530EIU8SfZpPSarHFCxhS0yolRzDEja2vF7NA3ORE5hICOGRG6Du\n\ttew5evzGqfP6/vpNDhBzAJE7IhPs4NnLo51hgLetL9if79Y=","X-Google-Smtp-Source":"ABdhPJxXgYri7kIcbcMbLA6Gh+1Pk2Dmo6FHHEPVA34dXmo7qGLnjnHb+ENZtlBgbS5Apag9ntDLs3M4og0t9ODahM8=","X-Received":"by 2002:a17:906:3589:: with SMTP id\n\to9mr14097035ejb.150.1630935162489; \n\tMon, 06 Sep 2021 06:32:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20210831183739.901729-1-hiroh@chromium.org>\n\t<20210906132707.7xhihht62gdl5wi7@uno.localdomain>","In-Reply-To":"<20210906132707.7xhihht62gdl5wi7@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 6 Sep 2021 22:32:31 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3] android: camera_device: Fix crash\n\tin calling CameraDevice::close()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]