[libcamera-devel,v2,1/4] gstreamer: convert from libcamera colorspace toGStreamer colorimetry.
diff mbox series

Message ID 20220707094402.28730-2-rishikeshdonadkar@gmail.com
State Superseded
Headers show
Series
  • Add colorimetry support to libcamera gstreamer element.
Related show

Commit Message

Rishikesh Donadkar July 7, 2022, 9:43 a.m. UTC 
Libcamera StreamConfiguration class has colorSpace attribute, which
holds the colorspace that is being applied to the camera after the
validation of the camera configuration.

Map the libcamera colorspace to GStreamer colorimetry and find
the colorimetry corresponding to the colorspace that is being applied to
the camera. This colorimetry if found will be pushed in the caps.

Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
---
 src/gstreamer/gstlibcamera-utils.cpp | 32 ++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Vedant Paranjape July 7, 2022, 11:25 a.m. UTC | #1 
Hello Rishikesh,

Thanks for the patch.

> gstreamer: convert from libcamera colorspace toGStreamer colorimetry

----^----
You might want to fix the subject (s/toGstreamer/to Gstreamer)

On Thu, Jul 7, 2022 at 11:45 AM Rishikesh Donadkar <
rishikeshdonadkar@gmail.com> wrote:

> Libcamera StreamConfiguration class has colorSpace attribute, which
> holds the colorspace that is being applied to the camera after the
> validation of the camera configuration.
>
> Map the libcamera colorspace to GStreamer colorimetry and find
> the colorimetry corresponding to the colorspace that is being applied to
> the camera. This colorimetry if found will be pushed in the caps.
>
> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 32 ++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> b/src/gstreamer/gstlibcamera-utils.cpp
> index c97c0d43..60ac8c8e 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -45,6 +45,12 @@ static struct {
>         /* \todo NV42 is used in libcamera but is not mapped in GStreamer
> yet. */
>  };
>
> +static const std::vector<std::pair<ColorSpace, std::string>>
> ColorSpaceTocolorimetry = {
>

This might be a nitpick, but I think the variable name should be
ColorSpaceToColorimetry (can someone confirm this please ?)


> +       { ColorSpace::Srgb, GST_VIDEO_COLORIMETRY_SRGB },
> +       { ColorSpace::Rec709, GST_VIDEO_COLORIMETRY_BT709 },
> +       { ColorSpace::Rec2020, GST_VIDEO_COLORIMETRY_BT2020 },
> +};
> +
>  static GstVideoFormat
>  pixel_format_to_gst_format(const PixelFormat &format)
>  {
> @@ -87,6 +93,32 @@ bare_structure_from_format(const PixelFormat &format)
>         }
>  }
>
> +static gchar *
> +colorimerty_from_colorspace(std::optional<ColorSpace> colorSpace)
> +{
> +       gchar *colorimetry_str = nullptr;
> +       gchar *colorimetry_found = nullptr;
> +       GstVideoColorimetry colorimetry;
> +       gboolean isColorimetryValid;
> +
> +       auto iterColorimetry =
> std::find_if(ColorSpaceTocolorimetry.begin(), ColorSpaceTocolorimetry.end(),
> +                                           [&colorSpace](const auto
> &item) {
> +                                                   return colorSpace ==
> item.first;
> +                                           });
> +       if (iterColorimetry != ColorSpaceTocolorimetry.end()) {
> +               colorimetry_found = (gchar
> *)iterColorimetry->second.c_str();
> +               isColorimetryValid =
> gst_video_colorimetry_from_string(&colorimetry, colorimetry_found);
> +       }
> +       if (isColorimetryValid) {
> +               colorimetry_str =
> gst_video_colorimetry_to_string(&colorimetry);
> +               return colorimetry_str;
> +       } else {
> +               g_free(colorimetry_found);
>

I don't think you need to free this, it is owned by the string
(iterColorimetry->second) and it's lifetime will be taken care by the
string.

+               g_free(colorimetry_str);
>

> Frees the memory pointed to by mem. If mem is NULL it simply returns.

Umm, you don't need to free this string here as the if statement above
never ran, colorimetry_str is still a nullptr.

+               return nullptr;
> +       }
> +}
> +
>

So, now the whole if-else block can be simplified as follows:

+       if (isColorimetryValid) {
+               colorimetry_str =
gst_video_colorimetry_to_string(&colorimetry);
+       }
+
+ return colorimetry_str;

 GstCaps *
