[libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust colorspace if pixel format is RGB
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 26 15:36:35 CEST 2022
Hi Umang,
On Fri, Aug 26, 2022 at 01:56:20PM +0530, Umang Jain wrote:
> On 8/25/22 8:39 PM, Laurent Pinchart wrote:
> > On Thu, Aug 25, 2022 at 02:05:57AM +0300, Laurent Pinchart via libcamera-devel wrote:
> >> On Wed, Aug 24, 2022 at 09:54:21PM +0530, Umang Jain via libcamera-devel wrote:
> >>> The V4L2_COLORSPACE_SRGB colorspace maps to ColorSpace::Srgb.
> >>> This is wrong as the V4L2_COLORSPACE_SRGB is ill-defined (defines
> >>> Rec 601 Y'CbCr encoding and limited range) in the kernel.
> >>>
> >>> The RGB pixel formats should not use any Y'CbCr encoding and is always
> >>> full range. Adjust the colorspace before reporting back to the
> >>> userspace in such a situation.
> >>>
> >>> Moving forwards, the ColorSpace::Srgb will be defined in the true sense
> >>> for RGB pixel formats.
> >>
> >> Mentioning sRGB here is a bit confusing I think. Let's focus on what
> >> this patch does (and this will also help making sure I understood this
> >> correctly):
> >>
> >> 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 at ideasonboard.com>
> >>> ---
> >>> include/libcamera/internal/v4l2_device.h | 5 ++++-
> >>> src/libcamera/v4l2_device.cpp | 19 +++++++++++++++----
> >>> src/libcamera/v4l2_subdevice.cpp | 10 ++++++++--
> >>> src/libcamera/v4l2_videodevice.cpp | 12 ++++++++----
> >>> 4 files changed, 35 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> >>> index a52a5f2c..5ae2ef8a 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,
> >>> + const PixelFormatInfo::ColourEncoding &colourEncoding);
> >>
> >> PixelFormatInfo::ColourEncoding colourEncoding);
> >> Same below.
> >>
> >>>
> >>> 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..1fb08b9d 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"
> >>>
> >>> /**
> >>> @@ -816,7 +817,8 @@ static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = {
> >>> * \retval std::nullopt One or more V4L2 color space fields were not recognised
> >>> */
> > The documentation needs to be updated with the new parameter.
> >
> >>> template<typename T>
> >>> -std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format)
> >>> +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format,
> >>> + const PixelFormatInfo::ColourEncoding &colourEncoding)
> >>> {
> >>> auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);
> >>> if (itColor == v4l2ToColorSpace.end())
> >>> @@ -839,6 +841,9 @@ std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format)
> >>> return std::nullopt;
> >>>
> >>> colorSpace.ycbcrEncoding = itYcbcrEncoding->second;
> >>> +
> >>
> >> I'd add a comment here, or we'll forget why.
> >>
> >> /*
> >> * 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::ColourEncodingRGB)
> >>
> >> Shouldn't this be
> >>
> >> if (colourEncoding != PixelFormatInfo::ColourEncodingYUV)
> >>
> >> ? Raw formats have no YCbCr encoding either. The subject line would need
> >> to be updated accordingly, I'd write "Adjust colorspace based on pixel
> >> format".
> >>
> >>> + colorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;
> >>> }
> >>>
> >>> if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {
> >>> @@ -847,14 +852,20 @@ std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format)
> >>> return std::nullopt;
> >>>
> >>> colorSpace.range = itRange->second;
> >>> +
> >>> + if (colourEncoding == PixelFormatInfo::ColourEncodingRGB)
> >>
> >> Same here.
> >>
> >>> + 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 &,
> >>> + const PixelFormatInfo::ColourEncoding &);
> >>> +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &,
> >>> + const PixelFormatInfo::ColourEncoding &);
> >>> +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &,
> >>> + const 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 ab74b9f0..a52c414f 100644
> >>> --- a/src/libcamera/v4l2_subdevice.cpp
> >>> +++ b/src/libcamera/v4l2_subdevice.cpp
> >>> @@ -502,7 +502,10 @@ 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;
> >>> - format->colorSpace = toColorSpace(subdevFmt.format);
> >>> + auto iter = formatInfoMap.find(format->mbus_code);
> >>> + if (iter == formatInfoMap.end())
> >>> + return -EINVAL;
> >>
> >> That seems a bit harsh. I'm thinking about the simple pipeline handler
> >> in particular, which will propagate media bus formats through the
> >> pipeline without caring much about the format itself. Would it better to
> >> instead pick a default ?
> >>
> >>> + format->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);
> >>>
> >>> return 0;
> >>> }
> >>> @@ -548,7 +551,10 @@ 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;
> >>> - format->colorSpace = toColorSpace(subdevFmt.format);
> >>> + auto iter = formatInfoMap.find(format->mbus_code);
> >>> + if (iter == formatInfoMap.end())
> >>> + return -EINVAL;
> >>
> >> Same here.
>
> Esp in V4l2Subdevice::setFormat() , picking a default and reporting that
> back makes me a bit unconformtable ....
>
> should we chose to fail hard here?
For the reasons explained in another reply in this thread, I'd rather
add hard failures separately, if we decide it's the way to go. We would
then need to consider the impact on different parts of libcamera (simple
pipeline handler, metadata formats, ...). Even an error/warning message
is something I would introduce in a separate patch on top, to be able to
test it thoroughly without blocking the rest of the series.
> diff --git a/src/libcamera/v4l2_subdevice.cpp
> b/src/libcamera/v4l2_subdevice.cpp
> index 70f179c2..d415a738 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -502,10 +502,19 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>
> format->size.width = subdevFmt.format.width;
> format->size.height = subdevFmt.format.height;
> +
> + auto iter = formatInfoMap.find(subdevFmt.format.code);
> + if (iter == formatInfoMap.end()) {
> + /* Pick a default format to continue */
> + uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;
> + iter = formatInfoMap.find(defCode);
You can avoid the lookup there and just use the RGB encoding:
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;
}
> + LOG(V4L2, Error)
Make it a warning.
> + << "Mapping for subdev format " << subdevFmt.format.code
Print the code in hex.
> + << " is missing, falling back to " << iter->second.name;
> +
> + subdevFmt.format.code = defCode;
Why do you override the media bus code returned by the driver ?
All these comments apply below.
> + }
> format->mbus_code = subdevFmt.format.code;
> - auto iter = formatInfoMap.find(format->mbus_code);
> - if (iter == formatInfoMap.end())
> - return -EINVAL;
> format->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);
>
> return 0;
> @@ -551,10 +560,20 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>
> format->size.width = subdevFmt.format.width;
> format->size.height = subdevFmt.format.height;
> +
> + auto iter = formatInfoMap.find(subdevFmt.format.code);
> + if (iter == formatInfoMap.end()) {
> + /* Pick a default format to continue */
> + uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;
> + iter = formatInfoMap.find(defCode);
> +
> + LOG(V4L2, Error)
> + << "Mapping for subdev format " << subdevFmt.format.code
> + << " is missing, falling back to " << iter->second.name;
> +
> + subdevFmt.format.code = defCode;
> + }
> format->mbus_code = subdevFmt.format.code;
> - auto iter = formatInfoMap.find(format->mbus_code);
> - if (iter == formatInfoMap.end())
> - return -EINVAL;
> format->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);
>
> return 0;
>
> >>> + format->colorSpace = toColorSpace(subdevFmt.format, iter->second.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)
> >>
> >> I'm tempted to add a helper just above this function:
> >>
> >> namespace {
> >>
> >> std::optional<ColorSpace> toColorSpace(const struct v4l2_pix_format_mplane &pix)
> >> {
> >> V4L2PixelFormat fourcc{ pix.pixelformat };
> >> return toColorSpace(pix, PixelFormatInfo::info(fourcc).colourEncoding);
> >> }
> >>
> >> } /* namespace */
> >>
> >> to make the color below a bit nicer (it would actually not need to be
> >> changed at all). Up to you.
> >>
> >>> 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;
> >>> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list