Message ID | 20190312121242.2253-2-jacopo@jmondi.org |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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 >
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
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
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
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(+)