[{"id":10983,"web_url":"https://patchwork.libcamera.org/comment/10983/","msgid":"<20200629202909.GP10681@pendragon.ideasonboard.com>","date":"2020-06-29T20:29:09","subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_device: support\n\tmultiple stream configurations","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Mon, Jun 29, 2020 at 05:39:16PM +0100, Kieran Bingham wrote:\n> Create an initial Camera Configuration using an empty role set, and\n> populate the StreamConfigurations manually from each of the streams\n> given by the Android camera3_stream_configuration_t stream_list.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/android/camera_device.cpp | 72 ++++++++++++++++++-----------------\n> 1 file changed, 38 insertions(+), 34 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index a6410f42fee8..568ba28067d0 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -956,48 +956,52 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\t\t << \" (\" << format.toString() << \")\";\n> \t}\n> \n> -\t/* Only one stream is supported. */\n> -\tif (stream_list->num_streams != 1) {\n> -\t\tLOG(HAL, Error) << \"Only one stream supported\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\tcamera3_stream_t *camera3Stream = stream_list->streams[0];\n> -\n> -\t/* Translate Android format code to libcamera pixel format. */\n> -\tPixelFormat format = toPixelFormat(camera3Stream->format);\n> -\tif (!format.isValid())\n> -\t\treturn -EINVAL;\n> -\n> \t/*\n> -\t * Hardcode viewfinder role, replacing the generated configuration\n> -\t * parameters with the ones requested by the Android framework.\n> +\t * Generate an empty configuration, and construct a StreamConfiguration\n> +\t * for each camera3_stream to add to it.\n> \t */\n> -\tStreamRoles roles = { StreamRole::Viewfinder };\n> -\tconfig_ = camera_->generateConfiguration(roles);\n> -\tif (!config_ || config_->empty()) {\n> +\tconfig_ = camera_->generateConfiguration();\n> +\tif (!config_) {\n> \t\tLOG(HAL, Error) << \"Failed to generate camera configuration\";\n> \t\treturn -EINVAL;\n> \t}\n\nThis should really really not fail, we could possibly drop the check (it\nshould be caught by pipeline handler tests, albeit we don't have them\nyet).\n\n> \n> -\tStreamConfiguration *streamConfiguration = &config_->at(0);\n> -\tstreamConfiguration->size.width = camera3Stream->width;\n> -\tstreamConfiguration->size.height = camera3Stream->height;\n> -\tstreamConfiguration->pixelFormat = format;\n> +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n> \n> -\tswitch (config_->validate()) {\n> -\tcase CameraConfiguration::Valid:\n> -\t\tbreak;\n> -\tcase CameraConfiguration::Adjusted:\n> -\t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n> -\t\tconfig_.reset();\n> -\t\treturn -EINVAL;\n> -\tcase CameraConfiguration::Invalid:\n> -\t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n> -\t\tconfig_.reset();\n> -\t\treturn -EINVAL;\n> -\t}\n> +\t\tPixelFormat format = toPixelFormat(stream->format);\n> +\t\tif (!format.isValid())\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tStreamConfiguration streamConfiguration;\n> +\n> +\t\tstreamConfiguration.size.width = stream->width;\n> +\t\tstreamConfiguration.size.height = stream->height;\n> +\t\tstreamConfiguration.pixelFormat = format;\n> +\n> +\t\tStreamConfiguration &cfg = config_->addConfiguration(streamConfiguration);\n> +\n> +\t\t/* 'cfg' can be modifed during the validation process */\n> +\t\tCameraConfiguration::Status status = config_->validate();\n\nIs there really a need to validate at each step instead of at the end ?\nIf it's for debugging purpose, you could print the whole camera\nconfiguration when the validation fails, so it van be compated with the\nrequested stream configurations that are printed earlier. Validation is\na potentially expensive operation (involving V4L2 ioctl calls, which,\nfor UVC for instance, even involve interactions with the hardware), so\na loop to create the camera configuration followed by a single\nvalidation would be best I think. You then wouldn't need patch 2/4 (I'd\nlike to keep changes to CameraConfiguration and StreamConfiguration\nminimal until we rework them, and I'm not sure the code above that\npasses a StreamConfiguration to addConfiguration() to get another\nStreamConfiguration out is any less confusing than what we have today).\n\n> +\n> +\t\tLOG(HAL, Info) << \"Stream #\" << i << \" (validated): \" << cfg.toString();\n\nLet's try to not make the log too verbose in normal cases.\n\n> \n> -\tcamera3Stream->max_buffers = streamConfiguration->bufferCount;\n> +\t\tswitch (status) {\n> +\t\tcase CameraConfiguration::Valid:\n> +\t\t\tbreak;\n> +\t\tcase CameraConfiguration::Adjusted:\n> +\t\t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n> +\t\t\tconfig_.reset();\n> +\t\t\treturn -EINVAL;\n> +\t\tcase CameraConfiguration::Invalid:\n> +\t\t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n> +\t\t\tconfig_.reset();\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\t/* Use the bufferCount confirmed by the validation process. */\n> +\t\tstream->max_buffers = cfg.bufferCount;\n> +\t}\n> \n> \t/*\n> \t * Once the CameraConfiguration has been adjusted/validated","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 72FD4BF415\n\tfor ;\n\tMon, 29 Jun 2020 20:29:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 09E2D609D6;\n\tMon, 29 Jun 2020 22:29:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 40531603B2\n\tfor ;\n\tMon, 29 Jun 2020 22:29:13 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFE18299;\n\tMon, 29 Jun 2020 22:29:12 +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=\"hN0bcly3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593462552;\n\tbh=4UMBS/f35UP/mxbY8Zwj9Gvm3zd6MrrRx8FHSGTE70k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hN0bcly3QOtNGh9SI0D/rV7Zc2taJZV3BHozqm1KEYTARKzS2M1nvO6ZseHghCqhb\n\tjQNScWWgYh/bNBKNJhhAPicqpvcaR+ER7KcAKVS8hiAWJdp0EJyKfO/UBvRlckg1M6\n\tBUMLDwe3G/0/1MJN3bjESEvDZvtloetZdigJAYVo=","Date":"Mon, 29 Jun 2020 23:29:09 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200629202909.GP10681@pendragon.ideasonboard.com>","References":"<20200629163916.1815321-1-kieran.bingham@ideasonboard.com>\n\t<20200629163916.1815321-5-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200629163916.1815321-5-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_device: support\n\tmultiple stream configurations","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11020,"web_url":"https://patchwork.libcamera.org/comment/11020/","msgid":"<14391233-1b3d-834d-cdea-d291af3b64d1@ideasonboard.com>","date":"2020-06-30T14:20:40","subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_device: support\n\tmultiple stream configurations","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 29/06/2020 21:29, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Mon, Jun 29, 2020 at 05:39:16PM +0100, Kieran Bingham wrote:\n>> Create an initial Camera Configuration using an empty role set, and\n>> populate the StreamConfigurations manually from each of the streams\n>> given by the Android camera3_stream_configuration_t stream_list.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/android/camera_device.cpp | 72 ++++++++++++++++++-----------------\n>> 1 file changed, 38 insertions(+), 34 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index a6410f42fee8..568ba28067d0 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -956,48 +956,52 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>> \t\t\t << \" (\" << format.toString() << \")\";\n>> \t}\n>> \n>> -\t/* Only one stream is supported. */\n>> -\tif (stream_list->num_streams != 1) {\n>> -\t\tLOG(HAL, Error) << \"Only one stream supported\";\n>> -\t\treturn -EINVAL;\n>> -\t}\n>> -\tcamera3_stream_t *camera3Stream = stream_list->streams[0];\n>> -\n>> -\t/* Translate Android format code to libcamera pixel format. */\n>> -\tPixelFormat format = toPixelFormat(camera3Stream->format);\n>> -\tif (!format.isValid())\n>> -\t\treturn -EINVAL;\n>> -\n>> \t/*\n>> -\t * Hardcode viewfinder role, replacing the generated configuration\n>> -\t * parameters with the ones requested by the Android framework.\n>> +\t * Generate an empty configuration, and construct a StreamConfiguration\n>> +\t * for each camera3_stream to add to it.\n>> \t */\n>> -\tStreamRoles roles = { StreamRole::Viewfinder };\n>> -\tconfig_ = camera_->generateConfiguration(roles);\n>> -\tif (!config_ || config_->empty()) {\n>> +\tconfig_ = camera_->generateConfiguration();\n>> +\tif (!config_) {\n>> \t\tLOG(HAL, Error) << \"Failed to generate camera configuration\";\n>> \t\treturn -EINVAL;\n>> \t}\n> \n> This should really really not fail, we could possibly drop the check (it\n> should be caught by pipeline handler tests, albeit we don't have them\n> yet).\n\n\nIndeed, it shouldn't. I'm not adding a check here though, but I agree it\ncould be removed if all the pipelines were guaranteed to successfully\ngenerate the configuration.\n\nPerhaps an assert for developer time checks, which could be compiled out\nat release builds.\n\n\n> \n>> \n>> -\tStreamConfiguration *streamConfiguration = &config_->at(0);\n>> -\tstreamConfiguration->size.width = camera3Stream->width;\n>> -\tstreamConfiguration->size.height = camera3Stream->height;\n>> -\tstreamConfiguration->pixelFormat = format;\n>> +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n>> +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n>> \n>> -\tswitch (config_->validate()) {\n>> -\tcase CameraConfiguration::Valid:\n>> -\t\tbreak;\n>> -\tcase CameraConfiguration::Adjusted:\n>> -\t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n>> -\t\tconfig_.reset();\n>> -\t\treturn -EINVAL;\n>> -\tcase CameraConfiguration::Invalid:\n>> -\t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n>> -\t\tconfig_.reset();\n>> -\t\treturn -EINVAL;\n>> -\t}\n>> +\t\tPixelFormat format = toPixelFormat(stream->format);\n>> +\t\tif (!format.isValid())\n>> +\t\t\treturn -EINVAL;\n>> +\n>> +\t\tStreamConfiguration streamConfiguration;\n>> +\n>> +\t\tstreamConfiguration.size.width = stream->width;\n>> +\t\tstreamConfiguration.size.height = stream->height;\n>> +\t\tstreamConfiguration.pixelFormat = format;\n>> +\n>> +\t\tStreamConfiguration &cfg = config_->addConfiguration(streamConfiguration);\n>> +\n>> +\t\t/* 'cfg' can be modifed during the validation process */\n>> +\t\tCameraConfiguration::Status status = config_->validate();\n> \n> Is there really a need to validate at each step instead of at the end ?\n\nWell I needed to be able to populate the stream->max_buffers from the\ncfg.bufferCount below.\n\nBut I do agree the validate should happen with all streams computed.\nIt will require a second loop, and being able to map back the stream\nconfigurations to the camera3Stream - but we need that anyway, so I'll\nkeep trying to fix that up.\n\n\n\n> If it's for debugging purpose, you could print the whole camera\n> configuration when the validation fails, so it van be compated with the\n> requested stream configurations that are printed earlier. Validation is\n> a potentially expensive operation (involving V4L2 ioctl calls, which,\n> for UVC for instance, even involve interactions with the hardware), so\n> a loop to create the camera configuration followed by a single\n> validation would be best I think. You then wouldn't need patch 2/4 (I'd\n> like to keep changes to CameraConfiguration and StreamConfiguration\n> minimal until we rework them, and I'm not sure the code above that\n> passes a StreamConfiguration to addConfiguration() to get another\n> StreamConfiguration out is any less confusing than what we have today).\n\nWe're lacking a clear way of mapping a StreamConfiguration back to the\ncamera3stream. But I'll work on a way to map this correctly.\n\n\n\n> \n>> +\n>> +\t\tLOG(HAL, Info) << \"Stream #\" << i << \" (validated): \" << cfg.toString();\n> \n> Let's try to not make the log too verbose in normal cases.\n\nThat's probably debug while I'm developing ;-)\n\nThanks for the feedback, Particularly on a half baked early patch.\n\n--\nKieran\n\n\n\n> \n>> \n>> -\tcamera3Stream->max_buffers = streamConfiguration->bufferCount;\n>> +\t\tswitch (status) {\n>> +\t\tcase CameraConfiguration::Valid:\n>> +\t\t\tbreak;\n>> +\t\tcase CameraConfiguration::Adjusted:\n>> +\t\t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n>> +\t\t\tconfig_.reset();\n>> +\t\t\treturn -EINVAL;\n>> +\t\tcase CameraConfiguration::Invalid:\n>> +\t\t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n>> +\t\t\tconfig_.reset();\n>> +\t\t\treturn -EINVAL;\n>> +\t\t}\n>> +\n>> +\t\t/* Use the bufferCount confirmed by the validation process. */\n>> +\t\tstream->max_buffers = cfg.bufferCount;\n>> +\t}\n>> \n>> \t/*\n>> \t * Once the CameraConfiguration has been adjusted/validated\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 55553BF415\n\tfor ;\n\tTue, 30 Jun 2020 14:20:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4F6F60C58;\n\tTue, 30 Jun 2020 16:20:45 +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 0413E609C7\n\tfor ;\n\tTue, 30 Jun 2020 16:20:44 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 64F6B31F;\n\tTue, 30 Jun 2020 16:20:43 +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=\"YuR3pyRx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593526843;\n\tbh=4AUDe9S1NW2LthFbqpiWCYpESAJh1vbHUdjlTEI5vno=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=YuR3pyRxkJz9Hj5MXn/rSKw8Bj9JAXEzNTKu7JfLm49iY++UDrQ07C0ybyWPz9SJi\n\tGJl0lJrZ/wU/Rtdpum1o+4A8V8VQQgwD6Se1mO1gFyDl5IfPOWkgdgFYwuycOLglwN\n\tQzsx6pgtOibbllTQ4yJ2KSE5La+lwyO+GTZ1izQ8=","To":"Laurent Pinchart ","References":"<20200629163916.1815321-1-kieran.bingham@ideasonboard.com>\n\t<20200629163916.1815321-5-kieran.bingham@ideasonboard.com>\n\t<20200629202909.GP10681@pendragon.ideasonboard.com>","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":"<14391233-1b3d-834d-cdea-d291af3b64d1@ideasonboard.com>","Date":"Tue, 30 Jun 2020 15:20:40 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"<20200629202909.GP10681@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_device: support\n\tmultiple stream configurations","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11024,"web_url":"https://patchwork.libcamera.org/comment/11024/","msgid":"<20200630145924.GJ5850@pendragon.ideasonboard.com>","date":"2020-06-30T14:59:24","subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_device: support\n\tmultiple stream configurations","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Jun 30, 2020 at 03:20:40PM +0100, Kieran Bingham wrote:\n> On 29/06/2020 21:29, Laurent Pinchart wrote:\n> > On Mon, Jun 29, 2020 at 05:39:16PM +0100, Kieran Bingham wrote:\n> >> Create an initial Camera Configuration using an empty role set, and\n> >> populate the StreamConfigurations manually from each of the streams\n> >> given by the Android camera3_stream_configuration_t stream_list.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/android/camera_device.cpp | 72 ++++++++++++++++++-----------------\n> >> 1 file changed, 38 insertions(+), 34 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index a6410f42fee8..568ba28067d0 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -956,48 +956,52 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >> \t\t\t << \" (\" << format.toString() << \")\";\n> >> \t}\n> >> \n> >> -\t/* Only one stream is supported. */\n> >> -\tif (stream_list->num_streams != 1) {\n> >> -\t\tLOG(HAL, Error) << \"Only one stream supported\";\n> >> -\t\treturn -EINVAL;\n> >> -\t}\n> >> -\tcamera3_stream_t *camera3Stream = stream_list->streams[0];\n> >> -\n> >> -\t/* Translate Android format code to libcamera pixel format. */\n> >> -\tPixelFormat format = toPixelFormat(camera3Stream->format);\n> >> -\tif (!format.isValid())\n> >> -\t\treturn -EINVAL;\n> >> -\n> >> \t/*\n> >> -\t * Hardcode viewfinder role, replacing the generated configuration\n> >> -\t * parameters with the ones requested by the Android framework.\n> >> +\t * Generate an empty configuration, and construct a StreamConfiguration\n> >> +\t * for each camera3_stream to add to it.\n> >> \t */\n> >> -\tStreamRoles roles = { StreamRole::Viewfinder };\n> >> -\tconfig_ = camera_->generateConfiguration(roles);\n> >> -\tif (!config_ || config_->empty()) {\n> >> +\tconfig_ = camera_->generateConfiguration();\n> >> +\tif (!config_) {\n> >> \t\tLOG(HAL, Error) << \"Failed to generate camera configuration\";\n> >> \t\treturn -EINVAL;\n> >> \t}\n> > \n> > This should really really not fail, we could possibly drop the check (it\n> > should be caught by pipeline handler tests, albeit we don't have them\n> > yet).\n> \n> Indeed, it shouldn't. I'm not adding a check here though, but I agree it\n> could be removed if all the pipelines were guaranteed to successfully\n> generate the configuration.\n> \n> Perhaps an assert for developer time checks, which could be compiled out\n> at release builds.\n\nA LOG(Fatal) then ? :-)\n\n> >> \n> >> -\tStreamConfiguration *streamConfiguration = &config_->at(0);\n> >> -\tstreamConfiguration->size.width = camera3Stream->width;\n> >> -\tstreamConfiguration->size.height = camera3Stream->height;\n> >> -\tstreamConfiguration->pixelFormat = format;\n> >> +\tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> >> +\t\tcamera3_stream_t *stream = stream_list->streams[i];\n> >> \n> >> -\tswitch (config_->validate()) {\n> >> -\tcase CameraConfiguration::Valid:\n> >> -\t\tbreak;\n> >> -\tcase CameraConfiguration::Adjusted:\n> >> -\t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n> >> -\t\tconfig_.reset();\n> >> -\t\treturn -EINVAL;\n> >> -\tcase CameraConfiguration::Invalid:\n> >> -\t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n> >> -\t\tconfig_.reset();\n> >> -\t\treturn -EINVAL;\n> >> -\t}\n> >> +\t\tPixelFormat format = toPixelFormat(stream->format);\n> >> +\t\tif (!format.isValid())\n> >> +\t\t\treturn -EINVAL;\n> >> +\n> >> +\t\tStreamConfiguration streamConfiguration;\n> >> +\n> >> +\t\tstreamConfiguration.size.width = stream->width;\n> >> +\t\tstreamConfiguration.size.height = stream->height;\n> >> +\t\tstreamConfiguration.pixelFormat = format;\n> >> +\n> >> +\t\tStreamConfiguration &cfg = config_->addConfiguration(streamConfiguration);\n> >> +\n> >> +\t\t/* 'cfg' can be modifed during the validation process */\n> >> +\t\tCameraConfiguration::Status status = config_->validate();\n> > \n> > Is there really a need to validate at each step instead of at the end ?\n> \n> Well I needed to be able to populate the stream->max_buffers from the\n> cfg.bufferCount below.\n> \n> But I do agree the validate should happen with all streams computed.\n> It will require a second loop, and being able to map back the stream\n> configurations to the camera3Stream - but we need that anyway, so I'll\n> keep trying to fix that up.\n\nFor this patch you could just iterate over the camera3Stream again, the\nStreamConfiguration instances in CameraConfiguration are stored in the\nsame order. Long term we'll need to map back from Stream to\ncamera3Stream, so a std::map (or something else) may already be useful.\n\n> > If it's for debugging purpose, you could print the whole camera\n> > configuration when the validation fails, so it van be compated with the\n> > requested stream configurations that are printed earlier. Validation is\n> > a potentially expensive operation (involving V4L2 ioctl calls, which,\n> > for UVC for instance, even involve interactions with the hardware), so\n> > a loop to create the camera configuration followed by a single\n> > validation would be best I think. You then wouldn't need patch 2/4 (I'd\n> > like to keep changes to CameraConfiguration and StreamConfiguration\n> > minimal until we rework them, and I'm not sure the code above that\n> > passes a StreamConfiguration to addConfiguration() to get another\n> > StreamConfiguration out is any less confusing than what we have today).\n> \n> We're lacking a clear way of mapping a StreamConfiguration back to the\n> camera3stream. But I'll work on a way to map this correctly.\n> \n> >> +\n> >> +\t\tLOG(HAL, Info) << \"Stream #\" << i << \" (validated): \" << cfg.toString();\n> > \n> > Let's try to not make the log too verbose in normal cases.\n> \n> That's probably debug while I'm developing ;-)\n> \n> Thanks for the feedback, Particularly on a half baked early patch.\n> \n> >> \n> >> -\tcamera3Stream->max_buffers = streamConfiguration->bufferCount;\n> >> +\t\tswitch (status) {\n> >> +\t\tcase CameraConfiguration::Valid:\n> >> +\t\t\tbreak;\n> >> +\t\tcase CameraConfiguration::Adjusted:\n> >> +\t\t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n> >> +\t\t\tconfig_.reset();\n> >> +\t\t\treturn -EINVAL;\n> >> +\t\tcase CameraConfiguration::Invalid:\n> >> +\t\t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n> >> +\t\t\tconfig_.reset();\n> >> +\t\t\treturn -EINVAL;\n> >> +\t\t}\n> >> +\n> >> +\t\t/* Use the bufferCount confirmed by the validation process. */\n> >> +\t\tstream->max_buffers = cfg.bufferCount;\n> >> +\t}\n> >> \n> >> \t/*\n> >> \t * Once the CameraConfiguration has been adjusted/validated","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 973E4BF415\n\tfor ;\n\tTue, 30 Jun 2020 14:59:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6577260C5B;\n\tTue, 30 Jun 2020 16:59:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 834F5609C5\n\tfor ;\n\tTue, 30 Jun 2020 16:59:29 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9C56929F;\n\tTue, 30 Jun 2020 16:59:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"l8gntOwn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593529169;\n\tbh=KcOcZZjbk00ghVNOT2d9ugsXIY9LX3SiYHk1aRwO0e4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l8gntOwnj2DdxUIDsAaUayJT/Q592NZRm6DJx3ikJAwanCtT3GMfLGufB4HZBE8Mu\n\tCdu8pIMYp5DyIGFN6qS3Z8/Q39TtjnkZn7MC1oT/DrlRM8//gttLV3PpMx42Fao/FA\n\tYhLAw0BpQADwT10UaKlJmHAYSzjiYEm6aMUIlZls=","Date":"Tue, 30 Jun 2020 17:59:24 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200630145924.GJ5850@pendragon.ideasonboard.com>","References":"<20200629163916.1815321-1-kieran.bingham@ideasonboard.com>\n\t<20200629163916.1815321-5-kieran.bingham@ideasonboard.com>\n\t<20200629202909.GP10681@pendragon.ideasonboard.com>\n\t<14391233-1b3d-834d-cdea-d291af3b64d1@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<14391233-1b3d-834d-cdea-d291af3b64d1@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] android: camera_device: support\n\tmultiple stream configurations","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]