| Message ID | 20260728054311.482231-1-elliot.chen@oss.nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Add a description about the issue that this patch resolves.
1. There is one issue if using libcamerasrc as a camera source in camerabin when changing from preview status to video recording mode. The fps drops a lot when starting recording with camerabin which is caused by libcamerasrc.
2. In preview status, there is no colorimetry field when libcamerasrc outputs the caps. When starting recording, v4l2 encoder will append colorimetry field to the caps.
Currently, libcamerasrc only handles empty peer caps and will not reconfigure to perform caps negotiation for above cases. Then videoconvert will perform colorimetry conversion which can trigger fps drop. The simplified pipeline is as below.
3. Should obtain the first caps in the queried caps information and compare it and the original caps.
+---------------------------------------------+
| | -----> + ... + preview sink
| libcamerasrc + videoconvert +... | -----> + ... + v4l2 encoder + ... + filesink
| | -----> + ... + image encoder + ... + filesink
+---------------------------------------------+
-----Original Message-----
From: Elliot Chen (OSS) <elliot.chen@oss.nxp.com>
Sent: Tuesday, July 28, 2026 1:43 PM
To: libcamera-devel@lists.libcamera.org; Elliot Chen <elliot.chen@nxp.com>
Subject: [PATCH] gstreamer: Reconfigure when peer caps gain extra fields
From: Elliot Chen <elliot.chen@nxp.com>
After initial negotiation, a downstream element may append extra fields to the caps (e.g. colorimetry). The existing check only handles empty peer caps. Add a structure comparison to detect such changes and trigger reconfiguration with caps renegotiation.
Signed-off-by: Elliot Chen <elliot.chen@nxp.com>
---
src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index 9061f9163..88717b048 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -744,6 +744,18 @@ gst_libcamera_src_task_run(gpointer user_data)
reconfigure = true;
break;
}
+
+ /*
+ * Caps may gain extra fields after the initial negotiation (e.g.
+ * colorimetry added by a downstream element). Detect such changes
+ * and trigger reconfiguration.
+ */
+ const GstStructure *s = caps ? gst_caps_get_structure(caps, 0) : nullptr;
+ const GstStructure *peer_s = peercaps ? gst_caps_get_structure(peercaps, 0) : nullptr;
+ if (!s || !peer_s || !gst_structure_is_equal(s, peer_s)) {
+ reconfigure = true;
+ break;
+ }
}
}
--
2.34.1
Hi, Le mardi 28 juillet 2026 à 14:43 +0900, elliot.chen@oss.nxp.com a écrit : > From: Elliot Chen <elliot.chen@nxp.com> > > After initial negotiation, a downstream element may append extra > fields to the caps (e.g. colorimetry). The existing check only > handles empty peer caps. Add a structure comparison to detect such > changes and trigger reconfiguration with caps renegotiation. > > Signed-off-by: Elliot Chen <elliot.chen@nxp.com> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 9061f9163..88717b048 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -744,6 +744,18 @@ gst_libcamera_src_task_run(gpointer user_data) > reconfigure = true; > break; > } > + > + /* > + * Caps may gain extra fields after the initial negotiation (e.g. > + * colorimetry added by a downstream element). Detect such changes > + * and trigger reconfiguration. > + */ > + const GstStructure *s = caps ? gst_caps_get_structure(caps, 0) : nullptr; > + const GstStructure *peer_s = peercaps ? gst_caps_get_structure(peercaps, 0) : nullptr; > + if (!s || !peer_s || !gst_structure_is_equal(s, peer_s)) { > + reconfigure = true; > + break; > + } > } > } >
Hi Elliot On Tue, Jul 28, 2026 at 02:43:11PM +0900, elliot.chen@oss.nxp.com wrote: > From: Elliot Chen <elliot.chen@nxp.com> > > After initial negotiation, a downstream element may append extra > fields to the caps (e.g. colorimetry). The existing check only > handles empty peer caps. Add a structure comparison to detect such > changes and trigger reconfiguration with caps renegotiation. > > Signed-off-by: Elliot Chen <elliot.chen@nxp.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 9061f9163..88717b048 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -744,6 +744,18 @@ gst_libcamera_src_task_run(gpointer user_data) > reconfigure = true; > break; > } > + > + /* > + * Caps may gain extra fields after the initial negotiation (e.g. > + * colorimetry added by a downstream element). Detect such changes > + * and trigger reconfiguration. > + */ > + const GstStructure *s = caps ? gst_caps_get_structure(caps, 0) : nullptr; > + const GstStructure *peer_s = peercaps ? gst_caps_get_structure(peercaps, 0) : nullptr; > + if (!s || !peer_s || !gst_structure_is_equal(s, peer_s)) { What is the case for '*s' and '*peer_s' to be null ? The above code seems to assume they're valid ? /* Check if the caps even need changing. */ g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad); g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(srcpad, caps); if (gst_caps_is_empty(peercaps)) { reconfigure = true; break; } Thanks j > + reconfigure = true; > + break; > + } > } > } > > -- > 2.34.1 >
Le lundi 17 août 2026 à 12:52 +0200, Jacopo Mondi a écrit : > Hi Elliot > > On Tue, Jul 28, 2026 at 02:43:11PM +0900, elliot.chen@oss.nxp.com wrote: > > From: Elliot Chen <elliot.chen@nxp.com> > > > > After initial negotiation, a downstream element may append extra > > fields to the caps (e.g. colorimetry). The existing check only > > handles empty peer caps. Add a structure comparison to detect such > > changes and trigger reconfiguration with caps renegotiation. > > > > Signed-off-by: Elliot Chen <elliot.chen@nxp.com> > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 9061f9163..88717b048 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -744,6 +744,18 @@ gst_libcamera_src_task_run(gpointer user_data) > > reconfigure = true; > > break; > > } > > + > > + /* > > + * Caps may gain extra fields after the initial negotiation (e.g. > > + * colorimetry added by a downstream element). Detect such changes > > + * and trigger reconfiguration. > > + */ > > + const GstStructure *s = caps ? gst_caps_get_structure(caps, 0) : nullptr; > > + const GstStructure *peer_s = peercaps ? gst_caps_get_structure(peercaps, 0) : nullptr; > > + if (!s || !peer_s || !gst_structure_is_equal(s, peer_s)) { > > What is the case for '*s' and '*peer_s' to be null ? Missed that, since we never enter task_run() without a successful negotiation, you are right that the check for null caps is spurious. Then I checked that peercaps, the function gst_pad_query_caps() simply never return null. So both null checks can be remove, thanks for catching this. Nicolas > > The above code seems to assume they're valid ? > > /* Check if the caps even need changing. */ > g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad); > g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(srcpad, caps); > if (gst_caps_is_empty(peercaps)) { > reconfigure = true; > break; > } > > Thanks > j > > > + reconfigure = true; > > + break; > > + } > > } > > } > > > > -- > > 2.34.1 > >
Hi Nicolas Dufresne and Jacopo Mondi, Yes. Thanks. I've sent v2 of this patch series. Please find it at: [PATCH v2] https://patchwork.libcamera.org/patch/27835/ Sorry for not threading it properly. Changes in v2: - remove redundant checks both for caps and peercaps NXP Confidential -----Original Message----- From: Nicolas Dufresne <nicolas@ndufresne.ca> Sent: Monday, August 17, 2026 9:12 PM To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>; Elliot Chen (OSS) <elliot.chen@oss.nxp.com> Cc: libcamera-devel@lists.libcamera.org; Elliot Chen <elliot.chen@nxp.com> Subject: Re: [PATCH] gstreamer: Reconfigure when peer caps gain extra fields Le lundi 17 août 2026 à 12:52 +0200, Jacopo Mondi a écrit : > Hi Elliot > > On Tue, Jul 28, 2026 at 02:43:11PM +0900, elliot.chen@oss.nxp.com wrote: > > From: Elliot Chen <elliot.chen@nxp.com> > > > > After initial negotiation, a downstream element may append extra > > fields to the caps (e.g. colorimetry). The existing check only > > handles empty peer caps. Add a structure comparison to detect such > > changes and trigger reconfiguration with caps renegotiation. > > > > Signed-off-by: Elliot Chen <elliot.chen@nxp.com> > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > > b/src/gstreamer/gstlibcamerasrc.cpp > > index 9061f9163..88717b048 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -744,6 +744,18 @@ gst_libcamera_src_task_run(gpointer user_data) > > reconfigure = true; > > break; > > } > > + > > + /* > > + * Caps may gain extra fields after the initial negotiation (e.g. > > + * colorimetry added by a downstream element). Detect such changes > > + * and trigger reconfiguration. > > + */ > > + const GstStructure *s = caps ? gst_caps_get_structure(caps, 0) : nullptr; > > + const GstStructure *peer_s = peercaps ? gst_caps_get_structure(peercaps, 0) : nullptr; > > + if (!s || !peer_s || !gst_structure_is_equal(s, peer_s)) { > > What is the case for '*s' and '*peer_s' to be null ? Missed that, since we never enter task_run() without a successful negotiation, you are right that the check for null caps is spurious. Then I checked that peercaps, the function gst_pad_query_caps() simply never return null. So both null checks can be remove, thanks for catching this. Nicolas > > The above code seems to assume they're valid ? > > /* Check if the caps even need changing. */ > g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad); > g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(srcpad, caps); > if (gst_caps_is_empty(peercaps)) { > reconfigure = true; > break; > } > > Thanks > j > > > + reconfigure = true; > > + break; > > + } > > } > > } > > > > -- > > 2.34.1 > >
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 9061f9163..88717b048 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -744,6 +744,18 @@ gst_libcamera_src_task_run(gpointer user_data) reconfigure = true; break; } + + /* + * Caps may gain extra fields after the initial negotiation (e.g. + * colorimetry added by a downstream element). Detect such changes + * and trigger reconfiguration. + */ + const GstStructure *s = caps ? gst_caps_get_structure(caps, 0) : nullptr; + const GstStructure *peer_s = peercaps ? gst_caps_get_structure(peercaps, 0) : nullptr; + if (!s || !peer_s || !gst_structure_is_equal(s, peer_s)) { + reconfigure = true; + break; + } } }