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

[Laurent Pinchart]

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().

-- 
Regards,

Laurent Pinchart