[{"id":12979,"web_url":"https://patchwork.libcamera.org/comment/12979/","msgid":"<20201005122816.GL3931@pendragon.ideasonboard.com>","date":"2020-10-05T12:28:16","subject":"Re: [libcamera-devel] [PATCH 10/15] android: camera_stream:\n\tAllocate FrameBuffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Oct 05, 2020 at 01:46:44PM +0300, Laurent Pinchart wrote:\n> From: Jacopo Mondi \n> \n> Allocate FrameBuffers using the allocator_ class member in the\n> CameraStream class at CameraStream::configure() time.\n> \n> As FrameBuffer allocation can only happen after the Camera has been\n> correctly configured, move the CameraStream configuration loop\n> after the Camera::configure() call in CameraDevice.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 22 +++++++++++-----------\n> src/android/camera_stream.cpp | 17 +++++++++++++++--\n> src/android/camera_stream.h | 2 ++\n> 3 files changed, 28 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index adaec54dbfdb..537883b63f15 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1282,6 +1282,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\t/*\n> +\t * Once the CameraConfiguration has been adjusted/validated\n> +\t * it can be applied to the camera.\n> +\t */\n> +\tint ret = camera_->configure(config_.get());\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n> +\t\t\t\t<< camera_->id() << \"'\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \t/*\n> \t * Configure the HAL CameraStream instances using the associated\n> \t * StreamConfiguration and set the number of required buffers in\n> @@ -1300,17 +1311,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\tstream->max_buffers = cfg.bufferCount;\n> \t}\n> \n> -\t/*\n> -\t * Once the CameraConfiguration has been adjusted/validated\n> -\t * it can be applied to the camera.\n> -\t */\n> -\tint ret = camera_->configure(config_.get());\n> -\tif (ret) {\n> -\t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n> -\t\t\t\t<< camera_->id() << \"'\";\n> -\t\treturn ret;\n> -\t}\n> -\n> \treturn 0;\n> }\n> \n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 9f3e7026b1a4..76e7d240f4e7 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -58,8 +58,21 @@ const Size &CameraStream::size() const\n> \n> int CameraStream::configure(const libcamera::StreamConfiguration &cfg)\n> {\n> -\tif (encoder_)\n> -\t\treturn encoder_->configure(cfg);\n> +\tif (encoder_) {\n> +\t\tint ret = encoder_->configure(cfg);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\tif (allocator_) {\n> +\t\tint ret = allocator_->allocate(stream());\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\n> +\t\t/* Save a pointer to the reserved frame buffers */\n> +\t\tfor (const auto &frameBuffer : allocator_->buffers(stream()))\n> +\t\t\tbuffers_.push_back(frameBuffer.get());\n\nI'd move this to patch 12/15, it's not clear in this patch why you need\nit.\n\nReviewed-by: Laurent Pinchart \n\n> +\t}\n> \n> \treturn 0;\n> }\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index eaf4357ed096..e399e17b2a2f 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -8,6 +8,7 @@\n> #define __ANDROID_CAMERA_STREAM_H__\n> \n> #include \n> +#include \n> \n> #include \n> \n> @@ -140,6 +141,7 @@ private:\n> \tlibcamera::CameraConfiguration *config_;\n> \tEncoder *encoder_;\n> \tlibcamera::FrameBufferAllocator *allocator_;\n> +\tstd::vector buffers_;\n> };\n> \n> #endif /* __ANDROID_CAMERA_STREAM__ */","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 E9393C3B5C\n\tfor ;\n\tMon, 5 Oct 2020 12:28:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 808BB63BE4;\n\tMon, 5 Oct 2020 14:28:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F9A963BD5\n\tfor ;\n\tMon, 5 Oct 2020 14:28:55 +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 299213B\n\tfor ;\n\tMon, 5 Oct 2020 14:28:55 +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=\"SSrEGpgp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601900935;\n\tbh=lTugqAfzXUn5ZOvdesSJxfEB8sMRhxuTPCufXTbyCbc=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=SSrEGpgp0hTOukUQl+Dudoc+1YVA5XMUsyUvCsdmZALunwaSjUhIzb2oWitMM3ZTy\n\teSkHtgyEcbCn2vLv6VUbhHu4aegY4364E7bfJKZJhUcxmN3yp6X4lcog5j+6gFnbi2\n\tnCuWmDnnwMKLB9s1mqxKXQUNYSJpHHXjfzn4ix5E=","Date":"Mon, 5 Oct 2020 15:28:16 +0300","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Message-ID":"<20201005122816.GL3931@pendragon.ideasonboard.com>","References":"<20201005104649.10812-1-laurent.pinchart@ideasonboard.com>\n\t<20201005104649.10812-11-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201005104649.10812-11-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 10/15] android: camera_stream:\n\tAllocate FrameBuffers","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13011,"web_url":"https://patchwork.libcamera.org/comment/13011/","msgid":"<712d1361-e03a-fcf3-5ce8-ea6fdebeb25f@ideasonboard.com>","date":"2020-10-06T12:29:30","subject":"Re: [libcamera-devel] [PATCH 10/15] android: camera_stream:\n\tAllocate FrameBuffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 05/10/2020 13:28, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> Thank you for the patch.\n> \n> On Mon, Oct 05, 2020 at 01:46:44PM +0300, Laurent Pinchart wrote:\n>> From: Jacopo Mondi \n>>\n>> Allocate FrameBuffers using the allocator_ class member in the\n>> CameraStream class at CameraStream::configure() time.\n>>\n>> As FrameBuffer allocation can only happen after the Camera has been\n>> correctly configured, move the CameraStream configuration loop\n>> after the Camera::configure() call in CameraDevice.\n\nIf we know that now, should we update the patch earlier to put it there\nin the first place?\n\n(Or is that more effort than it's worth, or possible not correct at that\npoint in the series).\n\n\n>>\n>> Signed-off-by: Jacopo Mondi \n>> ---\n>> src/android/camera_device.cpp | 22 +++++++++++-----------\n>> src/android/camera_stream.cpp | 17 +++++++++++++++--\n>> src/android/camera_stream.h | 2 ++\n>> 3 files changed, 28 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index adaec54dbfdb..537883b63f15 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1282,6 +1282,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> +\t/*\n>> +\t * Once the CameraConfiguration has been adjusted/validated\n>> +\t * it can be applied to the camera.\n>> +\t */\n>> +\tint ret = camera_->configure(config_.get());\n>> +\tif (ret) {\n>> +\t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n>> +\t\t\t\t<< camera_->id() << \"'\";\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> \t/*\n>> \t * Configure the HAL CameraStream instances using the associated\n>> \t * StreamConfiguration and set the number of required buffers in\n>> @@ -1300,17 +1311,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>> \t\tstream->max_buffers = cfg.bufferCount;\n>> \t}\n>> \n>> -\t/*\n>> -\t * Once the CameraConfiguration has been adjusted/validated\n>> -\t * it can be applied to the camera.\n>> -\t */\n>> -\tint ret = camera_->configure(config_.get());\n>> -\tif (ret) {\n>> -\t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n>> -\t\t\t\t<< camera_->id() << \"'\";\n>> -\t\treturn ret;\n>> -\t}\n>> -\n>> \treturn 0;\n>> }\n>> \n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index 9f3e7026b1a4..76e7d240f4e7 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -58,8 +58,21 @@ const Size &CameraStream::size() const\n>> \n>> int CameraStream::configure(const libcamera::StreamConfiguration &cfg)\n>> {\n>> -\tif (encoder_)\n>> -\t\treturn encoder_->configure(cfg);\n>> +\tif (encoder_) {\n>> +\t\tint ret = encoder_->configure(cfg);\n>> +\t\tif (ret)\n>> +\t\t\treturn ret;\n>> +\t}\n>> +\n>> +\tif (allocator_) {\n>> +\t\tint ret = allocator_->allocate(stream());\n>> +\t\tif (ret < 0)\n>> +\t\t\treturn ret;\n>> +\n>> +\t\t/* Save a pointer to the reserved frame buffers */\n>> +\t\tfor (const auto &frameBuffer : allocator_->buffers(stream()))\n>> +\t\t\tbuffers_.push_back(frameBuffer.get());\n> \n> I'd move this to patch 12/15, it's not clear in this patch why you need\n> it.\n\n\nIf the configure gets moved in the patch when it is originally added,\nand this buffers_.push_back gets moved to patch 12/15 - the only thing\nleft here might be the allocate - which at that piont might also be\neasily squashed into 12/15... but it's not a requirement - just what\nmight happen if the other blocks reduce this patch to very little ;-)\n\n> Reviewed-by: Laurent Pinchart \n\nIf it stays otherwise:\n\nReviewed-by: Kieran Bingham \n\n\n> \n>> +\t}\n>> \n>> \treturn 0;\n>> }\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index eaf4357ed096..e399e17b2a2f 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -8,6 +8,7 @@\n>> #define __ANDROID_CAMERA_STREAM_H__\n>> \n>> #include \n>> +#include \n>> \n>> #include \n>> \n>> @@ -140,6 +141,7 @@ private:\n>> \tlibcamera::CameraConfiguration *config_;\n>> \tEncoder *encoder_;\n>> \tlibcamera::FrameBufferAllocator *allocator_;\n>> +\tstd::vector buffers_;\n>> };\n>> \n>> #endif /* __ANDROID_CAMERA_STREAM__ */\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 60B23BEEE0\n\tfor ;\n\tTue, 6 Oct 2020 12:29:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F359763BBF;\n\tTue, 6 Oct 2020 14:29:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 64E1C60363\n\tfor ;\n\tTue, 6 Oct 2020 14:29:33 +0200 (CEST)","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 B3FC23B;\n\tTue, 6 Oct 2020 14:29:32 +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=\"vaxkZfO8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601987372;\n\tbh=iTfREjyt12dFLZVwyuXVB9Boqc94LqbRe2VwxzLQmjU=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=vaxkZfO8Sx/coqxeaZgNEotVTlNvjVqy7eMafxNgmkc/MlPb5LTIqufxRsNGWhEQy\n\tyMCEX3GmzDi6IXGoD/rHbmpxvd99YvgwOLp1sHYS0m150QvG3SGd3m2kc1l16bKwFt\n\tz10myZ9QEyEjpEYroJ+/nhK1+6SMJNbreTk1noNY=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20201005104649.10812-1-laurent.pinchart@ideasonboard.com>\n\t<20201005104649.10812-11-laurent.pinchart@ideasonboard.com>\n\t<20201005122816.GL3931@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":"<712d1361-e03a-fcf3-5ce8-ea6fdebeb25f@ideasonboard.com>","Date":"Tue, 6 Oct 2020 13:29:30 +0100","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":"<20201005122816.GL3931@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 10/15] android: camera_stream:\n\tAllocate FrameBuffers","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":13013,"web_url":"https://patchwork.libcamera.org/comment/13013/","msgid":"<20201006124047.yvdwu32imoispmiu@uno.localdomain>","date":"2020-10-06T12:40:47","subject":"Re: [libcamera-devel] [PATCH 10/15] android: camera_stream:\n\tAllocate FrameBuffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Tue, Oct 06, 2020 at 01:29:30PM +0100, Kieran Bingham wrote:\n> Hi Jacopo\n>\n> On 05/10/2020 13:28, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Oct 05, 2020 at 01:46:44PM +0300, Laurent Pinchart wrote:\n> >> From: Jacopo Mondi \n> >>\n> >> Allocate FrameBuffers using the allocator_ class member in the\n> >> CameraStream class at CameraStream::configure() time.\n> >>\n> >> As FrameBuffer allocation can only happen after the Camera has been\n> >> correctly configured, move the CameraStream configuration loop\n> >> after the Camera::configure() call in CameraDevice.\n>\n> If we know that now, should we update the patch earlier to put it there\n> in the first place?\n>\n> (Or is that more effort than it's worth, or possible not correct at that\n> point in the series).\n\nIt is not required before CameraStream::configure() has to allocate\nbuffers, so I think the change should happen at the same time.\n\n>\n>\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/android/camera_device.cpp | 22 +++++++++++-----------\n> >> src/android/camera_stream.cpp | 17 +++++++++++++++--\n> >> src/android/camera_stream.h | 2 ++\n> >> 3 files changed, 28 insertions(+), 13 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index adaec54dbfdb..537883b63f15 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -1282,6 +1282,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >>\n> >> +\t/*\n> >> +\t * Once the CameraConfiguration has been adjusted/validated\n> >> +\t * it can be applied to the camera.\n> >> +\t */\n> >> +\tint ret = camera_->configure(config_.get());\n> >> +\tif (ret) {\n> >> +\t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n> >> +\t\t\t\t<< camera_->id() << \"'\";\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> \t/*\n> >> \t * Configure the HAL CameraStream instances using the associated\n> >> \t * StreamConfiguration and set the number of required buffers in\n> >> @@ -1300,17 +1311,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >> \t\tstream->max_buffers = cfg.bufferCount;\n> >> \t}\n> >>\n> >> -\t/*\n> >> -\t * Once the CameraConfiguration has been adjusted/validated\n> >> -\t * it can be applied to the camera.\n> >> -\t */\n> >> -\tint ret = camera_->configure(config_.get());\n> >> -\tif (ret) {\n> >> -\t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n> >> -\t\t\t\t<< camera_->id() << \"'\";\n> >> -\t\treturn ret;\n> >> -\t}\n> >> -\n> >> \treturn 0;\n> >> }\n> >>\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index 9f3e7026b1a4..76e7d240f4e7 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -58,8 +58,21 @@ const Size &CameraStream::size() const\n> >>\n> >> int CameraStream::configure(const libcamera::StreamConfiguration &cfg)\n> >> {\n> >> -\tif (encoder_)\n> >> -\t\treturn encoder_->configure(cfg);\n> >> +\tif (encoder_) {\n> >> +\t\tint ret = encoder_->configure(cfg);\n> >> +\t\tif (ret)\n> >> +\t\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tif (allocator_) {\n> >> +\t\tint ret = allocator_->allocate(stream());\n> >> +\t\tif (ret < 0)\n> >> +\t\t\treturn ret;\n> >> +\n> >> +\t\t/* Save a pointer to the reserved frame buffers */\n> >> +\t\tfor (const auto &frameBuffer : allocator_->buffers(stream()))\n> >> +\t\t\tbuffers_.push_back(frameBuffer.get());\n> >\n> > I'd move this to patch 12/15, it's not clear in this patch why you need\n> > it.\n>\n>\n> If the configure gets moved in the patch when it is originally added,\n> and this buffers_.push_back gets moved to patch 12/15 - the only thing\n> left here might be the allocate - which at that piont might also be\n> easily squashed into 12/15... but it's not a requirement - just what\n> might happen if the other blocks reduce this patch to very little ;-)\n>\n\nI've moved everything to patch 12/15\n\n> > Reviewed-by: Laurent Pinchart \n>\n> If it stays otherwise:\n>\n> Reviewed-by: Kieran Bingham \n>\n\nThanks\n j\n\n>\n> >\n> >> +\t}\n> >>\n> >> \treturn 0;\n> >> }\n> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >> index eaf4357ed096..e399e17b2a2f 100644\n> >> --- a/src/android/camera_stream.h\n> >> +++ b/src/android/camera_stream.h\n> >> @@ -8,6 +8,7 @@\n> >> #define __ANDROID_CAMERA_STREAM_H__\n> >>\n> >> #include \n> >> +#include \n> >>\n> >> #include \n> >>\n> >> @@ -140,6 +141,7 @@ private:\n> >> \tlibcamera::CameraConfiguration *config_;\n> >> \tEncoder *encoder_;\n> >> \tlibcamera::FrameBufferAllocator *allocator_;\n> >> +\tstd::vector buffers_;\n> >> };\n> >>\n> >> #endif /* __ANDROID_CAMERA_STREAM__ */\n> >\n>\n> --\n> Regards\n> --\n> Kieran","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 4890DBEEE0\n\tfor ;\n\tTue, 6 Oct 2020 12:36:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2476163C13;\n\tTue, 6 Oct 2020 14:36:50 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62B8960363\n\tfor ;\n\tTue, 6 Oct 2020 14:36:48 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 0454C240013;\n\tTue, 6 Oct 2020 12:36:47 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 6 Oct 2020 14:40:47 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20201006124047.yvdwu32imoispmiu@uno.localdomain>","References":"<20201005104649.10812-1-laurent.pinchart@ideasonboard.com>\n\t<20201005104649.10812-11-laurent.pinchart@ideasonboard.com>\n\t<20201005122816.GL3931@pendragon.ideasonboard.com>\n\t<712d1361-e03a-fcf3-5ce8-ea6fdebeb25f@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<712d1361-e03a-fcf3-5ce8-ea6fdebeb25f@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 10/15] android: camera_stream:\n\tAllocate FrameBuffers","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\" "}}]