[libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <> ColorSpace mappings
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 1 15:30:34 CEST 2022
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.)
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.
> > >> + 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)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list