[libcamera-devel] [PATCH v3 1/1] gstreamer: Provide mulitple colorimetry support

[Nicolas Dufresne]

Le mardi 16 août 2022 à 11:11 +0530, Rishikesh Donadkar a écrit :
> > > +     if (gst_structure_has_field(s, "colorimetry")) {
> > > +             const gchar *colorimetry_str = gst_structure_get_string(s,
> > > "colorimetry");
> > > +             GstVideoColorimetry colorimetry;
> > > +
> > > +             if (!gst_video_colorimetry_from_string(&colorimetry,
> > > colorimetry_str))
> > > +                     g_critical("Invalid colorimetry %s",
> > > colorimetry_str);
> > 
> > 
> > If the parsing failed, the colorimetry structure will be left uninitialized.
> > Perhaps you should just early return if the caps string is invalid ?
> > 
> Noted.
> > 
> > > -     /* Configure colorimetry */
> > > -     if (gst_structure_has_field(s, "colorimetry")) {
> > > -             const gchar *colorimetry_str = gst_structure_get_string(s,
> > > "colorimetry");
> > > -             GstVideoColorimetry colorimetry;
> > > -
> > > -             if (!gst_video_colorimetry_from_string(&colorimetry,
> > > colorimetry_str))
> > > -                     g_critical("Invalid colorimetry %s",
> > > colorimetry_str);
> > > -
> > > -             stream_cfg.colorSpace =
> > > colorspace_from_colorimetry(colorimetry);
> > > -             /* Check if colorimetry had any identifiers which did not
> > > map */
> > > -             if (colorimetry.primaries !=
> > > GST_VIDEO_COLOR_PRIMARIES_UNKNOWN &&
> > > -                 stream_cfg.colorSpace == ColorSpace::Raw) {
> > > -                     GST_ERROR("One or more identifiers could not be
> > > mapped for %s colorimetry",
> > > -                               colorimetry_str);
> > > -                     stream_cfg.colorSpace = std::nullopt;
> > > +     /* Create new caps, copy the structure with best resolutions
> > > +      * and normalize the caps.
> > > +      */
> > > +     GstCaps *ncaps = gst_caps_copy_nth(caps, colorimetry_index);
> > > +     ncaps = gst_caps_normalize(ncaps);
> > 
> > I'm worried that original caps may have width/height ranges which will also
> > get
> > expended, leading to a really large number of caps. Any reason not to
> > enumerate
> > the colorimetry field instead instead ?
> 
>  Yes, expanding height/width range would create a lot of structures.
> The first idea I thought of was to enumerate the colorimetry list
> itself, but I don't know how ;-;

It is something along these lines, saving from copying anything, and creating
doing multiple trial for the same value.

const GstStructure *s = gst_caps_get_structure(caps, 0);
const GValue *colorimetry = gst_structure_get_value (s, "colorimetry");

if (GST_VALUE_HOLDS_LIST (colorimetry)) {
	int i;
	for (i = 0; i < g_value_list_get_size (colorimetry); i++) {
		const GValue *val = g_value_list_get_value (colorimetry, i);
		GstVideoColorimetry colorimetry;

		if (gst_video_colorimetry_from_string (&colorimetry, g_value_get_string (val))) {
			// Got a valid candidate, try it out !
		}
	}
}

> This is the reason I had to go with the approach of creating duplicate
> caps and normalizing it.
> If there is a way to enumerate the colorimetry list in your knowledge
> please let me know, that would be helpful.
> 
> Regards,
> 
> Rishikesh Donadkar