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 700222 - rtpbasepayload: Need to delay segments event after caps event
rtpbasepayload: Need to delay segments event after caps event
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-05-13 13:20 UTC by Nicolas Dufresne (ndufresne)
Modified: 2013-05-14 07:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] rtpbasepayload: Delay segment event after caps (3.24 KB, patch)
2013-05-13 13:21 UTC, Nicolas Dufresne (ndufresne)
needs-work Details | Review
[PATCH] rtpbasepayload: Delay segment event after caps (3.38 KB, patch)
2013-05-13 22:20 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
[PATCH] rtpbasepayload: Delay segment event after caps (3.77 KB, patch)
2013-05-13 22:31 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
[PATCH] rtpbasepayload: Delay segment event after caps (3.79 KB, patch)
2013-05-14 02:40 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2013-05-13 13:20:43 UTC
To avoid having to do it in all payloaders, the base payload class should delay incoming segments until caps has been set. To do so, I send the segment right before the first buffer, please review, I'm not 100% certain of this one. (patch coming)
Comment 1 Nicolas Dufresne (ndufresne) 2013-05-13 13:21:53 UTC
Created attachment 243989 [details] [review]
[PATCH] rtpbasepayload: Delay segment event after caps

 gst-libs/gst/rtp/gstrtpbasepayload.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)
Comment 2 Sebastian Dröge (slomo) 2013-05-13 13:35:35 UTC
Review of attachment 243989 [details] [review]:

::: gst-libs/gst/rtp/gstrtpbasepayload.c
@@ +1130,3 @@
       break;
     case GST_STATE_CHANGE_READY_TO_NULL:
+      gst_event_replace (&rtpbasepayload->priv->pending_segment, NULL);

Should be PAUSED_TO_READY, and also should be reset when receiving FLUSH_STOP
Comment 3 Sebastian Dröge (slomo) 2013-05-13 13:36:08 UTC
Review of attachment 243989 [details] [review]:

::: gst-libs/gst/rtp/gstrtpbasepayload.c
@@ +1130,3 @@
       break;
     case GST_STATE_CHANGE_READY_TO_NULL:
+      gst_event_replace (&rtpbasepayload->priv->pending_segment, NULL);

Should be PAUSED_TO_READY, and also should be reset when receiving FLUSH_STOP
Comment 4 Olivier Crête 2013-05-13 14:22:25 UTC
Any reason to not just push the segment at the end of set_outcaps instead ?
Comment 5 Nicolas Dufresne (ndufresne) 2013-05-13 14:46:20 UTC
Some depayloader seems not to use it, I ended up not sending the segment, but I could recheck.
Comment 6 Nicolas Dufresne (ndufresne) 2013-05-13 22:20:26 UTC
Created attachment 244117 [details] [review]
[PATCH] rtpbasepayload: Delay segment event after caps


https://bugzilla.gnome.org/show_bug.cgi?id=700222
---
 gst-libs/gst/rtp/gstrtpbasepayload.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)
Comment 7 Nicolas Dufresne (ndufresne) 2013-05-13 22:25:52 UTC
Oh, I forgot the flush stop
Comment 8 Nicolas Dufresne (ndufresne) 2013-05-13 22:31:42 UTC
Created attachment 244121 [details] [review]
[PATCH] rtpbasepayload: Delay segment event after caps


https://bugzilla.gnome.org/show_bug.cgi?id=700222
---
 gst-libs/gst/rtp/gstrtpbasepayload.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)
Comment 9 Olivier Crête 2013-05-13 23:16:27 UTC
Review of attachment 244121 [details] [review]:

I must say I'm not happy having to force the order like that everywhere.. I still feel like the core should be doing this.

::: gst-libs/gst/rtp/gstrtpbasepayload.c
@@ +921,3 @@
 
+  if (G_LIKELY (res == GST_FLOW_OK)) {
+    if (payload->priv->pending_segment) {

might want a G_UNLIKELY() here

@@ +954,3 @@
 
+  if (G_LIKELY (res == GST_FLOW_OK)) {
+    if (payload->priv->pending_segment) {

and here too
Comment 10 Nicolas Dufresne (ndufresne) 2013-05-14 02:40:22 UTC
Created attachment 244129 [details] [review]
[PATCH] rtpbasepayload: Delay segment event after caps


https://bugzilla.gnome.org/show_bug.cgi?id=700222
---
 gst-libs/gst/rtp/gstrtpbasepayload.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)
Comment 11 Sebastian Dröge (slomo) 2013-05-14 07:49:31 UTC
(In reply to comment #9)
> Review of attachment 244121 [details] [review]:
> 
> I must say I'm not happy having to force the order like that everywhere.. I
> still feel like the core should be doing this.

core can't do that for you, it's the elements that are producing the events and should make sure to send everything in the correct order.
Comment 12 Sebastian Dröge (slomo) 2013-05-14 07:54:32 UTC
commit 94b7ae776799a11351760752dc95853b08bb19d6
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Sun May 12 09:55:38 2013 -0400

    rtpbasepayload: Delay segment event after caps
    
    https://bugzilla.gnome.org/show_bug.cgi?id=700222