[v3] gstreamer: Reconfigure when peer caps gain extra fields
diff mbox series

Message ID 20260818094507.4082940-1-elliot.chen@oss.nxp.com
State Accepted
Headers show
Series
  • [v3] gstreamer: Reconfigure when peer caps gain extra fields
Related show

Commit Message

Elliot Chen (OSS) Aug. 18, 2026, 9:45 a.m. UTC 
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>

---
Changes in v3:
- rename caps structure

Changes in v2:
- remove redundant checks both for caps and peercaps

Changes in v1:
- Initial submission
---
 src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Jacopo Mondi Aug. 18, 2026, 9:52 a.m. UTC | #1 
Hi Elliot,

On Tue, Aug 18, 2026 at 06:45:07PM +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.
>

Don't worry, I'll add
Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/344

When applying

> Signed-off-by: Elliot Chen <elliot.chen@nxp.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Nicolas: your tag was dropped, should I add it back when applying ?

Thanks
  j

>
> ---
> Changes in v3:
> - rename caps structure
>
> Changes in v2:
> - remove redundant checks both for caps and peercaps
>
> Changes in v1:
> - Initial submission
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 9061f9163..9541aa3f6 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 *caps0 = gst_caps_get_structure(caps, 0);
> +			const GstStructure *peercaps0 = gst_caps_get_structure(peercaps, 0);
> +			if (!gst_structure_is_equal(caps0, peercaps0)) {
> +				reconfigure = true;
> +				break;
> +			}
>  		}
>  	}
>
> --
> 2.34.1
>
Elliot Chen (OSS) Aug. 18, 2026, 9:55 a.m. UTC | #2 
Hi Jacopo,
Thank you very much.


NXP Confidential
-----Original Message-----
From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Sent: Tuesday, August 18, 2026 5:52 PM
To: Elliot Chen (OSS) <elliot.chen@oss.nxp.com>; nicolas@ndufresne.ca
Cc: libcamera-devel@lists.libcamera.org; Elliot Chen <elliot.chen@nxp.com>
Subject: Re: [PATCH v3] gstreamer: Reconfigure when peer caps gain extra fields

[You don't often get email from jacopo.mondi@ideasonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

Hi Elliot,

On Tue, Aug 18, 2026 at 06:45:07PM +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.
>

Don't worry, I'll add
Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/344

When applying

> Signed-off-by: Elliot Chen <elliot.chen@nxp.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Nicolas: your tag was dropped, should I add it back when applying ?

Thanks
  j

>
> ---
> Changes in v3:
> - rename caps structure
>
> Changes in v2:
> - remove redundant checks both for caps and peercaps
>
> Changes in v1:
> - Initial submission
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> b/src/gstreamer/gstlibcamerasrc.cpp
> index 9061f9163..9541aa3f6 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 *caps0 = gst_caps_get_structure(caps, 0);
> +                     const GstStructure *peercaps0 = gst_caps_get_structure(peercaps, 0);
> +                     if (!gst_structure_is_equal(caps0, peercaps0)) {
> +                             reconfigure = true;
> +                             break;
> +                     }
>               }
>       }
>
> --
> 2.34.1
>
Nicolas Dufresne Aug. 20, 2026, 8:12 p.m. UTC | #3 
Hi,

Le mardi 18 août 2026 à 18:45 +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.


To bring everyone ont he same page. In this scenarios we already negotiated our
caps. But the reconfigure flag has been raised a pad, the pad raise it when it
receives reconfigure event.

Since we don't want to do a full reconfiguration everytime, which requires to
stop and start the camera, we currently check if the previously negotiated caps
intersect with the peer modified capabilities.

  # Get the currently configured caps, these are single structure, fixed
  caps = gst_pad_get_current_caps(srcpad);

  # Query downstream caps, passing our current caps as filter
  g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(pad: srcpad, filter: caps); 


At this point, what the current code does is that if the result is empty, it
means the current caps are not compatible with downstream. And this is always
true due to that fact the a missing feild imply supporting any value of this
field. The following return is entirely possible:

  current:  video/x-raw,width=1,height=1,format=x
  peercaps: video/x-raw,width=1,height=1,format=x,colorimetry=bt709


