[libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust colorspace if pixel format is RGB
Umang Jain
umang.jain at ideasonboard.com
Fri Aug 26 08:16:58 CEST 2022
Hi Laurent,
Thank you for the review.
On 8/25/22 4:35 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> 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.
Correct.
>
> Similarly, override the quantization range of non-YUV formats to full
> range, as limited range isn't used for RGB and raw formats.
Seems correct too, I agree my commit message was not at it's best.
>
>> 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
>> */
>> 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 ?
I wasn't aware of that. I assumed (formatInfoMap.find(format->mbus_code)
== formatInfoMap.end()) would be very hard to trigger and if it does at
all, we want to fail loudly.
I am not much aware of simple pipeline handler format's passing - I'll
take a look to answer the question - "Which default to pick"
>> + 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.
>
>> + 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.
Ack
>
>> 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;
>> }
More information about the libcamera-devel
mailing list