[libcamera-devel,7/9] libcamera: ipu3: Add helper class PipeConfigPreference

15351 diff mbox series

Add helper class PipeConfigPreference to load the caliberated ipu3
pipeline configuration files, and provides query interface for the
pipeline handler.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
---
 include/libcamera/geometry.h                  |   4 +
 src/libcamera/geometry.cpp                    |  20 ++
 src/libcamera/pipeline/ipu3/meson.build       |   1 +
 .../pipeline/ipu3/pipe_config_pref.cpp        | 285 ++++++++++++++++++
 .../pipeline/ipu3/pipe_config_pref.h          |  80 +++++
 5 files changed, 390 insertions(+)
 create mode 100644 src/libcamera/pipeline/ipu3/pipe_config_pref.cpp
 create mode 100644 src/libcamera/pipeline/ipu3/pipe_config_pref.h

Hi Han-Lin,

Thank you for the patch.

On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:
> Add helper class PipeConfigPreference to load the caliberated ipu3
> pipeline configuration files, and provides query interface for the
> pipeline handler.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>  include/libcamera/geometry.h                  |   4 +
>  src/libcamera/geometry.cpp                    |  20 ++

Could you please split the changes to geometry.h and geometry.cpp to a
separate patch ?

>  src/libcamera/pipeline/ipu3/meson.build       |   1 +
>  .../pipeline/ipu3/pipe_config_pref.cpp        | 285 ++++++++++++++++++
>  .../pipeline/ipu3/pipe_config_pref.h          |  80 +++++
>  5 files changed, 390 insertions(+)
>  create mode 100644 src/libcamera/pipeline/ipu3/pipe_config_pref.cpp
>  create mode 100644 src/libcamera/pipeline/ipu3/pipe_config_pref.h
> 
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index 7838b679..ede54981 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -46,6 +46,8 @@ static inline bool operator!=(const Point &lhs, const Point &rhs)
>  	return !(lhs == rhs);
>  }
>  
> +std::ostream &operator<<(std::ostream &out, const Point &d);
> +
>  class Size
>  {
>  public:
> @@ -192,6 +194,8 @@ static inline bool operator>=(const Size &lhs, const Size &rhs)
>  	return !(lhs < rhs);
>  }
>  
> +std::ostream &operator<<(std::ostream &out, const Size &s);
> +
>  class SizeRange
>  {
>  public:
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index cb3c2de1..a65f9f2f 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -83,6 +83,16 @@ bool operator==(const Point &lhs, const Point &rhs)
>   * \return True if the two points are not equal, false otherwise
>   */
>  
> +/**
> + * \brief Insert operation for Point with ostream
> + * \return The input std::ostream
> + */
> +std::ostream &operator<<(std::ostream &out, const Point &p)
> +{
> +	out << "(" << p.x << ", " << p.y << ")";

This doesn't match the format used by Point::toString(), which can be
confusing.

I'm actually wondering if we could use toString() instead of adding
operator<<() overloads. I half-recall having the same discussion a long
time ago and advocating against operator<<(), but I can't recall why. Or
maybe I don't remember correctly, and operator<<() is fine :-) Does
anyone have an opinion on this ?

> +	return out;
> +}
> +
>  /**
>   * \struct Size
>   * \brief Describe a two-dimensional size
> @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)
>   * \sa bool operator<(const Size &lhs, const Size &rhs)
>   */
>  
> +/**
> + * \brief Insert operation for Size with ostream
> + * \return The input std::ostream
> + */
> +std::ostream &operator<<(std::ostream &out, const Size &s)
> +{
> +	out << s.width << "x" << s.height;
> +	return out;
> +}
> +

For completeness, can you add the operators for SizeRange and Rectangle
too ?

I'll review the IPU3 part separately, possibly after updating it to the
next version of the YAML parser class as it will result in some changes.

>  /**
>   * \struct SizeRange
>   * \brief Describe a range of sizes
> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build
> index a1b0b31a..dcc450f0 100644
> --- a/src/libcamera/pipeline/ipu3/meson.build
> +++ b/src/libcamera/pipeline/ipu3/meson.build
> @@ -5,4 +5,5 @@ libcamera_sources += files([
>      'frames.cpp',
>      'imgu.cpp',
>      'ipu3.cpp',
> +    'pipe_config_pref.cpp',
>  ])
> diff --git a/src/libcamera/pipeline/ipu3/pipe_config_pref.cpp b/src/libcamera/pipeline/ipu3/pipe_config_pref.cpp
> new file mode 100644
> index 00000000..5b4a17c9
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/pipe_config_pref.cpp
> @@ -0,0 +1,285 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * pipe_config_pref.cpp - Helper class for IPU3 pipeline config preference
> + */
> +
> +#include "pipe_config_pref.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/internal/yaml_parser.h>
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPU3)
> +
> +namespace {
> +
> +std::ostream &operator<<(std::ostream &out,
> +			 const PipeConfigPreference::PipeConfig &d)
> +{
> +	out << "cio2: " << d.cio2 << " bds: " << d.bds
> +	    << " gdc: " << d.gdc << " iff: " << d.iff
> +	    << " main: " << d.main << " viewfinder: " << d.viewfinder;
> +	return out;
> +}
> +
> +int loadPipeConfig(const YamlObject &yamlObject,
> +		   PipeConfigPreference::PipeConfig &pipeConfig)
> +{
> +	if (!yamlObject.isMember("bds") ||
> +	    !yamlObject.isMember("gdc") ||
> +	    !yamlObject.isMember("iff") ||
> +	    !yamlObject.isMember("cio2") ||
> +	    !yamlObject.isMember("main") ||
> +	    !yamlObject.isMember("viewfinder")) {
> +		LOG(IPU3, Error) << "Missing mandatory attributes in a config";
> +		return -EINVAL;
> +	}
> +
> +	pipeConfig.bds = yamlObject.get("bds").asSize();
> +	pipeConfig.gdc = yamlObject.get("gdc").asSize();
> +	pipeConfig.iff = yamlObject.get("iff").asSize();
> +	pipeConfig.cio2 = yamlObject.get("cio2").asSize();
> +	pipeConfig.main = yamlObject.get("main").asSize();
> +	pipeConfig.viewfinder = yamlObject.get("viewfinder").asSize();
> +
> +	return 0;
> +}
> +
> +int loadPipeConfigs(const YamlObject &yamlPipeConfigs,
> +		    std::vector<PipeConfigPreference::PipeConfig> &pipeConfigs,
> +		    Size &maxResolution, Size &minResolution)
> +{
> +	for (int i = 0; i < yamlPipeConfigs.length(); i++) {
> +		const YamlObject &yamlConfig = yamlPipeConfigs[i];
> +		pipeConfigs.emplace_back();
> +		if (loadPipeConfig(yamlConfig, pipeConfigs.back()))
> +			return -EINVAL;
> +	}
> +
> +	if (pipeConfigs.size() == 0)
> +		return -EINVAL;
> +
> +	maxResolution = minResolution = pipeConfigs[0].main;
> +
> +	for (const PipeConfigPreference::PipeConfig &config : pipeConfigs) {
> +		maxResolution.expandTo(config.main);
> +		minResolution.boundTo(config.main);
> +	}
> +
> +	/* Sort configs by the size of the cio2 */
> +	sort(pipeConfigs.begin(), pipeConfigs.end(),
> +	     [](const auto &a, const auto &b) -> bool {
> +		     return a.cio2 < b.cio2;
> +	     });
> +
> +	return 0;
> +}
> +
> +} /* namespace */
> +
> +PipeConfigPreference::PipeConfigPreference()
> +	: valid_(false)
> +{
> +}
> +
> +/**
> + * \struct PipeConfig
> + * \brief Describe a valid ImgU configuration
> + *
> + * The ImgU unit processes images through several components, which have
> + * to be properly configured inspecting the input image size and the desired
> + * output sizes. This structure collects the ImgU configuration for IF, BDS
> + * and GDC, and the requested main output, viewfinder and the input (CIO2)
> + * resolutions.
> + */
> +
> +/**
> + * \brief Parse the configuration file from a path
> + * \param[in] path The path to the configuration file
> + *
> + * Parse the configuration file from a path and set isValid() to true if
> + * success.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int PipeConfigPreference::parsePreferenceFile(const std::string &path)
> +{
> +	valid_ = false;
> +
> +	FILE *fh = fopen(path.c_str(), "r");
> +	if (!fh) {
> +		LOG(IPU3, Error) << "Fail to open file: " << path;
> +		return -EINVAL;
> +	}
> +
> +	YamlParser yamlParser;
> +	YamlObject yamlObjectPreference;
> +
> +	int ret = yamlParser.ParseAsYamlObject(fh, yamlObjectPreference);
> +	fclose(fh);
> +
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = load(yamlObjectPreference);
> +
> +	if (ret)
> +		return -EINVAL;
> +
> +	valid_ = true;
> +	return 0;
> +}
> +
> +/**
> + * \brief Query the valid pipeline configuration for video and still pipe
> + * \param[in] videoMain The size of main output from video pipe
> + * \param[in] videoViewfinder The size of viewfinder output from video pipe
> + * \param[in] stillMain The size of main output from still pipe
> + * \param[in] stillViewfinder The size of viewfinder output from still pipe
> + * \param[out] videoResult The ImgU setting for video pipe
> + * \param[out] stillResult The ImgU setting for still pipe
> + *
> + * Helper function to query valid settings for ImgU with the desired
> + * output resolutions. The query interface is based on the assumption
> + * that both video and still ImgU might be used together.
> + * An output can be set disabled if not required. If both main and viewfinder
> + * are set disabled for a ImgU, video or still, the corresponding pipeConfig
> + * undefined. For example, a typical usage is to only one video output is
> + * required, the user may set:
> + *
> + * videoMain = [width, height]
> + * videoViewfinder = Disabled
> + * stillMain = Disabled
> + * stillViewfinder = Disabled
> + *
> + * In the case, only the videoResult would be valid, since still pipe is not
> + * used. When both video and still ImgU are used, their cio2 will have the
> + * same resolution, since they should use the same raw capture.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int PipeConfigPreference::queryPipeConfig(
> +	const Size &videoMain, const Size &videoViewfinder,
> +	const Size &stillMain, const Size &stillViewfinder,
> +	PipeConfig &videoResult, PipeConfig &stillResult) const
> +{
> +	bool hasVideo = (videoMain != Disabled) && (videoMain >= videoViewfinder);
> +	bool hasStill = (stillMain != Disabled) && (stillMain >= stillViewfinder);
> +
> +	if (!hasStill && !hasVideo)
> +		return -EINVAL;
> +
> +	std::vector<PipeConfig> validVideoConfigs;
> +	std::vector<PipeConfig> validStillConfigs;
> +
> +	for (auto &config : videoPipeConfigs_) {
> +		if (config.main == videoMain && config.viewfinder == videoViewfinder)
> +			validVideoConfigs.emplace_back(config);
> +	}
> +
> +	for (auto &config : stillPipeConfigs_) {
> +		if (config.main == stillMain && config.viewfinder == stillViewfinder)
> +			validStillConfigs.emplace_back(config);
> +	}
> +
> +	/*
> +	 * Since the configurations are sorted by the size of CIO2, pick
> +	 * the first valid resolution for lower bandwith.
> +	 */
> +	if (hasVideo && !hasStill) {
> +		if (validVideoConfigs.empty())
> +			return -EINVAL;
> +		videoResult = validVideoConfigs[0];
> +		return 0;
> +	}
> +
> +	if (hasStill && !hasVideo) {
> +		if (validStillConfigs.empty())
> +			return -EINVAL;
> +		stillResult = validStillConfigs[0];
> +		return 0;
> +	}
> +
> +	/* (hasVideo && hasStill) */
> +	bool found = false;
> +	for (const PipeConfig &videoConfig : validVideoConfigs) {
> +		for (const PipeConfig &stillConfig : validVideoConfigs) {
> +			if (videoConfig.cio2 == stillConfig.cio2) {
> +				found = true;
> +				videoResult = videoConfig;
> +				stillResult = stillConfig;
> +				break;
> +			}
> +		}
> +	}
> +
> +	return (found) ? 0 : -EINVAL;
> +}
> +
> +void PipeConfigPreference::dump()
> +{
> +	LOG(IPU3, Debug) << "Video Pipe configs: ";
> +	for (auto &configs : videoPipeConfigs_) {
> +		LOG(IPU3, Debug) << configs;
> +	}
> +
> +	LOG(IPU3, Debug) << "Still Pipe configs: ";
> +	for (auto &configs : stillPipeConfigs_) {
> +		LOG(IPU3, Debug) << configs;
> +	}
> +}
> +
> +int PipeConfigPreference::load(const YamlObject &configs)
> +{
> +	/*
> +	 * Load the pipeline configure file properties.
> +	 *
> +	 * Each valid configuration is a list of properties associated
> +	 * with the corresponding IMGU settings and grouped into still
> +	 * and video modes. For each configuration, the main output should
> +	 * be valid, and the viewfinder is optional. If the viewfinder is
> +	 * disabled, its width and height should be set to [0, 0];
> +	 *
> +	 * still_mode:
> +	 * - bds: [width, height]
> +	 *   cio2: [width, height]
> +	 *   gdc: [width, height]
> +	 *   iff: [width, height]
> +	 *   main: [width, height]
> +	 *   viewfinder: [0, 0]
> +	 * ...
> +	 *
> +	 * video_mode:
> +	 * - bds: [width, height]
> +	 *   cio2: [width, height]
> +	 *   gdc: [width, height]
> +	 *   iff: [width, height]
> +	 *   main: [width, height]
> +	 *   viewfinder: [0, 0]
> +	 * ...
> +	 */
> +
> +	if (!configs.isMember("video_mode") || !configs.isMember("still_mode"))
> +		return -EINVAL;
> +
> +	videoPipeConfigs_.clear();
> +	stillPipeConfigs_.clear();
> +
> +	int ret = loadPipeConfigs(configs.get("video_mode"), videoPipeConfigs_,
> +				  maxVideoResolution_, minVideoResolution_);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = loadPipeConfigs(configs.get("still_mode"), stillPipeConfigs_,
> +			      maxStillResolution_, minStillResolution_);
> +	if (ret)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/pipe_config_pref.h b/src/libcamera/pipeline/ipu3/pipe_config_pref.h
> new file mode 100644
> index 00000000..08626d4e
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/pipe_config_pref.h
> @@ -0,0 +1,80 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * pipe_config_pref.h - Helper class for IPU3 pipeline config preference
> + */
> +
> +#pragma once
> +
> +#include <vector>
> +
> +#include <libcamera/geometry.h>
> +
> +namespace libcamera {
> +
> +class YamlObject;
> +
> +class PipeConfigPreference final
> +{
> +public:
> +	constexpr static Size Disabled = Size(0, 0);
> +
> +	struct PipeConfig {
> +		Size cio2;
> +		Size bds;
> +		Size gdc;
> +		Size iff;
> +		Size main;
> +		Size viewfinder;
> +	};
> +
> +	PipeConfigPreference();
> +
> +	int parsePreferenceFile(const std::string &path);
> +	bool isValid() const
> +	{
> +		return valid_;
> +	}
> +	bool invalid()
> +	{
> +		return valid_ = false;
> +	}
> +	Size maxVideoResolution()
> +	{
> +		return maxVideoResolution_;
> +	}
> +	Size maxStillResolution()
> +	{
> +		return maxStillResolution_;
> +	}
> +	Size minVideoResolution()
> +	{
> +		return minVideoResolution_;
> +	}
> +	Size minStillResolution()
> +	{
> +		return minStillResolution_;
> +	}
> +
> +	int queryPipeConfig(const Size &videoMain, const Size &videoViewfinder,
> +			    const Size &stillMain, const Size &stillViewfinder,
> +			    PipeConfig &videoPipeConfig,
> +			    PipeConfig &stillPipeConfig) const;
> +	void dump();
> +
> +private:
> +	int load(const YamlObject &object);
> +	bool valid_;
> +
> +	std::vector<PipeConfig> videoPipeConfigs_;
> +	std::vector<PipeConfig> stillPipeConfigs_;
> +
> +	Size maxVideoResolution_;
> +	Size maxStillResolution_;
> +
> +	Size minVideoResolution_;
> +	Size minStillResolution_;
> +};
> +
> +} /* namespace libcamera */

Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)
> Hi Han-Lin,
> 
> Thank you for the patch.
> 
> On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:
> > Add helper class PipeConfigPreference to load the caliberated ipu3
> > pipeline configuration files, and provides query interface for the
> > pipeline handler.
> > 
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > ---
> >  include/libcamera/geometry.h                  |   4 +
> >  src/libcamera/geometry.cpp                    |  20 ++
> 
> Could you please split the changes to geometry.h and geometry.cpp to a
> separate patch ?
> 

