| Message ID | 20250212113328.304528-1-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2025-02-12 11:29:42) > Calling PixelFormat().toString() correctly returns "0x0-<INVALID>" but it > also produces the following, possibly confusing, warning: > > WARN Formats formats.cpp:1006 Unsupported pixel format 0x00000000 > > Silence the warning in PixelFormatInfo::info() in case the format is > invalid. > > While at it, remove code duplication by using > PixelFormatInfo::info(const PixelFormat &format) to implement > PixelFormatInfo::info(const V4L2PixelFormat &format). Just to check the obvious - if there is a V4L2 format on a device that we don't have mapped/supported in libcamera, will it still print WARN Formats formats.cpp:1006 Unsupported pixel format 0x23456789 ? As long as that's the case, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > Hi all, > > The attached patch has one noteworthy side effect: If > info(V4L2PixelFormat &format) is called with a valid but unsupported (by > libcamera) format, we now get the same warning as in the > info(PixelFormat &format) case. I believe that is sensible but maybe the > warning was left off in the V4L2 case on purpose. > > Best regards, > Stefan > > src/libcamera/formats.cpp | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp > index bfcdfc08960d..df7413f58ba8 100644 > --- a/src/libcamera/formats.cpp > +++ b/src/libcamera/formats.cpp > @@ -1001,6 +1001,9 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > */ > const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format) > { > + if (!format.isValid()) > + return pixelFormatInfoInvalid; > + > const auto iter = pixelFormatInfo.find(format); > if (iter == pixelFormatInfo.end()) { > LOG(Formats, Warning) Does the !isValid() check mean it's impossible to get here now? Or is this check still necessary? > @@ -1021,14 +1024,7 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format) > const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format) > { > PixelFormat pixelFormat = format.toPixelFormat(false); > - if (!pixelFormat.isValid()) > - return pixelFormatInfoInvalid; > - > - const auto iter = pixelFormatInfo.find(pixelFormat); > - if (iter == pixelFormatInfo.end()) > - return pixelFormatInfoInvalid; > - > - return iter->second; > + return info(pixelFormat); > > } > > /** > -- > 2.43.0 >
Hi Kieran, Thank you for the review. On Wed, Feb 12, 2025 at 11:50:19AM +0000, Kieran Bingham wrote: > Quoting Stefan Klug (2025-02-12 11:29:42) > > Calling PixelFormat().toString() correctly returns "0x0-<INVALID>" but it > > also produces the following, possibly confusing, warning: > > > > WARN Formats formats.cpp:1006 Unsupported pixel format 0x00000000 > > > > Silence the warning in PixelFormatInfo::info() in case the format is > > invalid. > > > > While at it, remove code duplication by using > > PixelFormatInfo::info(const PixelFormat &format) to implement > > PixelFormatInfo::info(const V4L2PixelFormat &format). > > Just to check the obvious - if there is a V4L2 format on a device that > we don't have mapped/supported in libcamera, will it still print > > WARN Formats formats.cpp:1006 Unsupported pixel format 0x23456789 ? > > As long as that's the case, Well, we got that warning only for formats passed as PixelFormat. If the format was passed as V4L2PixelFormat there was no warning at all. Now we get the warning in both cases. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > > > Hi all, > > > > The attached patch has one noteworthy side effect: If > > info(V4L2PixelFormat &format) is called with a valid but unsupported (by > > libcamera) format, we now get the same warning as in the > > info(PixelFormat &format) case. I believe that is sensible but maybe the > > warning was left off in the V4L2 case on purpose. > > > > Best regards, > > Stefan > > > > src/libcamera/formats.cpp | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp > > index bfcdfc08960d..df7413f58ba8 100644 > > --- a/src/libcamera/formats.cpp > > +++ b/src/libcamera/formats.cpp > > @@ -1001,6 +1001,9 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > > */ > > const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format) > > { > > + if (!format.isValid()) > > + return pixelFormatInfoInvalid; > > + > > const auto iter = pixelFormatInfo.find(format); > > if (iter == pixelFormatInfo.end()) { > > LOG(Formats, Warning) > > Does the !isValid() check mean it's impossible to get here now? Or is > this check still necessary? isValid() only checks if the fourcc code is 0. So the warning shows up if the format is valid but not supported by libcamera. Which was imho the intended case. Best regards, Stefan > > > > @@ -1021,14 +1024,7 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format) > > const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format) > > { > > PixelFormat pixelFormat = format.toPixelFormat(false); > > - if (!pixelFormat.isValid()) > > - return pixelFormatInfoInvalid; > > - > > - const auto iter = pixelFormatInfo.find(pixelFormat); > > - if (iter == pixelFormatInfo.end()) > > - return pixelFormatInfoInvalid; > > - > > - return iter->second; > > + return info(pixelFormat); > > > > } > > > > /** > > -- > > 2.43.0 > >
On Wed, Feb 12, 2025 at 01:00:21PM +0100, Stefan Klug wrote: > On Wed, Feb 12, 2025 at 11:50:19AM +0000, Kieran Bingham wrote: > > Quoting Stefan Klug (2025-02-12 11:29:42) > > > Calling PixelFormat().toString() correctly returns "0x0-<INVALID>" but it > > > also produces the following, possibly confusing, warning: > > > > > > WARN Formats formats.cpp:1006 Unsupported pixel format 0x00000000 > > > > > > Silence the warning in PixelFormatInfo::info() in case the format is > > > invalid. > > > > > > While at it, remove code duplication by using > > > PixelFormatInfo::info(const PixelFormat &format) to implement > > > PixelFormatInfo::info(const V4L2PixelFormat &format). > > > > Just to check the obvious - if there is a V4L2 format on a device that > > we don't have mapped/supported in libcamera, will it still print > > > > WARN Formats formats.cpp:1006 Unsupported pixel format 0x23456789 ? > > > > As long as that's the case, > > Well, we got that warning only for formats passed as PixelFormat. If the > format was passed as V4L2PixelFormat there was no warning at all. Now we > get the warning in both cases. This would be worth mentioning in the commit message. Or possible split in two patches, in case the additional warning for V4L2PixelFormat ends up being too noisy and we need to revert that part ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > > > > Hi all, > > > > > > The attached patch has one noteworthy side effect: If > > > info(V4L2PixelFormat &format) is called with a valid but unsupported (by > > > libcamera) format, we now get the same warning as in the > > > info(PixelFormat &format) case. I believe that is sensible but maybe the > > > warning was left off in the V4L2 case on purpose. > > > > > > Best regards, > > > Stefan > > > > > > src/libcamera/formats.cpp | 12 ++++-------- > > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp > > > index bfcdfc08960d..df7413f58ba8 100644 > > > --- a/src/libcamera/formats.cpp > > > +++ b/src/libcamera/formats.cpp > > > @@ -1001,6 +1001,9 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > > > */ > > > const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format) > > > { > > > + if (!format.isValid()) > > > + return pixelFormatInfoInvalid; > > > + > > > const auto iter = pixelFormatInfo.find(format); > > > if (iter == pixelFormatInfo.end()) { > > > LOG(Formats, Warning) > > > > Does the !isValid() check mean it's impossible to get here now? Or is > > this check still necessary? > > isValid() only checks if the fourcc code is 0. So the warning shows up > if the format is valid but not supported by libcamera. Which was imho > the intended case. > > > > @@ -1021,14 +1024,7 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format) > > > const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format) > > > { > > > PixelFormat pixelFormat = format.toPixelFormat(false); > > > - if (!pixelFormat.isValid()) > > > - return pixelFormatInfoInvalid; > > > - > > > - const auto iter = pixelFormatInfo.find(pixelFormat); > > > - if (iter == pixelFormatInfo.end()) > > > - return pixelFormatInfoInvalid; > > > - > > > - return iter->second; > > > + return info(pixelFormat); > > > > > > } > > > > > > /**
diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp index bfcdfc08960d..df7413f58ba8 100644 --- a/src/libcamera/formats.cpp +++ b/src/libcamera/formats.cpp @@ -1001,6 +1001,9 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ */ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format) { + if (!format.isValid()) + return pixelFormatInfoInvalid; + const auto iter = pixelFormatInfo.find(format); if (iter == pixelFormatInfo.end()) { LOG(Formats, Warning) @@ -1021,14 +1024,7 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format) const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format) { PixelFormat pixelFormat = format.toPixelFormat(false); - if (!pixelFormat.isValid()) - return pixelFormatInfoInvalid; - - const auto iter = pixelFormatInfo.find(pixelFormat); - if (iter == pixelFormatInfo.end()) - return pixelFormatInfoInvalid; - - return iter->second; + return info(pixelFormat); } /**
Calling PixelFormat().toString() correctly returns "0x0-<INVALID>" but it also produces the following, possibly confusing, warning: WARN Formats formats.cpp:1006 Unsupported pixel format 0x00000000 Silence the warning in PixelFormatInfo::info() in case the format is invalid. While at it, remove code duplication by using PixelFormatInfo::info(const PixelFormat &format) to implement PixelFormatInfo::info(const V4L2PixelFormat &format). Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Hi all, The attached patch has one noteworthy side effect: If info(V4L2PixelFormat &format) is called with a valid but unsupported (by libcamera) format, we now get the same warning as in the info(PixelFormat &format) case. I believe that is sensible but maybe the warning was left off in the V4L2 case on purpose. Best regards, Stefan src/libcamera/formats.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)