[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 22:40:45 CEST 2022
Hi Umang,
On Fri, Aug 26, 2022 at 11:03:03PM +0530, Umang Jain wrote:
> On 8/26/22 7:06 PM, Laurent Pinchart wrote:
> > 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.
>
> Ok.
> >
> >> 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 ?
>
> It wasn't explicit in the previous conversation that we need to pick a
> default colorspace /only/
>
> My thought process went on the lines "we need to pick a default media
> bus format" (if the returned code mapping is missing..)
I should have been clearer, sorry. A default color space should be fine
I think, because we need to do something, and we can't do better, but
overriding an unknown format will lead to possible problems.
> > 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