[libcamera-devel] [PATCH LIBCAMERA v3 4/6] libcamera: ipa_proxy: search for proxy in build tree

[Laurent Pinchart]

Hi Kaaira,

Thank you for the patch.

On Wed, Mar 18, 2020 at 05:28:44PM +0530, Kaaira Gupta wrote:
> When libcamera is built and tested before installing, it will
> be unable to locate the path to proxy files, or previously

I would write "the path to the proxy workers" (proxy workers are the
standalone executables that load an IPA module and handle communication
with the main libcamera process).

> installed files in the system path may be incorrect to load.
> 
> Hence, when libcamera is not installed and running from a build

This is a bit ambiguous, it would be understood as "not installed and
not running from a build tree". How about "when libcamera is not
installed, but is running from a build tree" ?

> tree, identify the location of that tree by using libcameraPath(), and
> from that point add relative path to the proxy file.

s/file/workers directory/ (the workers are all contained in a
directory).

> 
> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> ---
>  src/libcamera/ipa_proxy.cpp | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> index b409e1d..1014e79 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -91,10 +91,23 @@ std::string IPAProxy::resolvePath(const std::string &file) const
>  		}
>  	}
>  
> -	/* Try finding the exec target from the install directory. */
> -	std::string proxyPath = std::string(IPA_PROXY_DIR) + proxyFile;
> -	if (!access(proxyPath.c_str(), X_OK))
> -		return proxyPath;

This shouldn't be removed, otherwise, when libcamera *is* installed,
we won't load the proxies from the normal installation directory. It
should instead be moved after the !installed case.

> +	/*
> +	 * When libcamera is used before it is installed, load proxies from the

s/proxies/proxy workers/

> +	 * same build directory as the libcamera directory itself. This requires
> +	 * identifying the path of the libcamera.so, and referencing a relative
> +	 * path for the proxies from that point.

s/proxies/proxy workers/

> +	 */
> +	if (!utils::isLibcameraInstalled()) {
> +		std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) + "/../../proxy";

This isn't the correct path. In the source tree, the workers are located
in src/libcamera/proxy/worker/. In the build directory that should thus
be + "/proxy/worker".

Could you also wrap this line at a 80 columns limit ?

		std::string ipaProxyDir = utils::dirname(utils::libcameraPath())
					+ "/proxy/worker";

> +
> +		LOG(IPAProxy, Info)
> +			<< "libcamera is not installed. Adding '"
> +			<< ipaProxyDir << "' to the Proxy search path";

Here you're not adding the directory to a search path, but you will end
up using that directory only (IPA modules can be searched in multiple
paths, proxy workers don't need that mechanism). I would write this

		LOG(IPAProxy, Info)
			<< "libcamera is not installed. Loading proxy workers from '"
			<< ipaProxyDir << "'";

> +
> +		std::string proxyPath = ipaProxyDir + proxyFile;
> +		if (!access(proxyPath.c_str(), X_OK))
> +			return proxyPath;

I think you need a

		return std::string();

here, at least if you move the IPA_PROXY_DIR code from above to below
the !installed case. The reason is that proxy workers are tightly
coupled with libcamera. IPA modules can be developped independently and
libcamera will maintain ABI stability, so loading them from the system
directory makes sense as a fallback option even running a fresh
libcamera build from the build directory. For workers, we don't want the
ones installed on the system to be used at all if libcamera is not
installed, there's no use case for that.

> +	}
>  
>  	return std::string();
>  }

-- 
Regards,

Laurent Pinchart