<snip>

> > +/**
> > + * \brief Insert operation for Point with ostream
> > + * \return The input std::ostream
> > + */
> > +std::ostream &operator<<(std::ostream &out, const Point &p)
> > +{
> > +     out << "(" << p.x << ", " << p.y << ")";
> 
> This doesn't match the format used by Point::toString(), which can be
> confusing.
> 
> I'm actually wondering if we could use toString() instead of adding
> operator<<() overloads. I half-recall having the same discussion a long
> time ago and advocating against operator<<(), but I can't recall why. Or
> maybe I don't remember correctly, and operator<<() is fine :-) Does
> anyone have an opinion on this ?

Personally, I like having operator<< implementations, but I think they
should be of the form:


std::ostream &operator<<(std::ostream &out, const Point &p)
{
	return out << p.toString();
}

or such to ensure the implementation is consistent.

If we can have that, then I'd like to add lots more around for other
objects ;-)



> 
> > +     return out;
> > +}
> > +
> >  /**
> >   * \struct Size
> >   * \brief Describe a two-dimensional size
> > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)
> >   * \sa bool operator<(const Size &lhs, const Size &rhs)
> >   */
> >  
> > +/**
> > + * \brief Insert operation for Size with ostream
> > + * \return The input std::ostream
> > + */
> > +std::ostream &operator<<(std::ostream &out, const Size &s)
> > +{
> > +     out << s.width << "x" << s.height;
> > +     return out;
> > +}
> > +
> 
> For completeness, can you add the operators for SizeRange and Rectangle
> too ?

