[libcamera-devel] libcamera: v4l2_subdevice: Silence warning for unknown metadata formats
diff mbox series

Message ID 20220905164600.31629-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: v4l2_subdevice: Silence warning for unknown metadata formats
Related show

Commit Message

Laurent Pinchart Sept. 5, 2022, 4:46 p.m. UTC 
Commit e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on
pixel format") has introduced a warning when trying to convert a color
space from V4L2 to libcamera if the media bus code is unknown. This was
meant to catch unknown image formats, but turned out to be also
triggered for metadata formats.

Color spaces are not applicable to metadata formats, there should thus
be no warning. Fix it by skipping the color space translation and
returning std::nullopt directly if the kernel reports
V4L2_COLORSPACE_DEFAULT. This doesn't introduce any change in behaviour
other than getting rid of the warning, as the V4L2Device::toColorSpace()
function returns std::nullopt alread in that case.

Fixes: e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on pixel format")
Reported-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/v4l2_subdevice.h |  3 ++
 src/libcamera/v4l2_subdevice.cpp            | 51 ++++++++++-----------
 2 files changed, 26 insertions(+), 28 deletions(-)


base-commit: da9bb8dea6821edcfd3464e8cba37ad4a91087d6

Comments

Umang Jain Sept. 5, 2022, 7:42 p.m. UTC | #1 
Hi Laurent,

Thank you for the patch.

On 9/5/22 10:16 PM, Laurent Pinchart via libcamera-devel wrote:
> Commit e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on
> pixel format") has introduced a warning when trying to convert a color
> space from V4L2 to libcamera if the media bus code is unknown. This was
> meant to catch unknown image formats, but turned out to be also
> triggered for metadata formats.

err...
>
> Color spaces are not applicable to metadata formats, there should thus
> be no warning. Fix it by skipping the color space translation and
> returning std::nullopt directly if the kernel reports
> V4L2_COLORSPACE_DEFAULT. This doesn't introduce any change in behaviour
> other than getting rid of the warning, as the V4L2Device::toColorSpace()
> function returns std::nullopt alread in that case.

s/alread/already
>
> Fixes: e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on pixel format")
> Reported-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   include/libcamera/internal/v4l2_subdevice.h |  3 ++
>   src/libcamera/v4l2_subdevice.cpp            | 51 ++++++++++-----------
>   2 files changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 00be17bb1465..69862de0585a 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -101,6 +101,9 @@ protected:
>   private:
>   	LIBCAMERA_DISABLE_COPY(V4L2Subdevice)
>   
> +	std::optional<ColorSpace>
> +	toColorSpace(const v4l2_mbus_framefmt &format) const;
> +
>   	std::vector<unsigned int> enumPadCodes(unsigned int pad);
>   	std::vector<SizeRange> enumPadSizes(unsigned int pad,
>   					    unsigned int code);
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 95bfde340d8c..f3a9a0096c6c 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -477,6 +477,27 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
>   	return formats;
>   }
>   
> +std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
> +{

Comment about metadata formats <>  V4L2_COLORSPACE_DEFAULT please :-)

