[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 19:33:03 CEST 2022


Hi Laurent,

On 8/26/22 7:06 PM, Laurent Pinchart wrote:
> 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.

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..)
>
> 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;
>>>>>    }



More information about the libcamera-devel mailing list