856397 - test_peerConnection_bug834153.html doesn't ensure there are streams for createOffer()

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

Attachment: Fix to force offering reception of audio

With landing bug 837035 and preparing to land bug 855623, a bug in dom/media/tests/mochitest/test_peerConnection_bug834153.html became apparent.  Nothing in the test is ensuring that there's a media element (for generating an m= line).  This worked before, because we always offered m=application for datachannels.  With the update from bug 855623, we no longer do that if no one called createDataChannel before createOffer().

Marking for jsmith/henrik, but for this I'll take a review from anyone cc'd on this bug.

Comment 1

[Jason Smith [:jsmith]]

Comment on attachment 731614 [details] [diff] [review]
Fix to force offering reception of audio

I don't entirely understand the problem this is solving.

To my understanding, providing a media constraint to OfferToReceiveAudio with a mandatory true means that you are expecting the remote peer to provide an audio stream. But this test doesn't have anything to do with media streams. So why is this necessary?

Comment 2

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

When you create an offer in SIPCC/signaling, there must be at least one m-line in the offer.  I.e. you must agree to at least receive something.  I should note that SDP (RFC 4566) doesn't require an m-line, but many SDP-using devices do, even if it's recvonly or inactive.  There have been no statements about whether WebRTC would require m= lines.

Before, we always offered an m=application line (DataChannels), since we had no idea if the JS app would want to use DataChannels or not.  (This has caused problems with Chrome interop.)  So since there always was an m= line, you could just do "pc=new RTCPeerConnection(); pc.createOffer();"

Now with the bug 837035 adding proper SDP support, and the patches about to land in bug 855623 which hook up the JS API as proposed in the W3 WG, we only add m=application line if you call pc.createDataChannel() *before* createOffer().  So the sequence in the original test here would have no offers to receive or send media, which meant no m=lines, which kicks out as a failure to produce SDP in the signaling code.

We can change this test now (since this isn't trying to test no-m-lines), and file a bug against sipcc to allow generating SDP with no m= lines (and when landed add a specific test for that, or expand this test back to cover it - unless there's agreement in the IETF WG that we need to have m=lines.  

We may need 1 m= line to allow verification of identity (though you can argue if that's important if no media is being exchanged, but maybe it is).

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 731614 [details] [diff] [review]
Fix to force offering reception of audio

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

lgtm

Comment 4

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #2)
> When you create an offer in SIPCC/signaling, there must be at least one
> m-line in the offer.  I.e. you must agree to at least receive something.  I
> should note that SDP (RFC 4566) doesn't require an m-line, but many
> SDP-using devices do, even if it's recvonly or inactive.  There have been no
> statements about whether WebRTC would require m= lines.

Hmm...okay. So some followup questions then:

1. Will this affect the workflow of existing tests in test_peerConnection_bug835370.html? There's no stream logic going on in that test, so couldn't the test logic get altered unexpectedly?

2. Is createAnswer affected by the same rules? Why do we not have to declare a mandatory constraint for createAnswer in this test as well?

> 
> Before, we always offered an m=application line (DataChannels), since we had
> no idea if the JS app would want to use DataChannels or not.  (This has
> caused problems with Chrome interop.)  So since there always was an m= line,
> you could just do "pc=new RTCPeerConnection(); pc.createOffer();"
> 
> Now with the bug 837035 adding proper SDP support, and the patches about to
> land in bug 855623 which hook up the JS API as proposed in the W3 WG, we
> only add m=application line if you call pc.createDataChannel() *before*
> createOffer().  So the sequence in the original test here would have no
> offers to receive or send media, which meant no m=lines, which kicks out as
> a failure to produce SDP in the signaling code.

FWIW, in a separate bug, we should get automated tests then covering the scenarios you just described - both positive and negative cases.

Comment 5

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

(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Randell Jesup [:jesup] from comment #2)
> > When you create an offer in SIPCC/signaling, there must be at least one
> > m-line in the offer.  I.e. you must agree to at least receive something.  I
> > should note that SDP (RFC 4566) doesn't require an m-line, but many
> > SDP-using devices do, even if it's recvonly or inactive.  There have been no
> > statements about whether WebRTC would require m= lines.
> 
> Hmm...okay. So some followup questions then:
> 
> 1. Will this affect the workflow of existing tests in
> test_peerConnection_bug835370.html? There's no stream logic going on in that
> test, so couldn't the test logic get altered unexpectedly?

Yes, and a bunch of those are failing and not getting caught (which may point out other problems in that test).

> 2. Is createAnswer affected by the same rules? Why do we not have to declare
> a mandatory constraint for createAnswer in this test as well?

No, createAnswer always has the same number of m-lines as the offer (even if rejected).
 
> > Before, we always offered an m=application line (DataChannels), since we had
> > no idea if the JS app would want to use DataChannels or not.  (This has
> > caused problems with Chrome interop.)  So since there always was an m= line,
> > you could just do "pc=new RTCPeerConnection(); pc.createOffer();"
> > 
> > Now with the bug 837035 adding proper SDP support, and the patches about to
> > land in bug 855623 which hook up the JS API as proposed in the W3 WG, we
> > only add m=application line if you call pc.createDataChannel() *before*
> > createOffer().  So the sequence in the original test here would have no
> > offers to receive or send media, which meant no m=lines, which kicks out as
> > a failure to produce SDP in the signaling code.
> 
> FWIW, in a separate bug, we should get automated tests then covering the
> scenarios you just described - both positive and negative cases.

Agreed, absolutely - perhaps via 835370 (evolved)

Comment 6

[Jason Smith [:jsmith]]

Comment on attachment 731614 [details] [diff] [review]
Fix to force offering reception of audio

review+ then for updating this test. lgtm after getting questions answered.

Sounds like there could be potentially other tests like the one mentioned in comment above that need updating (maybe the crash tests as well? we might want to check if any of those are affected). Can you file bugs for updating those tests as well?

Clearing review on whimboo cause I think we've got enough r+s here to land.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e0379d69dd0

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e0379d69dd0