[libcamera-devel] [PATCH v3 2/6] libcamera: ipa_manager: Split path handling

[Kieran Bingham]

Hi Laurent,

On 20/02/2020 20:27, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Feb 20, 2020 at 04:57:00PM +0000, Kieran Bingham wrote:
>> Move the iteration of a colon-separated path to its own function.
>> This prepares for parsing extra path variables.
>>

In fact, we no longer parse 'extra' path variables.

Also - once addDir is changed to not return errors, this functionality
really is just simple enough to be inlined and avoid all the extra
documentation and layer required for little gain.

Very likely dropping this patch.

--

Kieran

>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/include/ipa_manager.h |  1 +
>>  src/libcamera/ipa_manager.cpp       | 35 ++++++++++++++++++++++-------
>>  2 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
>> index 126f8babbc8f..94d35d9062e4 100644
>> --- a/src/libcamera/include/ipa_manager.h
>> +++ b/src/libcamera/include/ipa_manager.h
>> @@ -33,6 +33,7 @@ private:
>>  	~IPAManager();
>>  
>>  	int addDir(const char *libDir);
>> +	int addPath(const char *path);
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index 4ffbdd712ac2..d87f2221b00b 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -110,14 +110,7 @@ IPAManager::IPAManager()
>>  		return;
>>  	}
>>  
>> -	for (const auto &dir : utils::split(modulePaths, ":")) {
>> -		if (dir.empty())
>> -			continue;
>> -
>> -		int ret = addDir(dir.c_str());
>> -		if (ret > 0)
>> -			ipaCount += ret;
>> -	}
>> +	ipaCount += addPath(modulePaths);
>>  
>>  	if (!ipaCount)
>>  		LOG(IPAManager, Warning)
>> @@ -197,6 +190,32 @@ int IPAManager::addDir(const char *libDir)
>>  	return count;
>>  }
>>  
>> +/**
>> + * \brief Load IPA modules from a search path
>> + * \param[in] path The colon-separated list of directories to load IPA modules from
>> + *
>> + * This method tries to create an IPAModule instance for every shared object
>> + * found in the directories listed in \a path.
>> + *
>> + * \return Number of modules loaded by this call, or a negative error code
>> + * otherwise
> 
> The function never returns a negative error code. I would drop the part
> after the comma, and change the function return type and the ipaCount to
> be unsigned int.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> + */
>> +int IPAManager::addPath(const char *path)
>> +{
>> +	int ipaCount = 0;
>> +
>> +	for (const auto &dir : utils::split(path, ":")) {
>> +		if (dir.empty())
>> +			continue;
>> +
>> +		int ret = addDir(dir.c_str());
>> +		if (ret > 0)
>> +			ipaCount += ret;
>> +	}
>> +
>> +	return ipaCount;
>> +}
>> +
>>  /**
>>   * \brief Create an IPA interface that matches a given pipeline handler
>>   * \param[in] pipe The pipeline handler that wants a matching IPA interface
> 

-- 
Regards
--
Kieran