Other than that, looks good to me.

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> +	if (format.colorspace == V4L2_COLORSPACE_DEFAULT)
> +		return std::nullopt;
> +
> +	PixelFormatInfo::ColourEncoding colourEncoding;
> +	auto iter = formatInfoMap.find(format.code);
> +	if (iter != formatInfoMap.end()) {
> +		colourEncoding = iter->second.colourEncoding;
> +	} else {
> +		LOG(V4L2, Warning)
> +			<< "Unknown subdev format "
> +			<< utils::hex(format.code, 4)
> +			<< ", defaulting to RGB encoding";
> +
> +		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> +	}
> +
> +	return V4L2Device::toColorSpace(format, colourEncoding);
> +}
> +
>   /**
>    * \brief Retrieve the image format set on one of the V4L2 subdevice pads
>    * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> @@ -503,20 +524,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>   	format->size.width = subdevFmt.format.width;
>   	format->size.height = subdevFmt.format.height;
>   	format->mbus_code = subdevFmt.format.code;
> -
> -	PixelFormatInfo::ColourEncoding colourEncoding;
> -	auto iter = formatInfoMap.find(subdevFmt.format.code);
> -	if (iter != formatInfoMap.end()) {
> -		colourEncoding = iter->second.colourEncoding;
> -	} else {
> -		LOG(V4L2, Warning)
> -			<< "Unknown subdev format "
> -			<< utils::hex(subdevFmt.format.code, 4)
> -			<< ", defaulting to RGB encoding";
> -
> -		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> -	}
> -	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
> +	format->colorSpace = toColorSpace(subdevFmt.format);
>   
>   	return 0;
>   }
> @@ -562,20 +570,7 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>   	format->size.width = subdevFmt.format.width;
>   	format->size.height = subdevFmt.format.height;
>   	format->mbus_code = subdevFmt.format.code;
> -
> -	PixelFormatInfo::ColourEncoding colourEncoding;
> -	auto iter = formatInfoMap.find(subdevFmt.format.code);
> -	if (iter != formatInfoMap.end()) {
> -		colourEncoding = iter->second.colourEncoding;
> -	} else {
> -		LOG(V4L2, Warning)
> -			<< "Unknown subdev format "
> -			<< utils::hex(subdevFmt.format.code, 4)
> -			<< ", defaulting to RGB encoding";
> -
> -		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> -	}
> -	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
> +	format->colorSpace = toColorSpace(subdevFmt.format);
>   
>   	return 0;
>   }
>
> base-commit: da9bb8dea6821edcfd3464e8cba37ad4a91087d6
Laurent Pinchart Sept. 5, 2022, 9:18 p.m. UTC | #2 
Hi Umang,

On Tue, Sep 06, 2022 at 01:12:39AM +0530, Umang Jain wrote:
> On 9/5/22 10:16 PM, Laurent Pinchart via libcamera-devel wrote:
> > Commit e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on
> > pixel format") has introduced a warning when trying to convert a color
> > space from V4L2 to libcamera if the media bus code is unknown. This was
> > meant to catch unknown image formats, but turned out to be also
> > triggered for metadata formats.
> 
> err...
> 
> > Color spaces are not applicable to metadata formats, there should thus
> > be no warning. Fix it by skipping the color space translation and
> > returning std::nullopt directly if the kernel reports
> > V4L2_COLORSPACE_DEFAULT. This doesn't introduce any change in behaviour
> > other than getting rid of the warning, as the V4L2Device::toColorSpace()
> > function returns std::nullopt alread in that case.
> 
> s/alread/already
> 
> > Fixes: e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on pixel format")
> > Reported-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   include/libcamera/internal/v4l2_subdevice.h |  3 ++
> >   src/libcamera/v4l2_subdevice.cpp            | 51 ++++++++++-----------
> >   2 files changed, 26 insertions(+), 28 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > index 00be17bb1465..69862de0585a 100644
> > --- a/include/libcamera/internal/v4l2_subdevice.h
> > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > @@ -101,6 +101,9 @@ protected:
> >   private:
> >   	LIBCAMERA_DISABLE_COPY(V4L2Subdevice)
> >   
> > +	std::optional<ColorSpace>
> > +	toColorSpace(const v4l2_mbus_framefmt &format) const;
> > +
> >   	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> >   	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> >   					    unsigned int code);
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 95bfde340d8c..f3a9a0096c6c 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -477,6 +477,27 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> >   	return formats;
> >   }
> >   
> > +std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
> > +{
> 
> Comment about metadata formats <>  V4L2_COLORSPACE_DEFAULT please :-)

I'll add

	/*
	 * Only image formats have a color space, for other formats (such as
	 * metadata formats) the color space concept isn't applicable. V4L2
	 * subdev drivers return a colorspace set to V4L2_COLORSPACE_DEFAULT in
	 * that case (as well as for image formats when the driver hasn't
	 * bothered implementing color space support). Check the colorspace
	 * field here and return std::nullopt directly to avoid logging a
	 * warning.
	 */

