[libcamera-devel] [PATCH v3 4/7] libcamera: proxy: add default linux IPA proxy
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 11 08:18:42 CEST 2019
Hi Paul,
Thank you for the patch.
On Wed, Jul 10, 2019 at 03:44:47AM +0900, Paul Elder wrote:
> Add a skeletal default linux IPA proxy. It currently lacks the IPA proxy
> protocol itself.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v3:
> - better logging (both main proxy and proxy worker)
> - renamed ProxyLinuxDefault to IPAProxyLinux
> - initialize listeners for IPC
>
> New in v2
> - replaces the dummy/linux default shim
>
> src/libcamera/meson.build | 7 +-
> src/libcamera/proxy/ipa_proxy_linux.cpp | 96 +++++++++++++++++++
> src/libcamera/proxy/meson.build | 3 +
> .../proxy/worker/ipa_proxy_linux_worker.cpp | 89 +++++++++++++++++
> src/libcamera/proxy/worker/meson.build | 16 ++++
> 5 files changed, 207 insertions(+), 4 deletions(-)
> create mode 100644 src/libcamera/proxy/ipa_proxy_linux.cpp
> create mode 100644 src/libcamera/proxy/meson.build
> create mode 100644 src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp
> create mode 100644 src/libcamera/proxy/worker/meson.build
>
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 73a6fc7..57238fe 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -64,10 +64,7 @@ includes = [
> ]
>
> subdir('pipeline')
> -
> -proxy_install_dir = join_paths(get_option('libdir'), 'libcamera', 'proxy')
> -config_h.set('IPA_PROXY_DIR',
> - '"' + join_paths(get_option('prefix'), proxy_install_dir) + '"')
> +subdir('proxy')
>
> libudev = dependency('libudev', required : false)
>
> @@ -111,3 +108,5 @@ libcamera = shared_library('camera',
> libcamera_dep = declare_dependency(sources : [libcamera_api, libcamera_h],
> include_directories : libcamera_includes,
> link_with : libcamera)
> +
> +subdir('proxy/worker')
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> new file mode 100644
> index 0000000..1f982f7
> --- /dev/null
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_proxy_linux.cpp - Default Image Processing Algorithm proxy for Linux
> + */
> +
> +#include <vector>
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/ipa_module_info.h>
> +
> +#include "ipa_module.h"
> +#include "ipa_proxy.h"
> +#include "ipc_unixsocket.h"
> +#include "log.h"
> +#include "process.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPAProxy)
> +
> +class IPAProxyLinux : public IPAProxy
> +{
> +public:
> + IPAProxyLinux(IPAModule *ipam);
> + ~IPAProxyLinux();
> +
> + int init();
> +
> +private:
> + void readyRead(IPCUnixSocket *ipc);
> +
> + Process *proc_;
> +
> + IPCUnixSocket *socket_;
> +};
> +
> +int IPAProxyLinux::init()
> +{
> + LOG(IPAProxy, Debug) << "initializing IPA via dummy proxy!";
> +
> + return 0;
> +}
> +
> +IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)
> +{
> + LOG(IPAProxy, Debug)
> + << "initializing dummy proxy: loading IPA from "
> + << ipam->path();
> +
> + std::vector<int> fds;
> + std::vector<std::string> args;
> + args.push_back(ipam->path());
> + const std::string path = resolvePath("ipa_proxy_linux");
> + if (path.empty()) {
> + LOG(IPAProxy, Error)
> + << "Failed to get IPAProxyLinux worker path";
As the file name is printed in the log, I would drop mentions of
IPAProxyLinux. "Failed to get proxy worker path".
> + return;
> + }
> +
> + socket_ = new IPCUnixSocket();
> + int fd = socket_->create();
> + if (fd < 0) {
> + LOG(IPAProxy, Error)
> + << "Failed to create socket for IPAProxyLinux";
And "Failed to create socket".
> + return;
> + }
> + args.push_back(std::to_string(fd));
> + fds.push_back(fd);
> +
> + proc_ = new Process();
> + int ret = proc_->start(path, args, fds);
> + if (ret) {
> + LOG(IPAProxy, Error)
> + << "Failed to start IPAProxyLinux worker process";
> + return;
> + }
> + socket_->readyRead.connect(this, &IPAProxyLinux::readyRead);
Let's move this right after creating the socket.
> +
> + valid_ = true;
> +}
> +
> +IPAProxyLinux::~IPAProxyLinux()
> +{
> + delete proc_;
> + delete socket_;
> +}
> +
> +void IPAProxyLinux::readyRead(IPCUnixSocket *ipc)
> +{
> +}
> +
> +REGISTER_IPA_PROXY(IPAProxyLinux)
> +
> +}; /* namespace libcamera */
> diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build
> new file mode 100644
> index 0000000..efc1132
> --- /dev/null
> +++ b/src/libcamera/proxy/meson.build
> @@ -0,0 +1,3 @@
> +libcamera_sources += files([
> + 'ipa_proxy_linux.cpp',
> +])
> diff --git a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp
> new file mode 100644
> index 0000000..adf4511
> --- /dev/null
> +++ b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp
> @@ -0,0 +1,89 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_proxy_linux_worker.cpp - Default Image Processing Algorithm proxy worker for Linux
> + */
> +
> +#include <iostream>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/ipa/ipa_interface.h>
> +
> +#include "ipa_module.h"
> +#include "ipc_unixsocket.h"
> +#include "log.h"
> +#include "utils.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(IPAProxyLinuxWorker)
> +
> +void readyRead(IPCUnixSocket *ipc)
> +{
> + IPCUnixSocket::Payload message;
> + int ret;
> +
> + ret = ipc->receive(&message);
> + if (ret) {
> + LOG(IPAProxyLinuxWorker, Error)
> + << "Receive message failed: " << ret;
> + return;
> + }
> +
> + LOG(IPAProxyLinuxWorker, Debug) << "Received a message!";
> +}
> +
> +int main(int argc, char **argv)
> +{
> + std::string logPath = "/tmp/libcamera.worker." + std::to_string(getpid()) + ".log";
> + int ret = setenv("LIBCAMERA_LOG_FILE", logPath.c_str(), 1);
> + if (ret < 0) {
> + ret = errno;
> + std::cerr << "Failed to read log file env variable" << std::endl;
This won't go anywhere as stderr is closed, so I think you can drop
this. And maybe even drop error checking for setenv().
> + return ret;
return EXIT_FAILURE;
> + }
Looks good to me, but you forgot to unset the variable in the Process
class :-)
> +
> + if (argc < 3) {
> + LOG(IPAProxyLinuxWorker, Debug)
> + << "Tried to start worker with no args";
> + return EXIT_FAILURE;
> + }
Hmmmm... thinking out loud, this will only happen if you try to start
the worker manually, in which case it will leave a log file in /tmp/
that will likely go unnoticed. Should we only set LIBCAMERA_LOG_FILE if
stdout is closed ? This could be checked with fstat(). But that's not
guaranteed to work, as stdout could have been closed and reused for the
IPC socket fd. Nevermind, let's keep it as-is.
> +
> + int fd = std::stoi(argv[2]);
> + LOG(IPAProxyLinuxWorker, Debug)
> + << "Starting worker for IPA module " << argv[1]
> + << " with IPC fd = " << fd;
> +
> + std::unique_ptr<IPAModule> ipam = utils::make_unique<IPAModule>(argv[1]);
> + if (!ipam->isValid() || !ipam->load()) {
> + LOG(IPAProxyLinuxWorker, Error)
> + << "IPAModule " << argv[1] << " should be valid but isn't";
> + return EXIT_FAILURE;
> + }
> +
> + IPCUnixSocket socket;
> + if (socket.bind(fd) < 0) {
> + LOG(IPAProxyLinuxWorker, Error) << "IPC socket binding failed";
> + return EXIT_FAILURE;
> + }
> + socket.readyRead.connect(&readyRead);
> +
> + std::unique_ptr<IPAInterface> ipa = ipam->createInstance();
> + if (!ipa) {
> + LOG(IPAProxyLinuxWorker, Error) << "Failed to create IPA interface";
> + return EXIT_FAILURE;
This error could be reported to libcamera through IPC, but that's for
later, when we'll define the IPA IPC protocol.
> + }
> +
> + LOG(IPAProxyLinuxWorker, Debug) << "Proxy worker successfully started!";
s/!//
> +
> + /* TODO upgrade listening loop */
s/TODO/\\todo/
With these small issues fixed,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> + while (1)
> + dispatcher->processEvents();
> +
> + return 0;
> +}
> diff --git a/src/libcamera/proxy/worker/meson.build b/src/libcamera/proxy/worker/meson.build
> new file mode 100644
> index 0000000..839156f
> --- /dev/null
> +++ b/src/libcamera/proxy/worker/meson.build
> @@ -0,0 +1,16 @@
> +ipa_proxy_sources = [
> + ['ipa_proxy_linux', 'ipa_proxy_linux_worker.cpp']
> +]
> +
> +proxy_install_dir = join_paths(get_option('libexecdir'), 'libcamera')
> +
> +foreach t : ipa_proxy_sources
> + proxy = executable(t[0], t[1],
> + include_directories : libcamera_internal_includes,
> + install : true,
> + install_dir : proxy_install_dir,
> + dependencies : libcamera_dep)
> +endforeach
> +
> +config_h.set('IPA_PROXY_DIR',
> + '"' + join_paths(get_option('prefix'), proxy_install_dir) + '"')
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list