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

[Laurent Pinchart]

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

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>

-- 
Regards,

Laurent Pinchart