Yes, I would say a single patch should update all objects in geometry to
use the same consistent addition. We can tackle other classes/files
later.

<snip>

Kieran

On Wed, Apr 06, 2022 at 07:47:50PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)
> > Hi Han-Lin,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:
> > > Add helper class PipeConfigPreference to load the caliberated ipu3
> > > pipeline configuration files, and provides query interface for the
> > > pipeline handler.
> > > 
> > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > ---
> > >  include/libcamera/geometry.h                  |   4 +
> > >  src/libcamera/geometry.cpp                    |  20 ++
> > 
> > Could you please split the changes to geometry.h and geometry.cpp to a
> > separate patch ?
> 
> <snip>
> 
> > > +/**
> > > + * \brief Insert operation for Point with ostream
> > > + * \return The input std::ostream
> > > + */
> > > +std::ostream &operator<<(std::ostream &out, const Point &p)
> > > +{
> > > +     out << "(" << p.x << ", " << p.y << ")";
> > 
> > This doesn't match the format used by Point::toString(), which can be
> > confusing.
> > 
> > I'm actually wondering if we could use toString() instead of adding
> > operator<<() overloads. I half-recall having the same discussion a long
> > time ago and advocating against operator<<(), but I can't recall why. Or
> > maybe I don't remember correctly, and operator<<() is fine :-) Does
> > anyone have an opinion on this ?
> 
> Personally, I like having operator<< implementations, but I think they
> should be of the form:
> 
> 
> std::ostream &operator<<(std::ostream &out, const Point &p)
> {
> 	return out << p.toString();
> }
> 
> or such to ensure the implementation is consistent.

I was thinking about that, and then wondered about the efficiency
compared to open-coding it. Maybe we could do it the other way around,
implement toString() based on operator<<() ? Or maybe it doesn't matter
?

> If we can have that, then I'd like to add lots more around for other
> objects ;-)
> 
> > > +     return out;
> > > +}
> > > +
> > >  /**
> > >   * \struct Size
> > >   * \brief Describe a two-dimensional size
> > > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)
> > >   * \sa bool operator<(const Size &lhs, const Size &rhs)
> > >   */
> > >  
> > > +/**
> > > + * \brief Insert operation for Size with ostream
> > > + * \return The input std::ostream
> > > + */
> > > +std::ostream &operator<<(std::ostream &out, const Size &s)
> > > +{
> > > +     out << s.width << "x" << s.height;
> > > +     return out;
> > > +}
> > > +
> > 
> > For completeness, can you add the operators for SizeRange and Rectangle
> > too ?
> 
> Yes, I would say a single patch should update all objects in geometry to
> use the same consistent addition. We can tackle other classes/files
> later.
> 
> <snip>

