[libcamera-devel,v3,4/9] libcamera: bayer_format: Add PixelFormat conversion helpers to BayerFormat class
diff mbox series

Message ID 20211027092803.3671096-5-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Conversion to media controller
Related show

Commit Message

Naushir Patuck Oct. 27, 2021, 9:27 a.m. UTC 
Add BayerFormat::toPixelFormat() and BayerFormat::fromPixelFormat() helper
functions to convert between BayerFormat and PixelFormat types.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/internal/bayer_format.h |  4 ++++
 src/libcamera/bayer_format.cpp            | 29 +++++++++++++++++++++++
 2 files changed, 33 insertions(+)

Comments

Kieran Bingham Oct. 27, 2021, 10:48 a.m. UTC | #1 
Quoting Naushir Patuck (2021-10-27 10:27:58)
> Add BayerFormat::toPixelFormat() and BayerFormat::fromPixelFormat() helper
> functions to convert between BayerFormat and PixelFormat types.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/internal/bayer_format.h |  4 ++++
>  src/libcamera/bayer_format.cpp            | 29 +++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> index 723382d4168d..ee96b9a86707 100644
> --- a/include/libcamera/internal/bayer_format.h
> +++ b/include/libcamera/internal/bayer_format.h
> @@ -10,6 +10,8 @@
>  #include <stdint.h>
>  #include <string>
>  
> +#include <libcamera/pixel_format.h>
> +
>  #include "libcamera/internal/v4l2_pixelformat.h"
>  
>  namespace libcamera {
> @@ -50,6 +52,8 @@ public:
>  
>         V4L2PixelFormat toV4L2PixelFormat() const;
>         static BayerFormat fromV4L2PixelFormat(V4L2PixelFormat v4l2Format);
> +       PixelFormat toPixelFormat() const;
> +       static BayerFormat fromPixelFormat(PixelFormat format);
>         BayerFormat transform(Transform t) const;
>  
>         Order order;
> diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> index 94e2294d7f6c..1edd238829ce 100644
> --- a/src/libcamera/bayer_format.cpp
> +++ b/src/libcamera/bayer_format.cpp
> @@ -305,6 +305,35 @@ BayerFormat BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format)
>         return BayerFormat();
>  }
>  
> +/**
> + * \brief Convert a BayerFormat into the corresponding PixelFormat
> + * \return The PixelFormat corresponding to this BayerFormat
> + */
> +PixelFormat BayerFormat::toPixelFormat() const
> +{
> +       const auto it = bayerToFormat.find(*this);
> +       if (it != bayerToFormat.end())
> +               return it->second.pixelFormat;
> +
> +       return PixelFormat();
> +}
> +
> +/**
> + * \brief Convert a PixelFormat into the corresponding BayerFormat
> + * \return The BayerFormat corresponding to this PixelFormat
> + */
> +BayerFormat BayerFormat::fromPixelFormat(PixelFormat format)
> +{
> +       auto it = std::find_if(bayerToFormat.begin(), bayerToFormat.end(),
> +                              [format](const auto &i) {
> +                                      return i.second.pixelFormat == format;
> +                              });
> +       if (it == bayerToFormat.end())
> +               return BayerFormat{};
> +
> +       return it->first;

I'm curious why you have used two styles in the two functions.

BayerFormat::toPixelFormat is 
  if (it != end)
     return it;
  return Empty;

while BayerFormat::fromPixelFormat is
  if (it == end)
     return Empty;

  return it;

If there's no technical reason for this, I think keeping them both using
the same pattern would be more consistent, but ... they are both
functional, so I don't see this as a blocker. (Personally I'd use the
BayerFormat::toPixelFormat style on both)

