[libcamera-devel,RFC,v2,4/7] libcamera: proxy: add default linux proxy

1592 diff mbox series

Add a default linux proxy.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
New in v2
- replaces the dummy/linux default shim

 src/libcamera/meson.build                     |  7 +-
 src/libcamera/proxy/meson.build               |  3 +
 src/libcamera/proxy/proxy_linux_default.cpp   | 90 +++++++++++++++++++
 src/libcamera/proxy_worker/meson.build        | 18 ++++
 .../proxy_linux_default_worker.cpp            | 46 ++++++++++
 5 files changed, 160 insertions(+), 4 deletions(-)
 create mode 100644 src/libcamera/proxy/meson.build
 create mode 100644 src/libcamera/proxy/proxy_linux_default.cpp
 create mode 100644 src/libcamera/proxy_worker/meson.build
 create mode 100644 src/libcamera/proxy_worker/proxy_linux_default_worker.cpp

Hi Paul,

Thank you for the patch.

On Wed, Jul 03, 2019 at 05:00:04PM +0900, Paul Elder wrote:
> Add a default linux proxy.

It's a proxy skeleton only, you should mention that in the commit
message, and explicitly state that the proxy protocol itself is missing.

> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> New in v2
> - replaces the dummy/linux default shim
> 
>  src/libcamera/meson.build                     |  7 +-
>  src/libcamera/proxy/meson.build               |  3 +
>  src/libcamera/proxy/proxy_linux_default.cpp   | 90 +++++++++++++++++++
>  src/libcamera/proxy_worker/meson.build        | 18 ++++
>  .../proxy_linux_default_worker.cpp            | 46 ++++++++++
>  5 files changed, 160 insertions(+), 4 deletions(-)
>  create mode 100644 src/libcamera/proxy/meson.build
>  create mode 100644 src/libcamera/proxy/proxy_linux_default.cpp
>  create mode 100644 src/libcamera/proxy_worker/meson.build
>  create mode 100644 src/libcamera/proxy_worker/proxy_linux_default_worker.cpp
> 
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 412564f..e470fe7 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -65,10 +65,7 @@ includes = [
>  ]
>  
>  subdir('pipeline')
> -
> -proxy_install_dir = join_paths(get_option('libdir'), 'libcamera', 'proxy')

I forgot to mention in my review of 3/7, should this be 'libexecdir' ?

http://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html

> -config_h.set('IPA_PROXY_DIR',
> -             '"' + join_paths(get_option('prefix'), proxy_install_dir) + '"')
> +subdir('proxy')
>  
>  libudev = dependency('libudev', required : false)
>  
> @@ -103,3 +100,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/meson.build b/src/libcamera/proxy/meson.build
> new file mode 100644
> index 0000000..508ed5f
> --- /dev/null
> +++ b/src/libcamera/proxy/meson.build
> @@ -0,0 +1,3 @@
> +libcamera_sources += files([
> +    'proxy_linux_default.cpp',
> +])
> diff --git a/src/libcamera/proxy/proxy_linux_default.cpp b/src/libcamera/proxy/proxy_linux_default.cpp
> new file mode 100644
> index 0000000..1de028a
> --- /dev/null
> +++ b/src/libcamera/proxy/proxy_linux_default.cpp
> @@ -0,0 +1,90 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * proxy_linux_default.cpp - Default Image Processing Algorithm proxy for Linux
> + */
> +
> +#include <iostream>
> +#include <memory>

What do you need memory for ?

> +#include <vector>
> +
> +#include <sched.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +#include <unistd.h>

You can mix the C and C++ headers. And none of the C headers seem to be
needed anymore.

> +
> +#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"
> +#include "process_manager.h"

ProcessManager should really be an internal class.

> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(ProxyLinuxDefault)

Should we define an IPAProxy log category used by all proxies ? I don't
think we really need one category per proxy.

> +
> +class ProxyLinuxDefault : public Proxy

I would name this class IPAProxyLinux to keep it short.

> +{
> +public:
> +	ProxyLinuxDefault(IPAModule *ipam);
> +	~ProxyLinuxDefault();
> +
> +	int init();
> +
> +private:
> +	Process *proc_;
> +
> +	IPCUnixSocket *socket_;
> +};
> +
> +int ProxyLinuxDefault::init()
> +{
> +	std::cout << "initializing IPA via dummy proxy!" << std::endl;

std::cout, really ? :-) (and you can remove iostream above)

