| Message ID | 20200326093327.GA9015@kaaira-HP-Pavilion-Notebook |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kaaira, On 26/03/2020 09:33, 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> Thank you, This looks good to me. Only a couple of quite minor formatting points, which if there are no further issues, and you're happy with; I can handle when applying. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > 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 | 19 ++++++++++++++++--- > test/v4l2_videodevice/formats.cpp | 20 ++++++++++++++++++++ > 2 files changed, 36 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index b778181..bb049ec 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -336,9 +336,22 @@ 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"); Probably a blank line here to make sure the return statement is distinct. > + return ss; > } > > /** > diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp > index d504d17..4e02afc 100644 > --- a/test/v4l2_videodevice/formats.cpp > +++ b/test/v4l2_videodevice/formats.cpp > @@ -47,6 +47,26 @@ protected: > return TestFail; > } > > + std::vector<std::pair<uint32_t, const char*>> formats{ Missing space between formats and { > + { V4L2_PIX_FMT_YUYV, "YUYV" }, { 0, "Invalid" }, > + { v4l2_fourcc( 0, 1, 2, 3 ), "...." }, Because this is a table, I would probably put each entry on it's own line, rather than condensing to fit 80chars or such. i.e. { 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 to its fourcc" > + << " string" << endl; > + return TestFail; > + } > + } > + > + if (V4L2PixelFormat().toString() != "Invalid") { > + cerr << "Failed to convert default V4L2PixelFormat to 'Invalid'" > + << " string" << endl; > + return TestFail; > + } > + > return TestPass; > } > }; >
Hi again :-) On 26/03/2020 15:37, Kieran Bingham wrote: > Hi Kaaira, > > On 26/03/2020 09:33, 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> > > Thank you, This looks good to me. > > Only a couple of quite minor formatting points, which if there are no > further issues, and you're happy with; I can handle when applying. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- >> >> 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 | 19 ++++++++++++++++--- >> test/v4l2_videodevice/formats.cpp | 20 ++++++++++++++++++++ >> 2 files changed, 36 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp >> index b778181..bb049ec 100644 >> --- a/src/libcamera/v4l2_videodevice.cpp >> +++ b/src/libcamera/v4l2_videodevice.cpp >> @@ -336,9 +336,22 @@ 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"); > > Probably a blank line here to make sure the return statement is distinct. > >> + return ss; >> } >> >> /** >> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp >> index d504d17..4e02afc 100644 >> --- a/test/v4l2_videodevice/formats.cpp >> +++ b/test/v4l2_videodevice/formats.cpp >> @@ -47,6 +47,26 @@ protected: >> return TestFail; >> } >> >> + std::vector<std::pair<uint32_t, const char*>> formats{ > > Missing space between formats and { Ahh - checkstyle correctly points out I'm wrong here, as this is hugged due to it being an initialiser, rather than a scope block. I guess the alternative is: formats = { But it doesn't make much differnce I don't think. Let's keep it as you posted without the space. > >> + { V4L2_PIX_FMT_YUYV, "YUYV" }, { 0, "Invalid" }, >> + { v4l2_fourcc( 0, 1, 2, 3 ), "...." }, > > Because this is a table, I would probably put each entry on it's own > line, rather than condensing to fit 80chars or such. > > > i.e. > { 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 to its fourcc" >> + << " string" << endl; >> + return TestFail; >> + } >> + } >> + >> + if (V4L2PixelFormat().toString() != "Invalid") { >> + cerr << "Failed to convert default V4L2PixelFormat to 'Invalid'" >> + << " string" << endl; >> + return TestFail; >> + } >> + >> return TestPass; >> } >> }; >> >
Hi Kieran, On Thu, Mar 26, 2020 at 03:49:04PM +0000, Kieran Bingham wrote: > On 26/03/2020 15:37, Kieran Bingham wrote: > > On 26/03/2020 09:33, 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> > > > > Thank you, This looks good to me. > > > > Only a couple of quite minor formatting points, which if there are no > > further issues, and you're happy with; I can handle when applying. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> --- > >> > >> 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 | 19 ++++++++++++++++--- > >> test/v4l2_videodevice/formats.cpp | 20 ++++++++++++++++++++ > >> 2 files changed, 36 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > >> index b778181..bb049ec 100644 > >> --- a/src/libcamera/v4l2_videodevice.cpp > >> +++ b/src/libcamera/v4l2_videodevice.cpp > >> @@ -336,9 +336,22 @@ 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"; This will result in messages such as "Pixel format set to Invalid". I wonder if "INVALID", or maybe something even more distinct from surrounding text such as "[INV]" or "<INV>" would make sense. > >> + > >> + 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"); > > > > Probably a blank line here to make sure the return statement is distinct. > > > >> + return ss; > >> } > >> > >> /** > >> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp > >> index d504d17..4e02afc 100644 > >> --- a/test/v4l2_videodevice/formats.cpp > >> +++ b/test/v4l2_videodevice/formats.cpp > >> @@ -47,6 +47,26 @@ protected: > >> return TestFail; > >> } > >> > >> + std::vector<std::pair<uint32_t, const char*>> formats{ > > > > Missing space between formats and { > > Ahh - checkstyle correctly points out I'm wrong here, as this is hugged > due to it being an initialiser, rather than a scope block. > > I guess the alternative is: > > formats = { > > But it doesn't make much differnce I don't think. > > Let's keep it as you posted without the space. Just wanted to note that we'll likely need to go through the code to make consistent usage of different initialization types at some point. This is certainly fine for now. > >> + { V4L2_PIX_FMT_YUYV, "YUYV" }, { 0, "Invalid" }, > >> + { v4l2_fourcc( 0, 1, 2, 3 ), "...." }, > > > > Because this is a table, I would probably put each entry on it's own > > line, rather than condensing to fit 80chars or such. > > > > > > i.e. > > { V4L2_PIX_FMT_YUYV, "YUYV" }, > > { 0, "Invalid" }, > > { v4l2_fourcc( 0, 1, 2, 3 ), "...." }, > > ... I was about to propose that too :-) > >> + { V4L2_PIX_FMT_Y16_BE, "Y16 -BE" } > >> + }; > >> + > >> + for (const auto &format : formats) { > >> + if (V4L2PixelFormat(format.first).toString() != format.second) { > >> + cerr << "Failed to convert V4L2PixelFormat to its fourcc" > >> + << " string" << endl; How about cerr << "Failed to convert V4L2PixelFormat " << utils::hex(format.first) << "to string" << endl; to tell which one failed ? > >> + return TestFail; > >> + } > >> + } > >> + > >> + if (V4L2PixelFormat().toString() != "Invalid") { > >> + cerr << "Failed to convert default V4L2PixelFormat to 'Invalid'" > >> + << " string" << endl; This could then be 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..bb049ec 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -336,9 +336,22 @@ 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..4e02afc 100644 --- a/test/v4l2_videodevice/formats.cpp +++ b/test/v4l2_videodevice/formats.cpp @@ -47,6 +47,26 @@ 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 to its fourcc" + << " string" << endl; + return TestFail; + } + } + + if (V4L2PixelFormat().toString() != "Invalid") { + cerr << "Failed to convert default V4L2PixelFormat to 'Invalid'" + << " 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 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 | 19 ++++++++++++++++--- test/v4l2_videodevice/formats.cpp | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-)