[libcamera-devel] [PATCH v3 1/3] libcamera: formats: PixelFormatInfo: Add name lookup function

[Laurent Pinchart]

On Mon, Jul 27, 2020 at 06:33:41PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Jul 27, 2020 at 04:30:01PM +0100, Kieran Bingham wrote:
> > On 27/07/2020 16:18, Kaaira Gupta wrote:
> > > Add a function which returns a format, given its name as a string.
> > > 
> > > Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> > > ---
> > >  include/libcamera/internal/formats.h |  1 +
> > >  src/libcamera/formats.cpp            | 20 ++++++++++++++++++++
> > >  2 files changed, 21 insertions(+)
> > > 
> > > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> > > index 0bb1510..2589e88 100644
> > > --- a/include/libcamera/internal/formats.h
> > > +++ b/include/libcamera/internal/formats.h
> > > @@ -38,6 +38,7 @@ public:
> > >  
> > >  	static const PixelFormatInfo &info(const PixelFormat &format);
> > >  	static const PixelFormatInfo &info(const V4L2PixelFormat &format);
> > > +	static const PixelFormat &info(const std::string &name);
> > 
> > Very close, but I think this is a layering violation.
> > 
> > Note the return type of the other two info() calls, I would expect this
> > function to have the same type.
> 
> I would expect this function to be part of the PixelFormat class
> 
> 	static PixelFormat fromString(const std::string &name);
> 
> and be called with
> 
> 	PixelFormat format = PixelFormat::fromString("YUYV");

I see that's what patch 2/2 implements :-) I wonder if we need a
name-based lookup in the PixelFormatInfo class, it should be enough to
implement it in PixelFormat only. However, as the PixelFormat class
doesn't have access to the pixelFormatInfo map, that's not something we
could implement without some refactoring. I'm thus fine with a lookup
function here.

> > >  	unsigned int stride(unsigned int width, unsigned int plane,
> > >  			    unsigned int align = 1) const;
> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > index 11774b0..8ea5461 100644
> > > --- a/src/libcamera/formats.cpp
> > > +++ b/src/libcamera/formats.cpp
> > > @@ -23,6 +23,8 @@ namespace libcamera {
> > >  
> > >  LOG_DEFINE_CATEGORY(Formats)
> > >  
> > > +const PixelFormat invalidPixelFormat = PixelFormat();
> > > +
> > >  /**
> > >   * \class PixelFormatPlaneInfo
> > >   * \brief Information about a single plane of a pixel format
> > > @@ -663,6 +665,24 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
> > >  	return info->second;
> > >  }
> > >  
> > > +/**
> > > + * \brief Retrieve pixel format from its name
> > > + * \param[in] name The name of pixel format
> > > + * \return The pixel format corresponding to \a name if known, or an invalid
> > > + * pixel format otherwise
> > > + */
> > > +const PixelFormat &PixelFormatInfo::info(const std::string &name)
> > > +{
> > > +	auto it = pixelFormatInfo.begin();
> > > +	while (it != pixelFormatInfo.end()) {
> > 
> > Can this be written as:
> > 
> >  	for (const PixelFormatInfo &info : pixelFormatInfo) {
> > 	...
> > 	}
> > 
> > There's probably not much in it, except for not needing to use manual
> > iterators then.
> 
> And it looks nicer (to me at least :-)).
> 
> > > +		if (it->second.name == name) {
> > > +			return it->first;
> > > +		}
> > > +		it++;
> > > +	}
> > > +	return invalidPixelFormat;
> > 
> > This function should return either the correct PixelFormatInfo, or the
> > invalidPixelFormatInfo.
> > 
> > then it's up to the caller to extract the PixelFormat from that const
> > reference.
> > 
> > If invalidPixelFormatInfo is returned, it will contain an
> > 'invalidPixelFormat'.
> > 
> > > +}
> > > +
> > >  /**
> > >   * \brief Compute the stride
> > >   * \param[in] width The width of the line, in pixels

-- 
Regards,

Laurent Pinchart