[Telepathy] new jingle engine for Gabble - more review

[Senko Rasic]

Simon McVittie wrote:
> In general (although under the circumstances these aren't merge blockers):
> 
> * delete dead code rather than commenting it out
> * don't use // comments

Fixed these (in several patches).

> * I'd prefer this style for priv stuff:
> 
>   /* in header */
>   typedef struct _Foo Foo;
>   typedef struct _FooPrivate FooPrivate;
> 
>   struct _Foo {
>       ...
>       FooPrivate *priv;
>   };
> 
>   /* in body */
>   struct _FooPrivate {
>       Bar *bar;
>   };
> 
>   void
>   foo_do_things (Foo *self)
>   {
>     bar_do_things_with_foo (self->priv->bar, self);
>   }

I've changed the structs as per the example, and modified
JINGLE_*_GET_PRIVATE accessor macro to not cast anything, but haven't
replaced it atm (it's trivial replacement which we can do later on, as
far as i can see other parts of gabble are also using
accessor+typecheck).

Changed in 0069b41ce15b941eef9eb98c9059bd377954513a

> I think it's worth trying this code out in some "real world" situations to
> make sure we don't have any huge regressions. I'll get some builds going and
> see what happens; we have Google Talk in the office that we can test interop
> with, too.

Right.

> 
> Senko wrote some code:
>> src/Makefile.am
> 
> alphabetical order of files please
> 
>> connection.[ch]
> 
> alphabetical order of private headers please, as per Style wiki page.
> Likewise in media-factory.c

Changed in 19f84d1ffcb797c1fab656f51b9315ba63168a76

>> "%s" stuff
> 
> Have you fast-tracked it in already? If not, do - I'll review

It's in my error-format-fix git branch (updated after your review).

> 
>> +#define NS_JINGLE_RTP_TMP       "urn:xmpp:jingle:apps:rtp:0"
> 
> Is this really TMPorary? It looks somewhat final to me... anyway, either
> tag the name with a version, or indicate that it's final.
> 
> It would be nice if each namespace had a one-line comment, like
> /* XEP-0123 v0.32 (last call) */, indicating what it means.
> 
>> PRESENCE_CAP_JINGLE_TMP
> 
> Do you mean PRESENCE_CAP_JINGLE_032? Please be consistent.

No, that was PRESENCE_CAP_JINGLE_RTP_TMP. I've renamed
caps/namespaces to make the names less confusing.
Changed in 8d84d2205e74f42225f579929859cff6506859ab

>> +# print "FIXME: jingle/test-incoming-call.py disabled due to race
>> condition"
> 
> Don't leave in dead code, please. If this test has stopped being racy,

Enabled it. It never breaks for me (possibly the new code emits signals
in slightly less racy way). If we do encounter (non-error) race
condition, we can deal with it with expect_racy.

In general -  When I was reorganising everything, I put FIXMEs
everywhere where I wasn't sure what to do (or I wasn't touching that
part of the code at the time), to remind me later. There's probably
several stale ones.

