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

9023 diff mbox series

Add a function which retrieves pixel format corrsponding to its name
from PixelFormatInfo.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 include/libcamera/pixel_format.h | 2 ++
 src/libcamera/pixel_format.cpp   | 9 +++++++++
 2 files changed, 11 insertions(+)

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



> + * \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@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....

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

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@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@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@ideasonboard.com>

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