Bug 89849 - nice_agent_attach_recv or nice_agent_recv* required for negotiation
Summary: nice_agent_attach_recv or nice_agent_recv* required for negotiation
Status: RESOLVED FIXED
Alias: None
Product: nice
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Ilya Konstantinov
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-31 17:39 UTC by Ilya Konstantinov
Modified: 2015-04-20 19:35 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
agent: Improve nice_agent_attach_recv and nice_agent_recv* docs (4.76 KB, patch)
2015-03-31 23:50 UTC, Ilya Konstantinov
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Konstantinov 2015-03-31 17:39:18 UTC
After the local candidate gathering phase, the user has to either:
1) attach a callback using nice_agent_attach_recv
2) keep using the nice_agent_recv* family of functions

Otherwise, ICE negotation never completes.

The documentation does not explain this. Furthermore, it's not intuitive or obvious that *my app* should keep reading data in order for *NiceAgent* to receive its own packets (that it handles internally and never forwards to me).

While I find this design choice confusing, if it's impossible to change it, let's update the docs of all relevant functions to explain this:
* nice_agent_recv
* nice_agent_recv_messages
* nice_agent_recv_nonblocking
* nice_agent_recv_messages_nonblocking
* nice_agent_attach_recv

Introducing sdp-recv-example.c that uses the nice_agent_recv* family of functions could be also useful.
Comment 1 Olivier Crête 2015-03-31 18:49:04 UTC
It's not possible to change it because we could be receiving valid data packets at any point, at least as soon as a single pair succeeds, so we can't separate data reception from negotiation.

We should definitely make this clear in the documentation.
Comment 2 Olivier Crête 2015-03-31 18:49:30 UTC
Patches to the documentation (inside agent/agent.h) would be very much welcome.
Comment 3 Ilya Konstantinov 2015-03-31 23:39:05 UTC
I'm currently taking care of documentation. Just a few questions though.

QUESTION 1

Do we handle all STUN packets at any time in-bound (i.e. not passing them to the user), or only when the state is NICE_COMPONENT_STATE_CONNECTING or NICE_COMPONENT_STATE_CONNECTED?

(If not, then why not? Aren't we running the risk of some other protocol being, by mere chance, matched as an STUN packet?)

QUESTION 2

Why would we pass data before NICE_COMPONENT_STATE_CONNECTING? Supposedly it's an entirely new pair, and nobody knows about it yet.

I suppose it can be filtered on the app side, even by inspecting the state reported by the NiceAgent, but to a new user, it feels counterintuitive and "weird".
Comment 4 Ilya Konstantinov 2015-03-31 23:50:43 UTC
Created attachment 114794 [details] [review]
agent: Improve nice_agent_attach_recv and nice_agent_recv*  docs

First attempt to explain it. Please review to make sure what I wrote reflects reality.
Comment 5 Olivier Crête 2015-04-08 21:27:20 UTC
(In reply to Ilya Konstantinov from comment #3)
> I'm currently taking care of documentation. Just a few questions though.
> 
> QUESTION 1
> 
> Do we handle all STUN packets at any time in-bound (i.e. not passing them to
> the user), or only when the state is NICE_COMPONENT_STATE_CONNECTING or
> NICE_COMPONENT_STATE_CONNECTED?
> 
> (If not, then why not? Aren't we running the risk of some other protocol
> being, by mere chance, matched as an STUN packet?)

We are, it's required by the RFC. There are two reasons:
 - We are required to send keepalives if we don't know for sure that the application protocol is.
 - It's totally valid to send STUN binding requests at any time. If we set the "keepalive-conncheck" property to TRUE, we even do that and declare the connection dead if we don't get a reply.

> QUESTION 2
> 
> Why would we pass data before NICE_COMPONENT_STATE_CONNECTING? Supposedly
> it's an entirely new pair, and nobody knows about it yet.
> 
> I suppose it can be filtered on the app side, even by inspecting the state
> reported by the NiceAgent, but to a new user, it feels counterintuitive and
> "weird".

Data should be dropped from pairs that are not authenticated, it's a todo item and is bug #82938
Comment 6 Olivier Crête 2015-04-08 21:39:46 UTC
Comment on attachment 114794 [details] [review]
agent: Improve nice_agent_attach_recv and nice_agent_recv*  docs

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

Patch looks good, will merge
Comment 7 Olivier Crête 2015-04-20 19:35:37 UTC
Merged

commit 718bd720969c01f2d9f56370850d8a8dee2e59ec
Author: Ilya Konstantinov <ilya.konstantinov@gmail.com>
Date:   Wed Apr 1 02:42:50 2015 +0300

    agent: Improve nice_agent_attach_recv and nice_agent_recv* docs
    
    Explain that either having an I/O callback or calling nice_agent_recv*
    is essential for ICE connection check to succeed.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=89849


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.