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

[Kieran Bingham]

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 at 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