Message ID | 20220829163742.1006102-3-umang.jain@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Umang, Thank you for the patch. On Mon, Aug 29, 2022 at 10:07:37PM +0530, Umang Jain via libcamera-devel wrote: > V4L2 has no "none" YCbCr encoding, and thus reports an encoding for all > formats, including RGB and raw formats. This causes the libcamera > ColorSpace to report incorrect encodings for non-YUV formats. Fix it by > overriding the encoding reported by the kernel to YCbCrEncoding::None > for non-YUV pixel formats and media bus formats. > > Similarly, override the quantization range of non-YUV formats to full > range, as limited range isn't used for RGB and raw formats. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > include/libcamera/internal/v4l2_device.h | 5 +++- > src/libcamera/v4l2_device.cpp | 29 ++++++++++++++++++--- > src/libcamera/v4l2_subdevice.cpp | 32 ++++++++++++++++++++++-- > src/libcamera/v4l2_videodevice.cpp | 12 ++++++--- > 4 files changed, 67 insertions(+), 11 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index a52a5f2c..75304be1 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -22,6 +22,8 @@ > #include <libcamera/color_space.h> > #include <libcamera/controls.h> > > +#include "libcamera/internal/formats.h" > + > namespace libcamera { > > class EventNotifier; > @@ -59,7 +61,8 @@ protected: > int fd() const { return fd_.get(); } > > template<typename T> > - static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format, > + PixelFormatInfo::ColourEncoding colourEncoding); > > template<typename T> > static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index b22a981f..301620f8 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -24,6 +24,7 @@ > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/sysfs.h" > > /** > @@ -805,6 +806,7 @@ static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > /** > * \brief Convert the color space fields in a V4L2 format to a ColorSpace > * \param[in] v4l2Format A V4L2 format containing color space information > + * \param[in] colourEncoding Type of colour encoding > * > * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > * V4L2 format structure are converted to a corresponding ColorSpace. > @@ -816,7 +818,8 @@ static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > * \retval std::nullopt One or more V4L2 color space fields were not recognised > */ > template<typename T> > -std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format, > + PixelFormatInfo::ColourEncoding colourEncoding) > { > auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); > if (itColor == v4l2ToColorSpace.end()) > @@ -839,6 +842,14 @@ std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > return std::nullopt; > > colorSpace.ycbcrEncoding = itYcbcrEncoding->second; > + > + /* > + * V4L2 has no "none" encoding, override the value returned by > + * the kernel for non-YUV formats as YCbCr encoding isn't > + * applicable in that case. > + */ > + if (colourEncoding != PixelFormatInfo::ColourEncodingYUV) > + colorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None; > } > > if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { > @@ -847,14 +858,24 @@ std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > return std::nullopt; > > colorSpace.range = itRange->second; > + > + /* > + * "Limited" quantization range is only meant for YUV formats. > + * Override the range to "Full" for all other formats. > + */ > + if (colourEncoding != PixelFormatInfo::ColourEncodingYUV) > + colorSpace.range = ColorSpace::Range::Full; > } > > return colorSpace; > } > > -template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); > -template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); > -template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &, > + PixelFormatInfo::ColourEncoding); > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &, > + PixelFormatInfo::ColourEncoding); > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &, > + PixelFormatInfo::ColourEncoding); > > /** > * \brief Fill in the color space fields of a V4L2 format from a ColorSpace > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index 07a0bb07..0389f1b3 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -502,8 +502,22 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format, > > format->size.width = subdevFmt.format.width; > format->size.height = subdevFmt.format.height; > + > + 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->mbus_code = subdevFmt.format.code; I'd move this line above just after size.height, to keep all the color space code grouped together. > - format->colorSpace = toColorSpace(subdevFmt.format); > + format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding); > > return 0; > } > @@ -548,8 +562,22 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format, > > format->size.width = subdevFmt.format.width; > format->size.height = subdevFmt.format.height; > + > + 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->mbus_code = subdevFmt.format.code; Same here. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > - format->colorSpace = toColorSpace(subdevFmt.format); > + format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding); > > return 0; > } > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 5a2d0e5b..0e3f5436 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > format->size.height = pix->height; > format->fourcc = V4L2PixelFormat(pix->pixelformat); > format->planesCount = pix->num_planes; > - format->colorSpace = toColorSpace(*pix); > + format->colorSpace = > + toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); > > for (unsigned int i = 0; i < format->planesCount; ++i) { > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > @@ -987,7 +988,8 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > format->planes[i].size = pix->plane_fmt[i].sizeimage; > } > - format->colorSpace = toColorSpace(*pix); > + format->colorSpace = > + toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); > > return 0; > } > @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > format->planesCount = 1; > format->planes[0].bpl = pix->bytesperline; > format->planes[0].size = pix->sizeimage; > - format->colorSpace = toColorSpace(*pix); > + format->colorSpace = > + toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); > > return 0; > } > @@ -1053,7 +1056,8 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > format->planesCount = 1; > format->planes[0].bpl = pix->bytesperline; > format->planes[0].size = pix->sizeimage; > - format->colorSpace = toColorSpace(*pix); > + format->colorSpace = > + toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); > > return 0; > }
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index a52a5f2c..75304be1 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -22,6 +22,8 @@ #include <libcamera/color_space.h> #include <libcamera/controls.h> +#include "libcamera/internal/formats.h" + namespace libcamera { class EventNotifier; @@ -59,7 +61,8 @@ protected: int fd() const { return fd_.get(); } template<typename T> - static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format, + PixelFormatInfo::ColourEncoding colourEncoding); template<typename T> static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index b22a981f..301620f8 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -24,6 +24,7 @@ #include <libcamera/base/log.h> #include <libcamera/base/utils.h> +#include "libcamera/internal/formats.h" #include "libcamera/internal/sysfs.h" /** @@ -805,6 +806,7 @@ static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { /** * \brief Convert the color space fields in a V4L2 format to a ColorSpace * \param[in] v4l2Format A V4L2 format containing color space information + * \param[in] colourEncoding Type of colour encoding * * The colorspace, ycbcr_enc, xfer_func and quantization fields within a * V4L2 format structure are converted to a corresponding ColorSpace. @@ -816,7 +818,8 @@ static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { * \retval std::nullopt One or more V4L2 color space fields were not recognised */ template<typename T> -std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format, + PixelFormatInfo::ColourEncoding colourEncoding) { auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); if (itColor == v4l2ToColorSpace.end()) @@ -839,6 +842,14 @@ std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) return std::nullopt; colorSpace.ycbcrEncoding = itYcbcrEncoding->second; + + /* + * V4L2 has no "none" encoding, override the value returned by + * the kernel for non-YUV formats as YCbCr encoding isn't + * applicable in that case. + */ + if (colourEncoding != PixelFormatInfo::ColourEncodingYUV) + colorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None; } if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { @@ -847,14 +858,24 @@ std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) return std::nullopt; colorSpace.range = itRange->second; + + /* + * "Limited" quantization range is only meant for YUV formats. + * Override the range to "Full" for all other formats. + */ + if (colourEncoding != PixelFormatInfo::ColourEncodingYUV) + colorSpace.range = ColorSpace::Range::Full; } return colorSpace; } -template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); -template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); -template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &, + PixelFormatInfo::ColourEncoding); +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &, + PixelFormatInfo::ColourEncoding); +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &, + PixelFormatInfo::ColourEncoding); /** * \brief Fill in the color space fields of a V4L2 format from a ColorSpace diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index 07a0bb07..0389f1b3 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -502,8 +502,22 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format, format->size.width = subdevFmt.format.width; format->size.height = subdevFmt.format.height; + + 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->mbus_code = subdevFmt.format.code; - format->colorSpace = toColorSpace(subdevFmt.format); + format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding); return 0; } @@ -548,8 +562,22 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format, format->size.width = subdevFmt.format.width; format->size.height = subdevFmt.format.height; + + 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->mbus_code = subdevFmt.format.code; - format->colorSpace = toColorSpace(subdevFmt.format); + format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding); return 0; } diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 5a2d0e5b..0e3f5436 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) format->size.height = pix->height; format->fourcc = V4L2PixelFormat(pix->pixelformat); format->planesCount = pix->num_planes; - format->colorSpace = toColorSpace(*pix); + format->colorSpace = + toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); for (unsigned int i = 0; i < format->planesCount; ++i) { format->planes[i].bpl = pix->plane_fmt[i].bytesperline; @@ -987,7 +988,8 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) format->planes[i].bpl = pix->plane_fmt[i].bytesperline; format->planes[i].size = pix->plane_fmt[i].sizeimage; } - format->colorSpace = toColorSpace(*pix); + format->colorSpace = + toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); return 0; } @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) format->planesCount = 1; format->planes[0].bpl = pix->bytesperline; format->planes[0].size = pix->sizeimage; - format->colorSpace = toColorSpace(*pix); + format->colorSpace = + toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); return 0; } @@ -1053,7 +1056,8 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) format->planesCount = 1; format->planes[0].bpl = pix->bytesperline; format->planes[0].size = pix->sizeimage; - format->colorSpace = toColorSpace(*pix); + format->colorSpace = + toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding); return 0; }
V4L2 has no "none" YCbCr encoding, and thus reports an encoding for all formats, including RGB and raw formats. This causes the libcamera ColorSpace to report incorrect encodings for non-YUV formats. Fix it by overriding the encoding reported by the kernel to YCbCrEncoding::None for non-YUV pixel formats and media bus formats. Similarly, override the quantization range of non-YUV formats to full range, as limited range isn't used for RGB and raw formats. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- include/libcamera/internal/v4l2_device.h | 5 +++- src/libcamera/v4l2_device.cpp | 29 ++++++++++++++++++--- src/libcamera/v4l2_subdevice.cpp | 32 ++++++++++++++++++++++-- src/libcamera/v4l2_videodevice.cpp | 12 ++++++--- 4 files changed, 67 insertions(+), 11 deletions(-)