[libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <> ColorSpace mappings
Umang Jain
umang.jain at ideasonboard.com
Mon Aug 1 14:31:40 CEST 2022
Hi Laurent,
On 8/1/22 01:14, Laurent Pinchart 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.
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 ;-)
>
>> + 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)
More information about the libcamera-devel
mailing list