So either way:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +}
> +
>  /**
>   * \brief Apply a transform to this BayerFormat
>   * \param[in] t The transform to apply
> -- 
> 2.25.1
>
Laurent Pinchart Oct. 27, 2021, 11:48 a.m. UTC | #2 
On Wed, Oct 27, 2021 at 11:48:20AM +0100, Kieran Bingham wrote:
> Quoting Naushir Patuck (2021-10-27 10:27:58)
> > Add BayerFormat::toPixelFormat() and BayerFormat::fromPixelFormat() helper
> > functions to convert between BayerFormat and PixelFormat types.
> > 
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/internal/bayer_format.h |  4 ++++
> >  src/libcamera/bayer_format.cpp            | 29 +++++++++++++++++++++++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > index 723382d4168d..ee96b9a86707 100644
> > --- a/include/libcamera/internal/bayer_format.h
> > +++ b/include/libcamera/internal/bayer_format.h
> > @@ -10,6 +10,8 @@
> >  #include <stdint.h>
> >  #include <string>
> >  
> > +#include <libcamera/pixel_format.h>
> > +
> >  #include "libcamera/internal/v4l2_pixelformat.h"
> >  
> >  namespace libcamera {
> > @@ -50,6 +52,8 @@ public:
> >  
> >         V4L2PixelFormat toV4L2PixelFormat() const;
> >         static BayerFormat fromV4L2PixelFormat(V4L2PixelFormat v4l2Format);
> > +       PixelFormat toPixelFormat() const;
> > +       static BayerFormat fromPixelFormat(PixelFormat format);
> >         BayerFormat transform(Transform t) const;
> >  
> >         Order order;
> > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > index 94e2294d7f6c..1edd238829ce 100644
> > --- a/src/libcamera/bayer_format.cpp
> > +++ b/src/libcamera/bayer_format.cpp
> > @@ -305,6 +305,35 @@ BayerFormat BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format)
> >         return BayerFormat();
> >  }
> >  
> > +/**
> > + * \brief Convert a BayerFormat into the corresponding PixelFormat
> > + * \return The PixelFormat corresponding to this BayerFormat
> > + */
> > +PixelFormat BayerFormat::toPixelFormat() const
> > +{
> > +       const auto it = bayerToFormat.find(*this);
> > +       if (it != bayerToFormat.end())
> > +               return it->second.pixelFormat;
> > +
> > +       return PixelFormat();
> > +}
> > +
> > +/**
> > + * \brief Convert a PixelFormat into the corresponding BayerFormat
> > + * \return The BayerFormat corresponding to this PixelFormat
> > + */
> > +BayerFormat BayerFormat::fromPixelFormat(PixelFormat format)
> > +{
> > +       auto it = std::find_if(bayerToFormat.begin(), bayerToFormat.end(),
> > +                              [format](const auto &i) {
> > +                                      return i.second.pixelFormat == format;
> > +                              });
> > +       if (it == bayerToFormat.end())
> > +               return BayerFormat{};
> > +
> > +       return it->first;
> 
> I'm curious why you have used two styles in the two functions.
> 
> BayerFormat::toPixelFormat is 
>   if (it != end)
>      return it;
>   return Empty;
> 
> while BayerFormat::fromPixelFormat is
>   if (it == end)
>      return Empty;
> 
>   return it;
> 
> If there's no technical reason for this, I think keeping them both using
> the same pattern would be more consistent, but ... they are both
> functional, so I don't see this as a blocker. (Personally I'd use the
> BayerFormat::toPixelFormat style on both)

Funny how personal preferences differ, I would have used the latter
(a.k.a. return on error) style :-)

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

