[libcamera-devel] [PATCH LIBCAMERA v3] libcamera: utils: adapt libcameraPath to match use cases

[Kaaira Gupta]

On Thu, Mar 19, 2020 at 02:05:23PM +0000, Kieran Bingham wrote:
> Hi Laurent, Kaaira,
> 
> On 19/03/2020 13:35, Laurent Pinchart wrote:
> > Hi Kaaira,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Mar 19, 2020 at 06:12:52PM +0530, Kaaira Gupta wrote:
> >> The two callers of functions libcameraPath() and isLibcameraInstalled()
> >> end up using the same process and finally use the path with
> >> libcamera.so. Hence write a function libcameraBuildPath() which
> >> combines their functions and returns the root of the build sources
> >> when the library has not been installed, but is running,thereby making
> > 
> > s/running,/running, /
> > 
> > We don't charge extra for white space :-)
> 
> Ooops I missed that one.
> 
> > 
> >> call sites simpler.
> >>
> >> When the library is imstalled, libcameraBuildPath() will return an empty
> > 
> > s/imstalled/installed/
> > 
> >> string.
> >>
> >> Make changes in the call sites accordingly.
> >>
> >> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> >> ---
> >> Changes since v2:
> >> 	- rename subject line and add necessary details in commit
> >> 	  message.
> >> 	- Solve whitespace issues.
> >> 	- specify the final 'slash' for directory path in return
> >> 	  statement itself.
> >>
> >> Changes since v1:
> >>         - rename function to libcameraBuildPath() instead, and return
> >>           the root of libcamera source, and not the source directory
> >>           root.
> >>         - Return empty string instead of nullptr when ret==0
> >>
> >>  src/libcamera/include/utils.h |  2 +-
> >>  src/libcamera/ipa_manager.cpp |  5 +++--
> >>  src/libcamera/ipa_proxy.cpp   |  6 +++---
> >>  src/libcamera/utils.cpp       | 16 +++++++++-------
> >>  4 files changed, 16 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> >> index bc96e79..5dea8d2 100644
> >> --- a/src/libcamera/include/utils.h
> >> +++ b/src/libcamera/include/utils.h
> >> @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim);
> >>  
> >>  bool isLibcameraInstalled();
> > 
> > This function isn't used externally anymore, I would drop it from the
> > header.
> 
> but it /could/ be used. It's not a static, and other locations might
> want to be able to detect this in the future.
> 
> That was sort of the reason of keeping the two functions separate rather
> than merging them.
> 
> Still it could easily be added back in if needed in the future too ...

Yes, we can drop it for now. I'll do that.

> 
> >>  
> >> -std::string libcameraPath();
> >> +std::string libcameraBuildPath();
> >>  
> >>  } /* namespace utils */
> >>  
> >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> >> index 0bd280c..bcaae35 100644
> >> --- a/src/libcamera/ipa_manager.cpp
> >> +++ b/src/libcamera/ipa_manager.cpp
> >> @@ -119,8 +119,9 @@ IPAManager::IPAManager()
> >>  	 * path for the IPA from that point. We need to recurse one level of
> >>  	 * sub-directories to match the build tree.
> >>  	 */
> >> -	if (!utils::isLibcameraInstalled()) {
> >> -		std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa";
> >> +	std::string root = utils::libcameraBuildPath();
> >> +	if (!root.empty()) {
> >> +		std::string ipaBuildPath = root + "src/ipa";
> > 
> > Very neat :-)
> > 
> >>  		constexpr int maxDepth = 1;
> >>  
> >>  		LOG(IPAManager, Info)
> >> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> >> index 2f866cc..5fd88a4 100644
> >> --- a/src/libcamera/ipa_proxy.cpp
> >> +++ b/src/libcamera/ipa_proxy.cpp
> >> @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const
> >>  	 * This requires identifying the path of the libcamera.so, and
> >>  	 * referencing a relative path for the proxy workers from that point.
> >>  	 */
> >> -	if (!utils::isLibcameraInstalled()) {
> >> -		std::string ipaProxyDir = utils::dirname(utils::libcameraPath())
> >> -					  + "/proxy/worker";
> >> +	std::string root = utils::libcameraBuildPath();
> >> +	if (!root.empty()) {
> >> +		std::string ipaProxyDir = root + "src/libcamera/proxy/worker";
> >>  
> >>  		LOG(IPAProxy, Info)
> >>  			<< "libcamera is not installed. Loading proxy workers from'"
> >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> >> index 7e118fa..42c5c76 100644
> >> --- a/src/libcamera/utils.cpp
> >> +++ b/src/libcamera/utils.cpp
> >> @@ -347,16 +347,18 @@ bool isLibcameraInstalled()
> >>   *
> >>   * \return A string stating the path
> >>   */
> >> -std::string libcameraPath()
> >> +std::string libcameraBuildPath()
> >>  {
> >> -	Dl_info info;
> >> +	if (!isLibcameraInstalled()) {
> >> +		Dl_info info;
> >>  
> >> -	/* Look up our own symbol. */
> >> -	int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info);
> >> -	if (ret == 0)
> >> -		return nullptr;
> >> +		/* Look up our own symbol. */
> >> +		int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info);
> >> +		if (ret)
> >> +			return dirname(info.dli_fname) + "/../../";
> >> +	}
> >>  
> >> -	return info.dli_fname;
> >> +	return std::string();
> > 
> > Maybe it's my personal preference, but I find code easier to read when
> > dealing away with errors immediately:
> > 
> > 	if (isLibcameraInstalled())
> > 		return std::string();
> > 
> > 	Dl_info info;
> > 
> > 	/* Look up our own symbol. */
> > 	int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info);
> > 	if (!ret)
> > 		return std::string();
> > 
> > 	return dirname(info.dli_fname) + "/../../";
> > 
> > This way you can reduce indentation for most of the code that implements
> > the real logic.
> > 
> > If you're fine with this, I'm sure Kieran can change it while applying.
> 
> Hrm, I usually look to reduce the number of return paths ... but I'm not
> fussed either way. It's only because I had to work on MISRA projects in
> the past.
> 
> Kaaira - up to you really I think. What do you prefer? If you like
> Laurent's version - please send a v4 with the other fixes and I can test
> and apply it.

I think I should change it, that way the code looks cleaner. I'll send
the updated version.

> 
> --
> Kieran
> 
> 
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> >>  }
> >>  
> >>  } /* namespace utils */
> >