| Message ID | 20241108091246.3753388-1-qi.hou@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Hou, (CC'ing Nicolas Dufresne) Thank you for the patch. On Fri, Nov 08, 2024 at 06:12:46PM +0900, Hou Qi wrote: > When libcamerasrc is negotiating with downstream element, it first > extracts colorimetry field from downstream supported caps, then set > this colorimetry to its stream configuration and propagates the > colorimetry downstream. > > Currently libamerasrc only considers the case there is one colorimetry > in colorimetry field of downstream caps. But the issue is that > downstream caps may report a list of supported colorimetry, which > causes libcamerasrc to set unknown colorimetry to stream configuration > and negotiate fail with downstream element. > > In order to fix the issue, get the first string if colorimetry field > holds string list to set it to stream configuration. > > Signed-off-by: Hou Qi <qi.hou@nxp.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 732987ef..2c1ebce8 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -489,9 +489,22 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > /* Configure colorimetry */ > if (gst_structure_has_field(s, "colorimetry")) { > - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); > + const GValue *mode; > + const gchar *colorimetry_str = NULL; > GstVideoColorimetry colorimetry; > > + mode = gst_structure_get_value(s, "colorimetry"); "mode" seems a weird name, how about calling it "value" ? > + > + if (G_VALUE_HOLDS_STRING(mode)) { > + colorimetry_str = gst_structure_get_string(s, "colorimetry"); > + } else if (GST_VALUE_HOLDS_LIST(mode)) { > + const GValue *first_element = gst_value_list_get_value(mode, 0); > + > + if (G_VALUE_HOLDS_STRING(first_element)) { > + colorimetry_str = g_value_get_string(first_element); > + } No need for curly brackets here. > + } Would the following be simpler and more readable ? if (GST_VALUE_HOLDS_LIST(value)) value = gst_value_list_get_value(value, 0); if (G_VALUE_HOLDS_STRING(value)) colorimetry_str = g_value_get_string(value); What happens if neither condition is true though, will gst_video_colorimetry_from_string() handle a null colorimetry_str gracefully ? > + > if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) > g_critical("Invalid colorimetry %s", colorimetry_str); >
Le vendredi 08 novembre 2024 à 18:12 +0900, Hou Qi a écrit : > When libcamerasrc is negotiating with downstream element, it first > extracts colorimetry field from downstream supported caps, then set > this colorimetry to its stream configuration and propagates the > colorimetry downstream. > > Currently libamerasrc only considers the case there is one colorimetry > in colorimetry field of downstream caps. But the issue is that > downstream caps may report a list of supported colorimetry, which > causes libcamerasrc to set unknown colorimetry to stream configuration > and negotiate fail with downstream element. > > In order to fix the issue, get the first string if colorimetry field > holds string list to set it to stream configuration. > > Signed-off-by: Hou Qi <qi.hou@nxp.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 732987ef..2c1ebce8 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -489,9 +489,22 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > /* Configure colorimetry */ > if (gst_structure_has_field(s, "colorimetry")) { > - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); > + const GValue *mode; > + const gchar *colorimetry_str = NULL; > GstVideoColorimetry colorimetry; > > + mode = gst_structure_get_value(s, "colorimetry"); > + > + if (G_VALUE_HOLDS_STRING(mode)) { > + colorimetry_str = gst_structure_get_string(s, "colorimetry"); > + } else if (GST_VALUE_HOLDS_LIST(mode)) { > + const GValue *first_element = gst_value_list_get_value(mode, 0); > + > + if (G_VALUE_HOLDS_STRING(first_element)) { > + colorimetry_str = g_value_get_string(first_element); > + } > + } > + gst_libcamera_configure_stream_from_caps() is exepected to be called with fixed caps. Feel free to add an assert on that, fixation should happen earlier instead. This could be as simple as gst_caps_fixate(), specially that you are picking the first value in the list. Nicolas > if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) > g_critical("Invalid colorimetry %s", colorimetry_str); >
Hi Nicolas, I see there is already assert check in gst_libcamera_configure_stream_from_caps(). /* First fixate the caps using default configuration value. */ g_assert(gst_caps_is_writable(caps)); Regards, Qi Hou -----Original Message----- From: Nicolas Dufresne <nicolas@ndufresne.ca> Sent: 2024年11月13日 2:03 To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com> Subject: [EXT] Re: [PATCH] gstreamer: Read correct colorimetry from caps into the stream configuration Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button Le vendredi 08 novembre 2024 à 18:12 +0900, Hou Qi a écrit : > When libcamerasrc is negotiating with downstream element, it first > extracts colorimetry field from downstream supported caps, then set > this colorimetry to its stream configuration and propagates the > colorimetry downstream. > > Currently libamerasrc only considers the case there is one colorimetry > in colorimetry field of downstream caps. But the issue is that > downstream caps may report a list of supported colorimetry, which > causes libcamerasrc to set unknown colorimetry to stream configuration > and negotiate fail with downstream element. > > In order to fix the issue, get the first string if colorimetry field > holds string list to set it to stream configuration. > > Signed-off-by: Hou Qi <qi.hou@nxp.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp > b/src/gstreamer/gstlibcamera-utils.cpp > index 732987ef..2c1ebce8 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -489,9 +489,22 @@ > gst_libcamera_configure_stream_from_caps(StreamConfiguration > &stream_cfg, > > /* Configure colorimetry */ > if (gst_structure_has_field(s, "colorimetry")) { > - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); > + const GValue *mode; > + const gchar *colorimetry_str = NULL; > GstVideoColorimetry colorimetry; > > + mode = gst_structure_get_value(s, "colorimetry"); > + > + if (G_VALUE_HOLDS_STRING(mode)) { > + colorimetry_str = gst_structure_get_string(s, "colorimetry"); > + } else if (GST_VALUE_HOLDS_LIST(mode)) { > + const GValue *first_element = > + gst_value_list_get_value(mode, 0); > + > + if (G_VALUE_HOLDS_STRING(first_element)) { > + colorimetry_str = g_value_get_string(first_element); > + } > + } > + gst_libcamera_configure_stream_from_caps() is exepected to be called with fixed caps. Feel free to add an assert on that, fixation should happen earlier instead. This could be as simple as gst_caps_fixate(), specially that you are picking the first value in the list. Nicolas > if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) > g_critical("Invalid colorimetry %s", > colorimetry_str); >
Hi Laurent Pinchart, Please check my comments. Thanks for your advice. I have sent out v2. Regards, Qi Hou -----Original Message----- From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Sent: 2024年11月12日 16:35 To: Qi Hou <qi.hou@nxp.com> Cc: libcamera-devel@lists.libcamera.org; Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com>; Nicolas Dufresne <nicolas@ndufresne.ca> Subject: [EXT] Re: [PATCH] gstreamer: Read correct colorimetry from caps into the stream configuration Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button Hi Hou, (CC'ing Nicolas Dufresne) Thank you for the patch. On Fri, Nov 08, 2024 at 06:12:46PM +0900, Hou Qi wrote: > When libcamerasrc is negotiating with downstream element, it first > extracts colorimetry field from downstream supported caps, then set > this colorimetry to its stream configuration and propagates the > colorimetry downstream. > > Currently libamerasrc only considers the case there is one colorimetry > in colorimetry field of downstream caps. But the issue is that > downstream caps may report a list of supported colorimetry, which > causes libcamerasrc to set unknown colorimetry to stream configuration > and negotiate fail with downstream element. > > In order to fix the issue, get the first string if colorimetry field > holds string list to set it to stream configuration. > > Signed-off-by: Hou Qi <qi.hou@nxp.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp > b/src/gstreamer/gstlibcamera-utils.cpp > index 732987ef..2c1ebce8 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -489,9 +489,22 @@ > gst_libcamera_configure_stream_from_caps(StreamConfiguration > &stream_cfg, > > /* Configure colorimetry */ > if (gst_structure_has_field(s, "colorimetry")) { > - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); > + const GValue *mode; > + const gchar *colorimetry_str = NULL; > GstVideoColorimetry colorimetry; > > + mode = gst_structure_get_value(s, "colorimetry"); "mode" seems a weird name, how about calling it "value" ? [Qi Hou] ok, I renamed to "value". > + > + if (G_VALUE_HOLDS_STRING(mode)) { > + colorimetry_str = gst_structure_get_string(s, "colorimetry"); > + } else if (GST_VALUE_HOLDS_LIST(mode)) { > + const GValue *first_element = > + gst_value_list_get_value(mode, 0); > + > + if (G_VALUE_HOLDS_STRING(first_element)) { > + colorimetry_str = g_value_get_string(first_element); > + } No need for curly brackets here. > + } Would the following be simpler and more readable ? [Qi Hou] Yes, thanks for your advice. if (GST_VALUE_HOLDS_LIST(value)) value = gst_value_list_get_value(value, 0); if (G_VALUE_HOLDS_STRING(value)) colorimetry_str = g_value_get_string(value); What happens if neither condition is true though, will gst_video_colorimetry_from_string() handle a null colorimetry_str gracefully ? [Qi Hou] If colorimetry_str is NULL, gst_video_colorimetry_from_string() return default unknown colorimetry 1:1:1:0. > + > if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) > g_critical("Invalid colorimetry %s", > colorimetry_str); > -- Regards, Laurent Pinchart
Le mercredi 13 novembre 2024 à 10:20 +0000, Qi Hou a écrit : > Hi Nicolas, > > I see there is already assert check in gst_libcamera_configure_stream_from_caps(). > > /* First fixate the caps using default configuration value. */ > g_assert(gst_caps_is_writable(caps)); Oh, sorry about this indirection, new comment below. > > Regards, > Qi Hou > > -----Original Message----- > From: Nicolas Dufresne <nicolas@ndufresne.ca> > Sent: 2024年11月13日 2:03 > To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org > Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com> > Subject: [EXT] Re: [PATCH] gstreamer: Read correct colorimetry from caps into the stream configuration > > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > Le vendredi 08 novembre 2024 à 18:12 +0900, Hou Qi a écrit : > > When libcamerasrc is negotiating with downstream element, it first > > extracts colorimetry field from downstream supported caps, then set > > this colorimetry to its stream configuration and propagates the > > colorimetry downstream. > > > > Currently libamerasrc only considers the case there is one colorimetry > > in colorimetry field of downstream caps. But the issue is that > > downstream caps may report a list of supported colorimetry, which > > causes libcamerasrc to set unknown colorimetry to stream configuration > > and negotiate fail with downstream element. > > > > In order to fix the issue, get the first string if colorimetry field > > holds string list to set it to stream configuration. > > > > Signed-off-by: Hou Qi <qi.hou@nxp.com> > > --- > > src/gstreamer/gstlibcamera-utils.cpp | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp > > b/src/gstreamer/gstlibcamera-utils.cpp > > index 732987ef..2c1ebce8 100644 > > --- a/src/gstreamer/gstlibcamera-utils.cpp > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > @@ -489,9 +489,22 @@ > > gst_libcamera_configure_stream_from_caps(StreamConfiguration > > &stream_cfg, > > > > /* Configure colorimetry */ > > if (gst_structure_has_field(s, "colorimetry")) { > > - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); > > + const GValue *mode; > > + const gchar *colorimetry_str = NULL; > > GstVideoColorimetry colorimetry; > > > > + mode = gst_structure_get_value(s, "colorimetry"); Just do: gst_structure_fixate_field (s, "colorimetry"); And keep the original code. The caps implementation seems sub-optimal though. In an ideal world, we'd do this in stage: - Fixate the caps - So caps to GstVideoInfo - Fill the stream_cfg from the VideoInfo - Finally do GstVideoInfo to caps in order to push complete caps. This will fill the proper default of the other video/x-raw field we are not careful about, like stereo views, pixel-aspect ratio, etc. If you have time, this can greatly improve caps negotiation an correctness for future users. > > + > > + if (G_VALUE_HOLDS_STRING(mode)) { > > + colorimetry_str = gst_structure_get_string(s, "colorimetry"); > > + } else if (GST_VALUE_HOLDS_LIST(mode)) { > > + const GValue *first_element = > > + gst_value_list_get_value(mode, 0); > > + > > + if (G_VALUE_HOLDS_STRING(first_element)) { > > + colorimetry_str = g_value_get_string(first_element); > > + } > > + } > > + > > gst_libcamera_configure_stream_from_caps() is exepected to be called with fixed caps. Feel free to add an assert on that, fixation should happen earlier instead. This could be as simple as gst_caps_fixate(), specially that you are picking the first value in the list. > > Nicolas > > > if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) > > g_critical("Invalid colorimetry %s", > > colorimetry_str); > > >
Hi Nicolas, I've sent out v3 to fixate colorimetry field. And for your suggestions to improve caps negotiating, I think I will do that later. - Fixate the caps - So caps to GstVideoInfo - Fill the stream_cfg from the VideoInfo - Finally do GstVideoInfo to caps in order to push complete caps. Regards, Qi Hou -----Original Message----- From: Nicolas Dufresne <nicolas@ndufresne.ca> Sent: 2024年11月14日 3:29 To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com> Subject: Re: [EXT] Re: [PATCH] gstreamer: Read correct colorimetry from caps into the stream configuration Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button Le mercredi 13 novembre 2024 à 10:20 +0000, Qi Hou a écrit : > Hi Nicolas, > > I see there is already assert check in gst_libcamera_configure_stream_from_caps(). > > /* First fixate the caps using default configuration value. */ > g_assert(gst_caps_is_writable(caps)); Oh, sorry about this indirection, new comment below. > > Regards, > Qi Hou > > -----Original Message----- > From: Nicolas Dufresne <nicolas@ndufresne.ca> > Sent: 2024年11月13日 2:03 > To: Qi Hou <qi.hou@nxp.com>; libcamera-devel@lists.libcamera.org > Cc: Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier > <julien.vuillaumier@nxp.com> > Subject: [EXT] Re: [PATCH] gstreamer: Read correct colorimetry from > caps into the stream configuration > > Caution: This is an external email. Please take care when clicking > links or opening attachments. When in doubt, report the message using > the 'Report this email' button > > > Le vendredi 08 novembre 2024 à 18:12 +0900, Hou Qi a écrit : > > When libcamerasrc is negotiating with downstream element, it first > > extracts colorimetry field from downstream supported caps, then set > > this colorimetry to its stream configuration and propagates the > > colorimetry downstream. > > > > Currently libamerasrc only considers the case there is one > > colorimetry in colorimetry field of downstream caps. But the issue > > is that downstream caps may report a list of supported colorimetry, > > which causes libcamerasrc to set unknown colorimetry to stream > > configuration and negotiate fail with downstream element. > > > > In order to fix the issue, get the first string if colorimetry field > > holds string list to set it to stream configuration. > > > > Signed-off-by: Hou Qi <qi.hou@nxp.com> > > --- > > src/gstreamer/gstlibcamera-utils.cpp | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp > > b/src/gstreamer/gstlibcamera-utils.cpp > > index 732987ef..2c1ebce8 100644 > > --- a/src/gstreamer/gstlibcamera-utils.cpp > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > @@ -489,9 +489,22 @@ > > gst_libcamera_configure_stream_from_caps(StreamConfiguration > > &stream_cfg, > > > > /* Configure colorimetry */ > > if (gst_structure_has_field(s, "colorimetry")) { > > - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); > > + const GValue *mode; > > + const gchar *colorimetry_str = NULL; > > GstVideoColorimetry colorimetry; > > > > + mode = gst_structure_get_value(s, "colorimetry"); Just do: gst_structure_fixate_field (s, "colorimetry"); And keep the original code. The caps implementation seems sub-optimal though. In an ideal world, we'd do this in stage: - Fixate the caps - So caps to GstVideoInfo - Fill the stream_cfg from the VideoInfo - Finally do GstVideoInfo to caps in order to push complete caps. This will fill the proper default of the other video/x-raw field we are not careful about, like stereo views, pixel-aspect ratio, etc. If you have time, this can greatly improve caps negotiation an correctness for future users. > > + > > + if (G_VALUE_HOLDS_STRING(mode)) { > > + colorimetry_str = gst_structure_get_string(s, "colorimetry"); > > + } else if (GST_VALUE_HOLDS_LIST(mode)) { > > + const GValue *first_element = > > + gst_value_list_get_value(mode, 0); > > + > > + if (G_VALUE_HOLDS_STRING(first_element)) { > > + colorimetry_str = g_value_get_string(first_element); > > + } > > + } > > + > > gst_libcamera_configure_stream_from_caps() is exepected to be called with fixed caps. Feel free to add an assert on that, fixation should happen earlier instead. This could be as simple as gst_caps_fixate(), specially that you are picking the first value in the list. > > Nicolas > > > if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) > > g_critical("Invalid colorimetry %s", > > colorimetry_str); > > >
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index 732987ef..2c1ebce8 100644 --- a/src/gstreamer/gstlibcamera-utils.cpp +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -489,9 +489,22 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, /* Configure colorimetry */ if (gst_structure_has_field(s, "colorimetry")) { - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); + const GValue *mode; + const gchar *colorimetry_str = NULL; GstVideoColorimetry colorimetry; + mode = gst_structure_get_value(s, "colorimetry"); + + if (G_VALUE_HOLDS_STRING(mode)) { + colorimetry_str = gst_structure_get_string(s, "colorimetry"); + } else if (GST_VALUE_HOLDS_LIST(mode)) { + const GValue *first_element = gst_value_list_get_value(mode, 0); + + if (G_VALUE_HOLDS_STRING(first_element)) { + colorimetry_str = g_value_get_string(first_element); + } + } + if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) g_critical("Invalid colorimetry %s", colorimetry_str);
When libcamerasrc is negotiating with downstream element, it first extracts colorimetry field from downstream supported caps, then set this colorimetry to its stream configuration and propagates the colorimetry downstream. Currently libamerasrc only considers the case there is one colorimetry in colorimetry field of downstream caps. But the issue is that downstream caps may report a list of supported colorimetry, which causes libcamerasrc to set unknown colorimetry to stream configuration and negotiate fail with downstream element. In order to fix the issue, get the first string if colorimetry field holds string list to set it to stream configuration. Signed-off-by: Hou Qi <qi.hou@nxp.com> --- src/gstreamer/gstlibcamera-utils.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)