[{"id":2137,"web_url":"https://patchwork.libcamera.org/comment/2137/","msgid":"<20190703220847.GK5007@pendragon.ideasonboard.com>","date":"2019-07-03T22:08:47","subject":"Re: [libcamera-devel] [RFC PATCH v2 4/7] libcamera: proxy: add\n\tdefault linux proxy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jul 03, 2019 at 05:00:04PM +0900, Paul Elder wrote:\n> Add a default linux proxy.\n\nIt's a proxy skeleton only, you should mention that in the commit\nmessage, and explicitly state that the proxy protocol itself is missing.\n\n> \n> Signed-off-by: Paul Elder \n> ---\n> New in v2\n> - replaces the dummy/linux default shim\n> \n> src/libcamera/meson.build | 7 +-\n> src/libcamera/proxy/meson.build | 3 +\n> src/libcamera/proxy/proxy_linux_default.cpp | 90 +++++++++++++++++++\n> src/libcamera/proxy_worker/meson.build | 18 ++++\n> .../proxy_linux_default_worker.cpp | 46 ++++++++++\n> 5 files changed, 160 insertions(+), 4 deletions(-)\n> create mode 100644 src/libcamera/proxy/meson.build\n> create mode 100644 src/libcamera/proxy/proxy_linux_default.cpp\n> create mode 100644 src/libcamera/proxy_worker/meson.build\n> create mode 100644 src/libcamera/proxy_worker/proxy_linux_default_worker.cpp\n> \n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 412564f..e470fe7 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -65,10 +65,7 @@ includes = [\n> ]\n> \n> subdir('pipeline')\n> -\n> -proxy_install_dir = join_paths(get_option('libdir'), 'libcamera', 'proxy')\n\nI forgot to mention in my review of 3/7, should this be 'libexecdir' ?\n\nhttp://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html\n\n> -config_h.set('IPA_PROXY_DIR',\n> - '\"' + join_paths(get_option('prefix'), proxy_install_dir) + '\"')\n> +subdir('proxy')\n> \n> libudev = dependency('libudev', required : false)\n> \n> @@ -103,3 +100,5 @@ libcamera = shared_library('camera',\n> libcamera_dep = declare_dependency(sources : [libcamera_api, libcamera_h],\n> include_directories : libcamera_includes,\n> link_with : libcamera)\n> +\n> +subdir('proxy_worker')\n> diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build\n> new file mode 100644\n> index 0000000..508ed5f\n> --- /dev/null\n> +++ b/src/libcamera/proxy/meson.build\n> @@ -0,0 +1,3 @@\n> +libcamera_sources += files([\n> + 'proxy_linux_default.cpp',\n> +])\n> diff --git a/src/libcamera/proxy/proxy_linux_default.cpp b/src/libcamera/proxy/proxy_linux_default.cpp\n> new file mode 100644\n> index 0000000..1de028a\n> --- /dev/null\n> +++ b/src/libcamera/proxy/proxy_linux_default.cpp\n> @@ -0,0 +1,90 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * proxy_linux_default.cpp - Default Image Processing Algorithm proxy for Linux\n> + */\n> +\n> +#include \n> +#include \n\nWhat do you need memory for ?\n\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n\nYou can mix the C and C++ headers. And none of the C headers seem to be\nneeded anymore.\n\n> +\n> +#include \n> +#include \n> +\n> +#include \"ipa_module.h\"\n> +#include \"ipa_proxy.h\"\n> +#include \"ipc_unixsocket.h\"\n> +#include \"log.h\"\n> +#include \"process.h\"\n> +#include \"process_manager.h\"\n\nProcessManager should really be an internal class.\n\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(ProxyLinuxDefault)\n\nShould we define an IPAProxy log category used by all proxies ? I don't\nthink we really need one category per proxy.\n\n> +\n> +class ProxyLinuxDefault : public Proxy\n\nI would name this class IPAProxyLinux to keep it short.\n\n> +{\n> +public:\n> +\tProxyLinuxDefault(IPAModule *ipam);\n> +\t~ProxyLinuxDefault();\n> +\n> +\tint init();\n> +\n> +private:\n> +\tProcess *proc_;\n> +\n> +\tIPCUnixSocket *socket_;\n> +};\n> +\n> +int ProxyLinuxDefault::init()\n> +{\n> +\tstd::cout << \"initializing IPA via dummy proxy!\" << std::endl;\n\nstd::cout, really ? :-) (and you can remove iostream above)\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +ProxyLinuxDefault::ProxyLinuxDefault(IPAModule *ipam)\n> +\t: Proxy(ipam)\n> +{\n> +\tstd::cout << \"initializing dummy proxy: loading IPA from \"\n> +\t\t << ipam->path() << std::endl;\n> +\n> +\tstd::vector fds;\n> +\tstd::vector args;\n> +\targs.push_back(ipam->path());\n> +\tconst std::string path = resolvePath(\"proxy_linux_default\");\n> +\tif (path.empty())\n> +\t\treturn;\n> +\n> +\tsocket_ = new IPCUnixSocket();\n> +\tint fd = socket_->create();\n> +\tif (fd < 0)\n> +\t\treturn;\n\nYou can already connect the readyRead signal of the socket to a member\nfunction of this class.\n\n> +\targs.push_back(std::to_string(fd));\n> +\n> +\tproc_ = new Process();\n> +\tproc_->exec(path, args, fds);\n\nThe fds vector is empty...\n\n> +\n> +\tvalid_ = true;\n> +\treturn;\n\nNo need for an explicit return statement.\n\nThat's a lot of work for a constructor that can't return a value. Would\nit make sense to add a start() virtual method to the base Proxy class in\norder to allow the caller to differentiate between the different error\ncauses ?\n\nBy the way I think the method specific to proxies should have a proxy\nprefix in the base Proxy class, as otherwise they could class with the\nIPAInterface methods. The start() method would then be called\nproxyStart(). What do you think ?\n\n> +}\n> +\n> +ProxyLinuxDefault::~ProxyLinuxDefault()\n> +{\n> +\tif (proc_)\n\nNo need for this check, delete nullptr is a no-op.\n\n> +\t\tdelete proc_;\n\nSounds like you need to do some cleanup in the Process destructor :-) It\nwould make sense to at least send the SIGKILL signal to the child there\n(which I would expose through a public kill() method in the Process\nclass). Ideally it should also wait for the process to finish but that's\na bit more work that could be handled later. It should be fine if you\nrecord that as a \\todo in the Process destructor.\n\n> +\tif (socket_)\n> +\t\tdelete socket_;\n> +}\n> +\n> +REGISTER_IPA_PROXY(ProxyLinuxDefault)\n> +\n> +}; /* namespace libcamera */\n> diff --git a/src/libcamera/proxy_worker/meson.build b/src/libcamera/proxy_worker/meson.build\n> new file mode 100644\n> index 0000000..6f927c5\n> --- /dev/null\n> +++ b/src/libcamera/proxy_worker/meson.build\n> @@ -0,0 +1,18 @@\n> +ipa_proxy_sources = [\n> + ['proxy_linux_default', 'proxy_linux_default_worker.cpp']\n\nipa_proxy_linux and ipa_proxy_linux_worker.cpp ?\n\n> +]\n> +\n> +proxy_install_dir = join_paths(get_option('libdir'), 'libcamera', 'proxy')\n> +\n> +foreach t : ipa_proxy_sources\n> + proxy = executable(t[0],\n> + t[1],\n\nYou could put t[0] and t[1] on the same line.\n\nWeird indentation.\n\n> + name_prefix : '',\n\nIsn't name_prefix empty by default for executable ?\n\n> + include_directories : includes,\n\nYou can set this to libcamera_internal_includes as libcamera_dep implies\nlibcamera_includes already (see below). If we were to make use of\nincludes outside of src/libcamera/meson.build then I would rename it to\nlibcamera_all_includes.\n\n> + install : true,\n> + install_dir : proxy_install_dir,\n> + link_with : libcamera)\n\nCould you use dependencies : libcamera_dep instead of link_with ?\n\n> +endforeach\n> +\n> +config_h.set('IPA_PROXY_DIR',\n> + '\"' + join_paths(get_option('prefix'), proxy_install_dir) + '\"')\n\nShould we merge the proxy_worker directory with the proxy directory ?\nThere will likely be header files shared by the proxies and their\nworkers for the definitions of the proxy protocols.\n\n> diff --git a/src/libcamera/proxy_worker/proxy_linux_default_worker.cpp b/src/libcamera/proxy_worker/proxy_linux_default_worker.cpp\n> new file mode 100644\n> index 0000000..86b7689\n> --- /dev/null\n> +++ b/src/libcamera/proxy_worker/proxy_linux_default_worker.cpp\n> @@ -0,0 +1,46 @@\n\nMissing SPDX + file header.\n\n> +#include \n> +\n\nNo need for a blank line.\n\n> +#include \n> +\n> +#include \n> +\n> +#include \"ipa_module.h\"\n> +#include \"ipc_unixsocket.h\"\n> +#include \"utils.h\"\n> +\n> +using namespace libcamera;\n> +\n> +int main(int argc, char **argv)\n> +{\n> +\tif (argc < 3) {\n> +\t\tstd::cout << \"hello world! no args\" << std::endl;\n\nWe need a log strategy for IPA workers... In the meantime could you use\nLOG() and make sure to unset LIBCAMERA_LOG_FILE before exec'ing the\nprocess in the Process class ? And for debugging purpose it would likely\nbe useful to set LIBCAMERA_LOG_FILE manually here to\n/tmp/libcamera.worker.$pid.log or something similar.\n\n> +\t\treturn 0;\n\n\t\treturn EXIT_FAILURE;\n\n> +\t}\n> +\n> +\tint fd = std::stoi(argv[2]);\n> +\tstd::cout << \"hello world! we're starting! fd = \" << fd << \", path = \" << argv[1] << std::endl;\n\nLet's already use proper message.\n\n\tLOG(IPAProxyWorker, Info)\n\t\t<< \"Starting worker for IPA module \" << argv[1]\n\t\t<< \" with IPC fd \" << fd;\n\n> +\n> +\tstd::unique_ptr ipam = utils::make_unique(argv[1]);\n> +\tif (!ipam->isValid() || !ipam->load()) {\n> +\t\tstd::cerr << \"Houston, we have a problem: IPAModule should be valid but isn't\" << std::endl;\n\n\t\tLOG(IPAProxyWorker, Error)\n\t\t\t<< \"IPA module \" << argv[1] << \" is not valid\";\n\n> +\t\treturn -1;\n\n\t\treturn EXIT_FAILURE;\n\n(and below too).\n\n> +\t}\n> +\n> +\tIPCUnixSocket socket;\n> +\tif (socket.bind(fd) < 0) {\n> +\t\tstd::cerr << \"IPC socket binding failed\" << std::endl;\n> +\t\treturn -1;\n> +\t}\n\nYou can already connect the socket readyRead signal to a local slot\nwhere you can receive() the payload and print a message.\n\n> +\n> +\tstd::unique_ptr ipa = ipam->createInstance();\n> +\tif (!ipa) {\n> +\t\tstd::cerr << \"Failed to create IPA interface\" << std::endl;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tstd::cout << \"hello world from the proxy worker!\" << std::endl;\n> +\n> +\t/* TODO IPC listening loop */\n\nI'll likely add an event loop class soon. In the meantime, you can use\n\n\tEventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();\n\twhile (1)\n\t\tdispatcher->processEvents();\n\n> +\n> +\treturn 0;\n> +}","headers":{"Return-Path":"","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 6991660C01\n\tfor ;\n\tThu, 4 Jul 2019 00:09:08 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D5C9A24B;\n\tThu, 4 Jul 2019 00:09:07 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1562191748;\n\tbh=OWTxmFZnKLnBqtfviiVNCSKlvTOenGYHiohPVGhpU0s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kss7qONfQg9nPoYaZcm/eK1fBv99TmQ29TTICNeQPHqdsku0J9aKn8Z1WiIkyXwwU\n\tcZXvUsWiBhbg8lQZ9NNaelqsun1N14USZJ94L3wRiQr9xu7foauO09JbhmEwqSJph0\n\t5H6Aw0LYlWIY3pK+sjUxH0ejVwPrl134IK0s00gk=","Date":"Thu, 4 Jul 2019 01:08:47 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190703220847.GK5007@pendragon.ideasonboard.com>","References":"<20190703080007.21376-1-paul.elder@ideasonboard.com>\n\t<20190703080007.21376-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190703080007.21376-5-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 4/7] libcamera: proxy: add\n\tdefault linux proxy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 03 Jul 2019 22:09:08 -0000"}}]