[libcamera-devel,v3,4/7] libcamera: proxy: add default linux IPA proxy

1637 diff mbox series

Add a skeletal default linux IPA proxy. It currently lacks the IPA proxy
protocol itself.

Signed-off-by: Paul Elder <paul.elder@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

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@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@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) + '"')