Created attachment 323435 [details] [review]
funnel: add 'forward-sticky-events-mode' property

Review of attachment 323435 [details] [review]:

::: plugins/elements/gstfunnel.c
@@ +118,3 @@
+  GstFunnelPad *pad = GST_FUNNEL_PAD_CAST (gobject);
+
+  g_slist_free_full (pad->events_sent, (GDestroyNotify) gst_event_unref);

These should be cleared in PAUSED->READY already

@@ +369,3 @@
+{
+  GstFunnelPad *funnelpad = GST_FUNNEL_PAD_CAST (sinkpad);
+  gboolean ret = FALSE;

You probably want to return TRUE if mode==NEVER

Created attachment 323479 [details] [review]
funnel: add 'forward-sticky-events-mode' property

Review of attachment 323435 [details] [review]:

::: plugins/elements/gstfunnel.c
@@ +118,3 @@
+  GstFunnelPad *pad = GST_FUNNEL_PAD_CAST (gobject);
+
+  g_slist_free_full (pad->events_sent, (GDestroyNotify) gst_event_unref);

Done

@@ +369,3 @@
+{
+  GstFunnelPad *funnelpad = GST_FUNNEL_PAD_CAST (sinkpad);
+  gboolean ret = FALSE;

Done

Review of attachment 323479 [details] [review]:

::: plugins/elements/gstfunnel.c
@@ +67,3 @@
+
+  if (!mode_type) {
+    mode_type = g_enum_register_static ("FunnelForwardStickyEventsMode", modes);

Prefix the type name with Gst

@@ +117,2 @@
 static void
+gst_funneL_pad_finalize (GObject * gobject)

Maybe make that a lowercase L ;)

@@ +120,3 @@
+  GstFunnelPad *pad = GST_FUNNEL_PAD_CAST (gobject);
+
+  g_slist_free_full (pad->events_sent, (GDestroyNotify) gst_event_unref);

This should also be completely unneeded now. Or not?

@@ +176,3 @@
     case PROP_FORWARD_STICKY_EVENTS:
+      GST_WARNING_OBJECT (funnel,
+          "'forward-stick-events' deprecated: use 'forward-stick-events-mode'");

You can let the property select ONCE / ALWAYS, right?

@@ +196,3 @@
     case PROP_FORWARD_STICKY_EVENTS:
+      GST_WARNING_OBJECT (funnel,
+          "'forward-stick-events' deprecated: use 'forward-stick-events-mode'");

It should put *something* into the GValue at least, ideally you can translate the new field.

Review of attachment 323479 [details] [review]:

::: plugins/elements/gstfunnel.c
@@ +67,3 @@
+
+  if (!mode_type) {
+    mode_type = g_enum_register_static ("FunnelForwardStickyEventsMode", modes);

Done

@@ +117,2 @@
 static void
+gst_funneL_pad_finalize (GObject * gobject)

What a fool mistake!!
Well seen ;)

Done

@@ +120,3 @@
+  GstFunnelPad *pad = GST_FUNNEL_PAD_CAST (gobject);
+
+  g_slist_free_full (pad->events_sent, (GDestroyNotify) gst_event_unref);

This is needed because pads can be removed without the elements goes to READY state.

@@ +176,3 @@
     case PROP_FORWARD_STICKY_EVENTS:
+      GST_WARNING_OBJECT (funnel,
+          "'forward-stick-events' deprecated: use 'forward-stick-events-mode'");

Done

@@ +196,3 @@
     case PROP_FORWARD_STICKY_EVENTS:
+      GST_WARNING_OBJECT (funnel,
+          "'forward-stick-events' deprecated: use 'forward-stick-events-mode'");

Done

Created attachment 323494 [details] [review]
funnel: add 'forward-sticky-events-mode' property

Review of attachment 323494 [details] [review]:

::: plugins/elements/gstfunnel.c
@@ +121,3 @@
+  GstFunnelPad *pad = GST_FUNNEL_PAD_CAST (gobject);
+
+  g_slist_free_full (pad->events_sent, (GDestroyNotify) gst_event_unref);

Ok, if it's needed please chain up to the parent class' finalize here :) Otherwise you leak memory

@@ +183,3 @@
+      funnel->forward_sticky_events_mode =
+          v ? GST_FUNNEL_FORWARD_STICKY_EVENTS_MODE_ALWAYS :
+          GST_FUNNEL_FORWARD_STICKY_EVENTS_MODE_NEVER;

Wasn't the old mode the same as ALWAYS / ONCE?

@@ +396,3 @@
+    ret = gst_pad_push_event (funnel->srcpad, gst_event_ref (event));
+    funnelpad->events_sent =
+        g_slist_append (funnelpad->events_sent, gst_event_ref (event));

Appending to a GSList/GList is expensive. You don't care about order (or do you?), so just prepend. Or use a GQueue

Review of attachment 323494 [details] [review]:

::: plugins/elements/gstfunnel.c
@@ +121,3 @@
+  GstFunnelPad *pad = GST_FUNNEL_PAD_CAST (gobject);
+
+  g_slist_free_full (pad->events_sent, (GDestroyNotify) gst_event_unref);

Done

@@ +183,3 @@
+      funnel->forward_sticky_events_mode =
+          v ? GST_FUNNEL_FORWARD_STICKY_EVENTS_MODE_ALWAYS :
+          GST_FUNNEL_FORWARD_STICKY_EVENTS_MODE_NEVER;

Done

@@ +396,3 @@
+    ret = gst_pad_push_event (funnel->srcpad, gst_event_ref (event));
+    funnelpad->events_sent =
+        g_slist_append (funnelpad->events_sent, gst_event_ref (event));

Done

Created attachment 323498 [details] [review]
funnel: add 'forward-sticky-events-mode' property

Created attachment 323649 [details] [review]
funnel: add 'forward-sticky-events-mode' property

The previous patch does not work properly and it changes the semantic of "on stream changes"

Created attachment 323650 [details] [review]
funnel: add 'forward-sticky-events-mode' property

The patch accepted to be committed after freeze does not work properly and it changes the semantic of "on stream changes"

What about a unit test then? :)

