[Telepathy-commits] [telepathy-gabble/master] Don't let CodecsUpdated change clockrate/channels

[Will Thompson]

Olivier informs me that this is bad and wrong.
---
 src/jingle-media-rtp.c                        |   50 ++++++++++++++++++------
 tests/twisted/jingle/test-description-info.py |   21 ++++++++--
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/src/jingle-media-rtp.c b/src/jingle-media-rtp.c
index a8bb47e..1df0156 100644
--- a/src/jingle-media-rtp.c
+++ b/src/jingle-media-rtp.c
@@ -599,19 +599,44 @@ string_string_maps_equal (GHashTable *a,
 }
 
 /**
- * codec_info_equal:
- * Compares the clockrate, channels and params of the supplied codecs,
- * returning TRUE iff they are all equal.
+ * codec_update_coherent:
  *
- * Does *not* compare the codecs' id or name.
+ * Compares @old_c and @new_c, which are assumed to have the same id, to check
+ * that the name, clockrate and number of channels hasn't changed. If they
+ * have, returns %FALSE and sets @e.
  */
 static gboolean
-codec_info_equal (const JingleCodec *c,
-                  const JingleCodec *d)
+codec_update_coherent (const JingleCodec *old_c,
+                       const JingleCodec *new_c,
+                       GError **e)
 {
-  return (c->clockrate == d->clockrate &&
-    c->channels == d->channels &&
-    string_string_maps_equal (c->params, d->params));
+#define INVALID(msg, ...) \
+    g_set_error (e, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT, msg, ##__VA_ARGS__);
+
+  if (tp_strdiff (new_c->name, old_c->name))
+    {
+      INVALID ("tried to change codec %u's name from %s to %s",
+          new_c->id, old_c->name, new_c->name);
+      return FALSE;
+    }
+
+  if (new_c->clockrate != old_c->clockrate)
+    {
+      INVALID ("tried to change codec %u (%s)'s clockrate from %u to %u",
+          new_c->id, new_c->name, new_c->clockrate, old_c->clockrate);
+      return FALSE;
+    }
+
+  if (new_c->channels != old_c->channels)
+    {
+      INVALID ("tried to change codec %u (%s)'s channels from %u to %u",
+          new_c->id, new_c->name, new_c->channels, old_c->channels);
+      return FALSE;
+    }
+
+#undef INVALID
+
+  return TRUE;
 }
 
 /**
@@ -653,11 +678,10 @@ compare_codecs (GList *old,
           if (new_c->id != old_c->id)
             continue;
 
-          if (tp_strdiff (new_c->name, old_c->name))
-            FAIL ("tried to change codec %u's name from %s to %s",
-                new_c->id, old_c->name, new_c->name);
+          if (!codec_update_coherent (old_c, new_c, e))
+            goto err;
 
-          if (!codec_info_equal (old_c, new_c))
+          if (!string_string_maps_equal (old_c->params, new_c->params))
             *changed = g_list_prepend (*changed, new_c);
 
           break;
diff --git a/tests/twisted/jingle/test-description-info.py b/tests/twisted/jingle/test-description-info.py
index 8e22a66..638032a 100644
--- a/tests/twisted/jingle/test-description-info.py
+++ b/tests/twisted/jingle/test-description-info.py
@@ -4,7 +4,7 @@ Test emition and handling of codec update using description-info
 
 from gabbletest import exec_test, make_result_iq, sync_stream, exec_tests
 from servicetest import make_channel_proxy, unwrap, tp_path_prefix, \
-        EventPattern
+        EventPattern, call_async
 import gabbletest
 import dbus
 import time
@@ -12,6 +12,8 @@ from twisted.words.xish import xpath
 
 from jingletest2 import *
 
+import constants as cs
+
 def extract_params(payload_type):
     ret = {}
     for node in payload_type.elements():
@@ -118,9 +120,20 @@ def test(q, bus, conn, stream):
     e = q.expect('stream-iq')
     assert jp.match_jingle_action(e.query, 'session-accept')
 
-    # We decide we want to update the clockrate of the first codec, and add a
-    # parameter to the second, not changing the third.
-    new_codecs = [ ('GSM', 3, 4000, {}),
+    # farstream is buggy, and tells tp-fs to tell Gabble to change the third
+    # codec's clockrate. This isn't legal, so Gabble says no.
+    new_codecs = [ ('GSM', 3, 8000),
+                   ('PCMA', 8, 8000),
+                   ('PCMU', 0, 4000) ]
+    call_async(q, stream_handler, 'CodecsUpdated',
+        jt2.dbusify_codecs(new_codecs))
+    event = q.expect('dbus-error', method='CodecsUpdated')
+    assert event.error.get_dbus_name() == cs.INVALID_ARGUMENT, \
+        event.error.get_dbus_name()
+
+    # With its tail between its legs, tp-fs decides it wants to add some
+    # parameters to the first two codecs, not changing the third.
+    new_codecs = [ ('GSM', 3, 8000, {'type': 'banana'}),
                    ('PCMA', 8, 8000, {'helix': 'BUFFERING'}),
                    ('PCMU', 0, 8000, {}) ]
     stream_handler.CodecsUpdated(jt2.dbusify_codecs_with_params(new_codecs))
-- 
1.5.6.5