[libcamera-devel] [PATCH] libcamera: v4l2PixelFormat: replace hex with 4CC
Kaaira Gupta
kgupta at es.iitr.ac.in
Sat Apr 18 14:54:17 CEST 2020
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 at 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
More information about the libcamera-devel
mailing list