Yes, I should do an unit test.
When I have a slot of time I will do it ;).

Created attachment 324189 [details] [review]
funnel: add 'forward-sticky-events-mode' property

Created attachment 324244 [details] [review]
funnel: add 'forward-sticky-events-mode' property

Review of attachment 324244 [details] [review]:

I'm not sure how "ONCE" mode can ever be correct? If it sends them once, maybe it should merge things like the caps, so create a caps that intersects all incoming caps, and somehow merge the segments also, etc. and then have a MERGE mode?

::: plugins/elements/gstfunnel.c
@@ +181,3 @@
+
+      GST_WARNING_OBJECT (funnel,
+          "'forward-stick-events' deprecated: use 'forward-stick-events-mode'");

s/stick/sticky/

@@ +206,3 @@
+    case PROP_FORWARD_STICKY_EVENTS:{
+      GST_WARNING_OBJECT (funnel,
+          "'forward-stick-events' deprecated: use 'forward-stick-events-mode'");

s/stick/sticky/

(In reply to Olivier Crête from comment #17)
> Review of attachment 324244 [details] [review] [review]:
> 
> I'm not sure how "ONCE" mode can ever be correct? If it sends them once,
> maybe it should merge things like the caps, so create a caps that intersects
> all incoming caps, and somehow merge the segments also, etc. and then have a
> MERGE mode?

Intersecting all the incoming caps will lead to empty caps unless they are all the same (or some are just missing fields) because caps are always fixed.

I think the use case here is application/x-rtp vs application/x-rtcp, where you don't want to switch all the time?

Good point, maybe drop everything except for the stream start and a segment event and retimestamp to running time?

Hello,
in our case we are using the funnel of the DtlsSrtpEnc bin and another for sending multiple RTP streams using an unique RtpSession.
In the last case we need that the funnel propagates the caps events in order to configure the internal RTPSources of the RtpSession.

The dtlssrtpenc should just produce application/x-dtls and ignore incoming caps, as it will be a mix of DTLS and SRTP? And pretty much useless to any other element than a dtlssrtpdec.

Don't the internal RTPSources of the rtpsession on the send side read the content of the packets instead of the caps for the SSRC? And you get the clock rate from the signal?

@oliver,the clock-rate is only got from the request_pt_map signal for the RECV side. See the [stack_trace_1]

But for the SEND side the clock-rate is got from the caps of the send_rtp_sink pad and if they are not set we can see this warning log:
WARN rtpsource rtpsource.c:1501:rtp_source_get_new_sr: no clock-rate, cannot interpolate rtp time. See the [stack_trace_2]

I tried to do a patch for getting the clock-rate using the request_pt_map signal, but I couldn't because I didn't find a way of getting the payload number in the rtp_source_get_new_sr function.

Refs:
[stack_trace_1]
gst_rtp_session_chain_recv_rtp (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/gstrtpsession.c#L1888)
rtp_session_process_rtp (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/rtpsession.c#L2099)
rtp_source_process_rtp (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/rtpsource.c#L1193)
calculate_jitter (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/rtpsource.c#L924)
get_clock_rate (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/rtpsource.c#L890)
source_clock_rate (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/rtpsession.c#L1385)
gst_rtp_session_clock_rate (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/gstrtpsession.c#L1588)
gst_rtp_session_get_caps_for_pt (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/gstrtpsession.c#L1535)

[stack_trace_2]
rtp_source_update_caps (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/rtpsource.c#L787)
rtp_session_update_send_caps (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/rtpsession.c#L2861)
gst_rtp_session_setcaps_send_rtp (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/gstrtpsession.c#L2240)
gst_rtp_session_event_send_rtp_sink (https://github.com/Kurento/gst-plugins-good/blob/1.7.1/gst/rtpmanager/gstrtpsession.c#L2063)

I wonder if we shouldn't have a specialized RTP funnel that drops the encoding-name & pt, but just ensures that all sources have the same clock-rate and just forwards caps like: "application/x-rtp, clock-rate=XXX"

Actually, forget what I just said, the rtpmuxer does that if you have only one SSRC, but if you have multiple SSRCs, you could have multiple clock rates (as you could be doing the bundle stuff), then sending through the caps won't work and you need to use the signal.

(In reply to Olivier Crête from comment #23)
> I wonder if we shouldn't have a specialized RTP funnel that drops the
> encoding-name & pt, but just ensures that all sources have the same
> clock-rate and just forwards caps like: "application/x-rtp, clock-rate=XXX"

You might also want to funnel RTP and RTCP together

but funneling RTP & RTCP kind of requires dropping all the caps anyway. As picking a random one won't work.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/160.