[libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <> ColorSpace mappings
Umang Jain
umang.jain at ideasonboard.com
Mon Aug 1 15:55:01 CEST 2022
Hi Laurent and David,
On 8/1/22 19:00, Laurent Pinchart wrote:
> Hi David,
>
> On Mon, Aug 01, 2022 at 02:20:12PM +0100, David Plowman wrote:
>> On Mon, 1 Aug 2022 at 13:31, Umang Jain wrote:
>>> On 8/1/22 01:14, Laurent Pinchart wrote:
>>>> On Fri, Jul 29, 2022 at 04:08:46PM +0530, Umang Jain via libcamera-devel wrote:
>>>>> From: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
>>>>>
>>>>> Provide colorimetry <=> libcamera::ColorSpace mappings via:
>>>>> - GstVideoColorimetry colorimetry_from_colorspace(colorspace);
>>>>> - ColorSpace colorspace_from_colorimetry(colorimetry);
>>>>>
>>>>> Read the colorimetry field from caps into the stream configuration.
>>>>> After stream validation, the sensor supported colorimetry will
>>>>> be retrieved and the caps will be updated accordingly.
>>>>>
>>>>> Colorimetry support for gstlibcamerasrc currently undertakes only one
>>>> s/gstlibcamerasrc/libcamerasrc/ (or gstlibcamera if you meant the
>>>> library, not the element name)
>>>>
>>>>> argument. Multiple colorimetry support shall be introduced in
>>>>> subsequent commits.
>>>> I'm probably missing something here, what do you mean by multiple
>>>> colorimetry support ?
>>>>
>>>>> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
>>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Drop "Default" Colorspace
>>>>> - Improve function signature of colorimetry_from_colorspace() and
>>>>> colorspace_from_colorimetry()
>>>>> - Map GST_VIDEO_COLOR_MATRIX_RGB to Colorspace::YcbcrEncoding::Rec601
>>>>> (comes from the kernel)
>>>>> - Map GST_VIDEO_COLOR_PRIMARIES_UNKNOWN to Raw colorspace
>>>>> - Map ColorSpace::YcbcrEncoding::Rec601 to GST_VIDEO_COLOR_MATRIX_RGB
>>>>> if colorspace is "sRGB". GST_VIDEO_COLOR_MATRIX_BT601 for all other
>>>>> cases
>>>>> - Minor nits regarding error reporting strings.
>>>>> ---
>>>>> src/gstreamer/gstlibcamera-utils.cpp | 164 +++++++++++++++++++++++++++
>>>>> 1 file changed, 164 insertions(+)
>>>>>
>>>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
>>>>> index c97c0d43..e4ff1269 100644
>>>>> --- a/src/gstreamer/gstlibcamera-utils.cpp
>>>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp
>>>>> @@ -45,6 +45,146 @@ static struct {
>>>>> /* \todo NV42 is used in libcamera but is not mapped in GStreamer yet. */
>>>>> };
>>>>>
>>>>> +static GstVideoColorimetry
>>>>> +colorimetry_from_colorspace(const ColorSpace &colorSpace)
>>>>> +{
>>>>> + GstVideoColorimetry colorimetry;
>>>>> +
>>>>> + switch (colorSpace.primaries) {
>>>>> + case ColorSpace::Primaries::Rec709:
>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;
>>>>> + break;
>>>>> + case ColorSpace::Primaries::Rec2020:
>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;
>>>>> + break;
>>>>> + case ColorSpace::Primaries::Smpte170m:
>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;
>>>>> + break;
>>>>> + case ColorSpace::Primaries::Raw:
>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;
>>>>> + break;
>>>>> + }
>>>> I'd sort the cases with the order of the enum in color_space.h (below
>>>> too). That's very minor.
>>> Ack.
>>>
>>>>> +
>>>>> + switch (colorSpace.transferFunction) {
>>>>> + case ColorSpace::TransferFunction::Rec709:
>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709;
>>>>> + break;
>>>>> + case ColorSpace::TransferFunction::Srgb:
>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;
>>>>> + break;
>>>>> + case ColorSpace::TransferFunction::Linear:
>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + switch (colorSpace.ycbcrEncoding) {
>>>>> + case ColorSpace::YcbcrEncoding::None:
>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;
>>>>> + break;
>>>>> + case ColorSpace::YcbcrEncoding::Rec709:
>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;
>>>>> + break;
>>>>> + case ColorSpace::YcbcrEncoding::Rec2020:
>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;
>>>>> + break;
>>>>> + case ColorSpace::YcbcrEncoding::Rec601:
>>>>> + if (colorSpace == ColorSpace::Srgb)
>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;
>>>> I don't think this one is right. GST_VIDEO_COLOR_MATRIX_RGB is the
>>>> identity matrix, which means no encoding to YCbCr, and
>>>> ColorSpace::YcbcrEncoding::Rec601 clearly defines an encoding.
>>>>
>>>> I think the problem is caused by our definition of ColorSpace::Srgb:
>>>>
>>>> const ColorSpace ColorSpace::Srgb = {
>>>> Primaries::Rec709,
>>>> TransferFunction::Srgb,
>>>> YcbcrEncoding::Rec601,
>>>> Range::Limited
>>>> };
>>>>
>>>> I propose modifying it to
>>>>
>>>> const ColorSpace ColorSpace::Srgb = {
>>>> Primaries::Rec709,
>>>> TransferFunction::Srgb,
>>>> YcbcrEncoding::None,
>>>> Range::Full
>>>> };
>>> ack!
>>>
>>> My question which I think I brought up in v1 itself now (you can choose
>>> to reply anywhere) is, do we need a preset to represent YUV-encoded RGB
>>> and what would we call it? ColorSpace::sYCC ?
>>>
>>>> and adapting the rest of libcamera accordingly.
>>>>
>>>> David, this will affect Raspberry Pi, so I'd appreciate your opinion.
>>> Patiently waiting ;-)
>> Sorry, the thread is getting a bit long so it takes me a while to
>> notice things!!
>>
>> So the original sRGB colour space definition comes from:
>>
>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/colorspaces-details.html#col-srgb
>>
>> I agree that the values aren't necessarily the ones you first think
>> of, there is some history behind some of them.
>>
>> I don't think it matters for the Pi, though. I believe the only colour
>> spaces we ask for are Jpeg, Smpte170m and Rec709. (When we really want
>> sRGB for still capture we ask for Jpeg.)
Thanks!
> Thank you for the information. Unless there's any objection, I then
> propose modifying Srgb to define it as
>
> const ColorSpace ColorSpace::Srgb = {
> Primaries::Rec709,
> TransferFunction::Srgb,
> YcbcrEncoding::None,
> Range::Full
> };
>
> We could also define
>
> const ColorSpace ColorSpace::Sycc = {
> Primaries::Rec709,
> TransferFunction::Srgb,
> YcbcrEncoding::Rec601,
> Range::Limited
> };
>
> but I'm tempted to hold off until it becomes needed.
I am tempted to add it because:
static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = {
{ V4L2_COLORSPACE_RAW, ColorSpace::Raw },
{ V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg },
- { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb },
+ { V4L2_COLORSPACE_SRGB, ColorSpace::Sycc },
@@ -772,7 +772,7 @@ static const std::map<uint32_t, ColorSpace::Range>
v4l2ToRange = {
static const std::vector<std::pair<ColorSpace, v4l2_colorspace>>
colorSpaceToV4l2 = {
{ ColorSpace::Raw, V4L2_COLORSPACE_RAW },
{ ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },
- { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },
+ { ColorSpace::Sycc, V4L2_COLORSPACE_SRGB },
Unless, you want to drop the preset mappings as well?
>
>>>>> + else
>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + switch (colorSpace.range) {
>>>>> + case ColorSpace::Range::Full:
>>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;
>>>>> + break;
>>>>> + case ColorSpace::Range::Limited:
>>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + return colorimetry;
>>>>> +}
>>>>> +
>>>>> +static ColorSpace
>>>>> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)
>>>>> +{
>>>>> + ColorSpace colorspace = ColorSpace::Raw;
>>>>> +
>>>>> + switch (colorimetry.primaries) {
>>>>> + case GST_VIDEO_COLOR_PRIMARIES_BT709:
>>>>> + colorspace.primaries = ColorSpace::Primaries::Rec709;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_PRIMARIES_BT2020:
>>>>> + colorspace.primaries = ColorSpace::Primaries::Rec2020;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:
>>>>> + colorspace.primaries = ColorSpace::Primaries::Smpte170m;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:
>>>>> + /* Unknown primaries map to raw colorspace in gstreamer */
>>>>> + return colorspace;
>>>> I would
>>>>
>>>> return ColorSpace::Raw;
>>>>
>>>> here to make it more explicit.
>>> ack
>>>
>>>>> + default:
>>>>> + g_error("Unsupported colorimetry primaries: %d", colorimetry.primaries);
>>>> g_error() seems very harsh, do we really need to abort() if the user
>>>> asks for unsupported primaries (or other colorimetry fields below) ?
>>> I agree it's harsh - initially I kept as 'warning' but then it was
>>> advised that 'warnings' always come with mitigation steps.
>>>
>>> I guess the only mitigation technique here is, making sure mappings exists!
>>>
>>>> This is probably a question for Nicolas.
>>>>
>>>>> + }
>>>>> +
>>>>> + switch (colorimetry.transfer) {
>>>>> + /* Transfer function mappings inspired from v4l2src plugin */
>>>>> + case GST_VIDEO_TRANSFER_GAMMA18:
>>>>> + case GST_VIDEO_TRANSFER_GAMMA20:
>>>>> + case GST_VIDEO_TRANSFER_GAMMA22:
>>>>> + case GST_VIDEO_TRANSFER_GAMMA28:
>>>>> + GST_WARNING("GAMMA 18, 20, 22, 28 transfer functions not supported");
>>>>> + /* fallthrough */
>>>>> + case GST_VIDEO_TRANSFER_GAMMA10:
>>>>> + colorspace.transferFunction = ColorSpace::TransferFunction::Linear;
>>>>> + break;
>>>>> + case GST_VIDEO_TRANSFER_BT601:
>>>>> + case GST_VIDEO_TRANSFER_BT2020_12:
>>>>> + case GST_VIDEO_TRANSFER_BT2020_10:
>>>>> + case GST_VIDEO_TRANSFER_BT709:
>>>>> + colorspace.transferFunction = ColorSpace::TransferFunction::Rec709;
>>>>> + break;
>>>>> + case GST_VIDEO_TRANSFER_SRGB:
>>>>> + colorspace.transferFunction = ColorSpace::TransferFunction::Srgb;
>>>>> + break;
>>>>> + default:
>>>>> + g_error("Unsupported colorimetry transfer function: %d", colorimetry.transfer);
>>>>> + }
>>>>> +
>>>>> + switch (colorimetry.matrix) {
>>>>> + /* FCC is about the same as BT601 with less digit */
>>>>> + case GST_VIDEO_COLOR_MATRIX_FCC:
>>>>> + case GST_VIDEO_COLOR_MATRIX_BT601:
>>>>> + /* v4l2_ycbcr_encoding of sRGB is ENC_601 in the kernel */
>>>> That's right, but I don't think we need to be concerned by that here.
>>>> GST_VIDEO_COLOR_MATRIX_RGB should be mapped to
>>>> ColorSpace::YcbcrEncoding::None, and we should handle the V4L2 sRGB YUV
>>>> encoding internally in libcamera.
>>> replied to the latter part, in detail, in v1
>>>
>>>>> + case GST_VIDEO_COLOR_MATRIX_RGB:
>>>>> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec601;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_MATRIX_BT709:
>>>>> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec709;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_MATRIX_BT2020:
>>>>> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec2020;
>>>>> + break;
>>>>> + default:
>>>>> + g_error("Unsupported colorimetry matrix: %d", colorimetry.matrix);
>>>>> + }
>>>>> +
>>>>> + switch (colorimetry.range) {
>>>>> + case GST_VIDEO_COLOR_RANGE_0_255:
>>>>> + colorspace.range = ColorSpace::Range::Full;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_RANGE_16_235:
>>>>> + colorspace.range = ColorSpace::Range::Limited;
>>>>> + break;
>>>>> + default:
>>>>> + g_error("Unsupported colorimetry range %d", colorimetry.range);
>>>>> + }
>>>>> +
>>>>> + return colorspace;
>>>>> +}
>>>>> +
>>>>> static GstVideoFormat
>>>>> pixel_format_to_gst_format(const PixelFormat &format)
>>>>> {
>>>>> @@ -139,6 +279,18 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
>>>>> "width", G_TYPE_INT, stream_cfg.size.width,
>>>>> "height", G_TYPE_INT, stream_cfg.size.height,
>>>>> nullptr);
>>>>> +
>>>>> + if (stream_cfg.colorSpace) {
>>>>> + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());
>>>>> + gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
>>>>> +
>>>>> + if (colorimetry_str)
>>>>> + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry_str, nullptr);
>>>>> + else
>>>>> + g_error("Got invalid colorimetry from ColorSpace: %s",
>>>>> + ColorSpace::toString(stream_cfg.colorSpace).c_str());
>>>> Same question as above about g_error(). As far as I understand,
>>>> gst_video_colorimetry_to_string() will return null either if it can't
>>>> alocate memory for the string (in which case we're screwed anyway, so
>>>> g_error() is fine) or if colorimetry is all UNKNOWN. The latter should
>>>> never happen given the implementation of colorimetry_from_colorspace(),
>>>> so I suppose it's fine ?
>>> Yes, I think it's fine
>>>
>>>>> + }
>>>>> +
>>>>> gst_caps_append_structure(caps, s);
>>>>>
>>>>> return caps;
>>>>> @@ -222,6 +374,18 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
>>>>> gst_structure_get_int(s, "height", &height);
>>>>> stream_cfg.size.width = width;
>>>>> stream_cfg.size.height = height;
>>>>> +
>>>>> + /* Configure colorimetry */
>>>>> + if (gst_structure_has_field(s, "colorimetry")) {
>>>>> + const gchar *colorimetry_caps = gst_structure_get_string(s, "colorimetry");
>>>>> + GstVideoColorimetry colorimetry;
>>>>> +
>>>>> + if(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {
>>>> Missing space after "if".
>>>>
>>>>> + stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);
>>>>> + } else {
>>>>> + g_critical("Invalid colorimetry %s", colorimetry_caps);
>>>>> + }
>>>>> + }
>>>>> }
>>>>>
>>>>> #if !GST_CHECK_VERSION(1, 17, 1)
More information about the libcamera-devel
mailing list