Implements these ProtoXEPs:

http://xmpp.org/extensions/inbox/rtp-hdrext.html
http://xmpp.org/extensions/inbox/rtcp-fb.html

Spec:

+         Feedback subtype, according to the Type, can be an empty string (""),
+         if there is no subtype.
+         For example, generic nack is Type="nack" Subtype="".

If I was feeling pendantic, this should use <var> for the arguments, and <code> for the strings. But I'm not.

+         Feedback parameters as a string. Format is defined in the relevant RFC

What's the relevant RFC?

But it looks fine apart from nitpicks like the above.

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?

(In reply to comment #2)
> +         Feedback parameters as a string. Format is defined in the relevant
> RFC
> 
> What's the relevant RFC?

Relevant for this specific parameter (so depends on the type.

I fixed most of your comments and updated the branch .. except those:

(In reply to comment #3)
> Gabble:
> +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.

I'm really not sure what do to about that ? Should I ignore the caps when parsing the description ? Should I cache them when we start a call? But what happens if I get a call from invisible? Should I cache them at the start if we initiate a call and guess them from the incoming description if we receive one ?

> I've been trying to avoid nitpicking indentation (and if (foo) vs if (foo !=
> NULL) and other style trivia)

For the (foo) vs (foo != NULL), I though the TP convention was to only use if (foo) is foo was a boolean and I tried to follow that everywhere
 
> +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.

It's not a gboolean, its 0, 1 or -1 for equal, larger or smaller..

I guess I could do a = g_ascii_strcasecmp (fb1->type, fb2->type); b = g_ascii_strcasecmp (fb1->subtype, fb2->subtype)); return a ? a : b; if I cared about the ordering
 
> I'm balking at jingle_media_description_simplify() — I will come back to it. It
> seems excessively complicated.

Added a bunch of comments to make it clearer what it's supposed to do.

Branch updated with the caps stuff we discussed.. It now reads the presence cache if it can (and remembers the result), but also remembers any incoming stanza (in the same content).

(In reply to comment #5)
> I fixed most of your comments and updated the branch .. except those:
> 
> (In reply to comment #3)
> > I've been trying to avoid nitpicking indentation (and if (foo) vs if (foo !=
> > NULL) and other style trivia)
> 
> For the (foo) vs (foo != NULL), I though the TP convention was to only use if
> (foo) is foo was a boolean and I tried to follow that everywhere

I thought I saw some places you didn't … I could have been wrong.

For instance, in these new patches:

-  return gabble_presence_resource_has_caps (presence,
+  return presence && gabble_presence_resource_has_caps (presence,
       gabble_jingle_session_get_peer_resource (content->session),
       gabble_capability_set_predicate_has, cap);

:)

-  type = lm_message_node_get_attribute (node, "type");
+ type = lm_message_node_get_attribute (node, "type");
   if (type == NULL)
     return NULL;

indentation…

> > +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.
> 
> It's not a gboolean, its 0, 1 or -1 for equal, larger or smaller..

The function's return type is gboolean …

> I guess I could do a = g_ascii_strcasecmp (fb1->type, fb2->type); b =
> g_ascii_strcasecmp (fb1->subtype, fb2->subtype)); return a ? a : b; if I cared
> about the ordering
> 
> > I'm balking at jingle_media_description_simplify() — I will come back to it. It
> > seems excessively complicated.
> 
> Added a bunch of comments to make it clearer what it's supposed to do.

Yeah, it's clearer now; cheers.

Oops, branch updated from you latest comment, hopefully it should be all good now

Can we get the spec branch merged, pushed into tp-glib and the gabble branch merged ? ...

gabble 0.11.10 has this stuff. it depends on telepathy-glib 0.14.3 which includes the spec which was added in 0.22.1.

Since this bug was filed against gabble, i am marking it as fixed. please feel free to open bugs against (telepathy-)farstream.