[libcamera-devel] [PATCH v2] gstreamer: Provide colorimetry <> ColorSpace mappings

[Nicolas Dufresne]

Le mardi 16 août 2022 à 05:47 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> On Mon, Aug 15, 2022 at 09:03:48AM -0400, Nicolas Dufresne wrote:
> > Slowly catching up ;-P
> 
> :-)
> 
> > Le lundi 01 août 2022 à 18:01 +0530, Umang Jain a écrit :
> > > > 
> > > > > +	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!
> > 
> > The colorimetry.primaries is being set by gst_video_colorimetry_from_string()
> > which ensure a valid value exists. The reduces the "unsupported" surface to
> > newly added colorspace in GStreamer itself, something that only occurs perhaps
> > once every 3 years.
> > 
> > Perhaps to make this less harsh (and aligned with the caller g_critical("Invalid
> > colorimetry %s", colorimetry_caps);), we could change
> > colorspace_from_colorimetry() to return a boolean (success/failure) and have the
> > colorSpace a return parameter ?
> 
> I've reviewed v3 and recommended returning a std::optional<ColorSpace>
> from colorspace_from_colorimetry(), so we had the same idea :-)
> 
> > We could then consistently use g_criticical and ignore the GstCaps
> > colorimetry field like we'd do if the user had set an invalid string.
> 
> This I'm not sure to follow though, I don't understand what you mean by
> consistent usage of g_critical().

This function call is always preceded by a call to
gst_video_colorimetry_from_string(), which returns false if the string is
invalid. In that case, we use g_critical(), which is the same thing as an
g_assert() but without the abort. While g_error() does abort the application.
The two errors are highly similar, one will just trace, the other (this one) is
aborting the entire process. I think both should equally just trace (or cleanly
fail), as this is user error, not a programming error.

regards,
Nicolas

>