>  gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
>  {
> --
> 2.25.1
>
>
Regards,
Vedant Paranjape
Vedant Paranjape July 7, 2022, 11:57 a.m. UTC | #2 
Hello Rishikesh,

I have one more comment relating to Patch 1 and 2.

Since colorimerty_from_colorspace is called in patch 2 only after
colorspace has been confirmed to have a value, why not change the
datatype of the argument to ColorSpace instead of
std::optional<ColorSpace>, you can use ColorSpace.value() to access
the value stored in the std::optional.

Just as a footnote, I found this blog useful to understand the
functioning of std::optional:
https://devblogs.microsoft.com/cppblog/stdoptional-how-when-and-why/

+       if (colorspace) {
+               colorimetry = colorimerty_from_colorspace(co
lorspace);

On Thu, Jul 7, 2022 at 11:45 AM Rishikesh Donadkar
<rishikeshdonadkar@gmail.com> wrote:
>
> Libcamera StreamConfiguration class has colorSpace attribute, which
> holds the colorspace that is being applied to the camera after the
> validation of the camera configuration.
>
> Map the libcamera colorspace to GStreamer colorimetry and find
> the colorimetry corresponding to the colorspace that is being applied to
> the camera. This colorimetry if found will be pushed in the caps.
>
> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 32 ++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index c97c0d43..60ac8c8e 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -45,6 +45,12 @@ static struct {
>         /* \todo NV42 is used in libcamera but is not mapped in GStreamer yet. */
>  };
>
> +static const std::vector<std::pair<ColorSpace, std::string>> ColorSpaceTocolorimetry = {
> +       { ColorSpace::Srgb, GST_VIDEO_COLORIMETRY_SRGB },
> +       { ColorSpace::Rec709, GST_VIDEO_COLORIMETRY_BT709 },
> +       { ColorSpace::Rec2020, GST_VIDEO_COLORIMETRY_BT2020 },
> +};
> +
>  static GstVideoFormat
>  pixel_format_to_gst_format(const PixelFormat &format)
>  {
> @@ -87,6 +93,32 @@ bare_structure_from_format(const PixelFormat &format)
>         }
>  }
>
> +static gchar *
> +colorimerty_from_colorspace(std::optional<ColorSpace> colorSpace)
> +{
> +       gchar *colorimetry_str = nullptr;
> +       gchar *colorimetry_found = nullptr;
> +       GstVideoColorimetry colorimetry;
> +       gboolean isColorimetryValid;
> +
> +       auto iterColorimetry = std::find_if(ColorSpaceTocolorimetry.begin(), ColorSpaceTocolorimetry.end(),
> +                                           [&colorSpace](const auto &item) {
> +                                                   return colorSpace == item.first;
> +                                           });
> +       if (iterColorimetry != ColorSpaceTocolorimetry.end()) {
> +               colorimetry_found = (gchar *)iterColorimetry->second.c_str();
> +               isColorimetryValid = gst_video_colorimetry_from_string(&colorimetry, colorimetry_found);
> +       }
> +       if (isColorimetryValid) {
> +               colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
> +               return colorimetry_str;
> +       } else {
> +               g_free(colorimetry_found);
> +               g_free(colorimetry_str);
> +               return nullptr;
> +       }
> +}
> +
>  GstCaps *
>  gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
>  {
> --
> 2.25.1
>
Vedant Paranjape July 7, 2022, 12:14 p.m. UTC | #3 
Hello Rishikesh,

On Thu, Jul 7, 2022 at 11:45 AM Rishikesh Donadkar
<rishikeshdonadkar@gmail.com> wrote:
>
> Libcamera StreamConfiguration class has colorSpace attribute, which
> holds the colorspace that is being applied to the camera after the
> validation of the camera configuration.
>
> Map the libcamera colorspace to GStreamer colorimetry and find
> the colorimetry corresponding to the colorspace that is being applied to
> the camera. This colorimetry if found will be pushed in the caps.
>
> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 32 ++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index c97c0d43..60ac8c8e 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -45,6 +45,12 @@ static struct {
>         /* \todo NV42 is used in libcamera but is not mapped in GStreamer yet. */
>  };
>
> +static const std::vector<std::pair<ColorSpace, std::string>> ColorSpaceTocolorimetry = {
> +       { ColorSpace::Srgb, GST_VIDEO_COLORIMETRY_SRGB },
> +       { ColorSpace::Rec709, GST_VIDEO_COLORIMETRY_BT709 },
> +       { ColorSpace::Rec2020, GST_VIDEO_COLORIMETRY_BT2020 },
> +};
> +
>  static GstVideoFormat
>  pixel_format_to_gst_format(const PixelFormat &format)
>  {
> @@ -87,6 +93,32 @@ bare_structure_from_format(const PixelFormat &format)
>         }
>  }
>
> +static gchar *
> +colorimerty_from_colorspace(std::optional<ColorSpace> colorSpace)
          ------^^^-----
It should be colorimetry

> +{
> +       gchar *colorimetry_str = nullptr;
> +       gchar *colorimetry_found = nullptr;
> +       GstVideoColorimetry colorimetry;
> +       gboolean isColorimetryValid;
> +
> +       auto iterColorimetry = std::find_if(ColorSpaceTocolorimetry.begin(), ColorSpaceTocolorimetry.end(),
> +                                           [&colorSpace](const auto &item) {
> +                                                   return colorSpace == item.first;
> +                                           });
> +       if (iterColorimetry != ColorSpaceTocolorimetry.end()) {
> +               colorimetry_found = (gchar *)iterColorimetry->second.c_str();
> +               isColorimetryValid = gst_video_colorimetry_from_string(&colorimetry, colorimetry_found);
> +       }
> +       if (isColorimetryValid) {
> +               colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
> +               return colorimetry_str;
> +       } else {
> +               g_free(colorimetry_found);
> +               g_free(colorimetry_str);
> +               return nullptr;
> +       }
> +}
> +
>  GstCaps *
>  gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
>  {
> --
> 2.25.1
>
Vedant Paranjape July 7, 2022, 12:16 p.m. UTC | #4 
Somehow the pointer to the mistake didn't format as it is visible in
the reply ;( About time I stop using Gmail.

On Thu, Jul 7, 2022 at 2:14 PM Vedant Paranjape
<vedantparanjape160201@gmail.com> wrote:
>
> Hello Rishikesh,
>
> On Thu, Jul 7, 2022 at 11:45 AM Rishikesh Donadkar
> <rishikeshdonadkar@gmail.com> wrote:
> >
> > Libcamera StreamConfiguration class has colorSpace attribute, which
> > holds the colorspace that is being applied to the camera after the
> > validation of the camera configuration.
> >
> > Map the libcamera colorspace to GStreamer colorimetry and find
> > the colorimetry corresponding to the colorspace that is being applied to
> > the camera. This colorimetry if found will be pushed in the caps.
> >
> > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
> > ---
> >  src/gstreamer/gstlibcamera-utils.cpp | 32 ++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > index c97c0d43..60ac8c8e 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -45,6 +45,12 @@ static struct {
> >         /* \todo NV42 is used in libcamera but is not mapped in GStreamer yet. */
> >  };
> >
> > +static const std::vector<std::pair<ColorSpace, std::string>> ColorSpaceTocolorimetry = {
> > +       { ColorSpace::Srgb, GST_VIDEO_COLORIMETRY_SRGB },
> > +       { ColorSpace::Rec709, GST_VIDEO_COLORIMETRY_BT709 },
> > +       { ColorSpace::Rec2020, GST_VIDEO_COLORIMETRY_BT2020 },
> > +};
> > +
> >  static GstVideoFormat
> >  pixel_format_to_gst_format(const PixelFormat &format)
> >  {
> > @@ -87,6 +93,32 @@ bare_structure_from_format(const PixelFormat &format)
> >         }
> >  }
> >
> > +static gchar *
> > +colorimerty_from_colorspace(std::optional<ColorSpace> colorSpace)
>           ------^^^-----
> It should be colorimetry
>
> > +{
> > +       gchar *colorimetry_str = nullptr;
> > +       gchar *colorimetry_found = nullptr;
> > +       GstVideoColorimetry colorimetry;
> > +       gboolean isColorimetryValid;
> > +
> > +       auto iterColorimetry = std::find_if(ColorSpaceTocolorimetry.begin(), ColorSpaceTocolorimetry.end(),
> > +                                           [&colorSpace](const auto &item) {
> > +                                                   return colorSpace == item.first;
> > +                                           });
> > +       if (iterColorimetry != ColorSpaceTocolorimetry.end()) {
> > +               colorimetry_found = (gchar *)iterColorimetry->second.c_str();
> > +               isColorimetryValid = gst_video_colorimetry_from_string(&colorimetry, colorimetry_found);
> > +       }
> > +       if (isColorimetryValid) {
> > +               colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
> > +               return colorimetry_str;
> > +       } else {
> > +               g_free(colorimetry_found);
> > +               g_free(colorimetry_str);
> > +               return nullptr;
> > +       }
> > +}
> > +
> >  GstCaps *
> >  gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
> >  {
> > --
> > 2.25.1
> >
Nicolas Dufresne July 7, 2022, 2 p.m. UTC | #5 
Le jeudi 07 juillet 2022 à 15:13 +0530, Rishikesh Donadkar via libcamera-devel a
écrit :
> Libcamera StreamConfiguration class has colorSpace attribute, which
> holds the colorspace that is being applied to the camera after the
> validation of the camera configuration.
> 
> Map the libcamera colorspace to GStreamer colorimetry and find
> the colorimetry corresponding to the colorspace that is being applied to
> the camera. This colorimetry if found will be pushed in the caps.
> 
> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 32 ++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index c97c0d43..60ac8c8e 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -45,6 +45,12 @@ static struct {
>  	/* \todo NV42 is used in libcamera but is not mapped in GStreamer yet. */
>  };
>  
> +static const std::vector<std::pair<ColorSpace, std::string>> ColorSpaceTocolorimetry = {
> +	{ ColorSpace::Srgb, GST_VIDEO_COLORIMETRY_SRGB },
> +	{ ColorSpace::Rec709, GST_VIDEO_COLORIMETRY_BT709 },
> +	{ ColorSpace::Rec2020, GST_VIDEO_COLORIMETRY_BT2020 },
> +};
> +
>  static GstVideoFormat
>  pixel_format_to_gst_format(const PixelFormat &format)
>  {
> @@ -87,6 +93,32 @@ bare_structure_from_format(const PixelFormat &format)
>  	}
>  }
>  
> +static gchar *
> +colorimerty_from_colorspace(std::optional<ColorSpace> colorSpace)
> +{
> +	gchar *colorimetry_str = nullptr;
> +	gchar *colorimetry_found = nullptr;
> +	GstVideoColorimetry colorimetry;
> +	gboolean isColorimetryValid;
> +
> +	auto iterColorimetry = std::find_if(ColorSpaceTocolorimetry.begin(), ColorSpaceTocolorimetry.end(),
> +					    [&colorSpace](const auto &item) {
> +						    return colorSpace == item.first;
> +					    });

Perhaps more readable if you make a helper libcamera_colorimetry_to_gst_string()
?

> +	if (iterColorimetry != ColorSpaceTocolorimetry.end()) {
> +		colorimetry_found = (gchar *)iterColorimetry->second.c_str();
> +		isColorimetryValid = gst_video_colorimetry_from_string(&colorimetry, colorimetry_found);
> +	}

I think you should cary the colorimetry structure over, as matching the
colorpsace is just a first step, there might be alteration to that colorspace.

> +	if (isColorimetryValid) {
> +		colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
> +		return colorimetry_str;
> +	} else {
> +		g_free(colorimetry_found);
> +		g_free(colorimetry_str);
> +		return nullptr;
> +	}
> +}
> +
>  GstCaps *
>  gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
>  {

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
index c97c0d43..60ac8c8e 100644
--- a/src/gstreamer/gstlibcamera-utils.cpp
+++ b/src/gstreamer/gstlibcamera-utils.cpp
@@ -45,6 +45,12 @@  static struct {
 	/* \todo NV42 is used in libcamera but is not mapped in GStreamer yet. */
 };
 
+static const std::vector<std::pair<ColorSpace, std::string>> ColorSpaceTocolorimetry = {
+	{ ColorSpace::Srgb, GST_VIDEO_COLORIMETRY_SRGB },
+	{ ColorSpace::Rec709, GST_VIDEO_COLORIMETRY_BT709 },
+	{ ColorSpace::Rec2020, GST_VIDEO_COLORIMETRY_BT2020 },
+};
+
 static GstVideoFormat
 pixel_format_to_gst_format(const PixelFormat &format)
 {
@@ -87,6 +93,32 @@  bare_structure_from_format(const PixelFormat &format)
 	}
 }
 
+static gchar *
+colorimerty_from_colorspace(std::optional<ColorSpace> colorSpace)
+{
+	gchar *colorimetry_str = nullptr;
+	gchar *colorimetry_found = nullptr;
+	GstVideoColorimetry colorimetry;
+	gboolean isColorimetryValid;
+
+	auto iterColorimetry = std::find_if(ColorSpaceTocolorimetry.begin(), ColorSpaceTocolorimetry.end(),
+					    [&colorSpace](const auto &item) {
+						    return colorSpace == item.first;
+					    });
+	if (iterColorimetry != ColorSpaceTocolorimetry.end()) {
+		colorimetry_found = (gchar *)iterColorimetry->second.c_str();
+		isColorimetryValid = gst_video_colorimetry_from_string(&colorimetry, colorimetry_found);
+	}
+	if (isColorimetryValid) {
+		colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
+		return colorimetry_str;
+	} else {
+		g_free(colorimetry_found);
+		g_free(colorimetry_str);
+		return nullptr;
+	}
+}
+
 GstCaps *
 gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
 {