784476 - SDP C-Line should give default ICE candidate instead of local host IP

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Ethan Hugg [:ehugg]]

Currently we are giving the localhost IP in the c= line of the SDP.  Instead of changing it to 0.0.0.0 or 192.0.0.1, we would put the default ICE candidate IP on that line.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: SDP C-Line should give default ICE candidate instead of local host IP

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: SDP C-Line should give default ICE candidate instead of local host IP

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 653948 [details] [diff] [review]
SDP C-Line should give default ICE candidate instead of local host IP

Review of attachment 653948 [details] [diff] [review]:
-----------------------------------------------------------------

I removed the concept of a local IP only from PeerConnection and CallControlManager.  Not sure if it might need to be revived at the lower levels for SIP case.

I removed several warnings.  I splintered the bug myself to point them out to avoid confusion.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -306,5 @@
>    , mWindow(NULL)
>    , mFingerprint("TempFingerprint")
>    , mLocalSourceStreamsLock(PR_NewLock())
>    , mIceCtx(NULL)
> -  , mIceStreams(NULL)

warning: mIceStreams is not a pointer

@@ +581,5 @@
>  // XXX Temporary - remove
>  void
>  PeerConnectionImpl::ListenThread(void *data)
>  {
> +#ifdef MOZILLA_INTERNAL_API

Warning: unused var

@@ +620,5 @@
>  // XXX Temporary - remove
>  void
>  PeerConnectionImpl::ConnectThread(void *data)
>  {
> +#ifdef MOZILLA_INTERNAL_API

Warning: unused var

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +151,5 @@
>      vcm_crypto_key_t        tx_key;        /* tx key                  */
>      vcm_crypto_key_t        rx_key;        /* rx key                  */
>      uint32_t                flags;         /* misc. flags.            */
> +    char                    algorithm[FSMDEF_MAX_DIGEST_ALG_LEN];
> +    char                    digest[FSMDEF_MAX_DIGEST_LEN];

Warning: trying to set const char*

::: media/webrtc/signaling/src/sipcc/core/gsm/h/lsm.h
@@ +42,5 @@
>  
>  #include "cpr_types.h"
>  #include "ccapi.h"
>  #include "uiapi.h"
> +#include "fsm.h"

warning: for the def of lsm_add_remote_stream

@@ +140,5 @@
>  void lsm_ui_display_status(const char *pStatusStr, line_t line,
>                             callid_t call_id);
>  string_t lsm_parse_displaystr(string_t displaystr);
>  void lsm_speaker_mode(short mode);
> +void lsm_add_remote_stream (line_t line, callid_t call_id, fsmdef_media_t *media, int *pc_stream_id);

Warning: lsm_add_remote_stream not defined when it's used.

Comment 4

[Ethan Hugg [:ehugg]]

Comment on attachment 653948 [details] [diff] [review]
SDP C-Line should give default ICE candidate instead of local host IP

Review of attachment 653948 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +732,3 @@
>  
>  #define FSM_NEGOTIATED_CRYPTO_DIGEST(media)       \
> +       media->negotiated_crypto.digest

Warning: should be return char*, not char**

Comment 5

[Eric Rescorla (:ekr)]

Nice catch. You want to just fix?

Comment 6

[enda mannion (:emannion)]

Comment on attachment 653948 [details] [diff] [review]
SDP C-Line should give default ICE candidate instead of local host IP

Review of attachment 653948 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/include/CallControlManager.h
@@ +114,5 @@
>          virtual void setMultiClusterMode(bool allowMultipleClusters) = 0;
>          virtual void setSIPCCLoggingMask(const cc_int32_t mask) = 0;
>          virtual void setAuthenticationString(const std::string &authString) = 0;
>          virtual void setSecureCachePath(const std::string &secureCachePath) = 0;
>  

I think we may need this API if we are to ever use SIPCC as a sipstack over a socket. B2G?

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +951,1 @@
>  {

I would be wary to remove this code but rather use SDPMode config item

@@ +956,4 @@
>      (void) sdp_set_conn_nettype(sdp_p, level, SDP_NT_INTERNET);
> +
> +    if (addr && (strlen(addr) > strlen("123.123.123.123")))
> +    {

Again this may break the sipstack's operation

@@ +2143,5 @@
>                      dcb_p->line, dcb_p->call_id, fname);
>          return;
>      }
>  
> +    gsmsdp_set_connection_address(sdp_p, media->level, dcb_p->ice_default_candidate_addr);

applies here also

@@ +2837,5 @@
>       * session level and determining not to include "c=" line
>       * at the media level can become complex. For this reason, the
>       * unsupported media line will have "c=" with 0.0.0.0 address instead.
>       */
> +    gsmsdp_set_connection_address(sdp_p->src_sdp, level, "0.0.0.0"); 

and here

Comment 7

[Eric Rescorla (:ekr)]

I don't expect us to send SIP directly. Rather, we're going to have it proxy through JS like in Paris.

I don't know if that changes your view

Comment 8

[Ethan Hugg [:ehugg]]

I was under the impression that we would never again bind/listen/connect at the socket layer from SIPCC.  These changes are easily reversible back to using local IP, but I would like to do bigger changes like removing all of cpr_socket which would be not so reversible. 

It would be good to get some clarity on what exactly will be expected so we could make a bug for building a unittest that exercises it at the PCImpl layer.

Comment 9

[Eric Rescorla (:ekr)]

I believe we will not be every doing direct socket connections, but let's see if we can get cullen to weigh in.

Have you thought about how you actually want to get the messages in and out? Last time (paris_demo) we just hacked cpr-socket

Comment 10

[Ethan Hugg [:ehugg]]

>Have you thought about how you actually want to get the messages in and out? Last >time (paris_demo) we just hacked cpr-socket

I would probably go one step higher and replace the code inside the functions that start with "sipTransport" in core/sipstack

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: SDP C-Line should give default ICE candidate instead of local host IP

Comment 12

[Ethan Hugg [:ehugg]]

 Pushed to Alder - https://hg.mozilla.org/projects/alder/rev/e3f27b9ab8d9