[{"id":11579,"web_url":"https://patchwork.libcamera.org/comment/11579/","msgid":"<20200724172452.GT5921@pendragon.ideasonboard.com>","date":"2020-07-24T17:24:52","subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","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, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote:\n> Utilise the MappedBuffer interface to map each of the planes provided\n> in the Camera3 buffer to facilitate use in software streams.\n> \n> The buffers will be automatically unmapped when the object goes out of\n> scope or is deleted.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> \n> This patch shows an addition of a new MappedBuffer type constructed from\n> a camera3buffer. The base class deals with the move-constructors and\n> destructor, and gives us a common interface to pass a set of mapped\n> dmabufs around.\n> \n> I had hoped to use this to pass in the camera3buffer for writing jpeg\n> buffers to, giving the same interface for both the source and\n> destination buffer - but for JPEG, I do also need to return the number\n> of bytes actually consumed, so this ended up potentially not adding the\n> benefits I hoped for.\n> \n> Still, it might still provide some benefits, so I've included it here as\n> something to talk about.\n\nThis, along with patch 4/8, is the only part of the series I'm not sure\nto like :-S It feels quite like a ad-hoc solution, which is probably\nnormal, as that's what it is :-) It may not be that bad, but doesn't\nqualify for the public API in my opinion. We could keep this as-is for\nnow as an internal helper, until we figure something better (if there's\never a need to do so).\n\nA random idea, would it make sense to convert buffer_handle_t to\nFrameBuffer as early as possible in the HAL and operate on FrameBuffer ?\nThere's at least one drawback in that it would dup() the fds, which may\ncall for a rework of the FileDescriptor class to make borrowing fds\npossible. Or maybe it's just a bad idea.\n\nI'd like to see how this ends up being used for the JPEG encoding to get\na better feeling of the API. I thus can't guarantee at this point I'll\nack the idea as-is, but I think it's worth continuing the experiment, I\nfeel there's something good here.\n\n> src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++\n> 1 file changed, 33 insertions(+)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 6212ccdd61ec..f78486117e9f 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -86,6 +86,39 @@ const std::map camera3FormatsMap = {\n> \n> LOG_DECLARE_CATEGORY(HAL);\n> \n> +class MappedCamera3Buffer : public MappedBuffer {\n> +public:\n> +\tMappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags)\n> +\t{\n> +\t\tmaps_.reserve(camera3buffer->numFds);\n> +\t\terror_ = 0;\n> +\n> +\t\tfor (int i = 0; i < camera3buffer->numFds; i++) {\n> +\t\t\tif (camera3buffer->data[i] == -1)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\toff_t length = lseek(camera3buffer->data[i], 0, SEEK_END);\n> +\t\t\tif (length < 0) {\n> +\t\t\t\terror_ = errno;\n> +\t\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n> +\t\t\t\t\tcamera3buffer->data[i], 0);\n> +\t\t\tif (address == MAP_FAILED) {\n> +\t\t\t\terror_ = errno;\n> +\t\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tmaps_.push_back({address, static_cast(length)});\n> +\t\t}\n> +\n> +\t\tvalid_ = error_ == 0;\n> +\t}\n> +};\n> +\n> /*\n> * \\struct Camera3RequestDescriptor\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 917AFBD86F\n\tfor ;\n\tFri, 24 Jul 2020 17:25:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4BF96121D;\n\tFri, 24 Jul 2020 19:25:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E5A86039B\n\tfor ;\n\tFri, 24 Jul 2020 19:25:00 +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 506D5538;\n\tFri, 24 Jul 2020 19:24:59 +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=\"ATFLNRCR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595611499;\n\tbh=Qypt98d2Q0UNAeeQhHMeFQuitEzGQkUNzwaF6hI0PpA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ATFLNRCR1314w5M0Q43SOvTomWTYgqpXTvk+WWS3E4hvr/jEjHbXJ5UvSmpcQT05M\n\tlo8i29SZ16IWGlj00lWi5KRqZB9mb1QKf1Yzvy8wEsZG5ODNlhm+t/dx6L4yswISCz\n\t1rGktuyT93XbmFJ+gkADiTohy2jqT4OFuncqlLUg=","Date":"Fri, 24 Jul 2020 20:24:52 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200724172452.GT5921@pendragon.ideasonboard.com>","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-9-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200720224232.153717-9-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","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":11714,"web_url":"https://patchwork.libcamera.org/comment/11714/","msgid":"","date":"2020-07-29T14:19:24","subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/07/2020 18:24, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote:\n>> Utilise the MappedBuffer interface to map each of the planes provided\n>> in the Camera3 buffer to facilitate use in software streams.\n>>\n>> The buffers will be automatically unmapped when the object goes out of\n>> scope or is deleted.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>>\n>> This patch shows an addition of a new MappedBuffer type constructed from\n>> a camera3buffer. The base class deals with the move-constructors and\n>> destructor, and gives us a common interface to pass a set of mapped\n>> dmabufs around.\n>>\n>> I had hoped to use this to pass in the camera3buffer for writing jpeg\n>> buffers to, giving the same interface for both the source and\n>> destination buffer - but for JPEG, I do also need to return the number\n>> of bytes actually consumed, so this ended up potentially not adding the\n>> benefits I hoped for.\n>>\n>> Still, it might still provide some benefits, so I've included it here as\n>> something to talk about.\n> \n> This, along with patch 4/8, is the only part of the series I'm not sure\n> to like :-S It feels quite like a ad-hoc solution, which is probably\n> normal, as that's what it is :-) It may not be that bad, but doesn't\n> qualify for the public API in my opinion. We could keep this as-is for\n> now as an internal helper, until we figure something better (if there's\n> ever a need to do so).\n\n\nThis *isn't* public api - this is under src/android/camera_device.\n\nPerhaps you mean the MappedBuffer API shouldn't be public (Though even\nif the base is still internal, the Android layer can still use this).\n\n\n> A random idea, would it make sense to convert buffer_handle_t to\n> FrameBuffer as early as possible in the HAL and operate on FrameBuffer ?\n> There's at least one drawback in that it would dup() the fds, which may\n> call for a rework of the FileDescriptor class to make borrowing fds\n> possible. Or maybe it's just a bad idea.\n\nI thnik it was the unnecessary dup's required to construct a FrameBuffer\nthat stopped me, but perhaps it might make some sense still.\n\n\n\n> I'd like to see how this ends up being used for the JPEG encoding to get\n> a better feeling of the API. I thus can't guarantee at this point I'll\n> ack the idea as-is, but I think it's worth continuing the experiment, I\n> feel there's something good here.\n\n\nThe idea was to make sure that the Mapped buffer object was the same\nregardless of whether it came from a FrameBuffer, or from a buffer_handle_t.\n\nOf course mapping the MappedFrameBuffer(FrameBuffer(buffer_handle_t)) is\na solution to that, it just wastes/duplicates more fds.\n\nSee:\n\n[RFC PATCH 6/6] android: camera_device: Provide a MappedCamera3Buffer\n\n(Ugh ,sorry same patch name, this was split from there)\n\nfor how this got used to remove the aribtrary mapAndroidBlobBuffer() and\nunmapAndroidBlobBuffer() fucntions that I had created earlier on.\n\n(Kept separate for exactly these discussions).\n\nI think for now, I'm going to end up mapping a FrameBuffer from every\nbuffer_handle_t() for consistency instead.\n\n--\nKieran\n\n\n>> src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++\n>> 1 file changed, 33 insertions(+)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 6212ccdd61ec..f78486117e9f 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -86,6 +86,39 @@ const std::map camera3FormatsMap = {\n>> \n>> LOG_DECLARE_CATEGORY(HAL);\n>> \n>> +class MappedCamera3Buffer : public MappedBuffer {\n>> +public:\n>> +\tMappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags)\n>> +\t{\n>> +\t\tmaps_.reserve(camera3buffer->numFds);\n>> +\t\terror_ = 0;\n>> +\n>> +\t\tfor (int i = 0; i < camera3buffer->numFds; i++) {\n>> +\t\t\tif (camera3buffer->data[i] == -1)\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\toff_t length = lseek(camera3buffer->data[i], 0, SEEK_END);\n>> +\t\t\tif (length < 0) {\n>> +\t\t\t\terror_ = errno;\n>> +\t\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\n>> +\t\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n>> +\t\t\t\t\tcamera3buffer->data[i], 0);\n>> +\t\t\tif (address == MAP_FAILED) {\n>> +\t\t\t\terror_ = errno;\n>> +\t\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\n>> +\t\t\tmaps_.push_back({address, static_cast(length)});\n>> +\t\t}\n>> +\n>> +\t\tvalid_ = error_ == 0;\n>> +\t}\n>> +};\n>> +\n>> /*\n>> * \\struct Camera3RequestDescriptor\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 AC730BD878\n\tfor ;\n\tWed, 29 Jul 2020 14:19:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A51F613C6;\n\tWed, 29 Jul 2020 16:19:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1ACAD6039D\n\tfor ;\n\tWed, 29 Jul 2020 16:19:28 +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 8C79431F;\n\tWed, 29 Jul 2020 16:19:27 +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=\"gMHEfU7x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596032367;\n\tbh=cKTd/mXKyCsEBz5fw6HkGgjie7U5z6H6TGioiQqUh9A=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=gMHEfU7xxOGyeaKEfRUw9toXIJm9T+w0f9C4M+6RH4PuZQ3swvdpE4c+Np1Dix9aS\n\txO9/GVJLor8lXbofgOQgflqLA3WedNGoNqWESGCUkt7Mqp4lO5+sO8XHsg1/RJqx/I\n\tztg3QtWVxjSmtMx99ndUWLgxIHQONaz3C0BD3H2c=","To":"Laurent Pinchart ","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-9-kieran.bingham@ideasonboard.com>\n\t<20200724172452.GT5921@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":"","Date":"Wed, 29 Jul 2020 15:19:24 +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":"<20200724172452.GT5921@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","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":11715,"web_url":"https://patchwork.libcamera.org/comment/11715/","msgid":"<20200729142720.GD6183@pendragon.ideasonboard.com>","date":"2020-07-29T14:27:20","subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jul 29, 2020 at 03:19:24PM +0100, Kieran Bingham wrote:\n> On 24/07/2020 18:24, Laurent Pinchart wrote:\n> > On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote:\n> >> Utilise the MappedBuffer interface to map each of the planes provided\n> >> in the Camera3 buffer to facilitate use in software streams.\n> >>\n> >> The buffers will be automatically unmapped when the object goes out of\n> >> scope or is deleted.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >>\n> >> This patch shows an addition of a new MappedBuffer type constructed from\n> >> a camera3buffer. The base class deals with the move-constructors and\n> >> destructor, and gives us a common interface to pass a set of mapped\n> >> dmabufs around.\n> >>\n> >> I had hoped to use this to pass in the camera3buffer for writing jpeg\n> >> buffers to, giving the same interface for both the source and\n> >> destination buffer - but for JPEG, I do also need to return the number\n> >> of bytes actually consumed, so this ended up potentially not adding the\n> >> benefits I hoped for.\n> >>\n> >> Still, it might still provide some benefits, so I've included it here as\n> >> something to talk about.\n> > \n> > This, along with patch 4/8, is the only part of the series I'm not sure\n> > to like :-S It feels quite like a ad-hoc solution, which is probably\n> > normal, as that's what it is :-) It may not be that bad, but doesn't\n> > qualify for the public API in my opinion. We could keep this as-is for\n> > now as an internal helper, until we figure something better (if there's\n> > ever a need to do so).\n> \n> This *isn't* public api - this is under src/android/camera_device.\n> \n> Perhaps you mean the MappedBuffer API shouldn't be public (Though even\n> if the base is still internal, the Android layer can still use this).\n\nI may also have been confused. I think I was referring to the idea of\ncreating a base class that can be inherited from, that part is in the\npublic API. Making it private should be fine, but exposing that design\nexternally would I think require a bit more work first.\n\n> > A random idea, would it make sense to convert buffer_handle_t to\n> > FrameBuffer as early as possible in the HAL and operate on FrameBuffer ?\n> > There's at least one drawback in that it would dup() the fds, which may\n> > call for a rework of the FileDescriptor class to make borrowing fds\n> > possible. Or maybe it's just a bad idea.\n> \n> I thnik it was the unnecessary dup's required to construct a FrameBuffer\n> that stopped me, but perhaps it might make some sense still.\n\nWe really want to avoid unnecessary dup() calls as much as possible, as\nthat's expensive. Do you think it would make sense to extend\nFileDescriptor to allow borrowing fds, or is that too complicated, or\ntoo hackish ?\n\n> > I'd like to see how this ends up being used for the JPEG encoding to get\n> > a better feeling of the API. I thus can't guarantee at this point I'll\n> > ack the idea as-is, but I think it's worth continuing the experiment, I\n> > feel there's something good here.\n> \n> The idea was to make sure that the Mapped buffer object was the same\n> regardless of whether it came from a FrameBuffer, or from a buffer_handle_t.\n> \n> Of course mapping the MappedFrameBuffer(FrameBuffer(buffer_handle_t)) is\n> a solution to that, it just wastes/duplicates more fds.\n> \n> See:\n> \n> [RFC PATCH 6/6] android: camera_device: Provide a MappedCamera3Buffer\n> \n> (Ugh ,sorry same patch name, this was split from there)\n> \n> for how this got used to remove the aribtrary mapAndroidBlobBuffer() and\n> unmapAndroidBlobBuffer() fucntions that I had created earlier on.\n> \n> (Kept separate for exactly these discussions).\n> \n> I think for now, I'm going to end up mapping a FrameBuffer from every\n> buffer_handle_t() for consistency instead.\n> \n> >> src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++\n> >> 1 file changed, 33 insertions(+)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 6212ccdd61ec..f78486117e9f 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -86,6 +86,39 @@ const std::map camera3FormatsMap = {\n> >> \n> >> LOG_DECLARE_CATEGORY(HAL);\n> >> \n> >> +class MappedCamera3Buffer : public MappedBuffer {\n> >> +public:\n> >> +\tMappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags)\n> >> +\t{\n> >> +\t\tmaps_.reserve(camera3buffer->numFds);\n> >> +\t\terror_ = 0;\n> >> +\n> >> +\t\tfor (int i = 0; i < camera3buffer->numFds; i++) {\n> >> +\t\t\tif (camera3buffer->data[i] == -1)\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\toff_t length = lseek(camera3buffer->data[i], 0, SEEK_END);\n> >> +\t\t\tif (length < 0) {\n> >> +\t\t\t\terror_ = errno;\n> >> +\t\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n> >> +\t\t\t\tbreak;\n> >> +\t\t\t}\n> >> +\n> >> +\t\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n> >> +\t\t\t\t\tcamera3buffer->data[i], 0);\n> >> +\t\t\tif (address == MAP_FAILED) {\n> >> +\t\t\t\terror_ = errno;\n> >> +\t\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> >> +\t\t\t\tbreak;\n> >> +\t\t\t}\n> >> +\n> >> +\t\t\tmaps_.push_back({address, static_cast(length)});\n> >> +\t\t}\n> >> +\n> >> +\t\tvalid_ = error_ == 0;\n> >> +\t}\n> >> +};\n> >> +\n> >> /*\n> >> * \\struct Camera3RequestDescriptor\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 D1B40BD86F\n\tfor ;\n\tWed, 29 Jul 2020 14:27:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BF4C613C6;\n\tWed, 29 Jul 2020 16:27:32 +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 D0A076039D\n\tfor ;\n\tWed, 29 Jul 2020 16:27:30 +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 A014C31F;\n\tWed, 29 Jul 2020 16:27:29 +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=\"lX1I+J6I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596032849;\n\tbh=gNTkaYorUe1fq0irfe1C3kuJ+Z2t6OPkClU9Rw2K2GU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lX1I+J6II1MeziisnfIdnwjceTzPEv0KTh9kzP40DAmkMb5kwQIzlO/nWoeYMm7qk\n\tNCCQEgqwObq/DpqSqcUeZ6WN8J+Lchx5BxzAJ9zFANX/czhswQ212Erc86O6jyJvtO\n\tzGJk6jAKHNnZIxTR6AOCgaEm0LOg4z9POoFrbvUM=","Date":"Wed, 29 Jul 2020 17:27:20 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200729142720.GD6183@pendragon.ideasonboard.com>","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-9-kieran.bingham@ideasonboard.com>\n\t<20200724172452.GT5921@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","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":11717,"web_url":"https://patchwork.libcamera.org/comment/11717/","msgid":"<524713d6-d4c8-d2e9-38a2-eef0f3c07143@ideasonboard.com>","date":"2020-07-29T14:37:16","subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","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/07/2020 15:27, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Wed, Jul 29, 2020 at 03:19:24PM +0100, Kieran Bingham wrote:\n>> On 24/07/2020 18:24, Laurent Pinchart wrote:\n>>> On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote:\n>>>> Utilise the MappedBuffer interface to map each of the planes provided\n>>>> in the Camera3 buffer to facilitate use in software streams.\n>>>>\n>>>> The buffers will be automatically unmapped when the object goes out of\n>>>> scope or is deleted.\n>>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>>\n>>>> This patch shows an addition of a new MappedBuffer type constructed from\n>>>> a camera3buffer. The base class deals with the move-constructors and\n>>>> destructor, and gives us a common interface to pass a set of mapped\n>>>> dmabufs around.\n>>>>\n>>>> I had hoped to use this to pass in the camera3buffer for writing jpeg\n>>>> buffers to, giving the same interface for both the source and\n>>>> destination buffer - but for JPEG, I do also need to return the number\n>>>> of bytes actually consumed, so this ended up potentially not adding the\n>>>> benefits I hoped for.\n>>>>\n>>>> Still, it might still provide some benefits, so I've included it here as\n>>>> something to talk about.\n>>>\n>>> This, along with patch 4/8, is the only part of the series I'm not sure\n>>> to like :-S It feels quite like a ad-hoc solution, which is probably\n>>> normal, as that's what it is :-) It may not be that bad, but doesn't\n>>> qualify for the public API in my opinion. We could keep this as-is for\n>>> now as an internal helper, until we figure something better (if there's\n>>> ever a need to do so).\n>>\n>> This *isn't* public api - this is under src/android/camera_device.\n>>\n>> Perhaps you mean the MappedBuffer API shouldn't be public (Though even\n>> if the base is still internal, the Android layer can still use this).\n> \n> I may also have been confused. I think I was referring to the idea of\n> creating a base class that can be inherited from, that part is in the\n> public API. Making it private should be fine, but exposing that design\n> externally would I think require a bit more work first.\n> \n>>> A random idea, would it make sense to convert buffer_handle_t to\n>>> FrameBuffer as early as possible in the HAL and operate on FrameBuffer ?\n>>> There's at least one drawback in that it would dup() the fds, which may\n>>> call for a rework of the FileDescriptor class to make borrowing fds\n>>> possible. Or maybe it's just a bad idea.\n>>\n>> I thnik it was the unnecessary dup's required to construct a FrameBuffer\n>> that stopped me, but perhaps it might make some sense still.\n> \n> We really want to avoid unnecessary dup() calls as much as possible, as\n> that's expensive. Do you think it would make sense to extend\n> FileDescriptor to allow borrowing fds, or is that too complicated, or\n> too hackish ?\n\nI don't think it's too hackish necessarily, but I don't know how that\ncould be expressed.\n\nAnd indeed it could make things complicated for lifetimes as suddenly\nwhat happens if the other owner closes them etc.. (ok so you could\ndocument the requirements for borrowing etc)...\n\nI presume we'd have to have some sort of a flag to determine the fd was\nborrowed and prevent the associated closes on destructor ... it feels a\nbit awkward ... :S\n\n\nBut I have two paths for 'now':\n\nMake a MappedBuffer generic base that can map from a buffer_t\n\nor\n\nMap all buffer_t's to a FrameBuffer, then make a MappedFrameBuffer from\nthat ...\n\nDo you have a particular preference on either?\n\nIf I construct a FrameBuffer for each buffer_t, we can always look to\nimprove performance later...\n\n--\nKieran\n\n\n\n\n>>> I'd like to see how this ends up being used for the JPEG encoding to get\n>>> a better feeling of the API. I thus can't guarantee at this point I'll\n>>> ack the idea as-is, but I think it's worth continuing the experiment, I\n>>> feel there's something good here.\n>>\n>> The idea was to make sure that the Mapped buffer object was the same\n>> regardless of whether it came from a FrameBuffer, or from a buffer_handle_t.\n>>\n>> Of course mapping the MappedFrameBuffer(FrameBuffer(buffer_handle_t)) is\n>> a solution to that, it just wastes/duplicates more fds.\n>>\n>> See:\n>>\n>> [RFC PATCH 6/6] android: camera_device: Provide a MappedCamera3Buffer\n>>\n>> (Ugh ,sorry same patch name, this was split from there)\n>>\n>> for how this got used to remove the aribtrary mapAndroidBlobBuffer() and\n>> unmapAndroidBlobBuffer() fucntions that I had created earlier on.\n>>\n>> (Kept separate for exactly these discussions).\n>>\n>> I think for now, I'm going to end up mapping a FrameBuffer from every\n>> buffer_handle_t() for consistency instead.\n>>\n>>>> src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++\n>>>> 1 file changed, 33 insertions(+)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 6212ccdd61ec..f78486117e9f 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -86,6 +86,39 @@ const std::map camera3FormatsMap = {\n>>>> \n>>>> LOG_DECLARE_CATEGORY(HAL);\n>>>> \n>>>> +class MappedCamera3Buffer : public MappedBuffer {\n>>>> +public:\n>>>> +\tMappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags)\n>>>> +\t{\n>>>> +\t\tmaps_.reserve(camera3buffer->numFds);\n>>>> +\t\terror_ = 0;\n>>>> +\n>>>> +\t\tfor (int i = 0; i < camera3buffer->numFds; i++) {\n>>>> +\t\t\tif (camera3buffer->data[i] == -1)\n>>>> +\t\t\t\tcontinue;\n>>>> +\n>>>> +\t\t\toff_t length = lseek(camera3buffer->data[i], 0, SEEK_END);\n>>>> +\t\t\tif (length < 0) {\n>>>> +\t\t\t\terror_ = errno;\n>>>> +\t\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\t}\n>>>> +\n>>>> +\t\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n>>>> +\t\t\t\t\tcamera3buffer->data[i], 0);\n>>>> +\t\t\tif (address == MAP_FAILED) {\n>>>> +\t\t\t\terror_ = errno;\n>>>> +\t\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\t}\n>>>> +\n>>>> +\t\t\tmaps_.push_back({address, static_cast(length)});\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tvalid_ = error_ == 0;\n>>>> +\t}\n>>>> +};\n>>>> +\n>>>> /*\n>>>> * \\struct Camera3RequestDescriptor\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 DB6CABD86F\n\tfor ;\n\tWed, 29 Jul 2020 14:37:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38A3B613C6;\n\tWed, 29 Jul 2020 16:37:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D4156039D\n\tfor ;\n\tWed, 29 Jul 2020 16:37:20 +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 A171A31F;\n\tWed, 29 Jul 2020 16:37:19 +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=\"JBL/u/uQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596033439;\n\tbh=QIUEukdmfwkkI/WycQG8JzYNmWzbexLzktysF1u6n8M=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=JBL/u/uQq6D9IlAri60vwgO8GaTxHp3CWkpWoggIiSvE8ncQkSlUw9aI5fzoQT9X+\n\t7TXSmdKmKxvsFmtbPInPT63g8stN770BzsRfhiYQYAGClv3CUmWlOB8vhnt/71ruO1\n\tMoaA9ZpB9Q0wJBLgE1iAKyYKq6iJBejlKojzbFmc=","To":"Laurent Pinchart ","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-9-kieran.bingham@ideasonboard.com>\n\t<20200724172452.GT5921@pendragon.ideasonboard.com>\n\t\n\t<20200729142720.GD6183@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":"<524713d6-d4c8-d2e9-38a2-eef0f3c07143@ideasonboard.com>","Date":"Wed, 29 Jul 2020 15:37:16 +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":"<20200729142720.GD6183@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","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":11718,"web_url":"https://patchwork.libcamera.org/comment/11718/","msgid":"<20200729144250.GE6183@pendragon.ideasonboard.com>","date":"2020-07-29T14:42:50","subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jul 29, 2020 at 03:37:16PM +0100, Kieran Bingham wrote:\n> On 29/07/2020 15:27, Laurent Pinchart wrote:\n> > On Wed, Jul 29, 2020 at 03:19:24PM +0100, Kieran Bingham wrote:\n> >> On 24/07/2020 18:24, Laurent Pinchart wrote:\n> >>> On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote:\n> >>>> Utilise the MappedBuffer interface to map each of the planes provided\n> >>>> in the Camera3 buffer to facilitate use in software streams.\n> >>>>\n> >>>> The buffers will be automatically unmapped when the object goes out of\n> >>>> scope or is deleted.\n> >>>>\n> >>>> Signed-off-by: Kieran Bingham \n> >>>> ---\n> >>>>\n> >>>> This patch shows an addition of a new MappedBuffer type constructed from\n> >>>> a camera3buffer. The base class deals with the move-constructors and\n> >>>> destructor, and gives us a common interface to pass a set of mapped\n> >>>> dmabufs around.\n> >>>>\n> >>>> I had hoped to use this to pass in the camera3buffer for writing jpeg\n> >>>> buffers to, giving the same interface for both the source and\n> >>>> destination buffer - but for JPEG, I do also need to return the number\n> >>>> of bytes actually consumed, so this ended up potentially not adding the\n> >>>> benefits I hoped for.\n> >>>>\n> >>>> Still, it might still provide some benefits, so I've included it here as\n> >>>> something to talk about.\n> >>>\n> >>> This, along with patch 4/8, is the only part of the series I'm not sure\n> >>> to like :-S It feels quite like a ad-hoc solution, which is probably\n> >>> normal, as that's what it is :-) It may not be that bad, but doesn't\n> >>> qualify for the public API in my opinion. We could keep this as-is for\n> >>> now as an internal helper, until we figure something better (if there's\n> >>> ever a need to do so).\n> >>\n> >> This *isn't* public api - this is under src/android/camera_device.\n> >>\n> >> Perhaps you mean the MappedBuffer API shouldn't be public (Though even\n> >> if the base is still internal, the Android layer can still use this).\n> > \n> > I may also have been confused. I think I was referring to the idea of\n> > creating a base class that can be inherited from, that part is in the\n> > public API. Making it private should be fine, but exposing that design\n> > externally would I think require a bit more work first.\n> > \n> >>> A random idea, would it make sense to convert buffer_handle_t to\n> >>> FrameBuffer as early as possible in the HAL and operate on FrameBuffer ?\n> >>> There's at least one drawback in that it would dup() the fds, which may\n> >>> call for a rework of the FileDescriptor class to make borrowing fds\n> >>> possible. Or maybe it's just a bad idea.\n> >>\n> >> I thnik it was the unnecessary dup's required to construct a FrameBuffer\n> >> that stopped me, but perhaps it might make some sense still.\n> > \n> > We really want to avoid unnecessary dup() calls as much as possible, as\n> > that's expensive. Do you think it would make sense to extend\n> > FileDescriptor to allow borrowing fds, or is that too complicated, or\n> > too hackish ?\n> \n> I don't think it's too hackish necessarily, but I don't know how that\n> could be expressed.\n> \n> And indeed it could make things complicated for lifetimes as suddenly\n> what happens if the other owner closes them etc.. (ok so you could\n> document the requirements for borrowing etc)...\n> \n> I presume we'd have to have some sort of a flag to determine the fd was\n> borrowed and prevent the associated closes on destructor ... it feels a\n> bit awkward ... :S\n\nYes, that's what would be needed, and it would be a bit awkward indeed.\n\n> But I have two paths for 'now':\n> \n> Make a MappedBuffer generic base that can map from a buffer_t\n> \n> or\n> \n> Map all buffer_t's to a FrameBuffer, then make a MappedFrameBuffer from\n> that ...\n> \n> Do you have a particular preference on either?\n> \n> If I construct a FrameBuffer for each buffer_t, we can always look to\n> improve performance later...\n\nAs we agreed it will be an internal API for now, I would avoid dup()s\nalready, at the expense of a future refactoring of the API.\n\n> >>> I'd like to see how this ends up being used for the JPEG encoding to get\n> >>> a better feeling of the API. I thus can't guarantee at this point I'll\n> >>> ack the idea as-is, but I think it's worth continuing the experiment, I\n> >>> feel there's something good here.\n> >>\n> >> The idea was to make sure that the Mapped buffer object was the same\n> >> regardless of whether it came from a FrameBuffer, or from a buffer_handle_t.\n> >>\n> >> Of course mapping the MappedFrameBuffer(FrameBuffer(buffer_handle_t)) is\n> >> a solution to that, it just wastes/duplicates more fds.\n> >>\n> >> See:\n> >>\n> >> [RFC PATCH 6/6] android: camera_device: Provide a MappedCamera3Buffer\n> >>\n> >> (Ugh ,sorry same patch name, this was split from there)\n> >>\n> >> for how this got used to remove the aribtrary mapAndroidBlobBuffer() and\n> >> unmapAndroidBlobBuffer() fucntions that I had created earlier on.\n> >>\n> >> (Kept separate for exactly these discussions).\n> >>\n> >> I think for now, I'm going to end up mapping a FrameBuffer from every\n> >> buffer_handle_t() for consistency instead.\n> >>\n> >>>> src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++\n> >>>> 1 file changed, 33 insertions(+)\n> >>>>\n> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>> index 6212ccdd61ec..f78486117e9f 100644\n> >>>> --- a/src/android/camera_device.cpp\n> >>>> +++ b/src/android/camera_device.cpp\n> >>>> @@ -86,6 +86,39 @@ const std::map camera3FormatsMap = {\n> >>>> \n> >>>> LOG_DECLARE_CATEGORY(HAL);\n> >>>> \n> >>>> +class MappedCamera3Buffer : public MappedBuffer {\n> >>>> +public:\n> >>>> +\tMappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags)\n> >>>> +\t{\n> >>>> +\t\tmaps_.reserve(camera3buffer->numFds);\n> >>>> +\t\terror_ = 0;\n> >>>> +\n> >>>> +\t\tfor (int i = 0; i < camera3buffer->numFds; i++) {\n> >>>> +\t\t\tif (camera3buffer->data[i] == -1)\n> >>>> +\t\t\t\tcontinue;\n> >>>> +\n> >>>> +\t\t\toff_t length = lseek(camera3buffer->data[i], 0, SEEK_END);\n> >>>> +\t\t\tif (length < 0) {\n> >>>> +\t\t\t\terror_ = errno;\n> >>>> +\t\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\t}\n> >>>> +\n> >>>> +\t\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n> >>>> +\t\t\t\t\tcamera3buffer->data[i], 0);\n> >>>> +\t\t\tif (address == MAP_FAILED) {\n> >>>> +\t\t\t\terror_ = errno;\n> >>>> +\t\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\t}\n> >>>> +\n> >>>> +\t\t\tmaps_.push_back({address, static_cast(length)});\n> >>>> +\t\t}\n> >>>> +\n> >>>> +\t\tvalid_ = error_ == 0;\n> >>>> +\t}\n> >>>> +};\n> >>>> +\n> >>>> /*\n> >>>> * \\struct Camera3RequestDescriptor\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 B56DCBD878\n\tfor ;\n\tWed, 29 Jul 2020 14:43:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E52B6114F;\n\tWed, 29 Jul 2020 16:43:02 +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 054576039D\n\tfor ;\n\tWed, 29 Jul 2020 16:43:00 +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 6C9FA31F;\n\tWed, 29 Jul 2020 16:42:59 +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=\"YRIofpAw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596033779;\n\tbh=HpCpe9aBegukMgvxWR04UUD6AQcenIBb6oa2Civ+kb8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YRIofpAwbDVzJWWeDf3xS6DzBqZUiT9Ch2288iZef+mrp1oea5PGBjuDrv8Eu1dwf\n\tF4NMSShTETE0mD9Ynd2tu4UdviFk4Wv9SCEhLFDM6tg8YCnDBE1+D+MQNqvM4Ka6+k\n\trmciDAlsZdGokNw9w0vn3Z1oIJdmBgFd/i/nAGgo=","Date":"Wed, 29 Jul 2020 17:42:50 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200729144250.GE6183@pendragon.ideasonboard.com>","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-9-kieran.bingham@ideasonboard.com>\n\t<20200724172452.GT5921@pendragon.ideasonboard.com>\n\t\n\t<20200729142720.GD6183@pendragon.ideasonboard.com>\n\t<524713d6-d4c8-d2e9-38a2-eef0f3c07143@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<524713d6-d4c8-d2e9-38a2-eef0f3c07143@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 8/8] RFC-Only: android:\n\tcamera_device: Provide a MappedCamera3Buffer","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\" "}}]