Its a very niche case, but this is what Elliot is trying to catch. Its not a
nice use case either, since the first negotiation had undefined colorimetry,
which results depends on the guessing of downstream element. For that reason, if
this is really the main reason for this patch, I would look into making sure we
always negotiate a fixed colorimetry. Having it fixed, if we didn't pick one
supported downstream before renegotation, then the return filter will be empty
since the two will no longer intersect.

> 
> Signed-off-by: Elliot Chen <elliot.chen@nxp.com>
> 
> ---
> Changes in v3:
> - rename caps structure
> 
> Changes in v2:
> - remove redundant checks both for caps and peercaps
> 
> Changes in v1:
> - Initial submission
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 9061f9163..9541aa3f6 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 *caps0 = gst_caps_get_structure(caps, 0);
> +			const GstStructure *peercaps0 = gst_caps_get_structure(peercaps, 0);
> +			if (!gst_structure_is_equal(caps0, peercaps0)) {
> +				reconfigure = true;
> +				break;
> +			}

So while I don't think this is fundamentally broken, its a bit off to me. We
should address undefined behaviors at their source first.

Not-Accepted

Nicolas

>  		}
>  	}
>
Elliot Chen (OSS) Aug. 21, 2026, 6:31 a.m. UTC | #4 
Hi Nicolas Dufresne,
1. For this patch itself, it ensures that the first caps in the new queried caps must be equal to the current one and should trigger caps negotiation again if not. The current caps may in the queried caps again but not the first one.
    It's not just check whether the new queried caps gain extra field. I'll modify the comment about it.
2. For many common cases, it will not add colorimetry field in the caps explicitly except v4l2 encoder. This behavior is no related to one plugin.

Some Examples without colorimetry field in the caps :
1. gst-launch-1.0 videotestsrc ! video/x-raw,format=RGBA ! waylandsink -v
2. gst-launch-1.0 videotestsrc ! video/x-raw,format=RGBA ! fakesink -v
3. gst-launch-1.0 libcamerasrc ! fakesink -v
4. gst-launch-1.0 libcamerasrc ! filesink location=test.bin -v


NXP Confidential
-----Original Message-----
From: Nicolas Dufresne <nicolas@ndufresne.ca>
Sent: Friday, August 21, 2026 4:13 AM
To: Elliot Chen (OSS) <elliot.chen@oss.nxp.com>; libcamera-devel@lists.libcamera.org
Cc: Elliot Chen <elliot.chen@nxp.com>
Subject: Re: [PATCH v3] gstreamer: Reconfigure when peer caps gain extra fields

Hi,

Le mardi 18 août 2026 à 18:45 +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.


To bring everyone ont he same page. In this scenarios we already negotiated our caps. But the reconfigure flag has been raised a pad, the pad raise it when it receives reconfigure event.

Since we don't want to do a full reconfiguration everytime, which requires to stop and start the camera, we currently check if the previously negotiated caps intersect with the peer modified capabilities.

  # Get the currently configured caps, these are single structure, fixed
  caps = gst_pad_get_current_caps(srcpad);

  # Query downstream caps, passing our current caps as filter
  g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(pad: srcpad, filter: caps);


At this point, what the current code does is that if the result is empty, it means the current caps are not compatible with downstream. And this is always true due to that fact the a missing feild imply supporting any value of this field. The following return is entirely possible:

  current:  video/x-raw,width=1,height=1,format=x
  peercaps: video/x-raw,width=1,height=1,format=x,colorimetry=bt709


Its a very niche case, but this is what Elliot is trying to catch. Its not a nice use case either, since the first negotiation had undefined colorimetry, which results depends on the guessing of downstream element. For that reason, if this is really the main reason for this patch, I would look into making sure we always negotiate a fixed colorimetry. Having it fixed, if we didn't pick one supported downstream before renegotation, then the return filter will be empty since the two will no longer intersect.

