Created attachment 115289 [details] [review]
agent: Move NiceAgent:reliable down to Component:reliable

By my reading of the ICE-TCP standard (RFC 6544), different stream
components should be able to differ in whether they are TCP or UDP, and
hence one component could run over TCP-over-UDP, while another runs over
raw UDP, for example. Otherwise, both components would have to be TCP,
and the UDP component would essentially run as framed UDP within
TCP-over-UDP.

Push the NiceAgent:reliable property down to a field in Component, and
update all the checks for it to use the component rather than the agent.

Created attachment 115290 [details] [review]
agent: Deprecate nice_agent_new_reliable() and NiceAgent:reliable

Reliability of components should now be specified using
nice_agent_add_stream_full().

nice_agent_new_reliable() has been deprecated but not removed. If
called, it will cause future calls to nice_agent_add_stream() for that
NiceAgent instance to create reliable streams; so API backwards
compatibility is preserved.

Created attachment 115291 [details] [review]
docs: Add new symbols for 0.1.12.1 to the documentation

git branch here: http://cgit.collabora.com/git/user/pwith/libnice.git/log/?h=component-reliability

This all works, passes the unit tests, and I'm reasonably happy with the API.

The only open question I have is whether it would be more future proof to simply expose the Stream and Component structs as NiceStream and NiceComponent GObjects, so that new flags or capabilities can be added to them as properties or methods in future.

Comment on attachment 115289 [details] [review]
agent: Move NiceAgent:reliable down to Component:reliable

Review of attachment 115289 [details] [review]:
-----------------------------------------------------------------

::: agent/agent.c
@@ +5951,5 @@
>    if (!agent_find_component (agent, stream_id, component_id, NULL, &component))
>      goto done;
>  
> +  if (!component->reliable)
> +    goto done;

Let put a g_critical() here, to make it easier for app developers.

::: agent/agent.h
@@ +484,5 @@
> + * nice_agent_add_stream_full:
> + * @agent: The #NiceAgent object
> + * @n_components: The number of components to add to the stream
> + * @...: Exactly @n_components parameters of type #NiceComponentFlags,
> + *   specifying the flags for each component

I don't think it makes sense to have different reliablities for different components on the same stream, may as well make separate streams. That would also avoid the vaargs, which aren't binding friendly.

Comment on attachment 115290 [details] [review]
agent: Deprecate nice_agent_new_reliable() and NiceAgent:reliable

Review of attachment 115290 [details] [review]:
-----------------------------------------------------------------

::: agent/agent.h
@@ +437,4 @@
>   * <para> See also: #NiceAgent::reliable-transport-writable </para>
>   *
>   * Since: 0.0.11
> + * Deprecated: 0.1.12.1: Use nice_agent_new() instead, and specify which

Deprecate in 0.1.13 ..

Comment on attachment 115291 [details] [review]
docs: Add new symbols for 0.1.12.1 to the documentation

Review of attachment 115291 [details] [review]:
-----------------------------------------------------------------

::: docs/reference/libnice/libnice-docs.xml
@@ +103,5 @@
>      </index>
> +    <index role="0.1.12.1">
> +      <title>Index of new symbols in 0.1.12.1</title>
> +      <xi:include href="xml/api-index-0.1.12.1.xml"><xi:fallback/></xi:include>
> +    </index>

Just put 0.1.13 instead of 0.1.12.1, we've added an API in 0.1.11, we should add a section there for that too...

