818680 - Tests to verify media negotiation failure scenarios on Peer Connection

Status: RESOLVED DUPLICATE of bug 1142320

(Core :: WebRTC: Signaling, defect)

Description

[Suhas]

More cases are needed to verify signaling failure and its impact on the media flows on Peer Connections.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

About which type of tests we are talking about here? Are those unit tests?

Comment 2

[Suhas]

Yes .. these are signaling unit tests.

Comment 3

[Suhas]

Attachment: PC Unit-Tests to verify media validity.

Comment 4

[Suhas]

Attachment: PC Unit-Tests to verify media validity.

Comment 5

[Ethan Hugg [:ehugg]]

[ RUN      ] SignalingTest.OneWayAudioSendOnly

This test often deadlocks or crashes on my Linux64 box.  Here's a callstack:

#0  0x00007ffff0410993 in std::_Rb_tree_increment(std::_Rb_tree_node_base*) ()
   from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#1  0x00000000005d574d in std::_Rb_tree_const_iterator<Fake_MediaStreamListener*>::operator++ (this=0x7fffe75fd7b0)
    at /usr/include/c++/4.6/bits/stl_tree.h:269
#2  0x00000000005c729f in Fake_SourceMediaStream::Periodic (
    this=0x7fffd6684470)
    at /home/ehugg/mozilla/mozilla-central/media/webrtc/signaling/test/FakeMediaStreamsImpl.h:52
#3  0x00000000005c77a0 in Fake_MediaPeriodic::Notify (this=0x7fffacfffc70, 
    timer=0x7fffae5f99a0)
    at /home/ehugg/mozilla/mozilla-central/media/webrtc/signaling/test/FakeMediaStreamsImpl.h:119
#4  0x00007ffff4b388a6 in nsTimerImpl::Fire (this=0x7fffae5f99a0)
    at /home/ehugg/mozilla/mozilla-central/xpcom/threads/nsTimerImpl.cpp:485
#5  0x00007ffff4b38c8a in nsTimerEvent::Run (this=0x7fffe945d128)
    at /home/ehugg/mozilla/mozilla-central/xpcom/threads/nsTimerImpl.cpp:565
#6  0x00007ffff4b30a63 in nsThread::ProcessNextEvent (this=0x7fffe941a120, 
    mayWait=true, result=0x7fffe75fda1f)
    at /home/ehugg/mozilla/mozilla-central/xpcom/threads/nsThread.cpp:627
#7  0x00007ffff4abe06a in NS_ProcessNextEvent_P (thread=0x7fffe941a120, 
    mayWait=true)
    at /home/ehugg/mozilla/mozilla-central/obj-ff-dbg/xpcom/build/nsThreadUtils.cpp:221
#8  0x00007ffff2f2e36e in nsSocketTransportService::Run (this=0x7fffe946a2c0)
    at /home/ehugg/mozilla/mozilla-central/netwerk/base/src/nsSocketTransportService2.cpp:653
#9  0x00007ffff4b30a63 in nsThread::ProcessNextEvent (this=0x7fffe941a120, 
    mayWait=true, result=0x7fffe75fdb7f)
    at /home/ehugg/mozilla/mozilla-central/xpcom/threads/nsThread.cpp:627
#10 0x00007ffff4abe06a in NS_ProcessNextEvent_P (thread=0x7fffe941a120, 
    mayWait=true)
    at /home/ehugg/mozilla/mozilla-central/obj-ff-dbg/xpcom/build/nsThreadUtils.cpp:221
#11 0x00007ffff4b2f8be in nsThread::ThreadFunc (arg=0x7fffe941a120)
    at /home/ehugg/mozilla/mozilla-central/xpcom/threads/nsThread.cpp:265
#12 0x00007ffff21ebf19 in _pt_root (arg=0x7fffe94147b0)
    at /home/ehugg/mozilla/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:156
#13 0x00007ffff7bc4e9a in start_thread (arg=0x7fffe75fe700)
    at pthread_create.c:308
#14 0x00007fffefbc8cbd in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#15 0x0000000000000000 in ?? ()

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: protect access to FakeMedia mListeners

I suspect reasonably strongly that the root cause is lack of locking around mListener access - Ethan, please give this a try

Comment 7

[Suhas]

With this patch  I was able to run the test-cases more than 50 times with no deadlocks and crashes on my OsX. I am bringing up my Linux Box. I will update what i find there ...

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 690173 [details]
protect access to FakeMedia mListeners

Review of attachment 690173 [details]:
-----------------------------------------------------------------

