[libcamera-devel,v2,01/14] libcamera: formats: Add toString() methods
diff mbox series

Message ID 20190312121242.2253-2-jacopo@jmondi.org
State Accepted
Headers show
Series
  • libcamera: ipu3: ImgU support
Related show

Commit Message

Jacopo Mondi March 12, 2019, 12:12 p.m. UTC 
Add toString() helpers to pretty print out a V4L2Device or V4L2Subdevice
format.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/v4l2_device.h    |  2 ++
 src/libcamera/include/v4l2_subdevice.h |  2 ++
 src/libcamera/v4l2_device.cpp          | 18 ++++++++++++++++++
 src/libcamera/v4l2_subdevice.cpp       | 18 ++++++++++++++++++
 4 files changed, 40 insertions(+)

Comments

Kieran Bingham March 12, 2019, 12:25 p.m. UTC | #1 
Hi Jacopo,

This is neat, and gives us a single place to handle this formatting in a
consistent fashion throughout. (well single for each V4L2 type).

I guess later we could use this to override the << operator too?

But that's not necessary for now.

LGTM.

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


On 12/03/2019 12:12, Jacopo Mondi wrote:
> Add toString() helpers to pretty print out a V4L2Device or V4L2Subdevice
> format.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_device.h    |  2 ++
>  src/libcamera/include/v4l2_subdevice.h |  2 ++
>  src/libcamera/v4l2_device.cpp          | 18 ++++++++++++++++++
>  src/libcamera/v4l2_subdevice.cpp       | 18 ++++++++++++++++++
>  4 files changed, 40 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 5c379fac66dc..258deee8d461 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -100,6 +100,8 @@ public:
>  		uint32_t bpl;
>  	} planes[3];
>  	unsigned int planesCount;
> +
> +	const std::string toString() const;
>  };
>  
>  class V4L2Device : protected Loggable
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 1cc0fab73103..700e66bcddac 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -21,6 +21,8 @@ struct V4L2SubdeviceFormat {
>  	uint32_t mbus_code;
>  	uint32_t width;
>  	uint32_t height;
> +
> +	const std::string toString() const;
>  };
>  
>  class V4L2Subdevice : protected Loggable
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index a88a5f5ff036..0bdcfc1d3c8f 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -6,6 +6,8 @@
>   */
>  
>  #include <fcntl.h>
> +#include <iomanip>
> +#include <sstream>
>  #include <string.h>
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
> @@ -223,6 +225,22 @@ LOG_DEFINE_CATEGORY(V4L2)
>   * \brief The number of valid data planes
>   */
>  
> +/**
> + * \brief Assemble and return a string describing the format
> + *
> + * \return A string describing the V4L2DeviceFormat
> + */
> +const std::string V4L2DeviceFormat::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss.fill(0);
> +	ss << width << "x" << height << "- 0x" << std::hex
> +	   << std::setw(8) << fourcc << " planes: " << planesCount;
> +
> +	return ss.str();
> +}
> +
>  /**
>   * \class V4L2Device
>   * \brief V4L2Device object and API
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 5f58904bf9e6..7c56ba3ba510 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -6,6 +6,8 @@
>   */
>  
>  #include <fcntl.h>
> +#include <iomanip>
> +#include <sstream>
>  #include <string.h>
>  #include <sys/ioctl.h>
>  #include <unistd.h>
> @@ -69,6 +71,22 @@ LOG_DEFINE_CATEGORY(V4L2Subdev)
>   * \brief The image height in pixels
>   */
>  
> +/**
> + * \brief Assemble and return a string describing the format
> + *
> + * \return A string describing the V4L2SubdeviceFormat
> + */
> +const std::string V4L2SubdeviceFormat::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss.fill(0);
> +	ss << width << "x" << height << " - 0x" << std::hex << std::setw(8)
> +	   << mbus_code;

Some time later, we can convert the mbus_codes and fourcc's to strings
too. But I think that's a future todo for now.


