[libcamera-devel] [PATCH v4 2/3] libcamera: pixel_format: Add a function to return format based on string

[Laurent Pinchart]

On Tue, Jul 28, 2020 at 11:43:28AM +0100, Kieran Bingham wrote:
> Hi Kaaira,
> 
> On 27/07/2020 17:21, Kaaira Gupta wrote:
> > Add a function which retrieves pixel format corrsponding to its name
> 
> s/corrsponding/corresponding/
> 
> > from PixelFormatInfo.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> > ---
> >  include/libcamera/pixel_format.h | 2 ++
> >  src/libcamera/pixel_format.cpp   | 9 +++++++++
> >  2 files changed, 11 insertions(+)
> > 
> > diff --git a/include/libcamera/pixel_format.h b/include/libcamera/pixel_format.h
> > index 6727315..c4ae088 100644
> > --- a/include/libcamera/pixel_format.h
> > +++ b/include/libcamera/pixel_format.h
> > @@ -38,6 +38,8 @@ public:
> >  
> >  	std::string toString() const;
> >  
> > +	static PixelFormat fromString(const std::string &name);
> > +
> >  private:
> >  	uint32_t fourcc_;
> >  	uint64_t modifier_;
> > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp
> > index 14addb5..9b07781 100644
> > --- a/src/libcamera/pixel_format.cpp
> > +++ b/src/libcamera/pixel_format.cpp
> > @@ -130,4 +130,13 @@ std::string PixelFormat::toString() const
> >  	return info.name;
> >  }
> >  
> > +/**
> > + * \brief Retrive pixel format corresponding to the string
> 
> s/Retrive/Retrieve/

I'd say "Create a PixelFormat from a string". It's a static function
that returns an instance, not a reference or pointer, so it's not
retrieving anything pre-existing.

> > + * \return Pixel format
> > + */
> 
> We might want to express a bit more about what is returned, especially
> as this function is in the user-facing API.
> 
> "\return The PixelFormat represented by the \a name if known, or an
> invalid pixel format otherwise."
> 
> 
> Those can also be fixed on applying if there's nothing else.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +PixelFormat PixelFormat::fromString(const std::string &name)
> 
> Actually, could this return a const reference? (which will save a copy).
> 
> const PixelFormat &PixelFormat::fromString....

PixelFormat is a lightweight class meant to be copied, returning an
instance instance of a reference should be fine here.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > +{
> > +	return PixelFormatInfo::info(name).format;
> > +}
> > +
> >  } /* namespace libcamera */
> > 

-- 
Regards,

Laurent Pinchart