> let's enable it. Likewise for all the stuff you commented out in
> media-stream.h.
> 
>> @@ -302,10 +322,12 @@ gabble_media_stream_set_property (GObject *object,
>>          {
>>            StreamSignallingState old = stream->signalling_state;
>>            stream->signalling_state = g_value_get_uint (value);
>> -          GMS_DEBUG_INFO (priv->session, "stream %s sig_state %d->%d",
>> +          DEBUG ("stream %s sig_state %d->%d",
>>                stream->name, old, stream->signalling_state);
>> +          /* FIXME 
>>            if (stream->signalling_state != old)
>>              push_native_candidates (stream);
>> +            */
> 
> What's going on here? What does the FIXME mean?

That's actually dead code. SignallingState deals with content
signalization in jingle, and JingleContent object does that now, but
I haven't removed the old property code from MediaStream.

>> +  param_spec = g_param_spec_uint ("mode", "Signalling mode",
> 
> Why is GabbleMediaStream:mode no longer an enum? Isn't this a bit of a
> regression?
> 
>> +                                  0, G_MAXUINT, MODE_JINGLE, // FIXME

Likewise, SignallingMode is predecessor to JingleDialect enum, and isn't
actually used in MediaStream anywhere. Removed

> 
> What needs fixing here?
> 
>> +      DEBUG ("ignoring native localhost candidate");
>> +      /*
>>        GMS_DEBUG_WARNING (priv->session,
>> -          "%s: ignoring native localhost candidate", G_STRFUNC);
>> +          "%s: ignoring native localhost candidate", G_STRFUNC); */

I've got rid of 'GMS_DEBUG_WARNING' and using normal DEBUG for this
(the line before the commented code), so it's just dead code.
> 
> Please eliminate dead code.

Eliminated all of the dead code from media-stream.[ch]. There's still
one commented out block left in direction handling logic,
which was commented out in the original code. I need to figure out what
exactly is going on there to see if I should refactor it or eliminate
that block.

Changed in 1865e79abaa4401d020e29917e05cd9dd316a22d

> 
>> @@ -846,18 +931,28 @@ gabble_media_stream_new_native_candidate (TpSvcMediaStreamHandler *iface,
>> +  c = g_new0 (JingleCandidate, 1);
> 
> g_slice_new0 would be better for a 7-word struct.
> 
>> +  gabble_jingle_content_add_candidates (priv->content, li);
>> +
>> +  // FIXME: the above instead this: push_native_candidates (self);
> 
> Please explain?

Stale comment from when _add_candidates was a stub; 
jingle_content_add_candidates indeed does what push_native_candidates
used to do in the old code.
> 
>> +      c = g_new0 (JingleCodec, 1);
> 
> g_slice_new0

Changed in 79275e9c7b7da81465210292d3129e447626f260 .
Since I'm creating the structs in media-stream and freeing them in
jingle-transport-google and jingle-media-rtp, do you think I should
have a helper function for creating them, and not having to rememer
about g_slice in various places?

> 
>> @@ -913,22 +1014,57 @@ gabble_media_stream_set_local_codecs (TpSvcMediaStreamHandler *iface,
>> +          5, &params,
> 
> The params seem to be discarded rather than passed to the jingle engine. Is
> this expected? If so, perhaps don't bother even pulling them out of the struct,
> and put a comment indicating that they're ignored.

These acually are used for extra video params (just check with sjoerd 
that params like "widht" and "height" actually are used in video calls).
So I've rewritten the code dealing with extra params, it's in
patch 6199c8e04d52eb076096b7ca2186c5f91c2817e6

> 
>> @@ -941,10 +1077,14 @@ gabble_media_stream_stream_state (TpSvcMediaStreamHandler *iface,
>> +  /* FIXME: we're relying on 1:1 mapping between tp and jingle enums */
>> +  gabble_jingle_content_set_transport_state (priv->content,
>> +      connection_state);
> 
> Is there error checking? D-Bus clients can pass in whatever they like here.

Reworked into a switch which checks legal values in patch
747abddfc8efe13fb1c34c5d64e2193c10ae52c8
(PS: later patch contains a fix for missing 'break' in there)

> 
>> @@ -1220,93 +1153,38 @@ _gabble_media_stream_post_remote_codecs (GabbleMediaStream *stream,
>> +          4, c->channels,
>> +            /* FIXME: valgrind shows leak in g_hash_table_new_full - doesn't
>> +             * dbus-glib free that? */
>> +          5, g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_free),
> 
> I believe dbus_g_type_struct_set copies its arguments, so you need to assign
> this hash table to a temporary, and free it afterwards.

Changed that part of the code in extra video param handling patch (see 
above), so now it's used and properly destroyed later. I still need to 
check whether I need to deep-copy the hashtable in set_local_codecs so
that strings can be used after that invocation finishes.

> 
>> +content_state_changed_cb (GabbleJingleContent *c,
>> ...
>> +      /* FIXME: check direction for these */
>> +      stream->playing = TRUE;
>> +      priv->sending = TRUE;
> 
> It looks as though this needs fixing?
> 
>> +/* FIXME: this doesn't work yet */
>> void
>> _gabble_media_stream_update_sending (GabbleMediaStream *stream,
>>                                      gboolean start_sending)
> 
> What doesn't work? Why not? Can it be fixed?

Yes, I have to properly hook up direction logic and write a test
to make sure it works.

> 
>> GabbleJingleContent
> 
> What's going on with PROP_READY? If it's not needed, kill it.

It's not needed, any more, now we have media_ready and
transport_ready flags, and helper methods to check readiness
depending on who initiates the stream.
Killed in 0abc600198a3114b813da9e8332833d6c882fe5c patch.

>> +#define SET_BAD_REQ(txt...) g_set_error (error, GABBLE_XMPP_ERROR, XMPP_ERROR_BAD_REQUEST, txt)
> 
> I'd prefer this to take error as a first parameter so it isn't reliant
> on naming.

I'm using these as syntactic sugar because there are a lot of places
where I have to return errors, so I thought the fewer characters the 
better (if i have to pass in XMPP_ERROR_{BAD_REQUEST,OUT_OF_ORDER,...}
each time, there's not much typing saved :)

> 
>> +send_content_add_or_accept (GabbleJingleContent *self)
>> ...
>> +      /* TODO: set a timer for acknowledgement */
> 
> Does this still need doing?

Yes.

> 
>> +++ b/src/jingle-content.h
>> +struct _JingleCandidate {
> 
> Does this duplicate one of the ones in the media stream?

No, those were JingleCodecs, but indeed they were duplicated. I've
fixed that (put JingleCodec definition in jingle-media-rtp.h, as they're
specific to RTP stuff) in patch 6199c8e04d52eb076096b7ca2186c5f91c2817e6
(extra codec param passing).

>> +gabble_jingle_content_add_candidates (GabbleJingleContent *self, GList *li)
> 
> Please comment in either the header or the source what the GList contains.

Commented in the source (at each function definition, as JingleContent, 
JingleTransportIface, JingleTransportGoogle and
JingleMediaRtp both have similar functions).

> 
>> +static JingleAction allowed_actions[6][11] = {
> 
> Eek, magic numbers! Can we have some sort of comment please, at least for what
> 6 means?

It's just a number of states, but yeah it could be more readable,
commented it in 3159f3264b866cc751d9c7705f12425d16c16e35 along with
other readability fixes.

>> +on_transport_info (GabbleJingleSession *sess, LmMessageNode *node,
>> ...
>> +      /* FIXME: report error to peer instead of assert */
>> +      g_assert (g_list_length (cs) == 1);
> 
> Please don't assert on network traffic.

It's not assertion on network traffic, it's a sanity check. Other parts
of jingle code should ensure that session in google mode never != 1
content (either by adding, removing, contents, or by putting
multiple descriptions in session initiate action). If that happens,
we're in serious problems anyways.

> Has libjingle 0.3 vs 0.4 detection been implemented? Do we need it?

We have it partially, meaning we can detect it by inspecting
session initiate or accept action. What's missing is detecting
gtalk3 <candidates> node and switching to gtalk4 (and retransmitting
our candidates). This can happen on outgoing calls, when candidates
are transmitted before session accept.

>> +  /* FIXME: we rely on the fact that JingleContent only signals
>> +   * "ready" on session contents; this is wrong, session contents
>> +   * added/accepted *after* session-accept are handled the same
>> +   * as early media */
> 
> Has this been fixed? Should it?

It's been fixed, jingle content now checks that it's session content,
and that session hasn't already been accepted. I've removed the
FIXME in 580dbd0b17c74000c104755c261b66f91b8fe529

> 
>> +  // FIXME: klass->produce = produce_candidates;
> 
> Has this been implemented for the Google transport? Is it needed?

It is not needed, add_candidates and transmit_candidates are used 
instead. Removed in 57c97738a99505a356f090484abefc2c46952335

> It appears to be assumed that all Google dialects are numerically less than
> all Jingle dialects. Either the enum of dialects should have a comment that
> says so, or the code that uses <= should be changed to use a function
> jingle_dialect_is_google().

I've created a macro JINGLE_IS_GOOGLE_DIALECT which checks whether 
dialect is one of _GTALK3 or _GTALK4.

I've pushed these patches to my jingle repo. Content direction code,
timeouts in content-add and <candidates> gtalk3 detection are still in
the TODO list, I'll attack them now.

Regards,
Senko