Quoting Laurent Pinchart (2022-04-07 08:26:48)
> On Wed, Apr 06, 2022 at 07:47:50PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)
> > > Hi Han-Lin,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:
> > > > Add helper class PipeConfigPreference to load the caliberated ipu3
> > > > pipeline configuration files, and provides query interface for the
> > > > pipeline handler.
> > > > 
> > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > > ---
> > > >  include/libcamera/geometry.h                  |   4 +
> > > >  src/libcamera/geometry.cpp                    |  20 ++
> > > 
> > > Could you please split the changes to geometry.h and geometry.cpp to a
> > > separate patch ?
> > 
> > <snip>
> > 
> > > > +/**
> > > > + * \brief Insert operation for Point with ostream
> > > > + * \return The input std::ostream
> > > > + */
> > > > +std::ostream &operator<<(std::ostream &out, const Point &p)
> > > > +{
> > > > +     out << "(" << p.x << ", " << p.y << ")";
> > > 
> > > This doesn't match the format used by Point::toString(), which can be
> > > confusing.
> > > 
> > > I'm actually wondering if we could use toString() instead of adding
> > > operator<<() overloads. I half-recall having the same discussion a long
> > > time ago and advocating against operator<<(), but I can't recall why. Or
> > > maybe I don't remember correctly, and operator<<() is fine :-) Does
> > > anyone have an opinion on this ?
> > 
> > Personally, I like having operator<< implementations, but I think they
> > should be of the form:
> > 
> > 
> > std::ostream &operator<<(std::ostream &out, const Point &p)
> > {
> >       return out << p.toString();
> > }
> > 
> > or such to ensure the implementation is consistent.
> 
> I was thinking about that, and then wondered about the efficiency
> compared to open-coding it. Maybe we could do it the other way around,
> implement toString() based on operator<<() ? Or maybe it doesn't matter
> ?

I think open coding and duplicating the prints leads to a strong chance
of inconsistent prints. I would be surprised if either the compiler can
not detect that this could be inlined, or if the extra function call
overhead here is troublesome compared to the actual generation of the
string.


> 
> > If we can have that, then I'd like to add lots more around for other
> > objects ;-)
> > 
> > > > +     return out;
> > > > +}
> > > > +
> > > >  /**
> > > >   * \struct Size
> > > >   * \brief Describe a two-dimensional size
> > > > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)
> > > >   * \sa bool operator<(const Size &lhs, const Size &rhs)
> > > >   */
> > > >  
> > > > +/**
> > > > + * \brief Insert operation for Size with ostream
> > > > + * \return The input std::ostream
> > > > + */
> > > > +std::ostream &operator<<(std::ostream &out, const Size &s)
> > > > +{
> > > > +     out << s.width << "x" << s.height;
> > > > +     return out;
> > > > +}
> > > > +
> > > 
> > > For completeness, can you add the operators for SizeRange and Rectangle
> > > too ?
> > 
> > Yes, I would say a single patch should update all objects in geometry to
> > use the same consistent addition. We can tackle other classes/files
> > later.
> > 
> > <snip>
> 
> -- 
> Regards,
> 
> Laurent Pinchart

