[libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <> libcamera::ColorSpace mappings
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 1 15:17:50 CEST 2022
Hi Umang,
On Mon, Aug 01, 2022 at 04:38:43PM +0530, Umang Jain wrote:
> On 7/29/22 20:24, Laurent Pinchart wrote:
> > On Fri, Jul 29, 2022 at 03:40:52PM +0530, Umang Jain wrote:
> >> On 7/29/22 13:36, Umang Jain via libcamera-devel wrote:
> >>> On 7/27/22 06:47, Laurent Pinchart wrote:
> >>>> On Sun, Jul 24, 2022 at 08:13:55PM +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
> >>>>> argument. Multiple colorimetry support shall be introduced in
> >>>>> subsequent commits.
> >>>>>
> >>>>> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
> >>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >>>>> ---
> >>>>> src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++
> >>>>> 1 file changed, 175 insertions(+)
> >>>>>
> >>>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> >>>>> index c97c0d43..fb4f0e5c 100644
> >>>>> --- a/src/gstreamer/gstlibcamera-utils.cpp
> >>>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> >>>>> @@ -45,6 +45,157 @@ 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;
> >>>>
> >>>> I would initialize the structure with all fields set to UNKNOWN here...
> >>>>
> >>>>> +
> >>>>> + 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;
> >>>>> + default:
> >>>>> + GST_WARNING("ColorSpace primaries not mapped in GstLibcameraSrc");
> >>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;
> >>>>
> >>>> ... and drop the default case. As ColorSpace::Primaries is an enum
> >>>> class, if you have cases for all the values, the compiler will warn only
> >>>> if one (or more) of the enum values isn't handled in explicit cases.
> >>>> This way you can avoid warnings, and have a guarantee that if the
> >>>> libcamera colorspaces are later extended, the compiler will remind that
> >>>> this function has to be updated. Same for the other components of the
> >>>> colorspace below.
> >>>>
> >>>>> + }
> >>>>> +
> >>>>> + 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;
> >>>>> + default:
> >>>>> + GST_WARNING("ColorSpace transfer function not mapped in GstLibcameraSrc");
> >>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;
> >>>>> + }
> >>>>> +
> >>>>> + switch (colorSpace.ycbcrEncoding) {
> >>>>> + 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:
> >>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;
> >>>>> + break;
> >>>>> + default:
> >>>>> + GST_WARNING("Colorspace YcbcrEncoding not mapped in GstLibcameraSrc");
> >>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;
> >>>>
> >>>> Should we map YcbcrEncoding::None to GST_VIDEO_COLOR_MATRIX_RGB ?
> >>>
> >>> See below.
> >>>
> >>>> Otherwise capturing RGB or raw bayer will produce a warning. Same for
> >>>> the Raw primaries, it seems that the best mapping would be
> >>>> GST_VIDEO_COLOR_PRIMARIES_UNKNOWN (unless I'm missing something), but I
> >>>> don't think we should warn in that case.
> >>>>
> >>>>> + }
> >>>>> +
> >>>>> + 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;
> >>>>> + default:
> >>>>> + GST_WARNING("Colorspace range not mapped in GstLibcameraSrc");
> >>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;
> >>>>> + }
> >>>>> +
> >>>>> + return colorimetry;
> >>>>> +}
> >>>>> +
> >>>>> +static std::optional<ColorSpace>
> >>>>> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)
> >>>>
> >>>> I'd pass a const reference instead of a pointer, as there's no use case
> >>>> for a null colorimetry.
> >>>>
> >>>>> +{
> >>>>> + std::optional<ColorSpace> colorspace = ColorSpace::Default;
> >>>>
> >>>> No need for std::optional here or in the return type.
> >>>>
> >>>>> +
> >>>>> + 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:
> >>>>> + colorspace->primaries = ColorSpace::Primaries::Default;
> >>>>> + break;
> >>>>> + default:
> >>>>> + GST_WARNING("Unknown primaries in colorimetry %d", colorimetry->primaries);
> >>>>> + }
> >>>>> +
> >>>>> + switch (colorimetry->transfer) {
> >>>>> + /* Tranfer 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;
> >>>>> + case GST_VIDEO_TRANSFER_UNKNOWN:
> >>>>> + colorspace->transferFunction = ColorSpace::TransferFunction::Default;
> >>>>> + break;
> >>>>> + default:
> >>>>> + GST_WARNING("Unknown colorimetry transfer %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:
> >>>>> + 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;
> >>>>> + case GST_VIDEO_COLOR_MATRIX_RGB:
> >>>>
> >>>> Shouldn't this map to YcbcrEncoding::None ?
> >>>
> >>> YcbcrEncoding for sRGB as per Kernel is V4L2_YCBCR_ENC_SYCC which is
> >>> deprecated so V4L2_YCBCR_ENC_601 is used instead [1]
> >>>
> >>> The same has been applied *already* in libcamera codebase:
> >>>
> >>> const ColorSpace ColorSpace::Srgb = {
> >>> Primaries::Rec709,
> >>> TransferFunction::Srgb,
> >>> YcbcrEncoding::Rec601,
> >>> Range::Limited
> >>> };
> >>>
> >>> So we shouldn't map GST_VIDEO_COLOR_MATRIX_RGB to YcbcrEncoding::None
> >>> but to Rec601 instead.
> >
> > I'm not sure that's right. GST_VIDEO_COLOR_MATRIX_RGB is defined as
> >
> > GST_VIDEO_COLOR_MATRIX_RGB (1) – identity matrix. Order of coefficients
> > is actually GBR, also IEC 61966-2-1 (sRGB)
> >
> > I understand this as meaning that the RGB data is not converted to YUV,
> > which is expressed in libcamera as YcbcrEncoding::None:
> >
> > * \var ColorSpace::YcbcrEncoding::None
> > * \brief There is no defined Y'CbCr encoding (used for non-YUV formats)
> >
> > The V4L2 sRGB colorspace [2] definition is confusing. sRGB is an RGB
> > colorspace, so it should have no YUV encoding, and use full range. The
> > sYCC colorspace is sRGB encoded in YUV using the Rec601 encoding, also
> > using full range. As it's typical for devices that produce sRGB images
> > to encode them to YUV using the Rec601 encoding and limited range, the
> > V4L2 sRGB colorspace defines default encoding and quantization range as
> > Rec601 and limited. However, when the pixels are stored as RGB, those
> > two parameters don't apply.
> >
> > Note also how GStreamer defines the SRGB colorspace ([3]):
> >
> > MAKE_COLORIMETRY (SRGB, _0_255, RGB, SRGB, BT709),
> >
> > We could do the same and define sRGB as
> >
> > const ColorSpace ColorSpace::Srgb = {
> > Primaries::Rec709,
> > TransferFunction::Srgb,
> > YcbcrEncoding::None,
> > Range::Full
> > };
> >
> > but we would then need to adapt code that produce YUV-encoded sRGB. We
> > could define, for that purpose,
> >
> > const ColorSpace ColorSpace::Sycc = {
> > Primaries::Rec709,
> > TransferFunction::Srgb,
> > YcbcrEncoding::Rec601,
> > Range::Full
> > };
> >
> > but that still wouldn't match the V4L2 definition. It's not necessarily
> > a problem though, devices can produce { Rec709, Srgb, Rec601, Limited }
> > without the need to explicitly define a ColorSpace preset in libcamera.
> > I've alos just realized that the above is identical to ColorSpace::Jpeg.
> >
> > Another option would be to keep the ColorSpace::Srgb definition as-is
> > and document that the YCbCrEncoding and Range must be ignored for
> > non-YUV formats, but I don't like that much, it sounds confusing.
> >
> > I'm tempted to bite the bullet and define ColorSpace::Srgb with
> > YcbcrEncoding::None and Range::Full, and never return ColorSpace::Srgb
> > when capturing YUV data. A pipeline handler that gets ColorSpace::Srgb
> > from an application would turn it into { Rec709, Srgb, Rec601, Limited }
> > for YUV streams (assuming that the hardware supports that of course).
>
> So I am getting a bit confused here for the part where YUV encoded RGB
> data needs to use which range? Limited or Full? You seem to have
> referred to limited, it seems - but I don't get the difference.
>
> I guess I'm asking the difference between ColorSpace::Jpeg vs { Rec709,
> Srgb, Rec601, Limited } really
The difference is that, after conversion from RGB to YUV using the
Rec601 matrix, the Y and U/V values will be limited to [16, 235] and
[16, 240] respectively (by shifting and scaling them to that range).
sYCC mandates usage of limited range, but many devices that use the sRGB
color space encoded in YUV produce full range instead, which led to the
naming of the informal JPEG color space.
> The pipeline-handler specific handling on sRGB colorspace is
> interesting. To re-iterate and if I am understanding things correctly
> (from API point of view):
>
> a) ColorSpace::sRGB preset will be used to present both sRGB and
> YUV-encoded RGB colorspace, by the user (i.e. the user doesn't seem to
> care/understand)
To be clear, assuming we will redefine sRGB the same way as GStreamer,
const ColorSpace ColorSpace::Srgb = {
Primaries::Rec709,
TransferFunction::Srgb,
YcbcrEncoding::None,
Range::Full
};
then it can represent RGB data only. If an application picks it to
capture RGB data, all is fine. If an application picks it to capture YUV
data, that's technically wrong, but libcamera should adjust it as our
color space API requires pipeline handlers to adjust color spaces
requested by applications and not supported by the device.
> - libcamera PH being intelligent enough to deduce the stream
> configuration and convert ColorSpace::sRGB to { Rec709, Srgb, Rec601,
> Limited } and validate + return back to application
Nearly. I think the logic should check the transfer function, and if
it's Srgb and the encoding is None for a YUV format, it should pick
Rec601 as the encoding. Actually, looking at the presets we have,
something like the following could work:
if (format == YUV && encoding == None) {
if (transferFunction == Rec709) {
switch (primaries) {
case Raw:
case Smpte170m:
encoding = Rec601;
break;
case Rec709:
encoding = Rec709;
break;
case Rec2020:
encoding = Rec2020;
break;
}
} else {
encoding = Rec601;
}
}
(the Raw case should never happen, I've picked a default encoding in
that case.)
I'm sure there are other ways to express the logic, looking at the
GStreamer v4l2src element could help. It would also be nice to
centralize this kind of adjustement logic somewhere (possibly in the
ColorSpace class).
I said "nearly" above as I'm not sure about the default range we should
select (limited vs. full). sYCC is limited, JPEG is full. Should we
assume that an application that incorrectly asks for ColorSpace::Srgb
for a YUV stream will actually want sYCC or JPEG ?
> - If there is no YUV stream requested, ColorSpace::sRGB preset is
> used, as-is.
Right.
> b) Application intelligent enough that it's requesting a YUV stream and
> ColorSpace::sRGB doesn't mean YUV-encoded RGB(which will be true after
> we modify sRGB preset)
>
> - In that case, application send in directly { Rec709, Srgb,
> Rec601, Limited } as colorspace.
That, or the same with Full range, depending on what the application
wants.
> Is this understanding correct on my part? If yes, follow up question is:
I think so.
> - The pipeline-handler colorspace adjustment logic(for YUV streams)
> - do you see it per-PH or something generic that all pipeline-handlers
> would use ? (I guess the latter)
I think I've answered that above :-) I'd like to centralize as much
logic as possible, and at least the logic that picks a default YCbCr
encoding for YUV formats. For the rest, the adjustment also depends on
what the hardware supports, so fully generic helpers may not be
possible.
Even if we can't implement the adjustment code in shared code that would
work with all pipeline handlers, I would like to document the adjustment
heuristics to have consistent behaviour across pipeline handlers.
> > David, how is ColorSpace::Srgb interpreted by the Raspberry Pi ISP when
> > producing YUV data ?
> >
> > [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/colorspaces-details.html#colorspace-srgb-v4l2-colorspace-srgb
> > [3] https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-base/gst-libs/gst/video/video-color.c#L66
> >
> >>> Now two question arises for reverse mapping it i.e. what should:
> >>>
> >>> a) YcbcrEncding::None map to
> >>>
> >>> b) YcbcrEncding::Rec601 map to.
> >>>
> >>> For b) we have two contenders:
> >>>
> >>> GST_VIDEO_COLOR_MATRIX_BT601
> >>> GST_VIDEO_COLOR_MATRIX_RGB
> >>
> >> b) is currently handled with:
> >>
> >> + case ColorSpace::YcbcrEncoding::Rec601:
> >> + if (colorSpace == ColorSpace::Srgb)
> >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;
> >> + else
> >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;
> >> + break;
> >>
> >> Let's review this in v2 (submitting shortly)
> >
> > As per the above, I think YcbcrEncding::None should map to
> > GST_VIDEO_COLOR_MATRIX_RGB and YcbcrEncding::Rec601 to
> > GST_VIDEO_COLOR_MATRIX_BT601, unconditionally.
> >
> >>> For a) I think GST_VIDEO_COLOR_MATRIX_RGB would be better ?
> >>>
> >>> [1] https://git.linuxtv.org/media_tree.git/tree/include/uapi/linux/videodev2.h#n344
> >>>
> >>>>> + case GST_VIDEO_COLOR_MATRIX_UNKNOWN:
> >>>>> + colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;
> >>>>> + break;
> >>>>> + default:
> >>>>> + GST_WARNING("Unknown 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;
> >>>>> + case GST_VIDEO_COLOR_RANGE_UNKNOWN:
> >>>>> + colorspace->range = ColorSpace::Range::Default;
> >>>>> + break;
> >>>>> + default:
> >>>>> + GST_WARNING("Unknown range in colorimetry %d", colorimetry->range);
> >>>>> + }
> >>>>> +
> >>>>> + return colorspace;
> >>>>> +}
> >>>>> +
> >>>>> static GstVideoFormat
> >>>>> pixel_format_to_gst_format(const PixelFormat &format)
> >>>>> {
> >>>>> @@ -139,6 +290,17 @@ 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 != nullptr)
> >>>>
> >>>> if (colorimetry_str)
> >>>>
> >>>>> + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry_str, nullptr);
> >>>>> + else
> >>>>> + g_warning("libcamera::ColorSpace found but GstVideoColorimetry unknown");
> >>>>> + }
> >>>>> +
> >>>>> gst_caps_append_structure(caps, s);
> >>>>> return caps;
> >>>>> @@ -222,6 +384,19 @@ 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".
> >>>>
> >>>>> + std::optional<ColorSpace> colorSpace = colorspace_from_colorimetry(&colorimetry);
> >>>>> + stream_cfg.colorSpace = colorSpace;
> >>>>
> >>>> stream_cfg.colorSpace = colorspace_from_colorimetry(&colorimetry);
> >>>>
> >>>>> + } else {
> >>>>> + g_print("Invalid colorimetry %s", colorimetry_caps);
> >>>>> + }
> >>>>> + }
> >>>>> }
> >>>>> #if !GST_CHECK_VERSION(1, 17, 1)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list