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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart