[libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <> libcamera::ColorSpace mappings
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 29 16:54:43 CEST 2022
Hi Umang,
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:
> > Hi Laurent,
> >
> > My anaylsis on sRGB mapping ...
> >
> > On 7/27/22 06:47, Laurent Pinchart wrote:
> >> Hi Umang and Rishikesh,
> >>
> >> Thank you for the patch.
> >>
> >> 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).
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