1298991 - nICEr creates duplicated prflx candidate pairs

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Bug 1298991: only create candidate pairs from STUN response when needed.

Review commit: https://reviewboard.mozilla.org/r/75104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75104/

Comment 2

[Nils Ohlmeier [:drno]]

Comment on attachment 8786141 [details]
Bug 1298991: only create candidate pairs from STUN response when needed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75104/diff/1-2/

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8786141 [details]
Bug 1298991: only create candidate pairs from STUN response when needed.

https://reviewboard.mozilla.org/r/75104/#review73170

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c:261
(Diff revision 2)
>               it probably matches one of our candidates */
>  
>            cand=TAILQ_FIRST(&pair->local->component->candidates);
>            while(cand){
> -            if(!nr_transport_addr_cmp(&cand->addr,&pair->stun_client->results.ice_binding_response.mapped_addr,NR_TRANSPORT_ADDR_CMP_MODE_ALL))
> +            if(!nr_transport_addr_cmp(&cand->addr,&pair->stun_client->results.ice_binding_response.mapped_addr,NR_TRANSPORT_ADDR_CMP_MODE_ALL)) {
> +              r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): found another candidate %s", pair->pctx->label,cand->addr.as_string);

This log string could be better. Maybe something like "found pre-existing local candidate for mapped address %s"

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c:284
(Diff revision 2)
> +          } else {
> +            /* Check if we have a pair for this candidate already. */
> +            orig_pair=pair;
> +            if(r=nr_ice_component_find_pair(cand, pair->remote, &pair)) {
> +              r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): no pair exists for %s and %s", pair->pctx->label,cand->addr.as_string, pair->remote->addr.as_string);
> +              pair=orig_pair;
> +              orig_pair=0;
> +            }

This could be easier to read, especially if we do a little refactoring to make this something like |pair| and |actual_pair|.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c:689
(Diff revision 2)
> +int nr_ice_candidate_pair_find(nr_ice_cand_pair_head *head,
> +  nr_ice_candidate *lcand, nr_ice_candidate *rcand, nr_ice_cand_pair **pair)

This would make more sense as a nr_ice_media_stream_ function, probably.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1538
(Diff revision 2)
>  
>      return(0);
>    }
>  
>  
> -int nr_ice_component_insert_pair(nr_ice_component *pcomp, nr_ice_cand_pair *pair)
> +int nr_ice_component_insert_pair(nr_ice_cand_pair *pair)

Let's not do this. If we want, we could maybe make this an nr_ice_candidate_pair_ function.

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8786141 [details]
Bug 1298991: only create candidate pairs from STUN response when needed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75104/diff/2-3/

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8786141 [details]
Bug 1298991: only create candidate pairs from STUN response when needed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75104/diff/3-4/

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8786141 [details]
Bug 1298991: only create candidate pairs from STUN response when needed.

https://reviewboard.mozilla.org/r/75104/#review73700

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f6e07e5c211
only create candidate pairs from STUN response when needed. r=bwc

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f6e07e5c211