Closed Bug 909179 Opened 6 years ago Closed 6 years ago

Disable one of the components in an NrIceMediaStream

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: ekr, Assigned: ehugg)

References

Details

Attachments

(1 file, 7 obsolete files)

In a number of cases (e.g. rtcp-mux) we want to only use one of the components in a media stream.
Attached patch Disable components (obsolete) — Splinter Review
Assignee: nobody → ekr
Status: NEW → ASSIGNED
Comment on attachment 795239 [details] [diff] [review]
Disable components

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

Ethan, this is a WIP patch. It needs the patch from bug 842549
Attachment #795239 - Flags: feedback?(ethanhugg)
Comment on attachment 795239 [details] [diff] [review]
Disable components


Just used this patch to make a successful rtcp-mux connection with Canary 31.  I had to make two changes though.

1. I took ice_unittest out of the moz.build because it didn't build.
2. I changed nr_ice_media_stream_disable_component to not throw an error if the component was already disabled.  I will look into why we were negotiating media and disabling this twice, but I think this makes sense to do anyway.
Attachment #795239 - Flags: feedback?(ethanhugg) → feedback+
Blocks: 907353
Can you paste the ice_unittest errors?
64-bit Ubuntu build:

/home/ehugg/mozilla/work/media/mtransport/test/ice_unittest.cpp: In member function ‘void {anonymous}::IceTestPeer::CandidateInitialized(mozilla::NrIceMediaStream*, const string&)’:
/home/ehugg/mozilla/work/media/mtransport/test/ice_unittest.cpp:321:61: error: no matching function for call to ‘find(std::vector<mozilla::RefPtr<mozilla::NrIceMediaStream> >::iterator, std::vector<mozilla::RefPtr<mozilla::NrIceMediaStream> >::iterator, mozilla::NrIceMediaStream*&)’
/home/ehugg/mozilla/work/media/mtransport/test/ice_unittest.cpp:321:61: note: candidate is:
/usr/include/c++/4.6/bits/streambuf_iterator.h:359:5: note: template<class _CharT2> typename __gnu_cxx::__enable_if<std::__is_char<_CharT2>::__value, std::istreambuf_iterator<_CharT2, std::char_traits<_CharT> > >::__type std::find(std::istreambuf_iterator<_CharT2, std::char_traits<_CharT> >, std::istreambuf_iterator<_CharT2, std::char_traits<_CharT> >, const _CharT2&)
In file included from /home/ehugg/mozilla/work/media/mtransport/test/ice_unittest.cpp:33:0:
/home/ehugg/mozilla/work/media/webrtc/trunk/testing/gtest/include/gtest/gtest.h: In function ‘testing::AssertionResult testing::internal::CmpHelperLT(const char*, const char*, const T1&, const T2&) [with T1 = int, T2 = long unsigned int]’:
/home/ehugg/mozilla/work/media/mtransport/test/ice_unittest.cpp:376:167:   instantiated from here
/home/ehugg/mozilla/work/media/webrtc/trunk/testing/gtest/include/gtest/gtest.h:1536:136: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
make[5]: *** [ice_unittest.o] Error 1
I should mention that the ice_unittests build and run fine for me on OSX.  

Let me know if you need me to figure out this Linux build problem.

Also, the change I needed to ice_media_stream.c is in the work-in-progress patch to bug 907353.
Try #include <algorithm>
Yep. I added #include <algorithm> after the #include <vector> line in ice_unittest.cpp and it now build and runs fine on my Linux box.

