After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 796801 - find_codec_preferences: use received caps
find_codec_preferences: use received caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal normal
: 1.14.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-07-12 15:57 UTC by Mathieu Duponchelle
Modified: 2018-07-18 16:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
find_codec_preferences: use received caps (1.46 KB, patch)
2018-07-12 15:57 UTC, Mathieu Duponchelle
none Details | Review
find_codec_preferences: use received caps (1.58 KB, patch)
2018-07-12 19:42 UTC, Mathieu Duponchelle
committed Details | Review

Description Mathieu Duponchelle 2018-07-12 15:57:36 UTC
When negotiation is triggered by receiving caps in our pad
probe, this addresses a race condition where gst_pad_get_current_caps
did not necessarily return the caps, as when the probe is
triggered the "current caps" have not yet been set.

Instead, as we save the caps in the probe callback anyway, it is better
and thread safe to use these if they were set.
Comment 1 Mathieu Duponchelle 2018-07-12 15:57:40 UTC
Created attachment 373009 [details] [review]
find_codec_preferences: use received caps
Comment 2 Thibault Saunier 2018-07-12 19:23:34 UTC
Review of attachment 373009 [details] [review]:

Makes sense but I think the commit message doesn't explain so well the issue :-)

::: ext/webrtc/gstwebrtcbin.c
@@ +1264,3 @@
+      if (pad->received_caps) {
+        caps = gst_caps_ref (pad->received_caps);
+      } else if ((caps = gst_pad_get_current_caps (GST_PAD (pad)))) {

Is that still needed now that you use the "cache" caps?
Comment 3 Mathieu Duponchelle 2018-07-12 19:42:05 UTC
Created attachment 373012 [details] [review]
find_codec_preferences: use received caps

When negotiation is triggered by receiving caps on our sink pad
probes, we could encounter a race condition where need-negotiation
is emitted and the application requires the creation of an offer
before the current caps were actually updated.

This led to retrieving incomplete caps when creating the offer,
using find_codec_preferences -> pad_get_current_caps.

Instead, as we save the caps in the probe callback anyway, it is better
and thread safe to use these if they were set.
Comment 4 Mathieu Duponchelle 2018-07-12 19:43:35 UTC
(In reply to Thibault Saunier from comment #2)
> Review of attachment 373009 [details] [review] [review]:
> Is that still needed now that you use the "cache" caps?

Potentially, if the user calls create-offer before need-negotiation was emitted, I do not know how legitimate that is but I'd rather we break^W address that in a separate patch :)
Comment 5 Thibault Saunier 2018-07-12 19:44:28 UTC
Review of attachment 373012 [details] [review]:

OK
Comment 6 Mathieu Duponchelle 2018-07-12 19:46:15 UTC
Attachment 373012 [details] pushed as 6fd3e2a - find_codec_preferences: use received caps