(In reply to Olivier Crête from comment #5)
> Comment on attachment 115289 [details] [review] [review]
> agent: Move NiceAgent:reliable down to Component:reliable
> 
> Review of attachment 115289 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: agent/agent.h
> @@ +484,5 @@
> > + * nice_agent_add_stream_full:
> > + * @agent: The #NiceAgent object
> > + * @n_components: The number of components to add to the stream
> > + * @...: Exactly @n_components parameters of type #NiceComponentFlags,
> > + *   specifying the flags for each component
> 
> I don't think it makes sense to have different reliablities for different
> components on the same stream, may as well make separate streams. That would
> also avoid the vaargs, which aren't binding friendly.

The idea about having the reliabilities differ for different components is that each stream performs its own ICE negotiation — I wanted to avoid repeating that for the different logical connections.

However, I’ve just realised the reliability *has* to be tied up with the ICE negotiation, since the generated candidates may differ if ICE-TCP is enabled or disabled (etc.). So I agree with this, and will update the patch.

Also, good point about the bindability.

Thanks for the rest of the comments — I’ll also update to fix them.

After some discussion, it turns out this is a bad idea. What libnice should be doing is to eliminate the concept of forcing a stream to be reliable or non-reliable, and returning to the user’s code a socket which best matches the code’s preferences. The preferences could be ‘either reliable or non-reliable, preferring reliable if given a choice’, or ‘always reliable, or failure’ or ‘always non-reliable, or failure’, etc. The user code can then check the socket it’s returned and behave accordingly.

This is needed to allow multiplexing of reliable connections within a single ICE stream — to allow interoperability between difference ICE stacks, this *has* to be done in a protocol layer implemented in the user’s code. It cannot be done in libnice with some kind of custom libnice multiplexing protocol. The multiplexing protocol used by the user might be PseudoTcpSocket, or it might be something else.

So that would require the following API changes to libnice:

Planned API changes:
 • Deprecate nice_agent_new_reliable()
 • Keep NiceAgent:ice-tcp and NiceAgent:ice-udp for disabling ICE-TCP or ICE-UDP entirely if the user code can’t cope with them
 • Potentially add something like nice_agent_set_transport_priority(NiceAgent*, guint stream_id, NiceCandidateTransport, guint priority_modifier) to tweak the priorities so that user code which prefers TCP ends up prioritising TCP candidates, etc. (this would need to be designed more carefully, with reference to RFC 6544, which I have not done in this draft design)
 • Add API GSocketInterface *nice_agent_get_socket(NiceAgent*, guint stream_id, guint component_id) which returns a GSocketInterface implementation matching the underlying stream — so it could be reliable or unreliable, and user code can check this by looking at the socket protocol
 • Deprecate the nice_agent_send(), nice_agent_recv(), etc. API in favour of the GSocketInterface
 • Deprecate the nice_agent_get_io_stream() API in favour of user code calling g_socket_factory_create_connection() on the return value from nice_agent_get_socket()
 • Refactor PseudoTcpSocket as an object implementing GSocketInterface, and wrapping a base GSocketInterface instance, which would typically be the return value from nice_agent_get_socket()
 • Keep nice_agent_get_io_stream() as the convenience API which guarantees to always return a reliable I/O stream, potentially by instantiating pseudo-TCP over an underlying ICE-UDP socket, or whatever

Note that all the deprecations above can be ignored and those APIs refactored as convenience wrappers. But I think we probably should deprecate the lot so we stop reinventing the wheel by copying every single one of GLib’s networking APIs (e.g. g_socket_receive() vs nice_agent_recv()).

GSocketInterface is proposed in https://bugzilla.gnome.org/show_bug.cgi?id=697907#c48. It needs to be implemented in GLib first.

Some more detailed reasoning, so it doesn’t get lost. On the topic of multiplexing logical connections within a single ICE stream:

I don’t want to design an API which forces TCP-over-UDP to be used forever. I want this API to be useful in general, regardless of whether ICE-TCP is available, or whether the stream is actually non-reliable. My design goals:

 1. Avoid re-performing ICE negotiation for new logical connections between two hosts which have already negotiated one connection.
 2. Do not hard-code usage of pseudo-TCP in the API. It’s an implementation detail.
 3. Allow libnice to transparently use ICE-TCP or ICE-UDP as appropriate for reliable and non-reliable connections.
 4. Remain compatible with other ICE implementations (specifically: no custom packet formats in libnice itself — pseudo-TCP was already a mistake here).
 5. Avoid HoL blocking or packet loss for reliable logical connections (i.e. have a decent flow control implementation per logical connection).

The possible approaches come from multiplexing the logical connections at different levels in the libnice stack. For each new logical connection, we could:
 • Create a new ICE connection (i.e. a new NiceAgent). Fails goal #1.
 • Create a new ICE stream within an existing NiceAgent and re-use existing candidates. Partially fails goal #1 — see below.
 • Create a new ICE component within an existing stream. Fails goal #1. ICE negotiation happens for each component in a stream. Still partially fails goal #1 if re-using existing candidates, as above.
 • Multiplex within ICE components using code in libnice (e.g. pseudo-TCP and something similar for UDP). Fails goal #4 due to needing a UDP framing protocol within libnice, or a TCP framing protocol if reliable connections are running over ICE-TCP rather than pseudo-TCP. Fails goal #2 due to hard-coding a dependency on pseudo-TCP conversations otherwise.
 • Multiplex within ICE components using user code outside libnice (e.g. a custom framing protocol). Fails goal #5 due to pseudo-TCP or ICE-TCP flow control being applied outside the multiplexed logical connections.

Re-using candidates won’t work quickly in cases where the chosen candidate pair uses reflexively discovered ports. For example, dynamically forwarded firewall ports, or ports which are the result of a STUN lookup. Since the host is going to open a bunch of new ports locally for each new ICE gathering operation, they will map to new reflexive ports, which is the bulk of the time in the ICE negotiation. So I don’t think that seeding new ICE negotiations with existing candidates will necessarily be a large speedup — but I could easily have misunderstood things, so please correct me if I’m wrong.

Goal #3 can always be met, due to the opportunistic nature of ICE-TCP and ICE-UDP has been applied, by creating one stream for reliable logical connections (preferring ICE-TCP), and one for non-reliable logical connections (preferring ICE-UDP).

For reference, neither STUN, ICE-TCP or RFC 4571 (RTP framing) provide a way of encoding multiple conversations like pseudo-TCP does.

(In reply to Philip Withnall from comment #10)
>  • Multiplex within ICE components using user code outside libnice (e.g. a
> custom framing protocol). Fails goal #5 due to pseudo-TCP or ICE-TCP flow
> control being applied outside the multiplexed logical connections.

Examining this in a bit more detail, there are 3 solutions to HoL blocking:

 1. Eliminate the multiplexed receive buffer, and only buffer in the demultiplexed streams. Not possible as the minimum value of SO_RCVBUF in Linux is 128 bytes (see socket(7)), so there’s always going to be a receive buffer. And we could never guarantee to dequeue incoming packets fast enough to not cause unnecessary retransmissions even in good conditions.
 2. Demultiplex packets out of order from the multiplexed queue. This requires support for MSG_PEEK and out-of-order dequeueing reads in the underlying reliable socket (ICE-TCP _or_ pseudo-TCP). Peeking is supported in ICE-TCP (and could be added to pseudo-TCP without too much difficulty), but out-of-order dequeueing reads are not supported, and would require kernel modifications to support. Not possible.
 3. Implement flow control in the multiplexing protocol. This adds packet size overhead, and the potential for exponentially long retransmission delays if both the multiplexed and demultiplexed flow control mechanisms back off at the same time. One of them (the multiplexed one? haven’t thought about this properly) has to have its receive buffer set very small to avoid this. Otherwise, though, it will work.




(In reply to Philip Withnall from comment #10)
>  • Multiplex within ICE components using code in libnice (e.g. pseudo-TCP
> and something similar for UDP). Fails goal #4 due to needing a UDP framing
> protocol within libnice, or a TCP framing protocol if reliable connections
> are running over ICE-TCP rather than pseudo-TCP. Fails goal #2 due to
> hard-coding a dependency on pseudo-TCP conversations otherwise.

For comparison, I think the failures of goals #4 and #2 are blockers here, and this approach should not be used.

Overall, then, I suggest we multiplex within ICE components using a multiplexing implementation built in user code. This can be done cleanly by:

 • Entirely ripping the concept of guaranteed-reliable streams out of libnice — that means removing nice_agent_new_reliable() and the API I suggested in comment #0. On re-reading RFC 6544, I noticed that it doesn’t guarantee the negotiated ICE connection will actually be reliable — a UDP candidate pair might be selected. I think the base libnice APIs should have a way to specify that a stream prefers a reliable (or non-reliable) connection, a way to entirely disable one or the other (which may result in negotiation failing), and a way to determine whether an established connection is reliable or non-reliable.
 • User code can then check whether the returned socket is reliable or non-reliable, and change its behaviour accordingly — either by setting up a custom framing protocol, or (as with RTP; see RFC 6544, §4.2), not caring and using the stream regardless. In particular, the user code could decide to set up pseudo-TCP with multiple conversation IDs to implement multiplexing. It is not libnice’s job to do this.
 • Provide pseudo-TCP as a separate API which user code can use on non-reliable connections to make them reliable.
 • Provide convenience API for the common case where user code is using libnice on both peers and hence doesn’t care about interoperability; so libnice can automatically set up pseudo-TCP inside a non-reliable stream if needed.

I think it is important to keep the multiplexing implementation in user code for exactly the reasons it’s implemented in pseudo-TCP: it can easily get tied up with other features of the user’s protocol (whether that be pseudo-TCP as used by Google, or RTP, or some custom protocol), so implementing it in libnice is essentially duplicating functionality which the user’s protocol implements already.

Note also that because of the HoL blocking problem, multiplexing within an ICE-TCP connection has to be implemented using pseudo-TCP within ICE-TCP (unless you have multiple ICE-TCP streams, but then you fail goal #1 again). For interoperability, this has to be implemented in user code.

At what point did the discussion switch from per-component reliability to multiplexing? :)

(In reply to Ilya Konstantinov from comment #12)
> At what point did the discussion switch from per-component reliability to
> multiplexing? :)

This was some offline discussion I had with Olivier about a related feature we were working on (multiplexing) for a project, which required per-stream reliability. I pasted the whole discussion in partly for context, and partly because I didn’t have time to edit it all down.

The crux of the reasoning is here:

(Quoting Philip Withnall from comment #9)
> After some discussion, it turns out this is a bad idea. What libnice should
> be doing is to eliminate the concept of forcing a stream to be reliable or
> non-reliable, and returning to the user’s code a socket which best matches
> the code’s preferences. The preferences could be ‘either reliable or
> non-reliable, preferring reliable if given a choice’, or ‘always reliable,
> or failure’ or ‘always non-reliable, or failure’, etc. The user code can
> then check the socket it’s returned and behave accordingly.

So libnice does need a concept of per-stream reliability, but not the way we were originally thinking. Implementing this properly depends on GSocketInterface being implemented in GLib, which requires a bit more plumbing. I am trying to find time for it, but I can make no guarantees.

(In reply to Philip Withnall from comment #13)
> So libnice does need a concept of per-stream reliability, but not the way we
> were originally thinking.

I agree. With reliability emulation, it feels like the core library is doing too much.

As for me, I think for now I'll be using SCTP like they do in WebRTC. If you're GStreamer-based, the OpenWebRTC project has developed an sctp plugin for GStreamer:
https://github.com/EricssonResearch/openwebrtc-gst-plugins/tree/master/ext/sctp