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

16260 diff mbox series

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@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/rkisp1/rkisp1.cpp | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Hi Florian,

Thank you for the patch.

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@baylibre.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@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);
> +		return ret;
> +	}
> +
> +	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 ?

> +
> +	/* 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;
>  }
>

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@baylibre.com>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Reviewed-by: Paul Elder <paul.elder@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;
>>>   }
>>>   
>

Hi Jean-Michel,

On Mon, Jun 20, 2022 at 07:20:07AM +0200, Jean-Michel Hautbois wrote:
> On 19/06/2022 17:47, Laurent Pinchart via libcamera-devel wrote:
> > 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@baylibre.com>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Reviewed-by: Paul Elder <paul.elder@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.

I went with an approach influenced by Raspberry Pi, which as an
"uncalibrated.json" file. v4 includes an "uncalibrated.yaml" file that
the pipeline handler selects if no other tuning file can be found. Let's
discuss whether that is a good or bad idea in the review of v4.

> >>> +
> >>> +	/* 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;
> >>>   }
> >>>