> So either way:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +}
> > +
> >  /**
> >   * \brief Apply a transform to this BayerFormat
> >   * \param[in] t The transform to apply
Naushir Patuck Oct. 27, 2021, 12:34 p.m. UTC | #3 
Hi,

On Wed, 27 Oct 2021 at 12:48, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> On Wed, Oct 27, 2021 at 11:48:20AM +0100, Kieran Bingham wrote:
> > Quoting Naushir Patuck (2021-10-27 10:27:58)
> > > Add BayerFormat::toPixelFormat() and BayerFormat::fromPixelFormat()
> helper
> > > functions to convert between BayerFormat and PixelFormat types.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/bayer_format.h |  4 ++++
> > >  src/libcamera/bayer_format.cpp            | 29 +++++++++++++++++++++++
> > >  2 files changed, 33 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/bayer_format.h
> b/include/libcamera/internal/bayer_format.h
> > > index 723382d4168d..ee96b9a86707 100644
> > > --- a/include/libcamera/internal/bayer_format.h
> > > +++ b/include/libcamera/internal/bayer_format.h
> > > @@ -10,6 +10,8 @@
> > >  #include <stdint.h>
> > >  #include <string>
> > >
> > > +#include <libcamera/pixel_format.h>
> > > +
> > >  #include "libcamera/internal/v4l2_pixelformat.h"
> > >
> > >  namespace libcamera {
> > > @@ -50,6 +52,8 @@ public:
> > >
> > >         V4L2PixelFormat toV4L2PixelFormat() const;
> > >         static BayerFormat fromV4L2PixelFormat(V4L2PixelFormat
> v4l2Format);
> > > +       PixelFormat toPixelFormat() const;
> > > +       static BayerFormat fromPixelFormat(PixelFormat format);
> > >         BayerFormat transform(Transform t) const;
> > >
> > >         Order order;
> > > diff --git a/src/libcamera/bayer_format.cpp
> b/src/libcamera/bayer_format.cpp
> > > index 94e2294d7f6c..1edd238829ce 100644
> > > --- a/src/libcamera/bayer_format.cpp
> > > +++ b/src/libcamera/bayer_format.cpp
> > > @@ -305,6 +305,35 @@ BayerFormat
> BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format)
> > >         return BayerFormat();
> > >  }
> > >
> > > +/**
> > > + * \brief Convert a BayerFormat into the corresponding PixelFormat
> > > + * \return The PixelFormat corresponding to this BayerFormat
> > > + */
> > > +PixelFormat BayerFormat::toPixelFormat() const
> > > +{
> > > +       const auto it = bayerToFormat.find(*this);
> > > +       if (it != bayerToFormat.end())
> > > +               return it->second.pixelFormat;
> > > +
> > > +       return PixelFormat();
> > > +}
> > > +
> > > +/**
> > > + * \brief Convert a PixelFormat into the corresponding BayerFormat
> > > + * \return The BayerFormat corresponding to this PixelFormat
> > > + */
> > > +BayerFormat BayerFormat::fromPixelFormat(PixelFormat format)
> > > +{
> > > +       auto it = std::find_if(bayerToFormat.begin(),
> bayerToFormat.end(),
> > > +                              [format](const auto &i) {
> > > +                                      return i.second.pixelFormat ==
> format;
> > > +                              });
> > > +       if (it == bayerToFormat.end())
> > > +               return BayerFormat{};
> > > +
> > > +       return it->first;
> >
> > I'm curious why you have used two styles in the two functions.
> >
> > BayerFormat::toPixelFormat is
> >   if (it != end)
> >      return it;
> >   return Empty;
> >
> > while BayerFormat::fromPixelFormat is
> >   if (it == end)
> >      return Empty;
> >
> >   return it;
> >
> > If there's no technical reason for this, I think keeping them both using
> > the same pattern would be more consistent, but ... they are both
> > functional, so I don't see this as a blocker. (Personally I'd use the
> > BayerFormat::toPixelFormat style on both)
>
> Funny how personal preferences differ, I would have used the latter
> (a.k.a. return on error) style :-)
>

I'll use the  BayerFormat::toPixelFormat style this time round as that
matches
what the other functions in this file do.

Regards,
Naush


> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > So either way:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > > +}
> > > +
> > >  /**
> > >   * \brief Apply a transform to this BayerFormat
> > >   * \param[in] t The transform to apply
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
index 723382d4168d..ee96b9a86707 100644
--- a/include/libcamera/internal/bayer_format.h
+++ b/include/libcamera/internal/bayer_format.h
@@ -10,6 +10,8 @@ 
 #include <stdint.h>
 #include <string>
 
+#include <libcamera/pixel_format.h>
+
 #include "libcamera/internal/v4l2_pixelformat.h"
 
 namespace libcamera {
@@ -50,6 +52,8 @@  public:
 
 	V4L2PixelFormat toV4L2PixelFormat() const;
 	static BayerFormat fromV4L2PixelFormat(V4L2PixelFormat v4l2Format);
+	PixelFormat toPixelFormat() const;
+	static BayerFormat fromPixelFormat(PixelFormat format);
 	BayerFormat transform(Transform t) const;
 
 	Order order;
diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
index 94e2294d7f6c..1edd238829ce 100644
--- a/src/libcamera/bayer_format.cpp
+++ b/src/libcamera/bayer_format.cpp
@@ -305,6 +305,35 @@  BayerFormat BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format)
 	return BayerFormat();
 }
 
+/**
+ * \brief Convert a BayerFormat into the corresponding PixelFormat
+ * \return The PixelFormat corresponding to this BayerFormat
+ */
+PixelFormat BayerFormat::toPixelFormat() const
+{
+	const auto it = bayerToFormat.find(*this);
+	if (it != bayerToFormat.end())
+		return it->second.pixelFormat;
+
+	return PixelFormat();
+}
+
+/**
+ * \brief Convert a PixelFormat into the corresponding BayerFormat
+ * \return The BayerFormat corresponding to this PixelFormat
+ */
+BayerFormat BayerFormat::fromPixelFormat(PixelFormat format)
+{
+	auto it = std::find_if(bayerToFormat.begin(), bayerToFormat.end(),
+			       [format](const auto &i) {
+				       return i.second.pixelFormat == format;
+			       });
+	if (it == bayerToFormat.end())
+		return BayerFormat{};
+
+	return it->first;
+}
+
 /**
  * \brief Apply a transform to this BayerFormat
  * \param[in] t The transform to apply