lgtm - can't make it crash now.

Comment 9

[Ethan Hugg [:ehugg]]

Comment on attachment 689497 [details]
PC Unit-Tests to verify media validity.

Works now with the fake media mutex in the second patch.

Comment 10

[Eric Rescorla (:ekr)]

Comment on attachment 689497 [details]
PC Unit-Tests to verify media validity.

Review of attachment 689497 [details]:
-----------------------------------------------------------------

I'm not following what the dummy's do here.

What we want is to be sending valid video and audio but not negotiate them.

I think what these tests should do is modify the SDP so that the media
negotiation fails.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +555,3 @@
>  
> +    nsRefPtr<nsDOMMediaStream> domMediaStream;
> +    if(audioFlags == CALLER_VALID_AUDIO) {

Why don't we have a check for CALLER_VALID_VIDEO

@@ +604,5 @@
> +                    uint32_t audioFlags = CALLEE_DUMMY_AUDIO,
> +                    uint32_t videoFlags = CALLEE_DUMMY_VIDEO) {
> +
> +    nsRefPtr<nsDOMMediaStream> domMediaStream;
> +    if(audioFlags == CALLEE_VALID_AUDIO) {

Can you refactor these so it's not cut-and-paste

Comment 11

[Suhas]

Comment on attachment 689497 [details]
PC Unit-Tests to verify media validity.

Review of attachment 689497 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +555,3 @@
>  
> +    nsRefPtr<nsDOMMediaStream> domMediaStream;
> +    if(audioFlags == CALLER_VALID_AUDIO) {

The dummy's here refer to the code we had already.  Initially for FullCall and Trickle Scenario we were adding AudioStream to the first PC and Fake_DOMMediaStream to the other. So Audio was always one way sent and received. I am keeping those test-cases as-is. In the new cases added, we do send 2 way valid audio (AudioSourceStreams) and manipulate SDP to do either - sendonly, recvonly, negotiation failure to verify if the media behaves as expected.

As far as video is concerned, I haven't changed anything.

@@ +604,5 @@
> +                    uint32_t audioFlags = CALLEE_DUMMY_AUDIO,
> +                    uint32_t videoFlags = CALLEE_DUMMY_VIDEO) {
> +
> +    nsRefPtr<nsDOMMediaStream> domMediaStream;
> +    if(audioFlags == CALLEE_VALID_AUDIO) {

Will Do

Comment 13

[Suhas]

Attachment: Tests to verify media negotiation failure scenarios on Peer Connection

Comment 14

[Suhas]

Comment on attachment 724736 [details] [diff] [review]
Tests to verify media negotiation failure scenarios on Peer Connection

This patch depends on patch from Bug 848189 to be applied to have Fake_VideoSource code. 
New things in this patch include:
1. Added Fake_AVSource to have audio + video flows
2. Added lot of new tests to verify media flows.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 724736 [details] [diff] [review]
Tests to verify media negotiation failure scenarios on Peer Connection

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

r-, though looking good.  I'm open to arguments from the other reviewers about the timing and audio chunk size issues, but currently the tests seem unrealistic and likely to cause NetEQ on the receiving side to be unhappy.  Having moderately realistic sources here should do a better job of testing the overall system, especially once we start allowing other rates and start passing in timestamps to GIPS.

If we ignore the audio chunk size issue (leave it at 100ms and don't pace it with a timer) this patch mostly has nits to address.  I would be ok with breaking that issue out into a separate bug, and if we do that, r+ with nits to fix.  Derf may have an opinion on this as well.

::: media/webrtc/signaling/test/FakeMediaStreamsImpl.h
@@ +167,5 @@
> +  for(std::set<Fake_MediaStreamListener *>::iterator it = mListeners.begin();
> +       it != mListeners.end(); ++it) {
> +    (*it)->NotifyQueuedTrackChanges(NULL,
> +                                    0,
> +                                    90000,

The rate we use for gUM and other video streams currently is USECS_PER_S (aka 1000000).   90000 is the RTP timestamp rate

@@ +186,5 @@
> +  //Generate Signed 16 Bit Audio samples
> +  nsRefPtr<mozilla::SharedBuffer> samples =
> +    mozilla::SharedBuffer::Create(AUDIO_BUFFER_SIZE * NUM_CHANNELS * sizeof(int16_t));
> +  int16_t* data = reinterpret_cast<int16_t *>(samples->Data());
> +  for(int i=0; i<(1600*2); i++) {

AUDIO_BUFFER_SIZE, not 1600.

On a related note, we should be using a realistic audio buffer size, not 1600.  Currently we use 10ms chunks from gUM.  The buffer size can be larger, but we should put in 10ms of data and feed it to NotifyQueuedTrackChanges each time. (Later we can modify to allow other rates and sizes.)

10ms at 16000Hz is 160 samples.

Ditto for plain audio generation.

@@ +195,5 @@
> +
> +  mozilla::AudioSegment aSegment;
> +  nsAutoTArray<const int16_t *,1> channels;
> +  channels.AppendElement(data);
> +  aSegment.AppendFrames(samples.forget(), channels, AUDIO_BUFFER_SIZE);

number of samples generated (per comment above)

@@ +199,5 @@
> +  aSegment.AppendFrames(samples.forget(), channels, AUDIO_BUFFER_SIZE);
> +
> +  //generate video image
> +  nsRefPtr<Fake_Image> image = new Fake_Image();
> +  memset(image->mData.get(),IMAGE_COLOR,image->YUVSize());

space after commas

@@ +210,5 @@
>    for(std::set<Fake_MediaStreamListener *>::iterator it = mListeners.begin();
>         it != mListeners.end(); ++it) {
> +    (*it)->NotifyQueuedTrackChanges(NULL,
> +                                    0,
> +                                    16000,

Make this a constant up top, and in plain audio generation as well.

@@ +216,5 @@
> +                                    0,
> +                                    aSegment);
> +    (*it)->NotifyQueuedTrackChanges(NULL,
> +                                    0,
> +                                    90000,

USECS_PER_S

@@ +220,5 @@
> +                                    90000,
> +                                    0,
> +                                    0,
> +                                    vSegment);
> +  }

General comment on the AV generation: we don't want one video frame per audio buffer (especially if we switch to 10ms audio).  This was generating the equivalent to 100ms video, so for 10ms audio, a video frame every 10th audio frame is equivalent.

Also: code in GIPS for audio sending and receiving is time-aware (timebase correction, etc).  You should send 10ms of audio (and maybe video) and then wait for the next 10ms repeating timer kick.  Poor man's version would be 10ms sleep (which will have some drift, but within reason usually).  This also means we should be careful in using these test about how long they run.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +646,5 @@
>      nsresult ret;
>      uint32_t aHintContents = 0;
>      nsRefPtr<DOMMediaStream> domMediaStream;
> +    // Do we want Audio + Video Stream ?
> +    if((offerFlags & OFFER_AUDIO) && (offerFlags & OFFER_VIDEO)) {

While there are a few spots that do it this way: space after if's before (
(throughout patch)

@@ +651,5 @@
> +      std::cout << "CreateOffer: Stream with Audio + Video Tracks" << std::endl;
> +      aHintContents |= DOMMediaStream::HINT_CONTENTS_AUDIO;
> +      aHintContents |= DOMMediaStream::HINT_CONTENTS_VIDEO;
> +      Fake_AVStreamSource *av_stream =
> +                                 new Fake_AVStreamSource();

one line

@@ +665,3 @@
>        // Create a media stream as if it came from GUM
>        Fake_AudioStreamSource *audio_stream =
> +                                 new Fake_AudioStreamSource();

one line (yes I know it was two).  Also make sure there are no tabs (hard to tell in splinter).

@@ +676,3 @@
>        aHintContents |= DOMMediaStream::HINT_CONTENTS_VIDEO;
> +      Fake_VideoStreamSource *video_stream
> +                              = new Fake_VideoStreamSource();

one line

@@ +2112,5 @@
> +
> +  ASSERT_GE(a1_.GetPacketsSent(0), 40);
> +
> +  ASSERT_GE(a1_.GetPacketsReceived(0), 0);
> +  ASSERT_GE(a2_.GetPacketsReceived(0), 40);

If we switch to 10ms audio, some of these will need to be changed (same for plain audio tests), probably to 400.
With 10ms audio and 100ms video, video packet numbers should be different than audio (1/10) in a time period (and these are minimums), so video would stay at 40.

kDefaultTimeout is 5s, so we sleep for 10s, which *should* be enough time to transfer 4s worth of audio/video.

Comment 16

[Byron Campen [:bwc]]

It looks like this patch has some of the work for bug 1142320 in it. This may be worth resurrecting.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Byron Campen [:bwc] from comment #16)
> It looks like this patch has some of the work for bug 1142320 in it. This
> may be worth resurrecting.

Yes.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Let's combine these into one bug, and we can use this patch as a starting point