> +
> +	return 0;
> +}
> +
> +ProxyLinuxDefault::ProxyLinuxDefault(IPAModule *ipam)
> +	: Proxy(ipam)
> +{
> +	std::cout << "initializing dummy proxy: loading IPA from "
> +		  << ipam->path() << std::endl;
> +
> +	std::vector<int> fds;
> +	std::vector<std::string> args;
> +	args.push_back(ipam->path());
> +	const std::string path = resolvePath("proxy_linux_default");
> +	if (path.empty())
> +		return;
> +
> +	socket_ = new IPCUnixSocket();
> +	int fd = socket_->create();
> +	if (fd < 0)
> +		return;

You can already connect the readyRead signal of the socket to a member
function of this class.

> +	args.push_back(std::to_string(fd));
> +
> +	proc_ = new Process();
> +	proc_->exec(path, args, fds);

The fds vector is empty...

> +
> +	valid_ = true;
> +	return;

No need for an explicit return statement.

That's a lot of work for a constructor that can't return a value. Would
it make sense to add a start() virtual method to the base Proxy class in
order to allow the caller to differentiate between the different error
causes ?

By the way I think the method specific to proxies should have a proxy
prefix in the base Proxy class, as otherwise they could class with the
IPAInterface methods. The start() method would then be called
proxyStart(). What do you think ?

> +}
> +
> +ProxyLinuxDefault::~ProxyLinuxDefault()
> +{
> +	if (proc_)

No need for this check, delete nullptr is a no-op.

> +		delete proc_;

Sounds like you need to do some cleanup in the Process destructor :-) It
would make sense to at least send the SIGKILL signal to the child there
(which I would expose through a public kill() method in the Process
class). Ideally it should also wait for the process to finish but that's
a bit more work that could be handled later. It should be fine if you
record that as a \todo in the Process destructor.

> +	if (socket_)
> +		delete socket_;
> +}
> +
> +REGISTER_IPA_PROXY(ProxyLinuxDefault)
> +
> +}; /* namespace libcamera */
> diff --git a/src/libcamera/proxy_worker/meson.build b/src/libcamera/proxy_worker/meson.build
> new file mode 100644
> index 0000000..6f927c5
> --- /dev/null
> +++ b/src/libcamera/proxy_worker/meson.build
> @@ -0,0 +1,18 @@
> +ipa_proxy_sources = [
> +    ['proxy_linux_default', 'proxy_linux_default_worker.cpp']

ipa_proxy_linux and ipa_proxy_linux_worker.cpp ?

> +]
> +
> +proxy_install_dir = join_paths(get_option('libdir'), 'libcamera', 'proxy')
> +
> +foreach t : ipa_proxy_sources
> +    proxy = executable(t[0],
> +                          t[1],

You could put t[0] and t[1] on the same line.

Weird indentation.

> +                          name_prefix : '',

Isn't name_prefix empty by default for executable ?

> +                          include_directories : includes,

You can set this to libcamera_internal_includes as libcamera_dep implies
libcamera_includes already (see below). If we were to make use of
includes outside of src/libcamera/meson.build then I would rename it to
libcamera_all_includes.

> +                          install : true,
> +                          install_dir : proxy_install_dir,
> +                          link_with : libcamera)

Could you use dependencies : libcamera_dep instead of link_with ?

> +endforeach
> +
> +config_h.set('IPA_PROXY_DIR',
> +             '"' + join_paths(get_option('prefix'), proxy_install_dir) + '"')

Should we merge the proxy_worker directory with the proxy directory ?
There will likely be header files shared by the proxies and their
workers for the definitions of the proxy protocols.

> diff --git a/src/libcamera/proxy_worker/proxy_linux_default_worker.cpp b/src/libcamera/proxy_worker/proxy_linux_default_worker.cpp
> new file mode 100644
> index 0000000..86b7689
> --- /dev/null
> +++ b/src/libcamera/proxy_worker/proxy_linux_default_worker.cpp
> @@ -0,0 +1,46 @@

Missing SPDX + file header.

> +#include <iostream>
> +

No need for a blank line.

> +#include <unistd.h>
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +
> +#include "ipa_module.h"
> +#include "ipc_unixsocket.h"
> +#include "utils.h"
> +
> +using namespace libcamera;
> +
> +int main(int argc, char **argv)
> +{
> +	if (argc < 3) {
> +		std::cout << "hello world! no args" << std::endl;

We need a log strategy for IPA workers... In the meantime could you use
LOG() and make sure to unset LIBCAMERA_LOG_FILE before exec'ing the
process in the Process class ? And for debugging purpose it would likely
be useful to set LIBCAMERA_LOG_FILE manually here to
/tmp/libcamera.worker.$pid.log or something similar.

> +		return 0;

		return EXIT_FAILURE;

> +	}
> +
> +	int fd = std::stoi(argv[2]);
> +	std::cout << "hello world! we're starting! fd = " << fd << ", path = " << argv[1] << std::endl;

Let's already use proper message.

	LOG(IPAProxyWorker, Info)
		<< "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()) {
> +		std::cerr << "Houston, we have a problem: IPAModule should be valid but isn't" << std::endl;

		LOG(IPAProxyWorker, Error)
			<< "IPA module " << argv[1] << " is not valid";

> +		return -1;

		return EXIT_FAILURE;

(and below too).

> +	}
> +
> +	IPCUnixSocket socket;
> +	if (socket.bind(fd) < 0) {
> +		std::cerr << "IPC socket binding failed" << std::endl;
> +		return -1;
> +	}

You can already connect the socket readyRead signal to a local slot
where you can receive() the payload and print a message.

> +
> +	std::unique_ptr<IPAInterface> ipa = ipam->createInstance();
> +	if (!ipa) {
> +		std::cerr << "Failed to create IPA interface" << std::endl;
> +		return -1;
> +	}
> +
> +	std::cout << "hello world from the proxy worker!" << std::endl;
> +
> +	/* TODO IPC listening loop */

I'll likely add an event loop class soon. In the meantime, you can use

	EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
	while (1)
		dispatcher->processEvents();

> +
> +	return 0;
> +}