(In reply to Eric Rescorla (:ekr) from comment #7)
> Try #include <algorithm>
Attachment #795239 - Attachment is obsolete: true
Comment on attachment 797389 [details] [diff] [review]
Add ability to disable ICE components


Re-based this patch on latest M-C so that it no longer needs the patch from bug 842549.  I just solved the merge issues and brought forward the static function nr_ice_peer_ctx_find_pstream.  

It seems to still work with the patch from bug 907353.
Attachment #797389 - Flags: review?(ekr)
Attachment #797389 - Flags: review?(adam)
Comment on attachment 797389 [details] [diff] [review]
Add ability to disable ICE components

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

Hard to review one's own code but this looks pretty much like what I wrote.

Adam, do you think we should forbid disabling component n unless components [1..n-1] are also disabled?

::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
@@ +232,5 @@
> +static int nr_ice_peer_ctx_find_pstream(nr_ice_peer_ctx *pctx, nr_ice_media_stream *stream, nr_ice_media_stream **pstreamp)
> +  {
> +    int _status;
> +    nr_ice_media_stream *pstream;
> +    /* Because we don't have forward pointers, iterate through all the

Whitespace please.

@@ +361,5 @@
> +int nr_ice_peer_ctx_disable_component(nr_ice_ctx *ctx, nr_ice_peer_ctx *pctx, nr_ice_media_stream *lstream, int component_id)
> +  {
> +    int r, _status;
> +    nr_ice_media_stream *pstream;
> +    nr_ice_component *component;

Let's mark ctx as unused.

@@ +363,5 @@
> +    int r, _status;
> +    nr_ice_media_stream *pstream;
> +    nr_ice_component *component;
> +
> +    int j;

Extra whitespace.

::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.h
@@ +77,5 @@
>  int nr_ice_peer_ctx_log_state(nr_ice_peer_ctx *pctx);
>  int nr_ice_peer_ctx_stream_done(nr_ice_peer_ctx *pctx, nr_ice_media_stream *stream);
>  int nr_ice_peer_ctx_find_component(nr_ice_peer_ctx *pctx, nr_ice_media_stream *str, int component_id, nr_ice_component **compp);
>  int nr_ice_peer_ctx_deliver_packet_maybe(nr_ice_peer_ctx *pctx, nr_ice_component *comp, nr_transport_addr *source_addr, UCHAR *data, int len);
> +    int nr_ice_peer_ctx_disable_component(nr_ice_ctx *ctx, nr_ice_peer_ctx *pctx, nr_ice_media_stream *lstream, int component_id);

Indentation.
Attachment #797389 - Flags: review?(ekr) → review+
>> +int nr_ice_peer_ctx_disable_component(nr_ice_ctx *ctx, nr_ice_peer_ctx *pctx,  
>     nr_ice_media_stream *lstream, int component_id)
>Let's mark ctx as unused.

If ctx is unused shouldn't I just remove it?  Otherwise, when you say mark as unused, do you mean add "/* unused */"?
Attachment #797389 - Attachment is obsolete: true
Attachment #797389 - Flags: review?(adam)
Comment on attachment 797512 [details] [diff] [review]
Add ability to disable ICE components


Fixed EKR's self-review nits.
Attachment #797512 - Flags: review?(adam)
Attached patch interdiff since EKR's review. (obsolete) — Splinter Review
Blocks: 901560
Unrotting patch
Attachment #797512 - Attachment is obsolete: true
Attachment #797512 - Flags: review?(adam)
Attachment #799685 - Flags: review?(adam)
Comment on attachment 799685 [details] [diff] [review]
Add ability to disable ICE components

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

Looks good to me. One nit below, and a question about state guarantees.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.h
@@ +54,5 @@
>  #define NR_ICE_COMPONENT_UNPAIRED           0
>  #define NR_ICE_COMPONENT_RUNNING            1
>  #define NR_ICE_COMPONENT_NOMINATED          2
>  #define NR_ICE_COMPONENT_FAILED             3
> +#define NR_ICE_COMPONENT_DISABLED           4

I think we want to add this new value to the switch statement in nr_ice_component_failed_pair() so as to avoid asserts.

Also, even though it shouldn't show up in nr_ice_component_process_incoming_check(), we probably do want to have some kind of sanity checking in there in case we do get a check on a disabled component (due to odd behavior on the other end).

::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
@@ +211,5 @@
> +    if (comp->state == NR_ICE_COMPONENT_DISABLED) {
> +      r_log(LOG_ICE,LOG_ERR,"Peer offered candidates for disabled remote component");
> +      ABORT(R_BAD_DATA);
> +    }
> +    if (comp->local_component->state == NR_ICE_COMPONENT_DISABLED) {

Are we guaranteed to have a local component at this point?
Attachment #799685 - Flags: review?(adam) → review+
(In reply to Adam Roach [:abr] from comment #17)
> Comment on attachment 799685 [details] [diff] [review]
> Add ability to disable ICE components
> 
> Review of attachment 799685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. One nit below, and a question about state guarantees.
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_component.h
> @@ +54,5 @@
> >  #define NR_ICE_COMPONENT_UNPAIRED           0
> >  #define NR_ICE_COMPONENT_RUNNING            1
> >  #define NR_ICE_COMPONENT_NOMINATED          2
> >  #define NR_ICE_COMPONENT_FAILED             3
> > +#define NR_ICE_COMPONENT_DISABLED           4
> 
> I think we want to add this new value to the switch statement in
> nr_ice_component_failed_pair() so as to avoid asserts.
> 
> Also, even though it shouldn't show up in
> nr_ice_component_process_incoming_check(), we probably do want to have some
> kind of sanity checking in there in case we do get a check on a disabled
> component (due to odd behavior on the other end).

Sure.


> ::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
> @@ +211,5 @@
> > +    if (comp->state == NR_ICE_COMPONENT_DISABLED) {
> > +      r_log(LOG_ICE,LOG_ERR,"Peer offered candidates for disabled remote component");
> > +      ABORT(R_BAD_DATA);
> > +    }
> > +    if (comp->local_component->state == NR_ICE_COMPONENT_DISABLED) {
> 
> Are we guaranteed to have a local component at this point?

I believe so. Here's the place where the media stream is created:

    /*
      Note: use component_ct from our own stream since components other
      than this offered by the other side are unusable */
    if(r=nr_ice_media_stream_create(pctx->ctx,stream->label,stream->component_ct,&pstream))
      ABORT(r);

    /* Match up the local and remote components */
    comp=STAILQ_FIRST(&stream->components);
    comp2=STAILQ_FIRST(&pstream->components);
    while(comp){
      comp2->local_component=comp;

      comp=STAILQ_NEXT(comp,entry);
      comp2=STAILQ_NEXT(comp2,entry);
    }

And component_ct should match the number of actual components
in the local stream.
This currently fails the unit tests. The problem is that we enumerate
all the components to see what pair is active, but we now have disabled
streams. (The code to check was introduced recently).

 void DumpAndCheckActiveCandidates() {
    std::cerr << "Active candidates:" << std::endl;
    for (size_t i=0; i < streams_.size(); ++i) {
      for (int j=0; j < streams_[i]->components(); ++j) {
        std::cerr << "Stream " << i << " component " << j+1 << std::endl;

        NrIceCandidate *local;
        NrIceCandidate *remote;

        nsresult res = streams_[i]->GetActivePair(j+1, &local, &remote);
        ASSERT_TRUE(NS_SUCCEEDED(res));
        DumpCandidate("Local  ", *local);
        ASSERT_EQ(expected_local_type_, local->type);
        DumpCandidate("Remote ", *remote);
        ASSERT_EQ(expected_remote_type_, remote->type);
        delete local;
        delete remote;
      }
    }
  }
Attachment #797514 - Attachment is obsolete: true
> > +#define NR_ICE_COMPONENT_DISABLED           4
> 
> I think we want to add this new value to the switch statement in
> nr_ice_component_failed_pair() so as to avoid asserts.
> 

The switch in nr_ice_component_failed_pair takes the values that start with NR_ICE_PAIR_STATE_ defined in ice_candidate_pair.h which there is not a new DISABLED value added.
Attachment #799685 - Attachment is obsolete: true
Assignee: ekr → ethanhugg
Comment on attachment 800329 [details] [diff] [review]
Add ability to disable ICE components


Fixed the unittest by adding a new return value to the GetActivePair calls and added a check for DISABLED component at the top of nr_ice_component_process_incoming_check.
Attachment #800329 - Flags: review?(ekr)
Comment on attachment 800330 [details] [diff] [review]
Interdiff between latest and previously reviewed patch

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

r+ with change to R_REJECTED

::: third/media/mtransport/nricemediastream.cpp
@@ +209,4 @@
>                                       stream_,
>                                       component,
>                                       &local_int, &remote_int);
> +  // If result is R_EOD then component is unpaired or disabled

Period after comment.

@@ +209,5 @@
>                                       stream_,
>                                       component,
>                                       &local_int, &remote_int);
> +  // If result is R_EOD then component is unpaired or disabled
> +  if (r == R_EOD)

Change to R_REJECTED

::: third/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ +744,5 @@
>        ABORT(r);
>  
> +    if (comp->state == NR_ICE_COMPONENT_UNPAIRED ||
> +        comp->state == NR_ICE_COMPONENT_DISABLED)
> +      ABORT(R_EOD);

Let's use R_REJECTED here.
Attachment #800330 - Flags: review+
Attachment #800329 - Attachment is obsolete: true
Attachment #800329 - Flags: review?(ekr)
Comment on attachment 800345 [details] [diff] [review]
Add ability to disable ICE components

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

Changed R_EOD to R_REJECTED.  Bringing forward r+ from abr and ekr.
Attachment #800345 - Flags: review+
Attachment #800330 - Attachment is obsolete: true
Looks like this line:
  void DisableComponent(int stream, int component_id) {
    ASSERT_LT(stream, streams_.size());
throws a warning which is fatal on Android builds:

/builds/slave/m-in-and-d-0000000000000000000/build/android-ndk/toolchains/arm-linux-androideabi-4.7/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.7/../../../../arm-linux-androideabi/bin/as: /lib64/libz.so.1: no version information available (required by /builds/slave/m-in-and-d-0000000000000000000/build/android-ndk/toolchains/arm-linux-androideabi-4.7/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.7/../../../../arm-linux-androideabi/bin/as)
In file included from ../../../../media/mtransport/test/ice_unittest.cpp:34:0:
../../../../media/webrtc/trunk/testing/gtest/include/gtest/gtest.h: In instantiation of 'testing::AssertionResult testing::internal::CmpHelperLT(const char*, const char*, const T1&, const T2&) [with T1 = int; T2 = unsigned int]':
../../../../media/mtransport/test/ice_unittest.cpp:393:167:   required from here
../../../../media/webrtc/trunk/testing/gtest/include/gtest/gtest.h:1536:136: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
cc1plus: all warnings being treated as errors
make[5]: *** [ice_unittest.o] Error 1
Evidently the default for these builds is now

ac_add_options --enable-warnings-as-errors

so we should add this to our mozconfigs.
https://hg.mozilla.org/mozilla-central/rev/85b39648b037
https://hg.mozilla.org/mozilla-central/rev/963ee7a9b46c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.