> +
> +	return ss.str();
> +}
> +
>  /**
>   * \class V4L2Subdevice
>   * \brief A V4L2 subdevice as exposed by the Linux kernel
>
Laurent Pinchart March 13, 2019, 4:59 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Tue, Mar 12, 2019 at 01:12:29PM +0100, Jacopo Mondi wrote:
> Add toString() helpers to pretty print out a V4L2Device or V4L2Subdevice
> format.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_device.h    |  2 ++
>  src/libcamera/include/v4l2_subdevice.h |  2 ++
>  src/libcamera/v4l2_device.cpp          | 18 ++++++++++++++++++
>  src/libcamera/v4l2_subdevice.cpp       | 18 ++++++++++++++++++
>  4 files changed, 40 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 5c379fac66dc..258deee8d461 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -100,6 +100,8 @@ public:
>  		uint32_t bpl;
>  	} planes[3];
>  	unsigned int planesCount;
> +
> +	const std::string toString() const;
>  };
>  
>  class V4L2Device : protected Loggable
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 1cc0fab73103..700e66bcddac 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -21,6 +21,8 @@ struct V4L2SubdeviceFormat {
>  	uint32_t mbus_code;
>  	uint32_t width;
>  	uint32_t height;
> +
> +	const std::string toString() const;
>  };
>  
>  class V4L2Subdevice : protected Loggable
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index a88a5f5ff036..0bdcfc1d3c8f 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -6,6 +6,8 @@
>   */
>  
>  #include <fcntl.h>
> +#include <iomanip>
> +#include <sstream>
>  #include <string.h>
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
> @@ -223,6 +225,22 @@ LOG_DEFINE_CATEGORY(V4L2)
>   * \brief The number of valid data planes
>   */
>  
> +/**
> + * \brief Assemble and return a string describing the format
> + *
> + * \return A string describing the V4L2DeviceFormat
> + */
> +const std::string V4L2DeviceFormat::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss.fill(0);
> +	ss << width << "x" << height << "- 0x" << std::hex
> +	   << std::setw(8) << fourcc << " planes: " << planesCount;

As we standardize on a textual representation of a format, I think we
should consider how it should look like. Would it make sense to avoid
spaces, in order to use the same representation as command line
arguments to the cam tool without the need for quotes ? Do we need to
print the number of planes ?

> +
> +	return ss.str();
> +}
> +
>  /**
>   * \class V4L2Device
>   * \brief V4L2Device object and API
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 5f58904bf9e6..7c56ba3ba510 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -6,6 +6,8 @@
>   */
>  
>  #include <fcntl.h>
> +#include <iomanip>
> +#include <sstream>
>  #include <string.h>
>  #include <sys/ioctl.h>
>  #include <unistd.h>
> @@ -69,6 +71,22 @@ LOG_DEFINE_CATEGORY(V4L2Subdev)
>   * \brief The image height in pixels
>   */
>  
> +/**
> + * \brief Assemble and return a string describing the format
> + *
> + * \return A string describing the V4L2SubdeviceFormat
> + */
> +const std::string V4L2SubdeviceFormat::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss.fill(0);
> +	ss << width << "x" << height << " - 0x" << std::hex << std::setw(8)
> +	   << mbus_code;

Same question here regarding spaces. And I think you can limit the
mbus_code to 4 digits.

