| Message ID | 20220707094402.28730-3-rishikeshdonadkar@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hello Rishikesh, Thanks for the patch, On Thu, Jul 7, 2022 at 11:45 AM Rishikesh Donadkar <rishikeshdonadkar@gmail.com> wrote: > > If the colorspace is set in the StreamConfiguration, get the colorimetry as > a result of conversion from the previous patch and update it into the caps. this sounds better: get the colorimetry after converting colorspace using functions from the previous patch and update it into the GStreamer caps. > > If the colorimetry corresponding to the colorspace set in the > StreamConfiguration is not available in GStreamer set the colorimetry > field to nullptr in the caps (this will fail the negotiation). > > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 60ac8c8e..eb9c49da 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -166,11 +166,26 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg > { > GstCaps *caps = gst_caps_new_empty(); > GstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat); > + gchar *colorimetry = nullptr; > + std::optional<ColorSpace> colorspace = stream_cfg.colorSpace; > > gst_structure_set(s, > "width", G_TYPE_INT, stream_cfg.size.width, > "height", G_TYPE_INT, stream_cfg.size.height, > nullptr); > + > + if (colorspace) { You might want to use > colorspace.has_value() Even though it's one and the same, this seems more verbose. Just a personal preference, anyone has comments about this ? > + colorimetry = colorimerty_from_colorspace(colorspace); > + if (colorimetry) { > + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry, nullptr); > + } else { > + gst_structure_set(s, "colorimetry", G_TYPE_STRING, nullptr, nullptr); > + g_free(colorimetry); > + } > + } else { > + g_free(colorimetry); > + } > + This if-else block can be further simplified as follows: <snip> if (colorspace) { colorimetry = colorimerty_from_colorspace(colorspace); gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry, nullptr); g_free(colorimetry); } </snip> Regards, Vedant Paranjape + + if (colorspace) { + colorimetry = colorimerty_from_colorspace(co lorspace); + if (colorimetry) { + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry, nullptr); + } else { + gst_structure_set(s, "colorimetry", G_TYPE_STRING, nullptr, nullptr); + g_free(colorimetry); + } + } else { + g_free(colorimetry); + } + > gst_caps_append_structure(caps, s); > > return caps; > -- > 2.25.1 >
Hello Rishikesh, On Thu, Jul 7, 2022 at 11:45 AM Rishikesh Donadkar <rishikeshdonadkar@gmail.com> wrote: > > If the colorspace is set in the StreamConfiguration, get the colorimetry as > a result of conversion from the previous patch and update it into the caps. > > If the colorimetry corresponding to the colorspace set in the > StreamConfiguration is not available in GStreamer set the colorimetry > field to nullptr in the caps (this will fail the negotiation). > > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 60ac8c8e..eb9c49da 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -166,11 +166,26 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg > { > GstCaps *caps = gst_caps_new_empty(); > GstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat); > + gchar *colorimetry = nullptr; > + std::optional<ColorSpace> colorspace = stream_cfg.colorSpace; > > gst_structure_set(s, > "width", G_TYPE_INT, stream_cfg.size.width, > "height", G_TYPE_INT, stream_cfg.size.height, > nullptr); > + > + if (colorspace) { > + colorimetry = colorimerty_from_colorspace(colorspace); s/colorimerty_from_colorspace/colorimetry_from_colorspace > + if (colorimetry) { > + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry, nullptr); > + } else { > + gst_structure_set(s, "colorimetry", G_TYPE_STRING, nullptr, nullptr); > + g_free(colorimetry); > + } > + } else { > + g_free(colorimetry); > + } > + > gst_caps_append_structure(caps, s); > > return caps; > -- > 2.25.1 >
Le jeudi 07 juillet 2022 à 15:14 +0530, Rishikesh Donadkar via libcamera-devel a écrit : > If the colorspace is set in the StreamConfiguration, get the colorimetry as > a result of conversion from the previous patch and update it into the caps. > > If the colorimetry corresponding to the colorspace set in the > StreamConfiguration is not available in GStreamer set the colorimetry > field to nullptr in the caps (this will fail the negotiation). > > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 60ac8c8e..eb9c49da 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -166,11 +166,26 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg > { > GstCaps *caps = gst_caps_new_empty(); > GstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat); > + gchar *colorimetry = nullptr; > + std::optional<ColorSpace> colorspace = stream_cfg.colorSpace; > > gst_structure_set(s, > "width", G_TYPE_INT, stream_cfg.size.width, > "height", G_TYPE_INT, stream_cfg.size.height, > nullptr); > + > + if (colorspace) { > + colorimetry = colorimerty_from_colorspace(colorspace); > + if (colorimetry) { > + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry, nullptr); > + } else { > + gst_structure_set(s, "colorimetry", G_TYPE_STRING, nullptr, nullptr); > + g_free(colorimetry); Setting a string field as null is not allowed in gstreamer and will cause assertion later. If you don't know the colorspace/colorimetry, don't set that field. If the field is there but unfixed, remove it. > + } > + } else { > + g_free(colorimetry); > + } > + > gst_caps_append_structure(caps, s); > > return caps;
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index 60ac8c8e..eb9c49da 100644 --- a/src/gstreamer/gstlibcamera-utils.cpp +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -166,11 +166,26 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg { GstCaps *caps = gst_caps_new_empty(); GstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat); + gchar *colorimetry = nullptr; + std::optional<ColorSpace> colorspace = stream_cfg.colorSpace; gst_structure_set(s, "width", G_TYPE_INT, stream_cfg.size.width, "height", G_TYPE_INT, stream_cfg.size.height, nullptr); + + if (colorspace) { + colorimetry = colorimerty_from_colorspace(colorspace); + if (colorimetry) { + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry, nullptr); + } else { + gst_structure_set(s, "colorimetry", G_TYPE_STRING, nullptr, nullptr); + g_free(colorimetry); + } + } else { + g_free(colorimetry); + } + gst_caps_append_structure(caps, s); return caps;
If the colorspace is set in the StreamConfiguration, get the colorimetry as a result of conversion from the previous patch and update it into the caps. If the colorimetry corresponding to the colorspace set in the StreamConfiguration is not available in GStreamer set the colorimetry field to nullptr in the caps (this will fail the negotiation). Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> --- src/gstreamer/gstlibcamera-utils.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+)