Please write some unit tests to exercise this functionality http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/nricectx.cpp File media/mtransport/nricectx.cpp (right): http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/nricectx.cpp#newcode344 media/mtransport/nricectx.cpp:344: ...
http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/ice/ice_ctx.c (right):
http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:175: int
nr_ice_ctx_set_interface_priority(nr_ice_ctx *ctx, nr_interface_priority *ip)
On 2013/07/01 14:05:49, ekr-webrtc wrote:
> Rename to something like "set_interface_prioritizer" I should think. Since
we're
> not setting the priority of an interface but rather the tool to enable it
Yeah, it's a better name. I think the nr_interface_priority is actually
nr_interface_prioritizer. Do you think it is fine if I change all the
nr_interface_priority name in this patch to prioritizer?
http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/stun/stun_util.c (right):
http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/stun/stun_util.c:140: if
((r=nr_reg_get_transport_addr(children[i], 0, &addrs[i].addr)))
On 2013/07/01 14:05:49, ekr-webrtc wrote:
> I'm not sure this is right. Let's put an #error () here and a comment instead
Do you mean using a compile time error to replace the if statement? If this
piece of code is not expected to be compiled, could it better if we add the
#error at the next line of #if 0?
http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/ice/ice_ctx.c (right):
http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:175: int
nr_ice_ctx_set_interface_priority(nr_ice_ctx *ctx, nr_interface_priority *ip)
On 2013/07/02 08:32:32, kk1fff-patrick-wang wrote:
> On 2013/07/01 14:05:49, ekr-webrtc wrote:
> > Rename to something like "set_interface_prioritizer" I should think. Since
> we're
> > not setting the priority of an interface but rather the tool to enable it
>
> Yeah, it's a better name. I think the nr_interface_priority is actually
> nr_interface_prioritizer. Do you think it is fine if I change all the
> nr_interface_priority name in this patch to prioritizer?
Sure.
http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
File media/mtransport/third_party/nICEr/src/stun/stun_util.c (right):
http://firefox-codereview.appspot.com/22001/diff/1/media/mtransport/third_par...
media/mtransport/third_party/nICEr/src/stun/stun_util.c:140: if
((r=nr_reg_get_transport_addr(children[i], 0, &addrs[i].addr)))
On 2013/07/02 08:32:32, kk1fff-patrick-wang wrote:
> On 2013/07/01 14:05:49, ekr-webrtc wrote:
> > I'm not sure this is right. Let's put an #error () here and a comment
instead
>
> Do you mean using a compile time error to replace the if statement? If this
> piece of code is not expected to be compiled, could it better if we add the
> #error at the next line of #if 0?
You're right.
I would just do an #error with some comment that explains the situation
https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nricectx.cpp File media/mtransport/nricectx.cpp (right): https://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nricectx.cpp#newcode344 media/mtransport/nricectx.cpp:344: #ifndef USE_INTERFACE_PRIORITY_FALLBACK When is this going to be set? ...
I have updated the patch on bugzilla and have run unit test on Linux (platform
that uses prioritizer), but haven't been tested on Mac (platform that doesn't
use prioritizer), I will mark r? after tested on Mac.
The update includes: moving fallback code back to nICEr, use them when
prioritizer is not set; changing the implementation of prioritizer to set, but I
use a map to store preference.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nricec...
File media/mtransport/nricectx.cpp (right):
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nricec...
media/mtransport/nricectx.cpp:344: #ifndef USE_INTERFACE_PRIORITY_FALLBACK
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> When is this going to be set?
It will be set when build for DARWIN, Android, Windows platforms.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
File media/mtransport/nrinterfaceprioritizer.cpp (right):
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:18: explicit
LocalAddress(nr_local_addr* aLocalAddr)
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> const nr_local_addr&
Done.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:21: ,
mTypePreferance(GetNetworkTypePreference(aLocalAddr->interface.type)) {
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> Please initialize all members with sensible default values.
Done.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:21: ,
mTypePreferance(GetNetworkTypePreference(aLocalAddr->interface.type)) {
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> s/Preferance/Preference/ here and below.
Done.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:23:
nr_transport_addr_fmt_ifname_addr_string(&aLocalAddr->addr, buf, 50);
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> s/50/sizeof(buf)/
Done.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:77: typedef
std::vector<LocalAddress> LocalAddrList;
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> Why not use a std::set here? Sets are stored in sorted order, so you don't
need
> to do an explicit sort. And if you implement the comparison function
correctly,
> you can do .find().
>
> Here's the idea:
>
> 0. Add a "preference" value to LocalAddress
> 1. When someone stores, just store in the set.
> 2. when sort() is called, just walk through the list and store the preference
in
> LocalAddress. Set a bool so you know you've done this.
> 3. Use .find() to retrieve the preference for a given key. This will require a
> tiny bit of cleverness with the comparison function (have a dummy object that
> bypasses all checks but the key....)
>
> Alternately to steps 2 and 3, just have a map that stores the preferences by
> key.
>
> also, I don't think this typedef is helping much.
Done.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:83:
nr_transport_addr_fmt_ifname_addr_string(&aIface->addr, buf, 50);
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> s/50/sizeof(buf)
Done.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:87: // Local address with the same
key is already in mLocalAddrs.
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> I would prefer this generated an error.
Done.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:101: UCHAR tmpPref = 127;
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> Why are you starting at 127?
Just to reserve some space between upper bound.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:130: static int sort_preference(void
*obj) {
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> Please give these prefixed names, like nr_..._add_interface
These functions are used for building vtable and the function names can only be
used in this file. Do they still need prefix?
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:135: static int destroy(void **obj)
{
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> objp, not obj.
Done.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/nrinte...
media/mtransport/nrinterfaceprioritizer.cpp:147: static
nr_interface_prioritizer_vtbl priorizer_vtbl = {
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> prioritizer.
Done.
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/third_...
File media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c
(right):
http://firefox-codereview.appspot.com/22001/diff/9001/media/mtransport/third_...
media/mtransport/third_party/nICEr/src/net/nr_interface_prioritizer.c:68: if
(ifp->vtbl)
On 2013/07/17 14:09:52, ekr-webrtc wrote:
> How can ifp->vtbl == 0?
I think it's not possible if using these APIs correctly. I'll remove this line,
because even we check here, calling other functions will still result in seg
fault.
https://firefox-codereview.appspot.com/22001/diff/28003/media/mtransport/thir...
File media/mtransport/third_party/nICEr/src/ice/ice_component.c (right):
https://firefox-codereview.appspot.com/22001/diff/28003/media/mtransport/thir...
media/mtransport/third_party/nICEr/src/ice/ice_component.c:179: nr_socket *sock;
On 2013/08/19 03:56:27, kk1fff-patrick-wang wrote:
> On 2013/08/15 23:51:05, ekr-webrtc wrote:
> > This appears to still be nr_local_addr in the current patch.
>
> Since nr_stun_find_local_addresses takes nr_local_addr* as the first argument
in
> patch part 1, I have changed type of |addrs| to nr_local_addr. |addrs| also
> carries interface properties in supported platform.
That's what I am confused about. Why does this patch here have it as
nr_transport_addr? Or is rietveld just getting confused.
https://firefox-codereview.appspot.com/22001/diff/28003/media/mtransport/thir...
File media/mtransport/third_party/nICEr/src/ice/ice_component.c (right):
https://firefox-codereview.appspot.com/22001/diff/28003/media/mtransport/thir...
media/mtransport/third_party/nICEr/src/ice/ice_component.c:179: nr_socket *sock;
On 2013/08/19 21:05:16, ekr-webrtc wrote:
> On 2013/08/19 03:56:27, kk1fff-patrick-wang wrote:
> > On 2013/08/15 23:51:05, ekr-webrtc wrote:
> > > This appears to still be nr_local_addr in the current patch.
> >
> > Since nr_stun_find_local_addresses takes nr_local_addr* as the first
argument
> in
> > patch part 1, I have changed type of |addrs| to nr_local_addr. |addrs| also
> > carries interface properties in supported platform.
>
> That's what I am confused about. Why does this patch here have it as
> nr_transport_addr? Or is rietveld just getting confused.
Github seems to display this line correctly.
https://github.com/kk1fff/mozilla-central/blob/b825708/c43/media/mtransport/t...
Maybe we can try to import these patches to Rietveld again?
Ah.
Do you think you could just upload this to a new Rietveld issue?
-Ekr
On Mon, Aug 19, 2013 at 8:25 PM, <kk1fff@gmail.com> wrote:
>
> https://firefox-codereview.**appspot.com/22001/diff/28003/**
>
media/mtransport/third_party/**nICEr/src/ice/ice_component.c<https://firefox-codereview.appspot.com/22001/diff/28003/media/mtransport/third_party/nICEr/src/ice/ice_component.c>
> File media/mtransport/third_party/**nICEr/src/ice/ice_component.c (right):
>
> https://firefox-codereview.**appspot.com/22001/diff/28003/**
>
media/mtransport/third_party/**nICEr/src/ice/ice_component.c#**newcode179<https://firefox-codereview.appspot.com/22001/diff/28003/media/mtransport/third_party/nICEr/src/ice/ice_component.c#newcode179>
> media/mtransport/third_party/**nICEr/src/ice/ice_component.c:**179:
> nr_socket *sock;
> On 2013/08/19 21:05:16, ekr-webrtc wrote:
>
>> On 2013/08/19 03:56:27, kk1fff-patrick-wang wrote:
>> > On 2013/08/15 23:51:05, ekr-webrtc wrote:
>> > > This appears to still be nr_local_addr in the current patch.
>> >
>> > Since nr_stun_find_local_addresses takes nr_local_addr* as the first
>>
> argument
>
>> in
>> > patch part 1, I have changed type of |addrs| to nr_local_addr.
>>
> |addrs| also
>
>> > carries interface properties in supported platform.
>>
>
> That's what I am confused about. Why does this patch here have it as
>> nr_transport_addr? Or is rietveld just getting confused.
>>
>
> Github seems to display this line correctly.
> https://github.com/kk1fff/**mozilla-central/blob/b825708/**
>
c43/media/mtransport/third_**party/nICEr/src/ice/ice_**component.c#L178<https://github.com/kk1fff/mozilla-central/blob/b825708/c43/media/mtransport/third_party/nICEr/src/ice/ice_component.c#L178>
> Maybe we can try to import these patches to Rietveld again?
>
>
https://firefox-codereview.**appspot.com/22001/<https://firefox-codereview.ap...>
>
Issue 22001: Issue 825708: use interface properties to determine priorities
Created 7 years, 6 months ago by ekr-webrtc
Modified 7 years, 5 months ago
Reviewers: ekr-webrtc, kk1fff-patrick-wang, ekr
Base URL:
Comments: 78