> +
> +	return ss.str();
> +}
> +
>  /**
>   * \class V4L2Subdevice
>   * \brief A V4L2 subdevice as exposed by the Linux kernel
Jacopo Mondi March 14, 2019, 9:26 a.m. UTC | #3 
HI Laurent,

On Wed, Mar 13, 2019 at 06:59:17PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Mar 12, 2019 at 01:12:29PM +0100, Jacopo Mondi wrote:
> > Add toString() helpers to pretty print out a V4L2Device or V4L2Subdevice
> > format.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_device.h    |  2 ++
> >  src/libcamera/include/v4l2_subdevice.h |  2 ++
> >  src/libcamera/v4l2_device.cpp          | 18 ++++++++++++++++++
> >  src/libcamera/v4l2_subdevice.cpp       | 18 ++++++++++++++++++
> >  4 files changed, 40 insertions(+)
> >
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 5c379fac66dc..258deee8d461 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -100,6 +100,8 @@ public:
> >  		uint32_t bpl;
> >  	} planes[3];
> >  	unsigned int planesCount;
> > +
> > +	const std::string toString() const;
> >  };
> >
> >  class V4L2Device : protected Loggable
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index 1cc0fab73103..700e66bcddac 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -21,6 +21,8 @@ struct V4L2SubdeviceFormat {
> >  	uint32_t mbus_code;
> >  	uint32_t width;
> >  	uint32_t height;
> > +
> > +	const std::string toString() const;
> >  };
> >
> >  class V4L2Subdevice : protected Loggable
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index a88a5f5ff036..0bdcfc1d3c8f 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -6,6 +6,8 @@
> >   */
> >
> >  #include <fcntl.h>
> > +#include <iomanip>
> > +#include <sstream>
> >  #include <string.h>
> >  #include <sys/ioctl.h>
> >  #include <sys/mman.h>
> > @@ -223,6 +225,22 @@ LOG_DEFINE_CATEGORY(V4L2)
> >   * \brief The number of valid data planes
> >   */
> >
> > +/**
> > + * \brief Assemble and return a string describing the format
> > + *
> > + * \return A string describing the V4L2DeviceFormat
> > + */
> > +const std::string V4L2DeviceFormat::toString() const
> > +{
> > +	std::stringstream ss;
> > +
> > +	ss.fill(0);
> > +	ss << width << "x" << height << "- 0x" << std::hex
> > +	   << std::setw(8) << fourcc << " planes: " << planesCount;
>
> As we standardize on a textual representation of a format, I think we
> should consider how it should look like. Would it make sense to avoid
> spaces, in order to use the same representation as command line
> arguments to the cam tool without the need for quotes ? Do we need to
> print the number of planes ?
>

How would you re-use the argument? The cam tool options syntax is
-f width=1920,height=1080

Would you print out the format with 'width' and 'heigh' ?

Thanks
  j

> > +
> > +	return ss.str();
> > +}
> > +
> >  /**
> >   * \class V4L2Device
> >   * \brief V4L2Device object and API
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 5f58904bf9e6..7c56ba3ba510 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -6,6 +6,8 @@
> >   */
> >
> >  #include <fcntl.h>
> > +#include <iomanip>
> > +#include <sstream>
> >  #include <string.h>
> >  #include <sys/ioctl.h>
> >  #include <unistd.h>
> > @@ -69,6 +71,22 @@ LOG_DEFINE_CATEGORY(V4L2Subdev)
> >   * \brief The image height in pixels
> >   */
> >
> > +/**
> > + * \brief Assemble and return a string describing the format
> > + *
> > + * \return A string describing the V4L2SubdeviceFormat
> > + */
> > +const std::string V4L2SubdeviceFormat::toString() const
> > +{
> > +	std::stringstream ss;
> > +
> > +	ss.fill(0);
> > +	ss << width << "x" << height << " - 0x" << std::hex << std::setw(8)
> > +	   << mbus_code;
>
> Same question here regarding spaces. And I think you can limit the
> mbus_code to 4 digits.
>
> > +
> > +	return ss.str();
> > +}
> > +
> >  /**
> >   * \class V4L2Subdevice
> >   * \brief A V4L2 subdevice as exposed by the Linux kernel
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
index 5c379fac66dc..258deee8d461 100644
--- a/src/libcamera/include/v4l2_device.h
+++ b/src/libcamera/include/v4l2_device.h
@@ -100,6 +100,8 @@  public:
 		uint32_t bpl;
 	} planes[3];
 	unsigned int planesCount;
+
+	const std::string toString() const;
 };
 
 class V4L2Device : protected Loggable
diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
index 1cc0fab73103..700e66bcddac 100644
--- a/src/libcamera/include/v4l2_subdevice.h
+++ b/src/libcamera/include/v4l2_subdevice.h
@@ -21,6 +21,8 @@  struct V4L2SubdeviceFormat {
 	uint32_t mbus_code;
 	uint32_t width;
 	uint32_t height;
+
+	const std::string toString() const;
 };
 
 class V4L2Subdevice : protected Loggable
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index a88a5f5ff036..0bdcfc1d3c8f 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -6,6 +6,8 @@ 
  */
 
 #include <fcntl.h>
+#include <iomanip>
+#include <sstream>
 #include <string.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
@@ -223,6 +225,22 @@  LOG_DEFINE_CATEGORY(V4L2)
  * \brief The number of valid data planes
  */
 
+/**
+ * \brief Assemble and return a string describing the format
+ *
+ * \return A string describing the V4L2DeviceFormat
+ */
+const std::string V4L2DeviceFormat::toString() const
+{
+	std::stringstream ss;
+
+	ss.fill(0);
+	ss << width << "x" << height << "- 0x" << std::hex
+	   << std::setw(8) << fourcc << " planes: " << planesCount;
+
+	return ss.str();
+}
+
 /**
  * \class V4L2Device
  * \brief V4L2Device object and API
diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
index 5f58904bf9e6..7c56ba3ba510 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -6,6 +6,8 @@ 
  */
 
 #include <fcntl.h>
+#include <iomanip>
+#include <sstream>
 #include <string.h>
 #include <sys/ioctl.h>
 #include <unistd.h>
@@ -69,6 +71,22 @@  LOG_DEFINE_CATEGORY(V4L2Subdev)
  * \brief The image height in pixels
  */
 
+/**
+ * \brief Assemble and return a string describing the format
+ *
+ * \return A string describing the V4L2SubdeviceFormat
+ */
+const std::string V4L2SubdeviceFormat::toString() const
+{
+	std::stringstream ss;
+
+	ss.fill(0);
+	ss << width << "x" << height << " - 0x" << std::hex << std::setw(8)
+	   << mbus_code;
+
+	return ss.str();
+}
+
 /**
  * \class V4L2Subdevice
  * \brief A V4L2 subdevice as exposed by the Linux kernel