[libcamera-devel] [PATCH v3 0/5] Add colorimetry support to libcamera gstreamer element.

[Nicolas Dufresne]

Le mardi 12 juillet 2022 à 11:49 +0530, Umang Jain via libcamera-devel a écrit :
> Hi Nicolas,
> 
> Thank you for the feedback.
> 
> On 7/11/22 23:02, Nicolas Dufresne via libcamera-devel wrote:
> > Hi Rishikesh,
> > 
> > I'm still uneasy on the design decision you are taking in this entire series.
> > I'm always available on IRC for discussions if you need to.
> > 
> > In general, your approach seems to only take a subset of libcamera and gstreamer
> > (the well known colorspace names). I would personally completely ignore these
> > user friendly goodies and just map four lower level enums: range, matrix,
> > transfer function and primaries. This way, we can map the full range of options
> > without ever having to update the code everytime something new is used by some
> > hardware. GStreamer will transparently translate this to a "well-known-name" in
> > caps.
> 
> 
> So, you prefer to breaking down the GST_VIDEO_COLORIMETRY to lower enums 
> and create a libcamera::Colorspace out of it?
> 
> I assumed that the GStreamer would need to work only on standard 
> colorspaces on both libcamera and GStreamer's side and we don't need to 
> carry all the lower level enums mapping in GStreamer element.

GStreamer have no strict requirement in this regard. There is a software
converter that should handle all the valid combination if converting is needed.

> 
> But your point of being able to use it when something new is used, is 
> good. So in that context, I agree on breaking it down to lower enums and 
> creating a libcamera::Colorspace  out of it. My only worry in that case 
> is that the assembled Colorspace might have one or more lower levels 
> enums as "Invalid" (i.e. not present in libcamera's colorspace.cpp) and 
> we proceed with such a Colorspace to be applied on the sensor?

If you found no match with what downstream have selected (no intersection), you
should fail the negotiation. Though, in general, downstream filter will only
suggest a default and leave it open for other values to be used.

> 
> Should we allow this? Or should we check if the constructed colorspace 
> belongs to a list of standard colorspaces (I sent a 
> isStandardColorSpace() helper patch on the list). Any thoughts?
> 
> 
> > 
> > regards,
> > Nicolas
> > 
> > Le lundi 11 juillet 2022 à 21:07 +0530, Rishikesh Donadkar a écrit :
> > > This series aims to add colorimetry support to the libcamera gstreamer
> > > element.
> > > 
> > > If colorimetry is specified by the user in the gstreamer pipeline, try to
> > > apply the libcamera colorspace corresponding to the GStreamer colorimetry
> > > to the camera if the colorspace passes the validation then push the
> > > colorimetry corresponding to the valid colorspace in the caps.
> > > If the colorimetry pushed into the caps is same as requested negotiation
> > > will succeed, else negotiation will fail.
> > > 
> > > If no colorimetry is specified by the user in the gstreamer pipeline,
> > > push the GStreamer colorimetry corresponding to the current colorspace
> > > applied to the camera into the caps and the negotiation will be take
> > > care of by the gstreamer. If the colorimetry corresponding to the current
> > > libcamera colorspace is not available in gstreamer then remove the
> > > colorimetry field if present in the caps.
> > > 
> > > [PATCH 1/5] - Made a verbose commit message. Introduced a new helper function
> > >                for converting std::string representation of colorimetry into
> > >                G_STRING_TYPE. Removed unnecessary g_free(). Removed the std::optional
> > >                wrapper around the colorSpace when converting into colorspace and
> > >                passed the value of the colorSpace instead to the function as it is
> > >                checked to have value before passing to the function.
> > >                Eliminated the code which was checking if the colorimetry is valid
> > >                colorimetry will be valid always because we are working with predefined
> > >                colorimetries in gstreamer.
> > > 
> > > [PATCH 2/5] - Simplified the if-else block. Eliminated the code where colorimetry
> > >                was set to nullptr when it was not known. Insted the field is left
> > >                unset. If the field is present and is unset then remove the field.
> > > 
> > > [PATCH 3/5] - colorSpace was passed by reference as a function argument in the
> > >                function colorimetry_form_colorspace(), Changed the function to
> > >                return the colorspace instead and setting the colorspace in the
> > >                StreamConfiguration was done after the colorspace is returned from
> > >                the function. The local functions that were used in the same file
> > >                were declared static.
> > > 
> > > [PATCH 4/5] - Made the changes required due to the changes made in patch 3
> > > 
> > > [PATCH 5/5] - If the camera cannot deliver the colorspace requested in the
> > >                gstreamer pipeline then the colorspace is reset to the value
> > >                that was present before the attempt was made to set the
> > >                colorspace corresponding to the requested colorimetry.
> > > 
> > > Rishikesh Donadkar (5):
> > >    gstreamer: convert from libcamera colorspace to GStreamer colorimetry.
> > >    gstreamer: Update the obtained colorimetry in caps.
> > >    gstreamer: Convert from GStreamer colorimetry to libcamera ColorSpace
> > >    gstreamer: Configure the colorSpace in the Stream configuration.
> > >    gstreamer: Reset the colorSpace if not the same after validation.
> > > 
> > >   src/gstreamer/gstlibcamera-utils.cpp | 69 ++++++++++++++++++++++++++++
> > >   src/gstreamer/gstlibcamerasrc.cpp    | 12 +++++
> > >   2 files changed, 81 insertions(+)
> > >