| Message ID | 20200326201400.GA29067@kaaira-HP-Pavilion-Notebook |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kaaira, On 26/03/2020 20:14, Kaaira Gupta wrote: > Print fourCC characters instead of the hex value in toString() as they are > more informative. Also, write the tests for this in formats.cpp Whenever someone gives you a tag, hang on to it - And keep it in further versions. Tags are really valuable (to you, the submitter) as evidence of existing reviews etc. Don't lose them :-) If code substantially changes beyond review comments, then it might be a time to drop given tags, but in this instance my tag from v3 certainly still applies. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thank you for your work. We'll merge this today :-) -- Kieran > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > > Changes since v3: > - Reformatted the code > - Changed 'invalid' to "<INVALID>" > - Changed cerr message. > > Changes since v2: > - reformatted the code > - added cerr messages before TestFail > - Changed test case to accomodate edge case > - Increased buffer size of char array > - Changed the maximum length of for loop. > > Changes since v1: > - Add tests for checking this function. > - use char[] instead of stringstream. > - add checks for default value. > - Print '.' for non-printable characters. > > src/libcamera/v4l2_videodevice.cpp | 20 +++++++++++++++++--- > test/v4l2_videodevice/formats.cpp | 22 ++++++++++++++++++++++ > 2 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index b778181..eb33a68 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -336,9 +336,23 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > */ > std::string V4L2PixelFormat::toString() const > { > - char str[11]; > - snprintf(str, 11, "0x%08x", fourcc_); > - return str; > + if (fourcc_ == 0) > + return "<INVALID>"; > + > + char ss[8] = { static_cast<char>(fourcc_ & 0x7f), > + static_cast<char>((fourcc_ >> 8) & 0x7f), > + static_cast<char>((fourcc_ >> 16) & 0x7f), > + static_cast<char>((fourcc_ >> 24) & 0x7f) }; > + > + for (unsigned int i = 0; i < 4; i++) { > + if (!isprint(ss[i])) > + ss[i] = '.'; > + } > + > + if (fourcc_ & (1 << 31)) > + strcat(ss, "-BE"); > + > + return ss; > } > > /** > diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp > index d504d17..d01eccd 100644 > --- a/test/v4l2_videodevice/formats.cpp > +++ b/test/v4l2_videodevice/formats.cpp > @@ -47,6 +47,28 @@ protected: > return TestFail; > } > > + std::vector<std::pair<uint32_t, const char*>> formats{ > + { V4L2_PIX_FMT_YUYV, "YUYV" }, > + { 0, "<INVALID>" }, > + { v4l2_fourcc(0, 1, 2, 3), "...." }, > + { V4L2_PIX_FMT_Y16_BE, "Y16 -BE" } > + }; > + > + for (const auto &format : formats) { > + if (V4L2PixelFormat(format.first).toString() != format.second) { > + cerr << "Failed to convert V4L2PixelFormat" > + << utils::hex(format.first) << "to string" > + << endl; > + return TestFail; > + } > + } > + > + if (V4L2PixelFormat().toString() != "<INVALID>") { > + cerr << "Failed to convert default V4L2PixelFormat to string" > + << endl; > + return TestFail; > + } > + > return TestPass; > } > }; >
Hi Kaaira, Thank you for the patch. On Fri, Mar 27, 2020 at 01:44:00AM +0530, Kaaira Gupta wrote: > Print fourCC characters instead of the hex value in toString() as they are > more informative. Also, write the tests for this in formats.cpp > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > > Changes since v3: > - Reformatted the code > - Changed 'invalid' to "<INVALID>" > - Changed cerr message. > > Changes since v2: > - reformatted the code > - added cerr messages before TestFail > - Changed test case to accomodate edge case > - Increased buffer size of char array > - Changed the maximum length of for loop. > > Changes since v1: > - Add tests for checking this function. > - use char[] instead of stringstream. > - add checks for default value. > - Print '.' for non-printable characters. > > src/libcamera/v4l2_videodevice.cpp | 20 +++++++++++++++++--- > test/v4l2_videodevice/formats.cpp | 22 ++++++++++++++++++++++ > 2 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index b778181..eb33a68 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -336,9 +336,23 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > */ > std::string V4L2PixelFormat::toString() const > { > - char str[11]; > - snprintf(str, 11, "0x%08x", fourcc_); > - return str; > + if (fourcc_ == 0) > + return "<INVALID>"; > + > + char ss[8] = { static_cast<char>(fourcc_ & 0x7f), > + static_cast<char>((fourcc_ >> 8) & 0x7f), > + static_cast<char>((fourcc_ >> 16) & 0x7f), > + static_cast<char>((fourcc_ >> 24) & 0x7f) }; > + > + for (unsigned int i = 0; i < 4; i++) { > + if (!isprint(ss[i])) > + ss[i] = '.'; > + } > + > + if (fourcc_ & (1 << 31)) > + strcat(ss, "-BE"); > + > + return ss; > } > > /** > diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp > index d504d17..d01eccd 100644 > --- a/test/v4l2_videodevice/formats.cpp > +++ b/test/v4l2_videodevice/formats.cpp > @@ -47,6 +47,28 @@ protected: > return TestFail; > } > > + std::vector<std::pair<uint32_t, const char*>> formats{ > + { V4L2_PIX_FMT_YUYV, "YUYV" }, > + { 0, "<INVALID>" }, > + { v4l2_fourcc(0, 1, 2, 3), "...." }, > + { V4L2_PIX_FMT_Y16_BE, "Y16 -BE" } > + }; > + > + for (const auto &format : formats) { > + if (V4L2PixelFormat(format.first).toString() != format.second) { > + cerr << "Failed to convert V4L2PixelFormat" > + << utils::hex(format.first) << "to string" This file should include "utils.h". With this fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Kieran, can you fix that when applying ? > + << endl; > + return TestFail; > + } > + } > + > + if (V4L2PixelFormat().toString() != "<INVALID>") { > + cerr << "Failed to convert default V4L2PixelFormat to string" > + << endl; > + return TestFail; > + } > + > return TestPass; > } > };
Hi Laurent, On 27/03/2020 15:13, Laurent Pinchart wrote: > Hi Kaaira, > > Thank you for the patch. > > On Fri, Mar 27, 2020 at 01:44:00AM +0530, Kaaira Gupta wrote: >> Print fourCC characters instead of the hex value in toString() as they are >> more informative. Also, write the tests for this in formats.cpp >> >> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> >> --- >> >> Changes since v3: >> - Reformatted the code >> - Changed 'invalid' to "<INVALID>" >> - Changed cerr message. >> >> Changes since v2: >> - reformatted the code >> - added cerr messages before TestFail >> - Changed test case to accomodate edge case >> - Increased buffer size of char array >> - Changed the maximum length of for loop. >> >> Changes since v1: >> - Add tests for checking this function. >> - use char[] instead of stringstream. >> - add checks for default value. >> - Print '.' for non-printable characters. >> >> src/libcamera/v4l2_videodevice.cpp | 20 +++++++++++++++++--- >> test/v4l2_videodevice/formats.cpp | 22 ++++++++++++++++++++++ >> 2 files changed, 39 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp >> index b778181..eb33a68 100644 >> --- a/src/libcamera/v4l2_videodevice.cpp >> +++ b/src/libcamera/v4l2_videodevice.cpp >> @@ -336,9 +336,23 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const >> */ >> std::string V4L2PixelFormat::toString() const >> { >> - char str[11]; >> - snprintf(str, 11, "0x%08x", fourcc_); >> - return str; >> + if (fourcc_ == 0) >> + return "<INVALID>"; >> + >> + char ss[8] = { static_cast<char>(fourcc_ & 0x7f), >> + static_cast<char>((fourcc_ >> 8) & 0x7f), >> + static_cast<char>((fourcc_ >> 16) & 0x7f), >> + static_cast<char>((fourcc_ >> 24) & 0x7f) }; >> + >> + for (unsigned int i = 0; i < 4; i++) { >> + if (!isprint(ss[i])) >> + ss[i] = '.'; >> + } >> + >> + if (fourcc_ & (1 << 31)) >> + strcat(ss, "-BE"); >> + >> + return ss; >> } >> >> /** >> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp >> index d504d17..d01eccd 100644 >> --- a/test/v4l2_videodevice/formats.cpp >> +++ b/test/v4l2_videodevice/formats.cpp >> @@ -47,6 +47,28 @@ protected: >> return TestFail; >> } >> >> + std::vector<std::pair<uint32_t, const char*>> formats{ >> + { V4L2_PIX_FMT_YUYV, "YUYV" }, >> + { 0, "<INVALID>" }, >> + { v4l2_fourcc(0, 1, 2, 3), "...." }, >> + { V4L2_PIX_FMT_Y16_BE, "Y16 -BE" } >> + }; >> + >> + for (const auto &format : formats) { >> + if (V4L2PixelFormat(format.first).toString() != format.second) { >> + cerr << "Failed to convert V4L2PixelFormat" >> + << utils::hex(format.first) << "to string" > > This file should include "utils.h". With this fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Kieran, can you fix that when applying ? Done. And pushed to master. -- Kieran > >> + << endl; >> + return TestFail; >> + } >> + } >> + >> + if (V4L2PixelFormat().toString() != "<INVALID>") { >> + cerr << "Failed to convert default V4L2PixelFormat to string" >> + << endl; >> + return TestFail; >> + } >> + >> return TestPass; >> } >> }; >
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index b778181..eb33a68 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -336,9 +336,23 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const */ std::string V4L2PixelFormat::toString() const { - char str[11]; - snprintf(str, 11, "0x%08x", fourcc_); - return str; + if (fourcc_ == 0) + return "<INVALID>"; + + char ss[8] = { static_cast<char>(fourcc_ & 0x7f), + static_cast<char>((fourcc_ >> 8) & 0x7f), + static_cast<char>((fourcc_ >> 16) & 0x7f), + static_cast<char>((fourcc_ >> 24) & 0x7f) }; + + for (unsigned int i = 0; i < 4; i++) { + if (!isprint(ss[i])) + ss[i] = '.'; + } + + if (fourcc_ & (1 << 31)) + strcat(ss, "-BE"); + + return ss; } /** diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp index d504d17..d01eccd 100644 --- a/test/v4l2_videodevice/formats.cpp +++ b/test/v4l2_videodevice/formats.cpp @@ -47,6 +47,28 @@ protected: return TestFail; } + std::vector<std::pair<uint32_t, const char*>> formats{ + { V4L2_PIX_FMT_YUYV, "YUYV" }, + { 0, "<INVALID>" }, + { v4l2_fourcc(0, 1, 2, 3), "...." }, + { V4L2_PIX_FMT_Y16_BE, "Y16 -BE" } + }; + + for (const auto &format : formats) { + if (V4L2PixelFormat(format.first).toString() != format.second) { + cerr << "Failed to convert V4L2PixelFormat" + << utils::hex(format.first) << "to string" + << endl; + return TestFail; + } + } + + if (V4L2PixelFormat().toString() != "<INVALID>") { + cerr << "Failed to convert default V4L2PixelFormat to string" + << endl; + return TestFail; + } + return TestPass; } };
Print fourCC characters instead of the hex value in toString() as they are more informative. Also, write the tests for this in formats.cpp Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- Changes since v3: - Reformatted the code - Changed 'invalid' to "<INVALID>" - Changed cerr message. Changes since v2: - reformatted the code - added cerr messages before TestFail - Changed test case to accomodate edge case - Increased buffer size of char array - Changed the maximum length of for loop. Changes since v1: - Add tests for checking this function. - use char[] instead of stringstream. - add checks for default value. - Print '.' for non-printable characters. src/libcamera/v4l2_videodevice.cpp | 20 +++++++++++++++++--- test/v4l2_videodevice/formats.cpp | 22 ++++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-)