[libcamera-devel,libcamera-devel,3/5] ipa: rkisp1: Add Yaml configuration file support
diff mbox series

Message ID 20220523092435.475510-4-fsylvestre@baylibre.com
State Superseded
Headers show
Series
  • Add configuration file support for rkisp1 blc algo
Related show

Commit Message

Florian Sylvestre May 23, 2022, 9:24 a.m. UTC 
Retrieve root node in Yaml configuration 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>
---
 src/ipa/rkisp1/algorithms/algorithm.h |  4 ++-
 src/ipa/rkisp1/rkisp1.cpp             | 36 +++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart May 25, 2022, 11:23 p.m. UTC | #1 
Hi Florian,

Thank you for the patch.

On Mon, May 23, 2022 at 11:24:33AM +0200, Florian Sylvestre via libcamera-devel wrote:
> Retrieve root node in Yaml configuration 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>
> ---
>  src/ipa/rkisp1/algorithms/algorithm.h |  4 ++-
>  src/ipa/rkisp1/rkisp1.cpp             | 36 +++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> index d46c3188..5b95fd30 100644
> --- a/src/ipa/rkisp1/algorithms/algorithm.h
> +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> @@ -17,9 +17,11 @@
>  
>  namespace libcamera {
>  
> +class YamlObject;
> +
>  namespace ipa::rkisp1 {
>  
> -using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
> +using Algorithm = libcamera::ipa::Algorithm<IPAContext, YamlObject, IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
>  
>  } /* namespace ipa::rkisp1 */
>  
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 23cb95b5..c44514c2 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -24,6 +24,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 +62,7 @@ public:
>  private:
>  	void setControls(unsigned int frame);
>  	void prepareMetadata(unsigned int frame, unsigned int aeState);
> +	int parseConfigurationFile(FILE *fh);
>  
>  	std::map<unsigned int, FrameBuffer> buffers_;
>  	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> @@ -126,6 +128,40 @@ 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 *fh = fopen(settings.configurationFile.c_str(), "r");
> +	if (!fh) {
> +		int ret = -errno;
> +		LOG(IPARkISP1, Error) << "Failed to open configuration file "

"tuning file" here too, as in patch 1/5 ?

> +				      << settings.configurationFile << ": " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	int ret = parseConfigurationFile(fh);
> +
> +	fclose(fh);

I've submitted a patch series that switches from FILE to File,
simplyfing error handling and cleanup. Could you rebase this series on
top of it for a v2 ?

> +	if (ret)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +int IPARkISP1::parseConfigurationFile(FILE *fh)
> +{
> +	std::unique_ptr<libcamera::YamlObject> root = YamlParser::parse(fh);
> +	if (!root)
> +		return -EINVAL;
> +
> +	if (!root->isDictionary())
> +		return -EINVAL;
> +
> +	/* Allow each algo to get parameters from configuration file. */
> +	for (auto const &algo : algorithms_) {
> +		int ret = algo->init(context_, root.get());
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>
Florian Sylvestre June 1, 2022, 8:13 a.m. UTC | #2 
>> +                                   << settings.configurationFile << ":
" << strerror(-ret);
>> +             return ret;
>> +     }
>> +
>> +     int ret = parseConfigurationFile(fh);
>> +
>> +     fclose(fh);
>I've submitted a patch series that switches from FILE to File,
>simplyfing error handling and cleanup. Could you rebase this series on
>top of it for a v2 ?
The issue that I have with that is YamlParser is consuming std::FILE
header, and if I did not see how to retrieve the file header while using
File Class (I suppose it's expected). So, do you have a hint (other than
updating YamlParser Class)?

Regards,
Florian.

Le jeu. 26 mai 2022 à 01:23, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> a écrit :

> Hi Florian,
>
> Thank you for the patch.
>
> On Mon, May 23, 2022 at 11:24:33AM +0200, Florian Sylvestre via
> libcamera-devel wrote:
> > Retrieve root node in Yaml configuration 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>
> > ---
> >  src/ipa/rkisp1/algorithms/algorithm.h |  4 ++-
> >  src/ipa/rkisp1/rkisp1.cpp             | 36 +++++++++++++++++++++++++++
> >  2 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h
> b/src/ipa/rkisp1/algorithms/algorithm.h
> > index d46c3188..5b95fd30 100644
> > --- a/src/ipa/rkisp1/algorithms/algorithm.h
> > +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> > @@ -17,9 +17,11 @@
> >
> >  namespace libcamera {
> >
> > +class YamlObject;
> > +
> >  namespace ipa::rkisp1 {
> >
> > -using Algorithm = libcamera::ipa::Algorithm<IPAContext,
> IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
> > +using Algorithm = libcamera::ipa::Algorithm<IPAContext, YamlObject,
> IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
> >
> >  } /* namespace ipa::rkisp1 */
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 23cb95b5..c44514c2 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -24,6 +24,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 +62,7 @@ public:
> >  private:
> >       void setControls(unsigned int frame);
> >       void prepareMetadata(unsigned int frame, unsigned int aeState);
> > +     int parseConfigurationFile(FILE *fh);
> >
> >       std::map<unsigned int, FrameBuffer> buffers_;
> >       std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> > @@ -126,6 +128,40 @@ 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 *fh = fopen(settings.configurationFile.c_str(), "r");
> > +     if (!fh) {
> > +             int ret = -errno;
> > +             LOG(IPARkISP1, Error) << "Failed to open configuration
> file "
>
> "tuning file" here too, as in patch 1/5 ?
>
> > +                                   << settings.configurationFile << ":
> " << strerror(-ret);
> > +             return ret;
> > +     }
> > +
> > +     int ret = parseConfigurationFile(fh);
> > +
> > +     fclose(fh);
>
> I've submitted a patch series that switches from FILE to File,
> simplyfing error handling and cleanup. Could you rebase this series on
> top of it for a v2 ?
>
> > +     if (ret)
> > +             return -EINVAL;
> > +
> > +     return 0;
> > +}
> > +
> > +int IPARkISP1::parseConfigurationFile(FILE *fh)
> > +{
> > +     std::unique_ptr<libcamera::YamlObject> root =
> YamlParser::parse(fh);
> > +     if (!root)
> > +             return -EINVAL;
> > +
> > +     if (!root->isDictionary())
> > +             return -EINVAL;
> > +
> > +     /* Allow each algo to get parameters from configuration file. */
> > +     for (auto const &algo : algorithms_) {
> > +             int ret = algo->init(context_, root.get());
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       return 0;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart June 8, 2022, 9:54 p.m. UTC | #3 
Hi Florian,

On Wed, Jun 01, 2022 at 10:13:09AM +0200, Florian Sylvestre wrote:
> >> +                                   << settings.configurationFile << ": " << strerror(-ret);
> >> +             return ret;
> >> +     }
> >> +
> >> +     int ret = parseConfigurationFile(fh);
> >> +
> >> +     fclose(fh);
> > 
> > I've submitted a patch series that switches from FILE to File,
> > simplyfing error handling and cleanup. Could you rebase this series on
> > top of it for a v2 ?
> 
> The issue that I have with that is YamlParser is consuming std::FILE
> header, and if I did not see how to retrieve the file header while using
> File Class (I suppose it's expected). So, do you have a hint (other than
> updating YamlParser Class)?

You can use the
https://gitlab.com/ideasonboard/nxp/libcamera/-/tree/pinchartl/yaml
branch which contains a rework of the YamlParser class. In particular,
https://gitlab.com/ideasonboard/nxp/libcamera/-/commit/40ec4b367c74252bb8a8ac4d81fed8068cac0b6f
switches from FILE to File. I expect the series to get merged soon.

> Le jeu. 26 mai 2022 à 01:23, Laurent Pinchart a écrit :
> > On Mon, May 23, 2022 at 11:24:33AM +0200, Florian Sylvestre wrote:
> > > Retrieve root node in Yaml configuration 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>
> > > ---
> > >  src/ipa/rkisp1/algorithms/algorithm.h |  4 ++-
> > >  src/ipa/rkisp1/rkisp1.cpp             | 36 +++++++++++++++++++++++++++
> > >  2 files changed, 39 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> > > index d46c3188..5b95fd30 100644
> > > --- a/src/ipa/rkisp1/algorithms/algorithm.h
> > > +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> > > @@ -17,9 +17,11 @@
> > >
> > >  namespace libcamera {
> > >
> > > +class YamlObject;
> > > +
> > >  namespace ipa::rkisp1 {
> > >
> > > -using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
> > > +using Algorithm = libcamera::ipa::Algorithm<IPAContext, YamlObject, IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
> > >
> > >  } /* namespace ipa::rkisp1 */
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 23cb95b5..c44514c2 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -24,6 +24,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 +62,7 @@ public:
> > >  private:
> > >       void setControls(unsigned int frame);
> > >       void prepareMetadata(unsigned int frame, unsigned int aeState);
> > > +     int parseConfigurationFile(FILE *fh);
> > >
> > >       std::map<unsigned int, FrameBuffer> buffers_;
> > >       std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> > > @@ -126,6 +128,40 @@ 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 *fh = fopen(settings.configurationFile.c_str(), "r");
> > > +     if (!fh) {
> > > +             int ret = -errno;
> > > +             LOG(IPARkISP1, Error) << "Failed to open configuration file "
> >
> > "tuning file" here too, as in patch 1/5 ?
> >
> > > +                                   << settings.configurationFile << ":
> > " << strerror(-ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     int ret = parseConfigurationFile(fh);
> > > +
> > > +     fclose(fh);
> >
> > I've submitted a patch series that switches from FILE to File,
> > simplyfing error handling and cleanup. Could you rebase this series on
> > top of it for a v2 ?
> >
> > > +     if (ret)
> > > +             return -EINVAL;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +int IPARkISP1::parseConfigurationFile(FILE *fh)
> > > +{
> > > +     std::unique_ptr<libcamera::YamlObject> root = YamlParser::parse(fh);
> > > +     if (!root)
> > > +             return -EINVAL;
> > > +
> > > +     if (!root->isDictionary())
> > > +             return -EINVAL;
> > > +
> > > +     /* Allow each algo to get parameters from configuration file. */
> > > +     for (auto const &algo : algorithms_) {
> > > +             int ret = algo->init(context_, root.get());
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > >       return 0;
> > >  }
> > >

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
index d46c3188..5b95fd30 100644
--- a/src/ipa/rkisp1/algorithms/algorithm.h
+++ b/src/ipa/rkisp1/algorithms/algorithm.h
@@ -17,9 +17,11 @@ 
 
 namespace libcamera {
 
+class YamlObject;
+
 namespace ipa::rkisp1 {
 
-using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
+using Algorithm = libcamera::ipa::Algorithm<IPAContext, YamlObject, IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
 
 } /* namespace ipa::rkisp1 */
 
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 23cb95b5..c44514c2 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -24,6 +24,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 +62,7 @@  public:
 private:
 	void setControls(unsigned int frame);
 	void prepareMetadata(unsigned int frame, unsigned int aeState);
+	int parseConfigurationFile(FILE *fh);
 
 	std::map<unsigned int, FrameBuffer> buffers_;
 	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
@@ -126,6 +128,40 @@  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 *fh = fopen(settings.configurationFile.c_str(), "r");
+	if (!fh) {
+		int ret = -errno;
+		LOG(IPARkISP1, Error) << "Failed to open configuration file "
+				      << settings.configurationFile << ": " << strerror(-ret);
+		return ret;
+	}
+
+	int ret = parseConfigurationFile(fh);
+
+	fclose(fh);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+int IPARkISP1::parseConfigurationFile(FILE *fh)
+{
+	std::unique_ptr<libcamera::YamlObject> root = YamlParser::parse(fh);
+	if (!root)
+		return -EINVAL;
+
+	if (!root->isDictionary())
+		return -EINVAL;
+
+	/* Allow each algo to get parameters from configuration file. */
+	for (auto const &algo : algorithms_) {
+		int ret = algo->init(context_, root.get());
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }