[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 10:26:20 CEST 2022
Hi Laurent,
On 8/25/22 8:39 PM, Laurent Pinchart wrote:
> Another comment.
>
> On Thu, Aug 25, 2022 at 02:05:57AM +0300, Laurent Pinchart via libcamera-devel 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.
>>
>> 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?
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);
+ 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;
@@ -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;
>>> }
More information about the libcamera-devel
mailing list