[libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <> ColorSpace mappings
Rishikesh Donadkar
rishikeshdonadkar at gmail.com
Mon Aug 1 04:12:51 CEST 2022
Hello Laurent,
> > argument. Multiple colorimetry support shall be introduced in
> > subsequent commits.
>
> I'm probably missing something here, what do you mean by multiple
> colorimetry support ?
In the GStreamer pipeline multiple colorimetries can be specified
like the following:
libcamerasrc ! "video/x-raw,colorimetry={bt709,bt2020}" ! glimagesink
Here we would try bt709 or bt2020, if the colorspace corresponding
to any one of the above gets applied the negotiation will
pass else it should fail.
Regards,
Rishikesh Donadkar
On Mon, Aug 1, 2022 at 1:14 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Umang and Rishikesh,
>
> (Cc'ing David)
>
> Thank you for the patch.
>
> 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.
>
> > +
> > + 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
> };
>
> and adapting the rest of libcamera accordingly.
>
> David, this will affect Raspberry Pi, so I'd appreciate your opinion.
>
> > + 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.
>
> > + 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) ?
> 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.
>
> > + 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 ?
>
> > + }
> > +
> > 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)
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list