[{"id":3107,"web_url":"https://patchwork.libcamera.org/comment/3107/","msgid":"<d1778194-e470-dedb-4b89-327b84c4dbf6@ideasonboard.com>","date":"2019-11-19T17:06:02","subject":"Re: [libcamera-devel] [PATCH v2 00/24] Control serialization and\n\tIPA C API","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nAs far as I can see, there are only a few minor comments across this\nseries, and has been reviewed extensively enough to say lets get this\nin, with the minors resolved.\n\nA topic such as this, which is so intrinsic to the rest of the system\nwill only block further development, so I think it's important that we\nget this in soon so that development and testing can continue on top.\n\nThanks for tackling such a large series.\n--\nKieran\n\n\n\n\nOn 08/11/2019 20:53, Laurent Pinchart wrote:\n> Hello,\n> \n> This patch series turns the IPA API towards IPA modules to a plain C\n> API. The rationale is explained in patch 19/24:\n> \n> The C++ objects that are expected to convey data through the IPA API\n> will have associated methods that would require IPAs to link to\n> libcamera. Even though the libcamera license allows this, suppliers of\n> closed-source IPAs may have a different interpretation. To ease their\n> mind and clearly separate vendor code and libcamera code, define a plain\n> C IPA API. The corresponding C objects are stored in plain C structures\n> or have their binary format documented, removing the need for linking to\n> libcamera code on the IPA side.\n> \n> Patches 01/24 to 12/24 are preparatory changes. Patches 01/24 to 03/24\n> add a test case for and fix a bug in the ControlInfoMap. Patches 04/24\n> to 12/24 then rework and expand the BufferMemory and control classes to\n> make translation between the C++ and C API possible.\n> \n> A plain C API requires storing all data passed to IPA context operations\n> in plain C data structures. For most of the data used in the\n> IPAInterface methods, this only requires defining corresponding C\n> structures and replacing std::vector and std::map with arrays. However,\n> for ControlInfoMap and ControlList, a different approach is needed due\n> to their complexity.\n> \n> Patch 13/24 defines a serialization format for those two classes, and\n> patches 14/24 to 17/24 implement serialization and deserialization\n> (including a test case).\n> \n> Patches 18/24 to 20/24 then define a plain C IPA API and switch to it.\n> Patch 21/24 is a small improvement to compile time test coverage related\n> to IPA modules, and patch 22/24 allows short-circuiting the C API for\n> IPA modules that are loaded without isolation. Patch 23/24 adds a test\n> case, and patch 24/24 finally fixes a typo.\n> \n> I would like to thank both Kieran and Jacopo for all their preparatory\n> work on this. We all spend lots of time burning brain cells, and\n> hopefully the result will make everybody happy.\n> \n> Jacopo Mondi (6):\n>   libcamera: controls: Make ControlId constructor public\n>   libcamera: controls: Store reference to the InfoMap\n>   ipa: Define serialized controls\n>   libcamera: Add ByteStreamBuffer\n>   test: Add control serialization test\n>   ipa: Switch to the plain C API\n> \n> Laurent Pinchart (18):\n>   test: Extract CameraTest class out of camera tests to libtest\n>   test: controls: Add ControlInfoMap test\n>   libcamera: controls: Avoid exception in ControlList count() and find()\n>   libcamera: controls: Add operator== and operator!= to ControlRange\n>   libcamera: controls: Index ControlList by unsigned int\n>   libcamera: controls: Add move constructor to ControlInfoMap\n>   libcamera: controls: Make ControList constructor public\n>   libcamera: controls: Catch type mismatch in ControlInfoMap\n>   libcamera: v4l2_controls: Fix control range construction for bool\n>   libcamera: buffer: Add const accessor to Buffer planes\n>   test: Add ByteStreamBuffer test\n>   libcamera: Add controls serializer\n>   ipa: Pass ControlInfoMap references to IPAInterface::configure()\n>   ipa: Define a plain C API\n>   ipa: Declare the ipaCreate() function prototype\n>   ipa: Allow short-circuiting the ipa_context_ops\n>   test: ipa: Add IPA wrappers test\n>   libcamera: Fix typo related to serialization\n> \n>  Documentation/Doxyfile.in                     |   1 +\n>  Documentation/meson.build                     |   2 +\n>  include/ipa/ipa_controls.h                    |  54 ++\n>  include/ipa/ipa_interface.h                   |  82 ++-\n>  include/ipa/meson.build                       |   1 +\n>  include/libcamera/buffer.h                    |   1 +\n>  include/libcamera/controls.h                  |  50 +-\n>  src/ipa/ipa_vimc.cpp                          |   9 +-\n>  src/ipa/libipa/ipa_interface_wrapper.cpp      | 248 +++++++++\n>  src/ipa/libipa/ipa_interface_wrapper.h        |  57 ++\n>  src/ipa/libipa/meson.build                    |  13 +\n>  src/ipa/meson.build                           |   3 +\n>  src/ipa/rkisp1/meson.build                    |   3 +-\n>  src/ipa/rkisp1/rkisp1.cpp                     |   9 +-\n>  src/libcamera/buffer.cpp                      |   6 +\n>  src/libcamera/byte_stream_buffer.cpp          | 286 ++++++++++\n>  src/libcamera/camera_controls.cpp             |   4 +-\n>  src/libcamera/control_serializer.cpp          | 501 ++++++++++++++++++\n>  src/libcamera/controls.cpp                    | 139 +++--\n>  src/libcamera/include/byte_stream_buffer.h    |  63 +++\n>  src/libcamera/include/camera_controls.h       |   2 +-\n>  src/libcamera/include/control_serializer.h    |  52 ++\n>  src/libcamera/include/control_validator.h     |   2 +-\n>  src/libcamera/include/ipa_context_wrapper.h   |  47 ++\n>  src/libcamera/include/ipa_module.h            |   5 +-\n>  src/libcamera/include/meson.build             |   3 +\n>  src/libcamera/ipa_context_wrapper.cpp         | 249 +++++++++\n>  src/libcamera/ipa_controls.cpp                | 187 +++++++\n>  src/libcamera/ipa_interface.cpp               | 325 ++++++++++--\n>  src/libcamera/ipa_manager.cpp                 |  67 ++-\n>  src/libcamera/ipa_module.cpp                  |  23 +-\n>  src/libcamera/meson.build                     |   4 +\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   2 +-\n>  src/libcamera/pipeline/uvcvideo.cpp           |   4 +-\n>  src/libcamera/pipeline/vimc.cpp               |   4 +-\n>  src/libcamera/proxy/ipa_proxy_linux.cpp       |   2 +-\n>  .../proxy/worker/ipa_proxy_linux_worker.cpp   |   8 +-\n>  src/libcamera/v4l2_controls.cpp               |  14 +-\n>  src/libcamera/v4l2_device.cpp                 |  23 +-\n>  test/byte-stream-buffer.cpp                   | 172 ++++++\n>  test/camera/buffer_import.cpp                 |  14 +-\n>  test/camera/capture.cpp                       |  15 +-\n>  test/camera/configuration_default.cpp         |  16 +-\n>  test/camera/configuration_set.cpp             |  15 +-\n>  test/camera/meson.build                       |   2 +-\n>  test/camera/statemachine.cpp                  |  15 +-\n>  test/controls/control_info.cpp                |  77 +++\n>  test/controls/control_list.cpp                |  39 +-\n>  test/controls/meson.build                     |   1 +\n>  test/ipa/ipa_wrappers_test.cpp                | 390 ++++++++++++++\n>  test/ipa/meson.build                          |   5 +-\n>  test/{camera => libtest}/camera_test.cpp      |  24 +-\n>  test/{camera => libtest}/camera_test.h        |  12 +-\n>  test/libtest/meson.build                      |   1 +\n>  test/meson.build                              |   2 +\n>  test/serialization/control_serialization.cpp  | 161 ++++++\n>  test/serialization/meson.build                |  11 +\n>  test/serialization/serialization_test.cpp     |  89 ++++\n>  test/serialization/serialization_test.h       |  33 ++\n>  59 files changed, 3432 insertions(+), 217 deletions(-)\n>  create mode 100644 include/ipa/ipa_controls.h\n>  create mode 100644 src/ipa/libipa/ipa_interface_wrapper.cpp\n>  create mode 100644 src/ipa/libipa/ipa_interface_wrapper.h\n>  create mode 100644 src/ipa/libipa/meson.build\n>  create mode 100644 src/libcamera/byte_stream_buffer.cpp\n>  create mode 100644 src/libcamera/control_serializer.cpp\n>  create mode 100644 src/libcamera/include/byte_stream_buffer.h\n>  create mode 100644 src/libcamera/include/control_serializer.h\n>  create mode 100644 src/libcamera/include/ipa_context_wrapper.h\n>  create mode 100644 src/libcamera/ipa_context_wrapper.cpp\n>  create mode 100644 src/libcamera/ipa_controls.cpp\n>  create mode 100644 test/byte-stream-buffer.cpp\n>  create mode 100644 test/controls/control_info.cpp\n>  create mode 100644 test/ipa/ipa_wrappers_test.cpp\n>  rename test/{camera => libtest}/camera_test.cpp (55%)\n>  rename test/{camera => libtest}/camera_test.h (77%)\n>  create mode 100644 test/serialization/control_serialization.cpp\n>  create mode 100644 test/serialization/meson.build\n>  create mode 100644 test/serialization/serialization_test.cpp\n>  create mode 100644 test/serialization/serialization_test.h\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["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 7C92760BEA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 19 Nov 2019 18:06:05 +0100 (CET)","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 D2681311;\n\tTue, 19 Nov 2019 18:06:04 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1574183165;\n\tbh=PUbwt+/zwrqtgM9i8aYxl6fKiPu+qO6/RzDncS7e+YI=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=nrYyhzVGWMUq2DEPIdho0TAIqCeOKCbaTxcMc+5u+ENnoGykqro1PRtmBB4gQn2lK\n\tcvZyBSpd4a9omTvrC9TrZCR5eCigq4KItiOZPVNVN6pBjZUmx82glvW/HZ+O7kaJq9\n\tlUHHFVXIWmgtoaazs6FbkNOPBuinhYoqdtCAvyN0=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","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":"<d1778194-e470-dedb-4b89-327b84c4dbf6@ideasonboard.com>","Date":"Tue, 19 Nov 2019 17:06:02 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 00/24] Control serialization and\n\tIPA C API","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 19 Nov 2019 17:06:05 -0000"}}]