On Thu, Apr 07, 2022 at 10:02:34AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-04-07 08:26:48)
> > On Wed, Apr 06, 2022 at 07:47:50PM +0100, Kieran Bingham wrote:
> > > Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)
> > > > Hi Han-Lin,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:
> > > > > Add helper class PipeConfigPreference to load the caliberated ipu3
> > > > > pipeline configuration files, and provides query interface for the
> > > > > pipeline handler.
> > > > > 
> > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > > > ---
> > > > >  include/libcamera/geometry.h                  |   4 +
> > > > >  src/libcamera/geometry.cpp                    |  20 ++
> > > > 
> > > > Could you please split the changes to geometry.h and geometry.cpp to a
> > > > separate patch ?
> > > 
> > > <snip>
> > > 
> > > > > +/**
> > > > > + * \brief Insert operation for Point with ostream
> > > > > + * \return The input std::ostream
> > > > > + */
> > > > > +std::ostream &operator<<(std::ostream &out, const Point &p)
> > > > > +{
> > > > > +     out << "(" << p.x << ", " << p.y << ")";
> > > > 
> > > > This doesn't match the format used by Point::toString(), which can be
> > > > confusing.
> > > > 
> > > > I'm actually wondering if we could use toString() instead of adding
> > > > operator<<() overloads. I half-recall having the same discussion a long
> > > > time ago and advocating against operator<<(), but I can't recall why. Or
> > > > maybe I don't remember correctly, and operator<<() is fine :-) Does
> > > > anyone have an opinion on this ?
> > > 
> > > Personally, I like having operator<< implementations, but I think they
> > > should be of the form:
> > > 
> > > 
> > > std::ostream &operator<<(std::ostream &out, const Point &p)
> > > {
> > >       return out << p.toString();
> > > }
> > > 
> > > or such to ensure the implementation is consistent.
> > 
> > I was thinking about that, and then wondered about the efficiency
> > compared to open-coding it. Maybe we could do it the other way around,
> > implement toString() based on operator<<() ? Or maybe it doesn't matter
> > ?
> 
> I think open coding and duplicating the prints leads to a strong chance
> of inconsistent prints. I would be surprised if either the compiler can
> not detect that this could be inlined, or if the extra function call
> overhead here is troublesome compared to the actual generation of the
> string.

The reason why I thought we could implement toString() based on
operator<<() instead of the other way around is that toString() already
uses a stringstream in some classes, so it could be more efficient.

> > > If we can have that, then I'd like to add lots more around for other
> > > objects ;-)
> > > 
> > > > > +     return out;
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * \struct Size
> > > > >   * \brief Describe a two-dimensional size
> > > > > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)
> > > > >   * \sa bool operator<(const Size &lhs, const Size &rhs)
> > > > >   */
> > > > >  
> > > > > +/**
> > > > > + * \brief Insert operation for Size with ostream
> > > > > + * \return The input std::ostream
> > > > > + */
> > > > > +std::ostream &operator<<(std::ostream &out, const Size &s)
> > > > > +{
> > > > > +     out << s.width << "x" << s.height;
> > > > > +     return out;
> > > > > +}
> > > > > +
> > > > 
> > > > For completeness, can you add the operators for SizeRange and Rectangle
> > > > too ?
> > > 
> > > Yes, I would say a single patch should update all objects in geometry to
> > > use the same consistent addition. We can tackle other classes/files
> > > later.
> > > 
> > > <snip>

