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

[Kieran Bingham]

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.


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