[libcamera-devel,v3,1/3] libcamera: formats: PixelFormatInfo: Add name lookup function
diff mbox series

Message ID 20200727151820.24466-2-kgupta@es.iitr.ac.in
State Superseded
Headers show
Series
  • Enable formats lookup based on name
Related show

Commit Message

Kaaira Gupta July 27, 2020, 3:18 p.m. UTC 
Add a function which returns a format, given its name as a string.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 include/libcamera/internal/formats.h |  1 +
 src/libcamera/formats.cpp            | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

Comments

Kieran Bingham July 27, 2020, 3:30 p.m. UTC | #1 
Hi Kaaira,

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

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


> +		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'.

--
Kieran



> +}
> +
>  /**
>   * \brief Compute the stride
>   * \param[in] width The width of the line, in pixels
>
Laurent Pinchart July 27, 2020, 3:33 p.m. UTC | #2 
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@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
Laurent Pinchart July 27, 2020, 3:38 p.m. UTC | #3 
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@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
Kieran Bingham July 27, 2020, 4:10 p.m. UTC | #4 
Hi Laurent,

On 27/07/2020 16:38, Laurent Pinchart wrote:
> 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@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.

Indeed, that's why I think we should have a PixelFormatInfo
  static const PixelFormatInfo &info(const std::string &name);


and a separate
 static PixelFormat fromString(const std::string &name);

as is implemented in 2/3 indeed.

--
Kieran


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

Patch
diff mbox series 

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);
 
 	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()) {
+		if (it->second.name == name) {
+			return it->first;
+		}
+		it++;
+	}
+	return invalidPixelFormat;
+}
+
 /**
  * \brief Compute the stride
  * \param[in] width The width of the line, in pixels