[Bug 34985] rtcp-fb and rtp-hdrext support in spec and gabble

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=34985

--- Comment #3 from Will Thompson <will.thompson at collabora.co.uk> 2011-03-11 07:42:48 PST ---
Gabble:

I guess you don't want to re-do this branch to use Wocky{Stanza,Node} rather
than LmMessage[Node]? :p

(It's fine, I'm sure sed will cope.)

src/jingle-media-rtp.c:

-  GList *local_codecs;
-  /* Holds (JingleCodec *)'s borrowed from local_codecs, namely those which
have
-   * changed from local_codecs' previous value. Since the contents are
-   * borrowed, this must be freed with g_list_free, not
-   * jingle_media_rtp_free_codecs().
+  JingleMediaDescription *local_media_description;
+  /* Holds (JingleCodec *)'s borrowed from local_media_description,
+   * namely codecs which havew* changed from local_media_description's
+   * previous value. Since the contents are * borrowed, this must be
+   * freed with g_list_free, not * jingle_media_rtp_free_codecs().
    */

That's one pretty mangled comment right there.

+      if (h->senders == JINGLE_CONTENT_SENDERS_BOTH)
+        direction = TP_MEDIA_STREAM_DIRECTION_BIDIRECTIONAL;
+      else if (h->senders == JINGLE_CONTENT_SENDERS_NONE)
+        direction = TP_MEDIA_STREAM_DIRECTION_NONE;
+      else
+        {
+          if (!have_initiator)
+            {
+              g_object_get (priv->content->session, "local-initiator",
+                  &initiated_by_us, NULL);
+              have_initiator = TRUE;
+            }
+
+          if (h->senders == JINGLE_CONTENT_SENDERS_INITIATOR)
+            direction = initiated_by_us ? TP_MEDIA_STREAM_DIRECTION_SEND :
+                TP_MEDIA_STREAM_DIRECTION_RECEIVE;
+          else if (h->senders == JINGLE_CONTENT_SENDERS_RESPONDER)
+            direction = initiated_by_us ? TP_MEDIA_STREAM_DIRECTION_RECEIVE :
+                TP_MEDIA_STREAM_DIRECTION_SEND;
+          else
+            g_assert_not_reached ();
+        }

Eh, if you pulled the if (!have_initiator) {} block out, then this could be a
switch statement, and the assert would be more obviously okay.

+          g_assert (hdrext);
+          g_assert (hdrext->n_values == 4);
+          g_assert (G_VALUE_HOLDS_UINT   (g_value_array_get_nth (hdrext, 0)));
+          g_assert (G_VALUE_HOLDS_UINT   (g_value_array_get_nth (hdrext, 1)));
+          g_assert (G_VALUE_HOLDS_STRING (g_value_array_get_nth (hdrext, 2)));
+          g_assert (G_VALUE_HOLDS_STRING (g_value_array_get_nth (hdrext, 3)));
+
+          tp_value_array_unpack (hdrext, 4,
+              &id,
+              &direction,
+              &uri,
+              &params);

I see your point that it would be good if tp_value_array_unpack() typechecked.
:)

+                   "SupportedCodecs msut have a non-empty list of codecs" };

msut.

+static gboolean
+content_has_cap (GabbleJingleContent *content, const gchar *cap)
+{
+  GabblePresence *presence = gabble_presence_cache_get (
+      content->conn->presence_cache, content->session->peer);
+
+  return gabble_presence_resource_has_caps (presence,
+      gabble_jingle_session_get_peer_resource (content->session),
+      gabble_capability_set_predicate_has, cap);
+}
+

This should check whether 'presence' is NULL. (IIRC gabble_presence_cache_get()
can return NULL.) In principle I guess this could happen if a contact becomes
invisible mid-call, for instance.

+static void
+jingle_feedback_message_list_free (GList *fbs)
+{
+  while (fbs)
+    {
+      jingle_feedback_message_free (fbs->data);
+      fbs = fbs->next;
+    }
+}
+
 void
 jingle_media_rtp_codec_free (JingleCodec *p)
 {
   g_hash_table_unref (p->params);
   g_free (p->name);
+  jingle_feedback_message_list_free (p->feedback_msgs);
   g_slice_free (JingleCodec, p);
 }

I think this leaks the spine of the feedback_msgs list.

@@ -441,19 +513,35 @@ parse_payload_type (LmMessageNode *node)
   for (i = node_iter (node); i; i = node_iter_next (i))
     {
       LmMessageNode *param = node_iter_data (i);
-      const gchar *param_name, *param_value;

-      if (tp_strdiff (lm_message_node_get_name (param), "parameter"))
-        continue;
+      if (!tp_strdiff (lm_message_node_get_name (param), "parameter"))
+        {
+          const gchar *param_name, *param_value;
+
+          param_name = lm_message_node_get_attribute (param, "name");
+          param_value = lm_message_node_get_attribute (param, "value");
+
+          if (param_name == NULL || param_value == NULL)
+            continue;

-      param_name = lm_message_node_get_attribute (param, "name");
-      param_value = lm_message_node_get_attribute (param, "value");
+          g_hash_table_insert (p->params, g_strdup (param_name),
+              g_strdup (param_value));
+        }
+        else if (!tp_strdiff (lm_message_node_get_name (param), "rtcp-fb"))
+        {
+          JingleFeedbackMessage *fb = parse_rtcp_fb (content, param);

-      if (param_name == NULL || param_value == NULL)
-        continue;
+          if (fb != NULL)
+            p->feedback_msgs = g_list_append (p->feedback_msgs, fb);
+        }
+        else if (!tp_strdiff (lm_message_node_get_name (param),
+                "rtcp-fb-trr-int"))
+        {
+          guint trr_int = parse_rtcp_fb_trr_int (content, param);

-      g_hash_table_insert (p->params, g_strdup (param_name),
-          g_strdup (param_value));
+          if (trr_int != G_MAXUINT)
+            p->trr_int = trr_int;
+       }

I've been trying to avoid nitpicking indentation (and if (foo) vs if (foo !=
NULL) and other style trivia), but the blocks here are woefully misaligned.


+static gboolean
+jingle_feedback_message_compare (const JingleFeedbackMessage *fb1,
+    const JingleFeedbackMessage *fb2)
+{
+  if (!g_ascii_strcasecmp (fb1->type, fb2->type) &&
+      !g_ascii_strcasecmp (fb1->subtype, fb2->subtype))
+    return 0;
+  else
+    return 1;
+}

FALSE and TRUE please. (I'm the kind of person who would write

  return g_ascii_strcasecmp (fb1->type, fb2->type) != 0 ||
      g_ascii_strcasecmp (fb1->subtype, fb2->subtype) != 0;

but I can see that that might be an acquired taste. ;-) )

I'm balking at jingle_media_description_simplify() — I will come back to it. It
seems excessively complicated.


+  trr_int_node = lm_message_node_add_child (node, "rtcp-fb_trr-int", NULL);

OOI why the mixture of - and _ in the element name?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.