| Message ID | 20200325122825.GA16172@kaaira-HP-Pavilion-Notebook |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kaaira, Thank you for the patch. On Wed, Mar 25, 2020 at 05:58:25PM +0530, Kaaira Gupta wrote: > Print 4CC characters instead of the hex value in toString() as they are > more informative. > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > src/libcamera/v4l2_videodevice.cpp | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index b778181..ea8ee50 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -336,9 +336,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > */ > std::string V4L2PixelFormat::toString() const > { > - char str[11]; > - snprintf(str, 11, "0x%08x", fourcc_); > - return str; > + std::stringstream ss; Given that the string will be at mose 7 characters long, and the first four characters are individually computed, a stringstream seems overkill. You can use a plain char[] buffer and write to it directly. > + ss << static_cast<char>(fourcc_ & 0x7f) Why 7 bits only ? > + << static_cast<char>((fourcc_ >> 8) & 0x7f) > + << static_cast<char>((fourcc_ >> 16) & 0x7f) > + << static_cast<char>((fourcc_ >> 24) & 0x7f); What happens if any of these values is non-printable ? What if it's a NUL character ? Or a newline ? We shouldn't have such cases normally in V4L2 4CCs, but should we protect against that ? There's also the case of a default-constructed V4L2PixelFormat, fourcc_ will be 0 in that case. Could you also please add a test case for this function ? > + > + if (fourcc_ & (1 << 31)) > + ss << "-BE"; > + return ss.str(); > } > > /**
Hi Laurent, On 25/03/2020 12:39, Laurent Pinchart wrote: > Hi Kaaira, > > Thank you for the patch. > > On Wed, Mar 25, 2020 at 05:58:25PM +0530, Kaaira Gupta wrote: >> Print 4CC characters instead of the hex value in toString() as they are >> more informative. >> >> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> >> --- >> src/libcamera/v4l2_videodevice.cpp | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp >> index b778181..ea8ee50 100644 >> --- a/src/libcamera/v4l2_videodevice.cpp >> +++ b/src/libcamera/v4l2_videodevice.cpp >> @@ -336,9 +336,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const >> */ >> std::string V4L2PixelFormat::toString() const >> { >> - char str[11]; >> - snprintf(str, 11, "0x%08x", fourcc_); >> - return str; >> + std::stringstream ss; > > Given that the string will be at mose 7 characters long, and the first > four characters are individually computed, a stringstream seems > overkill. You can use a plain char[] buffer and write to it directly. > >> + ss << static_cast<char>(fourcc_ & 0x7f) > > Why 7 bits only ? This code follows the patch submitted by Sakari/Hans which was destined to go into the V4L2 UAPI header - but seems stalled for no apparent reason. It has only positive feedback but didn't progress: https://lore.kernel.org/linux-media/20190916100433.24367-2-hverkuil-cisco@xs4all.nl/ > >> + << static_cast<char>((fourcc_ >> 8) & 0x7f) >> + << static_cast<char>((fourcc_ >> 16) & 0x7f) >> + << static_cast<char>((fourcc_ >> 24) & 0x7f); > > What happens if any of these values is non-printable ? What if it's a > NUL character ? Or a newline ? We shouldn't have such cases normally in > V4L2 4CCs, but should we protect against that ? I don't think so no. V4L2PixelFormat is *only* an internal thing. It should never be exposed to an application, or to user input. If A V4L2 device has a NULL, newline or unprintable character, I suspect it's a bug in the kernel. > There's also the case of a default-constructed V4L2PixelFormat, fourcc_ > will be 0 in that case. fourcc_ == 0 should indeed be guarded though, and return something like <NONE> or <EMPTY> > Could you also please add a test case for this function ? >>> + >> + if (fourcc_ & (1 << 31)) >> + ss << "-BE"; >> + return ss.str(); >> } >> >> /** >
On Wed, Mar 25, 2020 at 02:39:33PM +0200, Laurent Pinchart wrote: > Hi Kaaira, > > Thank you for the patch. > > On Wed, Mar 25, 2020 at 05:58:25PM +0530, Kaaira Gupta wrote: > > Print 4CC characters instead of the hex value in toString() as they are > > more informative. > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > --- > > src/libcamera/v4l2_videodevice.cpp | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index b778181..ea8ee50 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -336,9 +336,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > */ > > std::string V4L2PixelFormat::toString() const > > { > > - char str[11]; > > - snprintf(str, 11, "0x%08x", fourcc_); > > - return str; > > + std::stringstream ss; > > Given that the string will be at mose 7 characters long, and the first > four characters are individually computed, a stringstream seems > overkill. You can use a plain char[] buffer and write to it directly. Yes, I'll do that > > > + ss << static_cast<char>(fourcc_ & 0x7f) > > Why 7 bits only ? > > > + << static_cast<char>((fourcc_ >> 8) & 0x7f) > > + << static_cast<char>((fourcc_ >> 16) & 0x7f) > > + << static_cast<char>((fourcc_ >> 24) & 0x7f); > > What happens if any of these values is non-printable ? What if it's a > NUL character ? Or a newline ? We shouldn't have such cases normally in > V4L2 4CCs, but should we protect against that ? Yes, I'll add these cases too > > There's also the case of a default-constructed V4L2PixelFormat, fourcc_ > will be 0 in that case. > > Could you also please add a test case for this function ? How should I do that? When I made the changes and wanted to check my outputs, I just printed format.fourcc.toString() in test/v4l2_videodevice/v4l2_videodevice_test.cpp...what is the correct way to write a test? > > > + > > + if (fourcc_ & (1 << 31)) > > + ss << "-BE"; > > + return ss.str(); > > } > > > > /** > > -- > Regards, > > Laurent Pinchart
Hi Kieran, On Wed, Mar 25, 2020 at 01:12:09PM +0000, Kieran Bingham wrote: > On 25/03/2020 12:39, Laurent Pinchart wrote: > > On Wed, Mar 25, 2020 at 05:58:25PM +0530, Kaaira Gupta wrote: > >> Print 4CC characters instead of the hex value in toString() as they are > >> more informative. > >> > >> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > >> --- > >> src/libcamera/v4l2_videodevice.cpp | 12 +++++++++--- > >> 1 file changed, 9 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > >> index b778181..ea8ee50 100644 > >> --- a/src/libcamera/v4l2_videodevice.cpp > >> +++ b/src/libcamera/v4l2_videodevice.cpp > >> @@ -336,9 +336,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > >> */ > >> std::string V4L2PixelFormat::toString() const > >> { > >> - char str[11]; > >> - snprintf(str, 11, "0x%08x", fourcc_); > >> - return str; > >> + std::stringstream ss; > > > > Given that the string will be at mose 7 characters long, and the first > > four characters are individually computed, a stringstream seems > > overkill. You can use a plain char[] buffer and write to it directly. > > > >> + ss << static_cast<char>(fourcc_ & 0x7f) > > > > Why 7 bits only ? > > This code follows the patch submitted by Sakari/Hans which was destined > to go into the V4L2 UAPI header - but seems stalled for no apparent > reason. It has only positive feedback but didn't progress: > > https://lore.kernel.org/linux-media/20190916100433.24367-2-hverkuil-cisco@xs4all.nl/ > > >> + << static_cast<char>((fourcc_ >> 8) & 0x7f) > >> + << static_cast<char>((fourcc_ >> 16) & 0x7f) > >> + << static_cast<char>((fourcc_ >> 24) & 0x7f); > > > > What happens if any of these values is non-printable ? What if it's a > > NUL character ? Or a newline ? We shouldn't have such cases normally in > > V4L2 4CCs, but should we protect against that ? > > I don't think so no. V4L2PixelFormat is *only* an internal thing. It > should never be exposed to an application, or to user input. > > If A V4L2 device has a NULL, newline or unprintable character, I suspect > it's a bug in the kernel. Right, but what should we then do ? :-) Maybe replacing the non-printable character (as reported by isprint() maybe ?) with a dot ? > > There's also the case of a default-constructed V4L2PixelFormat, fourcc_ > > will be 0 in that case. > > fourcc_ == 0 should indeed be guarded though, and return something like > <NONE> or <EMPTY> It's documented as invalid ;-) > > Could you also please add a test case for this function ? > >>> + > >> + if (fourcc_ & (1 << 31)) > >> + ss << "-BE"; > >> + return ss.str(); > >> } > >> > >> /**
Hi Kaaira, On Wed, Mar 25, 2020 at 06:43:27PM +0530, Kaaira Gupta wrote: > On Wed, Mar 25, 2020 at 02:39:33PM +0200, Laurent Pinchart wrote: > > On Wed, Mar 25, 2020 at 05:58:25PM +0530, Kaaira Gupta wrote: > > > Print 4CC characters instead of the hex value in toString() as they are > > > more informative. > > > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > > --- > > > src/libcamera/v4l2_videodevice.cpp | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > index b778181..ea8ee50 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -336,9 +336,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > */ > > > std::string V4L2PixelFormat::toString() const > > > { > > > - char str[11]; > > > - snprintf(str, 11, "0x%08x", fourcc_); > > > - return str; > > > + std::stringstream ss; > > > > Given that the string will be at mose 7 characters long, and the first > > four characters are individually computed, a stringstream seems > > overkill. You can use a plain char[] buffer and write to it directly. > > Yes, I'll do that > > > > + ss << static_cast<char>(fourcc_ & 0x7f) > > > > Why 7 bits only ? > > > > > + << static_cast<char>((fourcc_ >> 8) & 0x7f) > > > + << static_cast<char>((fourcc_ >> 16) & 0x7f) > > > + << static_cast<char>((fourcc_ >> 24) & 0x7f); > > > > What happens if any of these values is non-printable ? What if it's a > > NUL character ? Or a newline ? We shouldn't have such cases normally in > > V4L2 4CCs, but should we protect against that ? > > Yes, I'll add these cases too > > > There's also the case of a default-constructed V4L2PixelFormat, fourcc_ > > will be 0 in that case. > > > > Could you also please add a test case for this function ? > > How should I do that? When I made the changes and wanted to check my > outputs, I just printed format.fourcc.toString() in > test/v4l2_videodevice/v4l2_videodevice_test.cpp...what is the correct > way to write a test? That's not very nice, as it requires manually looking at the output. When need automated tests :-) You can either add a test case to an existing test, or create a new one. For this specific case we have test/v4l2_videodevice/formats.cpp that seems to be a good enough option. The test should create V4L2PixelFormat instance, and compare the output of their .toString() method with expected output. You could do something like std::vector<std::pair<uint32_t, const char *>> formats{ { V4L2_PIX_FMT_..., "..." }, }; and use a loop for the tests: for (const auto &format : formats) { if (V4L2PixelFormat(format.first).toString() != format.second) { ... } } You should include in the vector a few different corner cases (a valid format, 0, a big-endian format, a numerical value that will result in non-printable characters, ...). You should also add one check for the default constructor with V4L2PixelFormat().toString() != "...." outside of the loop. > > > + > > > + if (fourcc_ & (1 << 31)) > > > + ss << "-BE"; > > > + return ss.str(); > > > } > > > > > > /**
On Wed, Mar 25, 2020 at 09:02:29PM +0200, Laurent Pinchart wrote: > Hi Kaaira, > > On Wed, Mar 25, 2020 at 06:43:27PM +0530, Kaaira Gupta wrote: > > On Wed, Mar 25, 2020 at 02:39:33PM +0200, Laurent Pinchart wrote: > > > On Wed, Mar 25, 2020 at 05:58:25PM +0530, Kaaira Gupta wrote: > > > > Print 4CC characters instead of the hex value in toString() as they are > > > > more informative. > > > > > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > > > --- > > > > src/libcamera/v4l2_videodevice.cpp | 12 +++++++++--- > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > index b778181..ea8ee50 100644 > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > @@ -336,9 +336,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > */ > > > > std::string V4L2PixelFormat::toString() const > > > > { > > > > - char str[11]; > > > > - snprintf(str, 11, "0x%08x", fourcc_); > > > > - return str; > > > > + std::stringstream ss; > > > > > > Given that the string will be at mose 7 characters long, and the first > > > four characters are individually computed, a stringstream seems > > > overkill. You can use a plain char[] buffer and write to it directly. > > > > Yes, I'll do that > > > > > > + ss << static_cast<char>(fourcc_ & 0x7f) > > > > > > Why 7 bits only ? > > > > > > > + << static_cast<char>((fourcc_ >> 8) & 0x7f) > > > > + << static_cast<char>((fourcc_ >> 16) & 0x7f) > > > > + << static_cast<char>((fourcc_ >> 24) & 0x7f); > > > > > > What happens if any of these values is non-printable ? What if it's a > > > NUL character ? Or a newline ? We shouldn't have such cases normally in > > > V4L2 4CCs, but should we protect against that ? > > > > Yes, I'll add these cases too > > > > > There's also the case of a default-constructed V4L2PixelFormat, fourcc_ > > > will be 0 in that case. > > > > > > Could you also please add a test case for this function ? > > > > How should I do that? When I made the changes and wanted to check my > > outputs, I just printed format.fourcc.toString() in > > test/v4l2_videodevice/v4l2_videodevice_test.cpp...what is the correct > > way to write a test? > > That's not very nice, as it requires manually looking at the output. > When need automated tests :-) > > You can either add a test case to an existing test, or create a new one. > For this specific case we have test/v4l2_videodevice/formats.cpp that > seems to be a good enough option. > > The test should create V4L2PixelFormat instance, and compare the output > of their .toString() method with expected output. You could do something > like > > std::vector<std::pair<uint32_t, const char *>> formats{ > { V4L2_PIX_FMT_..., "..." }, > }; > > and use a loop for the tests: > > for (const auto &format : formats) { > if (V4L2PixelFormat(format.first).toString() != format.second) { > ... > } > } > > You should include in the vector a few different corner cases (a valid > format, 0, a big-endian format, a numerical value that will result in > non-printable characters, ...). You should also add one check for the > default constructor with V4L2PixelFormat().toString() != "...." outside > of the loop. Yes, done and submitted..but I don't understand why we need to check for '0' separately, when we are already checking for the default constructor's value? > > > > > + > > > > + if (fourcc_ & (1 << 31)) > > > > + ss << "-BE"; > > > > + return ss.str(); > > > > } > > > > > > > > /** > > -- > Regards, > > Laurent Pinchart
Hi Kaaira, On Thu, Mar 26, 2020 at 03:41:25AM +0530, Kaaira Gupta wrote: > On Wed, Mar 25, 2020 at 09:02:29PM +0200, Laurent Pinchart wrote: > > On Wed, Mar 25, 2020 at 06:43:27PM +0530, Kaaira Gupta wrote: > >> On Wed, Mar 25, 2020 at 02:39:33PM +0200, Laurent Pinchart wrote: > >>> On Wed, Mar 25, 2020 at 05:58:25PM +0530, Kaaira Gupta wrote: > >>>> Print 4CC characters instead of the hex value in toString() as they are > >>>> more informative. > >>>> > >>>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > >>>> --- > >>>> src/libcamera/v4l2_videodevice.cpp | 12 +++++++++--- > >>>> 1 file changed, 9 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > >>>> index b778181..ea8ee50 100644 > >>>> --- a/src/libcamera/v4l2_videodevice.cpp > >>>> +++ b/src/libcamera/v4l2_videodevice.cpp > >>>> @@ -336,9 +336,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > >>>> */ > >>>> std::string V4L2PixelFormat::toString() const > >>>> { > >>>> - char str[11]; > >>>> - snprintf(str, 11, "0x%08x", fourcc_); > >>>> - return str; > >>>> + std::stringstream ss; > >>> > >>> Given that the string will be at mose 7 characters long, and the first > >>> four characters are individually computed, a stringstream seems > >>> overkill. You can use a plain char[] buffer and write to it directly. > >> > >> Yes, I'll do that > >> > >>>> + ss << static_cast<char>(fourcc_ & 0x7f) > >>> > >>> Why 7 bits only ? > >>> > >>>> + << static_cast<char>((fourcc_ >> 8) & 0x7f) > >>>> + << static_cast<char>((fourcc_ >> 16) & 0x7f) > >>>> + << static_cast<char>((fourcc_ >> 24) & 0x7f); > >>> > >>> What happens if any of these values is non-printable ? What if it's a > >>> NUL character ? Or a newline ? We shouldn't have such cases normally in > >>> V4L2 4CCs, but should we protect against that ? > >> > >> Yes, I'll add these cases too > >> > >>> There's also the case of a default-constructed V4L2PixelFormat, fourcc_ > >>> will be 0 in that case. > >>> > >>> Could you also please add a test case for this function ? > >> > >> How should I do that? When I made the changes and wanted to check my > >> outputs, I just printed format.fourcc.toString() in > >> test/v4l2_videodevice/v4l2_videodevice_test.cpp...what is the correct > >> way to write a test? > > > > That's not very nice, as it requires manually looking at the output. > > When need automated tests :-) > > > > You can either add a test case to an existing test, or create a new one. > > For this specific case we have test/v4l2_videodevice/formats.cpp that > > seems to be a good enough option. > > > > The test should create V4L2PixelFormat instance, and compare the output > > of their .toString() method with expected output. You could do something > > like > > > > std::vector<std::pair<uint32_t, const char *>> formats{ > > { V4L2_PIX_FMT_..., "..." }, > > }; > > > > and use a loop for the tests: > > > > for (const auto &format : formats) { > > if (V4L2PixelFormat(format.first).toString() != format.second) { > > ... > > } > > } > > > > You should include in the vector a few different corner cases (a valid > > format, 0, a big-endian format, a numerical value that will result in > > non-printable characters, ...). You should also add one check for the > > default constructor with V4L2PixelFormat().toString() != "...." outside > > of the loop. > > Yes, done and submitted..but I don't understand why we need to check for > '0' separately, when we are already checking for the default > constructor's value? There's no current test case to ensure that V4L2PixelFormat() creates an instance with fourcc == 0, that's why I've proposed a separate case. It's hijacking your work a little bit :-) > >>>> + > >>>> + if (fourcc_ & (1 << 31)) > >>>> + ss << "-BE"; > >>>> + return ss.str(); > >>>> } > >>>> > >>>> /**
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index b778181..ea8ee50 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -336,9 +336,15 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const */ std::string V4L2PixelFormat::toString() const { - char str[11]; - snprintf(str, 11, "0x%08x", fourcc_); - return str; + std::stringstream ss; + ss << static_cast<char>(fourcc_ & 0x7f) + << static_cast<char>((fourcc_ >> 8) & 0x7f) + << static_cast<char>((fourcc_ >> 16) & 0x7f) + << static_cast<char>((fourcc_ >> 24) & 0x7f); + + if (fourcc_ & (1 << 31)) + ss << "-BE"; + return ss.str(); } /**
Print 4CC characters instead of the hex value in toString() as they are more informative. Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- src/libcamera/v4l2_videodevice.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)