[{"id":13532,"web_url":"https://patchwork.libcamera.org/comment/13532/","msgid":"<844d81ff-0fc6-c1ba-0d78-d855e899eaf6@ideasonboard.com>","date":"2020-10-28T10:33:04","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash of\n\taccessing a missing map element","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hiro-san,\n\nOn 28/10/2020 08:57, Hirokazu Honda wrote:\n> std::map::at() searches std::map by the given key. The commit\n> e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to\n> accessing the first element of the map, but actually access the\n> element whose key is nullptr. This causes the crash because the map\n> doesn't have the element with nullptr. This fixes the issue by\n> replacing the std::map::at() operation by std::map::begin().\n> \n\nAyeee - I'm sorry - It looks like I certainly messed up something there.\n\nBefore pushing, I ran that series through our unit-tests, but of course\nthat doesn't cover src/android, and I had obviously neglected to further\ntest the android layer. The clue that I should have done so is in the\nprefix of the patches, so I'm sorry that I missed that.\n\n\nEven the code it came from was correct:\n\n- FrameBuffer *buffer = buffers.begin()->second;\n- resultMetadata = getResultMetadata(descriptor->frameNumber_,\n- buffer->metadata().timestamp);\n+ uint64_t timestamp = buffers.at(0)->metadata().timestamp;\n+ resultMetadata = getResultMetadata(descriptor->frameNumber_,\ntimestamp);\n\nSo your fix is certainly better.\n\nReviewed-by: Kieran Bingham \n\n\nIt makes me wonder if the integration processes needs to be able to\ncover more use-cases, but it's not going to be easy to automate testing\nof the android layer from a normal build currently.\n\nWe'd have to mock up the Android HAL calls ... hrm. .. not impossible,\nbut I wonder what the best way to do all that would be (without\nrequiring a full CTS).\n\n\n\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ca60f51..ead8a43 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t * It might be appropriate to return a 'correct' (as determined by\n> \t * pipeline handlers) timestamp in the Request itself.\n> \t */\n> -\tuint64_t timestamp = buffers.at(0)->metadata().timestamp;\n> +\tuint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> \tresultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp);\n> \n> \t/* Handle any JPEG compression. */\n> --\n> 2.29.0.rc2.309.g374f81d7ae-goog\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 D57F9BDB1E\n\tfor ;\n\tWed, 28 Oct 2020 10:33:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3835625F9;\n\tWed, 28 Oct 2020 11:33:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2B0962067\n\tfor ;\n\tWed, 28 Oct 2020 11:33:06 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4454F99A;\n\tWed, 28 Oct 2020 11:33:06 +0100 (CET)"],"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=\"jma6atyi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603881186;\n\tbh=AfmnByji1u9SwuCxQNCSA+HzybFvsl98g4YLxDeKf/Q=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=jma6atyiNOwTDZiCJgO2Vkt/go3y3ylbAZDXypEbrZRmoimT+rhp1KxhePajG/5sl\n\tL3s9SqTGQuOmoCPCCMa8PCEbRrait6n4yVuXA9w83ZguM8bUzXaZne36aDtcSrOryf\n\tWjrivVvvoppcUuFc390K5DWCjNOdwk6Xi3FXr9mc=","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","References":"<20201028085726.2983867-1-hiroh@chromium.org>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<844d81ff-0fc6-c1ba-0d78-d855e899eaf6@ideasonboard.com>","Date":"Wed, 28 Oct 2020 10:33:04 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201028085726.2983867-1-hiroh@chromium.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash of\n\taccessing a missing map element","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13533,"web_url":"https://patchwork.libcamera.org/comment/13533/","msgid":"<20201028103517.GA20561@pendragon.ideasonboard.com>","date":"2020-10-28T10:35:17","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash of\n\taccessing a missing map element","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 28, 2020 at 10:33:04AM +0000, Kieran Bingham wrote:\n> Hi Hiro-san,\n> \n> On 28/10/2020 08:57, Hirokazu Honda wrote:\n> > std::map::at() searches std::map by the given key. The commit\n> > e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to\n> > accessing the first element of the map, but actually access the\n> > element whose key is nullptr. This causes the crash because the map\n> > doesn't have the element with nullptr. This fixes the issue by\n> > replacing the std::map::at() operation by std::map::begin().\n> > \n> \n> Ayeee - I'm sorry - It looks like I certainly messed up something there.\n> \n> Before pushing, I ran that series through our unit-tests, but of course\n> that doesn't cover src/android, and I had obviously neglected to further\n> test the android layer. The clue that I should have done so is in the\n> prefix of the patches, so I'm sorry that I missed that.\n> \n> \n> Even the code it came from was correct:\n> \n> - FrameBuffer *buffer = buffers.begin()->second;\n> - resultMetadata = getResultMetadata(descriptor->frameNumber_,\n> - buffer->metadata().timestamp);\n> + uint64_t timestamp = buffers.at(0)->metadata().timestamp;\n> + resultMetadata = getResultMetadata(descriptor->frameNumber_,\n> timestamp);\n> \n> So your fix is certainly better.\n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> It makes me wonder if the integration processes needs to be able to\n> cover more use-cases, but it's not going to be easy to automate testing\n> of the android layer from a normal build currently.\n> \n> We'd have to mock up the Android HAL calls ... hrm. .. not impossible,\n> but I wonder what the best way to do all that would be (without\n> requiring a full CTS).\n\nWe could have basic smoketests, but we're missing the infrastructure to\nrun them automatically on a real (or even virtual) device at the moment.\nClearly something we'll have to address.\n\n> > Signed-off-by: Hirokazu Honda \n\nReviewed-by: Laurent Pinchart \n\nAnd we really need to move the timestamp from the buffers to the\nrequest.\n\n> > ---\n> > src/android/camera_device.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ca60f51..ead8a43 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request)\n> > \t * It might be appropriate to return a 'correct' (as determined by\n> > \t * pipeline handlers) timestamp in the Request itself.\n> > \t */\n> > -\tuint64_t timestamp = buffers.at(0)->metadata().timestamp;\n> > +\tuint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> > \tresultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp);\n> > \n> > \t/* Handle any JPEG compression. */","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 CE4D0C3B5C\n\tfor ;\n\tWed, 28 Oct 2020 10:36:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 70CFB62067;\n\tWed, 28 Oct 2020 11:36:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9C6162067\n\tfor ;\n\tWed, 28 Oct 2020 11:36:05 +0100 (CET)","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 6124499A;\n\tWed, 28 Oct 2020 11:36:05 +0100 (CET)"],"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=\"IVpbUx1t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603881365;\n\tbh=YmdgbyMdpNUroe9UD+9HG24bMc4PvTzy/19E3sohhXo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IVpbUx1tLELWefdc+DlCX1xnRYShZ5Zq0UoqfQR6jgwQI749pJDqP55bz1cxVxhOM\n\tdn3GxO0Q8SCiBuZNPc0VygQIJU30XLIWQ7w9GvTxaWm4VYUx+Me64Rm1+3PjVhQ014\n\tTbIcukegI96uTPBf628eqGB+pKZzuak+oxj8YTyY=","Date":"Wed, 28 Oct 2020 12:35:17 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20201028103517.GA20561@pendragon.ideasonboard.com>","References":"<20201028085726.2983867-1-hiroh@chromium.org>\n\t<844d81ff-0fc6-c1ba-0d78-d855e899eaf6@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<844d81ff-0fc6-c1ba-0d78-d855e899eaf6@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash of\n\taccessing a missing map element","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13546,"web_url":"https://patchwork.libcamera.org/comment/13546/","msgid":"","date":"2020-10-29T07:01:14","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash of\n\taccessing a missing map element","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Wed, Oct 28, 2020 at 7:36 PM Laurent Pinchart\n wrote:\n>\n> On Wed, Oct 28, 2020 at 10:33:04AM +0000, Kieran Bingham wrote:\n> > Hi Hiro-san,\n> >\n> > On 28/10/2020 08:57, Hirokazu Honda wrote:\n> > > std::map::at() searches std::map by the given key. The commit\n> > > e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to\n> > > accessing the first element of the map, but actually access the\n> > > element whose key is nullptr. This causes the crash because the map\n> > > doesn't have the element with nullptr. This fixes the issue by\n> > > replacing the std::map::at() operation by std::map::begin().\n> > >\n> >\n> > Ayeee - I'm sorry - It looks like I certainly messed up something there.\n> >\n> > Before pushing, I ran that series through our unit-tests, but of course\n> > that doesn't cover src/android, and I had obviously neglected to further\n> > test the android layer. The clue that I should have done so is in the\n> > prefix of the patches, so I'm sorry that I missed that.\n> >\n> >\n> > Even the code it came from was correct:\n> >\n> > - FrameBuffer *buffer = buffers.begin()->second;\n> > - resultMetadata = getResultMetadata(descriptor->frameNumber_,\n> > - buffer->metadata().timestamp);\n> > + uint64_t timestamp = buffers.at(0)->metadata().timestamp;\n> > + resultMetadata = getResultMetadata(descriptor->frameNumber_,\n> > timestamp);\n> >\n> > So your fix is certainly better.\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> > It makes me wonder if the integration processes needs to be able to\n> > cover more use-cases, but it's not going to be easy to automate testing\n> > of the android layer from a normal build currently.\n> >\n> > We'd have to mock up the Android HAL calls ... hrm. .. not impossible,\n> > but I wonder what the best way to do all that would be (without\n> > requiring a full CTS).\n>\n> We could have basic smoketests, but we're missing the infrastructure to\n> run them automatically on a real (or even virtual) device at the moment.\n> Clearly something we'll have to address.\n>\n\nI honestly haven't run the unit tests. Where are unit tests for\nAndroid HAL adaptation layer?\nI test with the ChromeOS camera stack + libcamera.\n+1 to adding smoketests.\nA HAL client of doing a simple (still) capture + mock/fake libcamera\nclasses should be nice to have.\n\nBest Regards,\n-Hiro\n> > > Signed-off-by: Hirokazu Honda \n>\n> Reviewed-by: Laurent Pinchart \n>\n> And we really need to move the timestamp from the buffers to the\n> request.\n>\n> > > ---\n> > > src/android/camera_device.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index ca60f51..ead8a43 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > * It might be appropriate to return a 'correct' (as determined by\n> > > * pipeline handlers) timestamp in the Request itself.\n> > > */\n> > > - uint64_t timestamp = buffers.at(0)->metadata().timestamp;\n> > > + uint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> > > resultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp);\n> > >\n> > > /* Handle any JPEG compression. */\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 6D275BDB9B\n\tfor ;\n\tThu, 29 Oct 2020 07:01:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED5E662808;\n\tThu, 29 Oct 2020 08:01:26 +0100 (CET)","from mail-ed1-x542.google.com (mail-ed1-x542.google.com\n\t[IPv6:2a00:1450:4864:20::542])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A8904627D7\n\tfor ;\n\tThu, 29 Oct 2020 08:01:25 +0100 (CET)","by mail-ed1-x542.google.com with SMTP id p93so1965405edd.7\n\tfor ;\n\tThu, 29 Oct 2020 00:01:25 -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=\"Xuh8+u/j\"; 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=5pGdV+PDcbyBiXaiqneAkVskkWXuqOsKoz+Vw0qq32c=;\n\tb=Xuh8+u/jCDcPTfRX8MHT17thLLYtfnf0nQbZGaQO13qYQtgn+TAj9zqq7lUxnzFNIg\n\t8gaz+4mjfnZiS/GK2SSY4+Bs2TiDoSDJYLcbqHntjBKwKYW+FoUO0zrzGE8ONvMB23br\n\tOhK3fsfKoAXVMnNREqZGyB8GL3CBn4BtqASYs=","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=5pGdV+PDcbyBiXaiqneAkVskkWXuqOsKoz+Vw0qq32c=;\n\tb=eqtQkfxBdZsMlFcJEB3c2+6eFPduqFIM78hPSqRPtxF5jXFKKXUVl43PNAV/1mVMGQ\n\td7ddrzAaQeb+n4uznT1JWBjU+NtyU3qBUQVz3ISPxYGeO2P683V7D0hS71wdOZJn8tem\n\tD+z3IL80mj2IHMouvIy74M36LZYODeywXUL0Rn+II/DPsnkf0mbIr/447/YI5+b0v9EI\n\tmDS7ONpkWpr/zCONg+777buzuKIgRZ9uudPiEAUGxhPFcdM+bilDwa2MyptvQXBrveIF\n\t+hea3jC5MaaRWmVcWJJrMUtHB+CAfl+iKlFKBA3w7E7tLbY9VdiR7a29yOJIzOkY2W//\n\tYFGA==","X-Gm-Message-State":"AOAM533F7LHlLaZnLlSqHN05TON4JUdD7hBRuN3rqfOXoHbLbuX3NsMm\n\takHm763BoM/EtrD2qmdZW1A8DpyAR1OPoPYZY522zA==","X-Google-Smtp-Source":"ABdhPJxTQU3GTbx+t7/8Y9aP2yeaeFhnCiLoITNHtFb+pu/lxx5s4Y+IuhB4uLE4fswJjdQU2tRHpHUZC20EDiToE5U=","X-Received":"by 2002:a50:decd:: with SMTP id\n\td13mr2597081edl.202.1603954885252; \n\tThu, 29 Oct 2020 00:01:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20201028085726.2983867-1-hiroh@chromium.org>\n\t<844d81ff-0fc6-c1ba-0d78-d855e899eaf6@ideasonboard.com>\n\t<20201028103517.GA20561@pendragon.ideasonboard.com>","In-Reply-To":"<20201028103517.GA20561@pendragon.ideasonboard.com>","From":"Hirokazu Honda ","Date":"Thu, 29 Oct 2020 16:01:14 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash of\n\taccessing a missing map element","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13566,"web_url":"https://patchwork.libcamera.org/comment/13566/","msgid":"<20201030040657.GV15024@pendragon.ideasonboard.com>","date":"2020-10-30T04:06:57","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash of\n\taccessing a missing map element","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro-san,\n\nOn Thu, Oct 29, 2020 at 04:01:14PM +0900, Hirokazu Honda wrote:\n> On Wed, Oct 28, 2020 at 7:36 PM Laurent Pinchart wrote:\n> > On Wed, Oct 28, 2020 at 10:33:04AM +0000, Kieran Bingham wrote:\n> > > On 28/10/2020 08:57, Hirokazu Honda wrote:\n> > > > std::map::at() searches std::map by the given key. The commit\n> > > > e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to\n> > > > accessing the first element of the map, but actually access the\n> > > > element whose key is nullptr. This causes the crash because the map\n> > > > doesn't have the element with nullptr. This fixes the issue by\n> > > > replacing the std::map::at() operation by std::map::begin().\n> > > >\n> > >\n> > > Ayeee - I'm sorry - It looks like I certainly messed up something there.\n> > >\n> > > Before pushing, I ran that series through our unit-tests, but of course\n> > > that doesn't cover src/android, and I had obviously neglected to further\n> > > test the android layer. The clue that I should have done so is in the\n> > > prefix of the patches, so I'm sorry that I missed that.\n> > >\n> > >\n> > > Even the code it came from was correct:\n> > >\n> > > - FrameBuffer *buffer = buffers.begin()->second;\n> > > - resultMetadata = getResultMetadata(descriptor->frameNumber_,\n> > > - buffer->metadata().timestamp);\n> > > + uint64_t timestamp = buffers.at(0)->metadata().timestamp;\n> > > + resultMetadata = getResultMetadata(descriptor->frameNumber_,\n> > > timestamp);\n> > >\n> > > So your fix is certainly better.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > >\n> > > It makes me wonder if the integration processes needs to be able to\n> > > cover more use-cases, but it's not going to be easy to automate testing\n> > > of the android layer from a normal build currently.\n> > >\n> > > We'd have to mock up the Android HAL calls ... hrm. .. not impossible,\n> > > but I wonder what the best way to do all that would be (without\n> > > requiring a full CTS).\n> >\n> > We could have basic smoketests, but we're missing the infrastructure to\n> > run them automatically on a real (or even virtual) device at the moment.\n> > Clearly something we'll have to address.\n> \n> I honestly haven't run the unit tests. Where are unit tests for\n> Android HAL adaptation layer?\n> I test with the ChromeOS camera stack + libcamera.\n\nWe don't have custom unit tests for the camera HAL, we also rely on the\nChrome OS camera tests, and CTS. We however have no infrastructure to\nautomate this at the moment, so it's all manual, and doesn't get tested\non every commit :-S That's something I would really like to fix, but of\ncourse we can't pause development for a month to put all the focus on\ntest automation :-)\n\n> +1 to adding smoketests.\n> A HAL client of doing a simple (still) capture + mock/fake libcamera\n> classes should be nice to have.\n> \n> > > > Signed-off-by: Hirokazu Honda \n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > And we really need to move the timestamp from the buffers to the\n> > request.\n> >\n> > > > ---\n> > > > src/android/camera_device.cpp | 2 +-\n> > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index ca60f51..ead8a43 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > > * It might be appropriate to return a 'correct' (as determined by\n> > > > * pipeline handlers) timestamp in the Request itself.\n> > > > */\n> > > > - uint64_t timestamp = buffers.at(0)->metadata().timestamp;\n> > > > + uint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> > > > resultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp);\n> > > >\n> > > > /* Handle any JPEG compression. */","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 9471EBDB9B\n\tfor ;\n\tFri, 30 Oct 2020 04:07:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB9066294E;\n\tFri, 30 Oct 2020 05:07:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 89883620BE\n\tfor ;\n\tFri, 30 Oct 2020 05:07:47 +0100 (CET)","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 E19989B7;\n\tFri, 30 Oct 2020 05:07:46 +0100 (CET)"],"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=\"p9cwOm7K\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1604030867;\n\tbh=vkBI85R9/Ft3mLC2p1Uy8G9Ul5F4zumfyS9MaAFz8uo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p9cwOm7KVURZyD4LeiH2d8ZOQ1/2k3uEHHw96ZJcDBriCEW0XVr33zCzXkYMqX0Ud\n\tAJJziNjrCeM4rjCrIbWxw38yYhutYNx0tMalbQ6X6pPI6rAnAtcegPSSq+hahSZaox\n\teICIzG5S3wSgCb3Mv4oB55Tqz+Dn2JG8Y7fAdPOQ=","Date":"Fri, 30 Oct 2020 06:06:57 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"<20201030040657.GV15024@pendragon.ideasonboard.com>","References":"<20201028085726.2983867-1-hiroh@chromium.org>\n\t<844d81ff-0fc6-c1ba-0d78-d855e899eaf6@ideasonboard.com>\n\t<20201028103517.GA20561@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Fix crash of\n\taccessing a missing map element","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]