[libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <> libcamera::ColorSpace mappings
Nicolas Dufresne
nicolas.dufresne at collabora.com
Fri Jul 29 17:15:20 CEST 2022
Le vendredi 29 juillet 2022 à 17:54 +0300, Laurent Pinchart a écrit :
> 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)
To add to this, sRGB and GST_VIDEO_COLOR_MATRIX_RGB have absolutely no
relationship. Please try not to confuse things. GST_VIDEO_COLOR_MATRIX_RGB is
strictly used for the case the matrix is not applicable. Notably for all the RGB
type of data. This is clearly a 1 to 1 match with YcbcrEncoding::None.
>
> 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)
>
More information about the libcamera-devel
mailing list