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

[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 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
> > ?
> 
> 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>

-- 
Regards,

Laurent Pinchart