Quoting Laurent Pinchart (2022-04-07 10:04:18)
> On Thu, Apr 07, 2022 at 10:02:34AM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2022-04-07 08:26:48)
> > > On Wed, Apr 06, 2022 at 07:47:50PM +0100, Kieran Bingham wrote:
> > > > Quoting Laurent Pinchart via libcamera-devel (2022-04-06 02:47:27)
> > > > > Hi Han-Lin,
> > > > > 
> > > > > Thank you for the patch.
> > > > > 
> > > > > On Wed, Feb 09, 2022 at 03:19:15PM +0800, Han-Lin Chen wrote:
> > > > > > Add helper class PipeConfigPreference to load the caliberated ipu3
> > > > > > pipeline configuration files, and provides query interface for the
> > > > > > pipeline handler.
> > > > > > 
> > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > > > > ---
> > > > > >  include/libcamera/geometry.h                  |   4 +
> > > > > >  src/libcamera/geometry.cpp                    |  20 ++
> > > > > 
> > > > > Could you please split the changes to geometry.h and geometry.cpp to a
> > > > > separate patch ?
> > > > 
> > > > <snip>
> > > > 
> > > > > > +/**
> > > > > > + * \brief Insert operation for Point with ostream
> > > > > > + * \return The input std::ostream
> > > > > > + */
> > > > > > +std::ostream &operator<<(std::ostream &out, const Point &p)
> > > > > > +{
> > > > > > +     out << "(" << p.x << ", " << p.y << ")";
> > > > > 
> > > > > This doesn't match the format used by Point::toString(), which can be
> > > > > confusing.
> > > > > 
> > > > > I'm actually wondering if we could use toString() instead of adding
> > > > > operator<<() overloads. I half-recall having the same discussion a long
> > > > > time ago and advocating against operator<<(), but I can't recall why. Or
> > > > > maybe I don't remember correctly, and operator<<() is fine :-) Does
> > > > > anyone have an opinion on this ?
> > > > 
> > > > Personally, I like having operator<< implementations, but I think they
> > > > should be of the form:
> > > > 
> > > > 
> > > > std::ostream &operator<<(std::ostream &out, const Point &p)
> > > > {
> > > >       return out << p.toString();
> > > > }
> > > > 
> > > > or such to ensure the implementation is consistent.
> > > 
> > > I was thinking about that, and then wondered about the efficiency
> > > compared to open-coding it. Maybe we could do it the other way around,
> > > implement toString() based on operator<<() ? Or maybe it doesn't matter
> > > ?
> > 
> > I think open coding and duplicating the prints leads to a strong chance
> > of inconsistent prints. I would be surprised if either the compiler can
> > not detect that this could be inlined, or if the extra function call
> > overhead here is troublesome compared to the actual generation of the
> > string.
> 
> The reason why I thought we could implement toString() based on
> operator<<() instead of the other way around is that toString() already
> uses a stringstream in some classes, so it could be more efficient.
> 

Ok - so sounds like it could be an implementation detail when it's used
anyway... unless you want to mandate that they are always coded a set
direction?


> > > > If we can have that, then I'd like to add lots more around for other
> > > > objects ;-)
> > > > 
> > > > > > +     return out;
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * \struct Size
> > > > > >   * \brief Describe a two-dimensional size
> > > > > > @@ -428,6 +438,16 @@ bool operator<(const Size &lhs, const Size &rhs)
> > > > > >   * \sa bool operator<(const Size &lhs, const Size &rhs)
> > > > > >   */
> > > > > >  
> > > > > > +/**
> > > > > > + * \brief Insert operation for Size with ostream
> > > > > > + * \return The input std::ostream
> > > > > > + */
> > > > > > +std::ostream &operator<<(std::ostream &out, const Size &s)
> > > > > > +{
> > > > > > +     out << s.width << "x" << s.height;
> > > > > > +     return out;
> > > > > > +}
> > > > > > +
> > > > > 
> > > > > For completeness, can you add the operators for SizeRange and Rectangle
> > > > > too ?
> > > > 
> > > > Yes, I would say a single patch should update all objects in geometry to
> > > > use the same consistent addition. We can tackle other classes/files
> > > > later.
> > > > 
> > > > <snip>
> 
> -- 
> Regards,
> 
> Laurent Pinchart