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

[Laurent Pinchart]

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");

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