| Message ID | 20200529140433.GA18070@kaaira-HP-Pavilion-Notebook |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kaaira, Thanks for your patch. On 2020-05-29 19:34:33 +0530, Kaaira Gupta wrote: > Replace hex input for pixelformats with their fourcc values, > in cam and qcam. > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > src/cam/stream_options.cpp | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp > index bd12c8f..9f9536e 100644 > --- a/src/cam/stream_options.cpp > +++ b/src/cam/stream_options.cpp > @@ -6,6 +6,7 @@ > */ > #include "stream_options.h" > > +#include <bits/stdc++.h> > #include <iostream> > > using namespace libcamera; > @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser() > ArgumentRequired); > addOption("height", OptionInteger, "Height in pixels", > ArgumentRequired); > - addOption("pixelformat", OptionInteger, "Pixel format", > + addOption("pixelformat", OptionString, "Pixel format fourcc", > ArgumentRequired); > } > > @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config, > } > > /* \todo Translate 4CC string to pixelformat with modifier. */ > - if (opts.isSet("pixelformat")) > - cfg.pixelFormat = PixelFormat(opts["pixelformat"]); > + if (opts.isSet("pixelformat")) { > + std::string fourcc = opts["pixelformat"]; > + transform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper); I fear we don't know all fourcc codes will be uppercase. It's currently true all DRM defined ones are, but might change. As an example look at V4L2 fourcc codes where lower/upper case is already mixed. > + char char_array[5]; > + strcpy(char_array, fourcc.c_str()); I think you can simplify this, or something similar (not compile tested). const char *fourcc = opts["pixelformat"].toString().c_str(); > + cfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) | > + ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24)); > + } > } > > return 0; > -- > 2.17.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Tue, Jun 02, 2020 at 05:20:14PM +0200, Niklas Söderlund wrote: > On 2020-05-29 19:34:33 +0530, Kaaira Gupta wrote: > > Replace hex input for pixelformats with their fourcc values, > > in cam and qcam. > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > --- > > src/cam/stream_options.cpp | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp > > index bd12c8f..9f9536e 100644 > > --- a/src/cam/stream_options.cpp > > +++ b/src/cam/stream_options.cpp > > @@ -6,6 +6,7 @@ > > */ > > #include "stream_options.h" > > > > +#include <bits/stdc++.h> What is this header used for ? It doesn't seem to be standard. > > #include <iostream> > > > > using namespace libcamera; > > @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser() > > ArgumentRequired); > > addOption("height", OptionInteger, "Height in pixels", > > ArgumentRequired); > > - addOption("pixelformat", OptionInteger, "Pixel format", > > + addOption("pixelformat", OptionString, "Pixel format fourcc", s/fourcc/FourCC/ > > ArgumentRequired); > > } > > > > @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config, > > } > > > > /* \todo Translate 4CC string to pixelformat with modifier. */ > > - if (opts.isSet("pixelformat")) > > - cfg.pixelFormat = PixelFormat(opts["pixelformat"]); > > + if (opts.isSet("pixelformat")) { > > + std::string fourcc = opts["pixelformat"]; > > + transform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper); > > I fear we don't know all fourcc codes will be uppercase. It's currently > true all DRM defined ones are, but might change. As an example look at > V4L2 fourcc codes where lower/upper case is already mixed. > > > + char char_array[5]; > > + strcpy(char_array, fourcc.c_str()); > > I think you can simplify this, or something similar (not compile > tested). > > const char *fourcc = opts["pixelformat"].toString().c_str(); OptionValue::toString() returns a temporary object, you'll have a use-after-free. > > > + cfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) | > > + ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24)); static_cast<uint32_t>() instead of (__u32), to use C++-style casts, and because __u32 is a kernel type. Let's also split this to shorten the lines. This change goes in the right direction, but I think we need to expand it a little bit: - Some FourCC contain spaces at the end. It would be nice to write -s pixelformat=R8 instead of -s 'pixelformat=R8 ' - Should we support both the numerical value and the 4CC representation ? It could be useful in some cases, to avoid having to convert manually from hex to 4CC first, but maybe there are only few use cases for that feature. - With a 4CC we can't easily handle the formats that use DRM_FORMAT_BIG_ENDIAN. Maybe that's no big deal though, as we don't any use such format at the moment, and DRM_FORMAT_BIG_ENDIAN seems more like a hack than something that should be used going forward. Not all this need to be addressed now. > > + } > > } > > > > return 0;
On Tue, Jun 02, 2020 at 11:33:09PM +0300, Laurent Pinchart wrote: > Hello, > > On Tue, Jun 02, 2020 at 05:20:14PM +0200, Niklas Söderlund wrote: > > On 2020-05-29 19:34:33 +0530, Kaaira Gupta wrote: > > > Replace hex input for pixelformats with their fourcc values, > > > in cam and qcam. > > > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > > --- > > > src/cam/stream_options.cpp | 13 ++++++++++--- > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp > > > index bd12c8f..9f9536e 100644 > > > --- a/src/cam/stream_options.cpp > > > +++ b/src/cam/stream_options.cpp > > > @@ -6,6 +6,7 @@ > > > */ > > > #include "stream_options.h" > > > > > > +#include <bits/stdc++.h> > > What is this header used for ? It doesn't seem to be standard. it is used for "toupper" functionality, I added it so that we can write, for example, ba24 instead of BA24..but as pointed out DRM formats can have a mixture of both upper and lower cases in future, so I'll remove that. > > > > #include <iostream> > > > > > > using namespace libcamera; > > > @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser() > > > ArgumentRequired); > > > addOption("height", OptionInteger, "Height in pixels", > > > ArgumentRequired); > > > - addOption("pixelformat", OptionInteger, "Pixel format", > > > + addOption("pixelformat", OptionString, "Pixel format fourcc", > > s/fourcc/FourCC/ > > > > ArgumentRequired); > > > } > > > > > > @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config, > > > } > > > > > > /* \todo Translate 4CC string to pixelformat with modifier. */ > > > - if (opts.isSet("pixelformat")) > > > - cfg.pixelFormat = PixelFormat(opts["pixelformat"]); > > > + if (opts.isSet("pixelformat")) { > > > + std::string fourcc = opts["pixelformat"]; > > > + transform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper); > > > > I fear we don't know all fourcc codes will be uppercase. It's currently > > true all DRM defined ones are, but might change. As an example look at > > V4L2 fourcc codes where lower/upper case is already mixed. > > > > > + char char_array[5]; > > > + strcpy(char_array, fourcc.c_str()); > > > > I think you can simplify this, or something similar (not compile > > tested). > > > > const char *fourcc = opts["pixelformat"].toString().c_str(); > > OptionValue::toString() returns a temporary object, you'll have a > use-after-free. > > > > > > + cfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) | > > > + ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24)); > > static_cast<uint32_t>() instead of (__u32), to use C++-style casts, and > because __u32 is a kernel type. Let's also split this to shorten the > lines. > > This change goes in the right direction, but I think we need to expand > it a little bit: > > - Some FourCC contain spaces at the end. It would be nice to write > > -s pixelformat=R8 > > instead of > > -s 'pixelformat=R8 ' Noted > > - Should we support both the numerical value and the 4CC representation > ? It could be useful in some cases, to avoid having to convert > manually from hex to 4CC first, but maybe there are only few use cases > for that feature. What are the use cases for direct hex values? I think FourCC are easier to remember? > > - With a 4CC we can't easily handle the formats that use > DRM_FORMAT_BIG_ENDIAN. Maybe that's no big deal though, as we don't > any use such format at the moment, and DRM_FORMAT_BIG_ENDIAN seems > more like a hack than something that should be used going forward. > > Not all this need to be addressed now. > > > > + } > > > } > > > > > > return 0; > > -- > Regards, > > Laurent Pinchart
Hi Kaaira, On Wed, Jun 03, 2020 at 06:20:23PM +0530, Kaaira Gupta wrote: > On Tue, Jun 02, 2020 at 11:33:09PM +0300, Laurent Pinchart wrote: > > On Tue, Jun 02, 2020 at 05:20:14PM +0200, Niklas Söderlund wrote: > > > On 2020-05-29 19:34:33 +0530, Kaaira Gupta wrote: > > > > Replace hex input for pixelformats with their fourcc values, > > > > in cam and qcam. > > > > > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > > > --- > > > > src/cam/stream_options.cpp | 13 ++++++++++--- > > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp > > > > index bd12c8f..9f9536e 100644 > > > > --- a/src/cam/stream_options.cpp > > > > +++ b/src/cam/stream_options.cpp > > > > @@ -6,6 +6,7 @@ > > > > */ > > > > #include "stream_options.h" > > > > > > > > +#include <bits/stdc++.h> > > > > What is this header used for ? It doesn't seem to be standard. > > it is used for "toupper" functionality, I added it so that we can write, > for example, ba24 instead of BA24..but as pointed out DRM formats can > have a mixture of both upper and lower cases in future, so I'll remove > that. > > > > > #include <iostream> > > > > > > > > using namespace libcamera; > > > > @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser() > > > > ArgumentRequired); > > > > addOption("height", OptionInteger, "Height in pixels", > > > > ArgumentRequired); > > > > - addOption("pixelformat", OptionInteger, "Pixel format", > > > > + addOption("pixelformat", OptionString, "Pixel format fourcc", > > > > s/fourcc/FourCC/ > > > > > > ArgumentRequired); > > > > } > > > > > > > > @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config, > > > > } > > > > > > > > /* \todo Translate 4CC string to pixelformat with modifier. */ > > > > - if (opts.isSet("pixelformat")) > > > > - cfg.pixelFormat = PixelFormat(opts["pixelformat"]); > > > > + if (opts.isSet("pixelformat")) { > > > > + std::string fourcc = opts["pixelformat"]; > > > > + transform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper); > > > > > > I fear we don't know all fourcc codes will be uppercase. It's currently > > > true all DRM defined ones are, but might change. As an example look at > > > V4L2 fourcc codes where lower/upper case is already mixed. > > > > > > > + char char_array[5]; > > > > + strcpy(char_array, fourcc.c_str()); > > > > > > I think you can simplify this, or something similar (not compile > > > tested). > > > > > > const char *fourcc = opts["pixelformat"].toString().c_str(); > > > > OptionValue::toString() returns a temporary object, you'll have a > > use-after-free. > > > > > > > > > + cfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) | > > > > + ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24)); > > > > static_cast<uint32_t>() instead of (__u32), to use C++-style casts, and > > because __u32 is a kernel type. Let's also split this to shorten the > > lines. > > > > This change goes in the right direction, but I think we need to expand > > it a little bit: > > > > - Some FourCC contain spaces at the end. It would be nice to write > > > > -s pixelformat=R8 > > > > instead of > > > > -s 'pixelformat=R8 ' > > Noted > > > > > - Should we support both the numerical value and the 4CC representation > > ? It could be useful in some cases, to avoid having to convert > > manually from hex to 4CC first, but maybe there are only few use cases > > for that feature. > > What are the use cases for direct hex values? I think FourCC are easier > to remember? A FourCC is indeed easier to remember, but I've found myself in situations where one tool (a display test tool for instance) would give me a hex value, it would be nice to be able to pass it directly to cam without having to convert it manually. > > - With a 4CC we can't easily handle the formats that use > > DRM_FORMAT_BIG_ENDIAN. Maybe that's no big deal though, as we don't > > any use such format at the moment, and DRM_FORMAT_BIG_ENDIAN seems > > more like a hack than something that should be used going forward. > > > > Not all this need to be addressed now. > > > > > > + } > > > > } > > > > > > > > return 0;
diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp index bd12c8f..9f9536e 100644 --- a/src/cam/stream_options.cpp +++ b/src/cam/stream_options.cpp @@ -6,6 +6,7 @@ */ #include "stream_options.h" +#include <bits/stdc++.h> #include <iostream> using namespace libcamera; @@ -19,7 +20,7 @@ StreamKeyValueParser::StreamKeyValueParser() ArgumentRequired); addOption("height", OptionInteger, "Height in pixels", ArgumentRequired); - addOption("pixelformat", OptionInteger, "Pixel format", + addOption("pixelformat", OptionString, "Pixel format fourcc", ArgumentRequired); } @@ -96,8 +97,14 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config, } /* \todo Translate 4CC string to pixelformat with modifier. */ - if (opts.isSet("pixelformat")) - cfg.pixelFormat = PixelFormat(opts["pixelformat"]); + if (opts.isSet("pixelformat")) { + std::string fourcc = opts["pixelformat"]; + transform(fourcc.begin(), fourcc.end(), fourcc.begin(), ::toupper); + char char_array[5]; + strcpy(char_array, fourcc.c_str()); + cfg.pixelFormat = PixelFormat((__u32)(char_array[0]) | ((__u32)(char_array[1]) << 8) | + ((__u32)(char_array[2]) << 16) | ((__u32)(char_array[3]) << 24)); + } } return 0;
Replace hex input for pixelformats with their fourcc values, in cam and qcam. Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- src/cam/stream_options.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)