>
> Signed-off-by: Elliot Chen <elliot.chen@nxp.com>
>
> ---
> Changes in v3:
> - rename caps structure
>
> Changes in v2:
> - remove redundant checks both for caps and peercaps
>
> Changes in v1:
> - Initial submission
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> b/src/gstreamer/gstlibcamerasrc.cpp
> index 9061f9163..9541aa3f6 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 *caps0 = gst_caps_get_structure(caps, 0);
> +                     const GstStructure *peercaps0 = gst_caps_get_structure(peercaps, 0);
> +                     if (!gst_structure_is_equal(caps0, peercaps0)) {
> +                             reconfigure = true;
> +                             break;
> +                     }

So while I don't think this is fundamentally broken, its a bit off to me. We should address undefined behaviors at their source first.

Not-Accepted

Nicolas

>               }
>       }
>
Jacopo Mondi Aug. 21, 2026, 7:28 a.m. UTC | #5 
On Thu, Aug 20, 2026 at 04:12:59PM -0400, Nicolas Dufresne wrote:
> Hi,
>
> Le mardi 18 août 2026 à 18:45 +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.
>
>
> To bring everyone ont he same page. In this scenarios we already negotiated our
> caps. But the reconfigure flag has been raised a pad, the pad raise it when it
> receives reconfigure event.
>
> Since we don't want to do a full reconfiguration everytime, which requires to
> stop and start the camera, we currently check if the previously negotiated caps
> intersect with the peer modified capabilities.
>
>   # Get the currently configured caps, these are single structure, fixed
>   caps = gst_pad_get_current_caps(srcpad);
>
>   # Query downstream caps, passing our current caps as filter
>   g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(pad: srcpad, filter: caps);
>
>
> At this point, what the current code does is that if the result is empty, it
> means the current caps are not compatible with downstream. And this is always
> true due to that fact the a missing feild imply supporting any value of this
> field. The following return is entirely possible:
>
>   current:  video/x-raw,width=1,height=1,format=x
>   peercaps: video/x-raw,width=1,height=1,format=x,colorimetry=bt709
>
>
> Its a very niche case, but this is what Elliot is trying to catch. Its not a
> nice use case either, since the first negotiation had undefined colorimetry,
> which results depends on the guessing of downstream element. For that reason, if
> this is really the main reason for this patch, I would look into making sure we
> always negotiate a fixed colorimetry. Having it fixed, if we didn't pick one
> supported downstream before renegotation, then the return filter will be empty
> since the two will no longer intersect.
>
> >
> > Signed-off-by: Elliot Chen <elliot.chen@nxp.com>
> >
> > ---
> > Changes in v3:
> > - rename caps structure
> >
> > Changes in v2:
> > - remove redundant checks both for caps and peercaps
> >
> > Changes in v1:
> > - Initial submission
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 9061f9163..9541aa3f6 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 *caps0 = gst_caps_get_structure(caps, 0);
> > +			const GstStructure *peercaps0 = gst_caps_get_structure(peercaps, 0);
> > +			if (!gst_structure_is_equal(caps0, peercaps0)) {
> > +				reconfigure = true;
> > +				break;
> > +			}
>
> So while I don't think this is fundamentally broken, its a bit off to me. We
> should address undefined behaviors at their source first.
>
> Not-Accepted
>

I already merged it yesterday unfortunately.

What would you like me to do here ? revert or can this be worked on
top ?

> Nicolas
>
> >  		}
> >  	}
> >
Nicolas Dufresne Aug. 26, 2026, 3:04 p.m. UTC | #6 
Le vendredi 21 août 2026 à 06:31 +0000, Elliot Chen (OSS) a écrit :
> Hi Nicolas Dufresne,
> 1. For this patch itself, it ensures that the first caps in the new queried caps must be equal to the current one and should trigger caps negotiation again if not. The current caps may in the queried caps again but not the first one.
>     It's not just check whether the new queried caps gain extra field. I'll modify the comment about it.
> 2. For many common cases, it will not add colorimetry field in the caps explicitly except v4l2 encoder. This behavior is no related to one plugin.
> 
> Some Examples without colorimetry field in the caps :
> 1. gst-launch-1.0 videotestsrc ! video/x-raw,format=RGBA ! waylandsink -v
> 2. gst-launch-1.0 videotestsrc ! video/x-raw,format=RGBA ! fakesink -v
> 3. gst-launch-1.0 libcamerasrc ! fakesink -v
> 4. gst-launch-1.0 libcamerasrc ! filesink location=test.bin -v