> Other than that, looks good to me.
> 
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> 
> > +	if (format.colorspace == V4L2_COLORSPACE_DEFAULT)
> > +		return std::nullopt;
> > +
> > +	PixelFormatInfo::ColourEncoding colourEncoding;
> > +	auto iter = formatInfoMap.find(format.code);
> > +	if (iter != formatInfoMap.end()) {
> > +		colourEncoding = iter->second.colourEncoding;
> > +	} else {
> > +		LOG(V4L2, Warning)
> > +			<< "Unknown subdev format "
> > +			<< utils::hex(format.code, 4)
> > +			<< ", defaulting to RGB encoding";
> > +
> > +		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> > +	}
> > +
> > +	return V4L2Device::toColorSpace(format, colourEncoding);
> > +}
> > +
> >   /**
> >    * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> >    * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> > @@ -503,20 +524,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> >   	format->size.width = subdevFmt.format.width;
> >   	format->size.height = subdevFmt.format.height;
> >   	format->mbus_code = subdevFmt.format.code;
> > -
> > -	PixelFormatInfo::ColourEncoding colourEncoding;
> > -	auto iter = formatInfoMap.find(subdevFmt.format.code);
> > -	if (iter != formatInfoMap.end()) {
> > -		colourEncoding = iter->second.colourEncoding;
> > -	} else {
> > -		LOG(V4L2, Warning)
> > -			<< "Unknown subdev format "
> > -			<< utils::hex(subdevFmt.format.code, 4)
> > -			<< ", defaulting to RGB encoding";
> > -
> > -		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> > -	}
> > -	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
> > +	format->colorSpace = toColorSpace(subdevFmt.format);
> >   
> >   	return 0;
> >   }
> > @@ -562,20 +570,7 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> >   	format->size.width = subdevFmt.format.width;
> >   	format->size.height = subdevFmt.format.height;
> >   	format->mbus_code = subdevFmt.format.code;
> > -
> > -	PixelFormatInfo::ColourEncoding colourEncoding;
> > -	auto iter = formatInfoMap.find(subdevFmt.format.code);
> > -	if (iter != formatInfoMap.end()) {
> > -		colourEncoding = iter->second.colourEncoding;
> > -	} else {
> > -		LOG(V4L2, Warning)
> > -			<< "Unknown subdev format "
> > -			<< utils::hex(subdevFmt.format.code, 4)
> > -			<< ", defaulting to RGB encoding";
> > -
> > -		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> > -	}
> > -	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
> > +	format->colorSpace = toColorSpace(subdevFmt.format);
> >   
> >   	return 0;
> >   }
> >
> > base-commit: da9bb8dea6821edcfd3464e8cba37ad4a91087d6
Naushir Patuck Sept. 8, 2022, 1:26 p.m. UTC | #3 
Hi Laurent,

Thank you for this fix.

On Mon, 5 Sept 2022 at 17:46, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Commit e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on
> pixel format") has introduced a warning when trying to convert a color
> space from V4L2 to libcamera if the media bus code is unknown. This was
> meant to catch unknown image formats, but turned out to be also
> triggered for metadata formats.
>
> Color spaces are not applicable to metadata formats, there should thus
> be no warning. Fix it by skipping the color space translation and
> returning std::nullopt directly if the kernel reports
> V4L2_COLORSPACE_DEFAULT. This doesn't introduce any change in behaviour
> other than getting rid of the warning, as the V4L2Device::toColorSpace()
> function returns std::nullopt alread in that case.
>
> Fixes: e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on
> pixel format")
> Reported-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>

This does indeed remove the WARN message that I highlighted.

Tested-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

---
>  include/libcamera/internal/v4l2_subdevice.h |  3 ++
>  src/libcamera/v4l2_subdevice.cpp            | 51 ++++++++++-----------
>  2 files changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_subdevice.h
> b/include/libcamera/internal/v4l2_subdevice.h
> index 00be17bb1465..69862de0585a 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -101,6 +101,9 @@ protected:
>  private:
>         LIBCAMERA_DISABLE_COPY(V4L2Subdevice)
>
> +       std::optional<ColorSpace>
> +       toColorSpace(const v4l2_mbus_framefmt &format) const;
> +
>         std::vector<unsigned int> enumPadCodes(unsigned int pad);
>         std::vector<SizeRange> enumPadSizes(unsigned int pad,
>                                             unsigned int code);
> diff --git a/src/libcamera/v4l2_subdevice.cpp
> b/src/libcamera/v4l2_subdevice.cpp
> index 95bfde340d8c..f3a9a0096c6c 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -477,6 +477,27 @@ V4L2Subdevice::Formats
> V4L2Subdevice::formats(unsigned int pad)
>         return formats;
>  }
>
> +std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const
> v4l2_mbus_framefmt &format) const
> +{
> +       if (format.colorspace == V4L2_COLORSPACE_DEFAULT)
> +               return std::nullopt;
> +
> +       PixelFormatInfo::ColourEncoding colourEncoding;
> +       auto iter = formatInfoMap.find(format.code);
> +       if (iter != formatInfoMap.end()) {
> +               colourEncoding = iter->second.colourEncoding;
> +       } else {
> +               LOG(V4L2, Warning)
> +                       << "Unknown subdev format "
> +                       << utils::hex(format.code, 4)
> +                       << ", defaulting to RGB encoding";
> +
> +               colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> +       }
> +
> +       return V4L2Device::toColorSpace(format, colourEncoding);
> +}
> +
>  /**
>   * \brief Retrieve the image format set on one of the V4L2 subdevice pads
>   * \param[in] pad The 0-indexed pad number the format is to be retrieved
> from
> @@ -503,20 +524,7 @@ int V4L2Subdevice::getFormat(unsigned int pad,
> V4L2SubdeviceFormat *format,
>         format->size.width = subdevFmt.format.width;
>         format->size.height = subdevFmt.format.height;
>         format->mbus_code = subdevFmt.format.code;
> -
> -       PixelFormatInfo::ColourEncoding colourEncoding;
> -       auto iter = formatInfoMap.find(subdevFmt.format.code);
> -       if (iter != formatInfoMap.end()) {
> -               colourEncoding = iter->second.colourEncoding;
> -       } else {
> -               LOG(V4L2, Warning)
> -                       << "Unknown subdev format "
> -                       << utils::hex(subdevFmt.format.code, 4)
> -                       << ", defaulting to RGB encoding";
> -
> -               colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> -       }
> -       format->colorSpace = toColorSpace(subdevFmt.format,
> colourEncoding);
> +       format->colorSpace = toColorSpace(subdevFmt.format);
>
>         return 0;
>  }
> @@ -562,20 +570,7 @@ int V4L2Subdevice::setFormat(unsigned int pad,
> V4L2SubdeviceFormat *format,
>         format->size.width = subdevFmt.format.width;
>         format->size.height = subdevFmt.format.height;
>         format->mbus_code = subdevFmt.format.code;
> -
> -       PixelFormatInfo::ColourEncoding colourEncoding;
> -       auto iter = formatInfoMap.find(subdevFmt.format.code);
> -       if (iter != formatInfoMap.end()) {
> -               colourEncoding = iter->second.colourEncoding;
> -       } else {
> -               LOG(V4L2, Warning)
> -                       << "Unknown subdev format "
> -                       << utils::hex(subdevFmt.format.code, 4)
> -                       << ", defaulting to RGB encoding";
> -
> -               colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> -       }
> -       format->colorSpace = toColorSpace(subdevFmt.format,
> colourEncoding);
> +       format->colorSpace = toColorSpace(subdevFmt.format);
>
>         return 0;
>  }
>
> base-commit: da9bb8dea6821edcfd3464e8cba37ad4a91087d6
> --
> Regards,
>
> Laurent Pinchart
>
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
index 00be17bb1465..69862de0585a 100644
--- a/include/libcamera/internal/v4l2_subdevice.h
+++ b/include/libcamera/internal/v4l2_subdevice.h
@@ -101,6 +101,9 @@  protected:
 private:
 	LIBCAMERA_DISABLE_COPY(V4L2Subdevice)
 
+	std::optional<ColorSpace>
+	toColorSpace(const v4l2_mbus_framefmt &format) const;
+
 	std::vector<unsigned int> enumPadCodes(unsigned int pad);
 	std::vector<SizeRange> enumPadSizes(unsigned int pad,
 					    unsigned int code);
diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
index 95bfde340d8c..f3a9a0096c6c 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -477,6 +477,27 @@  V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
 	return formats;
 }
 
+std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
+{
+	if (format.colorspace == V4L2_COLORSPACE_DEFAULT)
+		return std::nullopt;
+
+	PixelFormatInfo::ColourEncoding colourEncoding;
+	auto iter = formatInfoMap.find(format.code);
+	if (iter != formatInfoMap.end()) {
+		colourEncoding = iter->second.colourEncoding;
+	} else {
+		LOG(V4L2, Warning)
+			<< "Unknown subdev format "
+			<< utils::hex(format.code, 4)
+			<< ", defaulting to RGB encoding";
+
+		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
+	}
+
+	return V4L2Device::toColorSpace(format, colourEncoding);
+}
+
 /**
  * \brief Retrieve the image format set on one of the V4L2 subdevice pads
  * \param[in] pad The 0-indexed pad number the format is to be retrieved from
@@ -503,20 +524,7 @@  int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
 	format->size.width = subdevFmt.format.width;
 	format->size.height = subdevFmt.format.height;
 	format->mbus_code = subdevFmt.format.code;
-
-	PixelFormatInfo::ColourEncoding colourEncoding;
-	auto iter = formatInfoMap.find(subdevFmt.format.code);
-	if (iter != formatInfoMap.end()) {
-		colourEncoding = iter->second.colourEncoding;
-	} else {
-		LOG(V4L2, Warning)
-			<< "Unknown subdev format "
-			<< utils::hex(subdevFmt.format.code, 4)
-			<< ", defaulting to RGB encoding";
-
-		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
-	}
-	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
+	format->colorSpace = toColorSpace(subdevFmt.format);
 
 	return 0;
 }
@@ -562,20 +570,7 @@  int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
 	format->size.width = subdevFmt.format.width;
 	format->size.height = subdevFmt.format.height;
 	format->mbus_code = subdevFmt.format.code;
-
-	PixelFormatInfo::ColourEncoding colourEncoding;
-	auto iter = formatInfoMap.find(subdevFmt.format.code);
-	if (iter != formatInfoMap.end()) {
-		colourEncoding = iter->second.colourEncoding;
-	} else {
-		LOG(V4L2, Warning)
-			<< "Unknown subdev format "
-			<< utils::hex(subdevFmt.format.code, 4)
-			<< ", defaulting to RGB encoding";
-
-		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
-	}
-	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
+	format->colorSpace = toColorSpace(subdevFmt.format);
 
 	return 0;
 }