[libcamera-devel] [PATCH v3 3/5] ipa: rkisp1: Add Yaml tuning file support

[Jean-Michel Hautbois]

Hi Laurent,

On 19/06/2022 17:47, Laurent Pinchart via libcamera-devel wrote:
> Hi Florian,
> 
> Another question.
> 
> On Fri, Jun 17, 2022 at 08:39:10PM +0300, Laurent Pinchart via libcamera-devel wrote:
>> On Fri, Jun 17, 2022 at 11:23:13AM +0200, Florian Sylvestre wrote:
>>> Retrieve root node in Yaml tuning file and provide to
>>> each algorithm this YamlObject to allow them to grab their default
>>> parameters by calling init() function.
>>>
>>> Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>>> ---
>>>   src/ipa/rkisp1/rkisp1.cpp | 34 ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 34 insertions(+)
>>>
>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>>> index 61a3bab9..5eb23669 100644
>>> --- a/src/ipa/rkisp1/rkisp1.cpp
>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>>> @@ -14,6 +14,7 @@
>>>   #include <linux/rkisp1-config.h>
>>>   #include <linux/v4l2-controls.h>
>>>   
>>> +#include <libcamera/base/file.h>
>>>   #include <libcamera/base/log.h>
>>>   
>>>   #include <libcamera/control_ids.h>
>>> @@ -24,6 +25,7 @@
>>>   #include <libcamera/request.h>
>>>   
>>>   #include "libcamera/internal/mapped_framebuffer.h"
>>> +#include "libcamera/internal/yaml_parser.h"
>>>   
>>>   #include "algorithms/agc.h"
>>>   #include "algorithms/algorithm.h"
>>> @@ -61,6 +63,7 @@ public:
>>>   private:
>>>   	void setControls(unsigned int frame);
>>>   	void prepareMetadata(unsigned int frame, unsigned int aeState);
>>> +	int parseConfigurationFile(File &file);
>>>   
>>>   	std::map<unsigned int, FrameBuffer> buffers_;
>>>   	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>>> @@ -126,6 +129,37 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
>>>   	algorithms_.push_back(std::make_unique<algorithms::Awb>());
>>>   	algorithms_.push_back(std::make_unique<algorithms::BlackLevelCorrection>());
>>>   
>>> +	/* Load the tuning file for this sensor. */
>>> +	File file(settings.configurationFile.c_str());
>>> +	if (!file.open(File::OpenModeFlag::ReadOnly)) {
>>> +		int ret = file.error();
>>> +		LOG(IPARkISP1, Error)
>>> +			<< "Failed to open configuration file "
>>> +			<< settings.configurationFile << ": " << strerror(-ret);
> 
> Let's add quotes here, as otherwise when the file doesn't exist the
> error message isn't very clear:
> 
> [0:41:48.257336778] [318] ERROR IPARkISP1 rkisp1.cpp:136 Failed to open configuration file : No such file or directory
> 
> 		LOG(IPARkISP1, Error)
> 			<< "Failed to open configuration file '"
> 			<< settings.configurationFile << "': "
> 			<< strerror(-ret);
> 
>>> +		return ret;
> 
> This introduces a regression as all platforms that currently work with
> the rkisp1 pipeline handler will fail here.
> 
> I wonder how we should handle this, both short term and long term. In
> the short term we could treat this as a warning and use defaults for the
> algorithms (or just disable them). In the long term, do we want to make
> the tuning file mandatory ?
> 
>>> +	}
>>> +
>>> +	return parseConfigurationFile(file);
>>> +}
>>> +
>>> +int IPARkISP1::parseConfigurationFile(File &file)
>>> +{
>>> +	std::unique_ptr<libcamera::YamlObject> root = YamlParser::parse(file);
>>> +	if (!root)
>>> +		return -EINVAL;
>>> +
>>> +	const YamlObject &algoObject = (*root)["algorithms"];
>>> +
>>> +	if (!algoObject["algorithms"].isDictionary())
>>> +		return -EINVAL;
>>
>> I'm surprised that we go past this, as the tuning data file now has a
>> list for the algorithms element, not a dictionary. I'll test this.
>>
>> This makes me realize that my suggestion of using a list will also cause
>> other issues. I'll experiment a bit. Do you mind if I take over and send
>> a v4, in case that's easier than explaining my findings and asking you
>> to implement them ?

My 2 cents: I don't think we want tuning files to be mandatory, as 
having them means you tuned the sensor you use in the first place. It 
might be the case most of the time, but the algorithms should be able to 
do something relatively correct without those too.

>>
>>> +
>>> +	/* Allow each algo to get parameters from configuration file. */
>>> +	for (auto const &algo : algorithms_) {
>>> +		int ret = algo->init(context_, algoObject);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>>   	return 0;
>>>   }
>>>   
>