But this is bugs, not feature. The expected principle that allow adding field is
not being respected. If a source makes any decision or have knowledge around a
field, it should explicitly add it, regardless if downstream depends on it or
not.

waylandsink is a good example, it does not require colorimetry for backward
compatibility reason, but the rendering behaviour is undefined if you don't
specify it. I'll follow up fixing the lack of colorimetry produced by
videotestsrc caps.

> 
> 
> NXP Confidential

driver by: this is probably accidental footer, but it has no place on public
mailing list.

> -----Original Message-----
> From: Nicolas Dufresne <nicolas@ndufresne.ca>
> Sent: Friday, August 21, 2026 4:13 AM
> To: Elliot Chen (OSS) <elliot.chen@oss.nxp.com>; libcamera-devel@lists.libcamera.org
> Cc: Elliot Chen <elliot.chen@nxp.com>
> Subject: Re: [PATCH v3] gstreamer: Reconfigure when peer caps gain extra fields
> 
> Hi,
> 
> Le mardi 18 août 2026 à 18:45 +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.
> 
> 
> To bring everyone ont he same page. In this scenarios we already negotiated our caps. But the reconfigure flag has been raised a pad, the pad raise it when it receives reconfigure event.
> 
> Since we don't want to do a full reconfiguration everytime, which requires to stop and start the camera, we currently check if the previously negotiated caps intersect with the peer modified capabilities.
> 
>   # Get the currently configured caps, these are single structure, fixed
>   caps = gst_pad_get_current_caps(srcpad);
> 
>   # Query downstream caps, passing our current caps as filter
>   g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(pad: srcpad, filter: caps);
> 
> 
> At this point, what the current code does is that if the result is empty, it means the current caps are not compatible with downstream. And this is always true due to that fact the a missing feild imply supporting any value of this field. The following return is entirely possible:
> 
>   current:  video/x-raw,width=1,height=1,format=x
>   peercaps: video/x-raw,width=1,height=1,format=x,colorimetry=bt709
> 
> 
> Its a very niche case, but this is what Elliot is trying to catch. Its not a nice use case either, since the first negotiation had undefined colorimetry, which results depends on the guessing of downstream element. For that reason, if this is really the main reason for this patch, I would look into making sure we always negotiate a fixed colorimetry. Having it fixed, if we didn't pick one supported downstream before renegotation, then the return filter will be empty since the two will no longer intersect.
> 

Since Jacopo merged your patch, I'll looking forward a fix on top that ensure
relevant field are being fixated (and added) where needed.

Nicolas

> > 
> > Signed-off-by: Elliot Chen <elliot.chen@nxp.com>
> > 
> > ---
> > Changes in v3:
> > - rename caps structure
> > 
> > Changes in v2:
> > - remove redundant checks both for caps and peercaps
> > 
> > Changes in v1:
> > - Initial submission
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> > b/src/gstreamer/gstlibcamerasrc.cpp
> > index 9061f9163..9541aa3f6 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 *caps0 = gst_caps_get_structure(caps, 0);
> > +                     const GstStructure *peercaps0 = gst_caps_get_structure(peercaps, 0);
> > +                     if (!gst_structure_is_equal(caps0, peercaps0)) {
> > +                             reconfigure = true;
> > +                             break;
> > +                     }
> 
> So while I don't think this is fundamentally broken, its a bit off to me. We should address undefined behaviors at their source first.
> 
> Not-Accepted
> 
> Nicolas
> 
> >               }
> >       }
> >

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index 9061f9163..9541aa3f6 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 *caps0 = gst_caps_get_structure(caps, 0);
+			const GstStructure *peercaps0 = gst_caps_get_structure(peercaps, 0);
+			if (!gst_structure_is_equal(caps0, peercaps0)) {
+				reconfigure = true;
+				break;
+			}
 		}
 	}