803318 - Improved handling of constraints and more tests

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[enda mannion (:emannion)]

Attachment: Impoved handling of hints and more tests v1

This patch is needed as it allows handling on offer with no audio and a number of other fixes.  It also included a wider array of tests.

Comment 1

[Eric Rescorla (:ekr)]

Comment on attachment 672985 [details] [diff] [review]
Impoved handling of hints and more tests v1

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

This doesn't seem to have tests for offer...

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +3927,5 @@
>   */
>  cc_causes_t
>  gsmsdp_negotiate_media_lines (fsm_fcb_t *fcb_p, cc_sdp_t *sdp_p,
> +                             boolean initial_offer, boolean offer, boolean notify_stream_added,
> +                             boolean has_audio, boolean has_video, boolean has_data, boolean createanswer)

Where are these used?

Comment 2

[enda mannion (:emannion)]

Attachment: Impoved handling of hints and more tests

Applied comment to the patch.  Good catch.

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 673270 [details] [diff] [review]
Impoved handling of hints and more tests

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

Looks good to me.  Tried it out on Linux64 and OSX.

Comment 4

[enda mannion (:emannion)]

Attachment: Impoved handling of hints and more tests

Re-merged to m-c head

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 674620 [details] [diff] [review]
Impoved handling of hints and more tests

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

I haven't really gone over the tests. I have one question below. If you think that works, and ehugg is happy, I think you should commit it.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3054,5 @@
>  
>      /* TODO([e-mail]): The second true is because we are acting as if we are
>         processing an offer. The first, however, is for an initial offer and we may
>         want to set that conditionally. */
> +    cause = gsmsdp_negotiate_media_lines(fcb, dcb->sdp, TRUE, TRUE, FALSE, TRUE);

Does this still produce onaddstream callbacks?

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 674620 [details] [diff] [review]
Impoved handling of hints and more tests

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

Reassigned the review to ehugg

Comment 7

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #5)
> Comment on attachment 674620 [details] [diff] [review]
> Impoved handling of hints and more tests
> 
> Review of attachment 674620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't really gone over the tests. I have one question below. If you
> think that works, and ehugg is happy, I think you should commit it.
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +3054,5 @@
> >  
> >      /* TODO([e-mail]): The second true is because we are acting as if we are
> >         processing an offer. The first, however, is for an initial offer and we may
> >         want to set that conditionally. */
> > +    cause = gsmsdp_negotiate_media_lines(fcb, dcb->sdp, TRUE, TRUE, FALSE, TRUE);
> 
> Does this still produce onaddstream callbacks?

Yes, still works.

Comment 8

[Eric Rescorla (:ekr)]

This code does not seem to handle the case where *no* constraints are provided correctly,
at least based on my tests with the browser.

Please add tests along the following lines:


TEST_F(SignalingTest, OfferAnswerDontAddAudioStreamOnAnswerNoConstraints)
{
  sipcc::MediaConstraints offerconstraints;
  offerconstraints.setBooleanConstraint("OfferToReceiveAudio", true, false);
  offerconstraints.setBooleanConstraint("OfferToReceiveVideo", true, false);
  sipcc::MediaConstraints answerconstraints;
  OfferAnswer(offerconstraints, answerconstraints, true, true, false, true, false);
}

This does not appear to generate the right answer.

Comment 9

[Eric Rescorla (:ekr)]

Here is what I get when I run that test. note that I had to do some porting to make the test harness work, but it still looks to me like something is wrong internally. the answer contains two video m-lines and the offer contains one audio and one video. Also, the first one is in the wrong place.

Init Complete
Init Complete
onCreateOfferSuccess = v=0
o=Mozilla-SIPUA 21928 0 IN IP4 0.0.0.0
s=SIP Call
t=0 0
a=ice-ufrag:08425380
a=ice-pwd:d14929ac0fa3cdd4bcc82e7b1ee9267a
a=fingerprint:sha-256 98:50:BF:13:5E:90:A1:D8:37:CD:36:AC:04:E5:75:C8:80:8A:0A:42:76:F6:B4:B3:5D:38:4C:22:AA:46:BA:18
m=audio 65476 RTP/SAVPF 109 0 8 101
c=IN IP4 74.95.2.174
a=rtpmap:109 opus/48000
a=ptime:20
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=sendrecv
a=candidate:0 1 UDP 2113601791 192.168.1.101 65476 typ host
a=candidate:1 1 UDP 1694236671 74.95.2.174 65476 typ srflx raddr 192.168.1.101 rport 65476
a=candidate:0 2 UDP 2113601790 192.168.1.101 64073 typ host
a=candidate:1 2 UDP 1694236670 74.95.2.174 64073 typ srflx raddr 192.168.1.101 rport 64073
m=video 49957 RTP/SAVPF 120
c=IN IP4 74.95.2.174
a=rtpmap:120 VP8/90000
a=sendrecv
a=candidate:0 1 UDP 2113601791 192.168.1.101 49957 typ host
a=candidate:1 1 UDP 1694236671 74.95.2.174 49957 typ srflx raddr 192.168.1.101 rport 49957
a=candidate:0 2 UDP 2113601790 192.168.1.101 56249 typ host
a=candidate:1 2 UDP 1694236670 74.95.2.174 56249 typ srflx raddr 192.168.1.101 rport 56249
m=application 52238 SCTP/DTLS 5000 
c=IN IP4 74.95.2.174
a=fmtp:5000 protocol=webrtc-datachannel;streams=16
a=sendrecv
a=candidate:0 1 UDP 2113601791 192.168.1.101 52238 typ host
a=candidate:1 1 UDP 1694236671 74.95.2.174 52238 typ srflx raddr 192.168.1.101 rport 52238
a=candidate:0 2 UDP 2113601790 192.168.1.101 62611 typ host
a=candidate:1 2 UDP 1694236670 74.95.2.174 62611 typ srflx raddr 192.168.1.101 rport 62611

OnAddStream called hints=1
OnAddStream called hints=2
onCreateAnswerSuccess = v=0
o=Mozilla-SIPUA 13603 0 IN IP4 0.0.0.0
s=SIP Call
t=0 0
a=ice-ufrag:384d9014
a=ice-pwd:23276e0f301a6224a72c9094e78d41f3
a=fingerprint:sha-256 63:3E:7F:0D:D5:C2:76:EC:D3:FD:A9:33:9F:70:17:CE:FF:04:49:EF:8E:AC:63:BE:BC:49:45:09:5A:D7:F0:63
m=video 61862 RTP/SAVPF 120
c=IN IP4 74.95.2.174
a=rtpmap:120 VP8/90000
a=sendrecv
a=candidate:0 1 UDP 2113601791 192.168.1.101 61862 typ host
a=candidate:1 1 UDP 1694236671 74.95.2.174 61862 typ srflx raddr 192.168.1.101 rport 61862
a=candidate:0 2 UDP 2113601790 192.168.1.101 64399 typ host
a=candidate:1 2 UDP 1694236670 74.95.2.174 64399 typ srflx raddr 192.168.1.101 rport 64399
m=video 0 RTP/SAVPF 30840
c=IN IP4 0.0.0.0
m=application 54084 SCTP/DTLS 5001 
c=IN IP4 74.95.2.174
a=fmtp:5001 protocol=webrtc-datachannel;streams=16
a=sendrecv
a=candidate:0 1 UDP 2113601791 192.168.1.101 54084 typ host
a=candidate:1 1 UDP 1694236671 74.95.2.174 54084 typ srflx raddr 192.168.1.101 rport 54084
a=candidate:0 2 UDP 2113601790 192.168.1.101 58068 typ host
a=candidate:1 2 UDP 1694236670 74.95.2.174 58068 typ srflx raddr 192.168.1.101 rport 58068

Close

Comment 10

[Ethan Hugg [:ehugg]]

Attachment: Improved handling of constraints

Comment 11

[enda mannion (:emannion)]

Attachment: Impoved handling of hints and more tests

I think I have this working now,  one big change I made is related to AddStream and RemoveStream.  Wherein if a stream is not added I do not leave the stream disabled but just ensure it is set to RECVONLY,  then I set it to SENDRECV when the stream is added.  I do the inverse for RemoveStream.   

I also had to add code to make negotiation work when there are different number of m= line levels between the offer and the answer, this was awkward as before SIPCC assumed there was always the same number sof levels.

ToDo:  It would be useful to add more unit test validation for the SDP based on constraints, like check each m= line for correct directionality (sendrecv, recvonly etc)

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 675845 [details] [diff] [review]
Impoved handling of hints and more tests

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

Tested OK on Linux.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +541,5 @@
>      pc->AddStream(domMediaStream);
>  
> +    // Decide if streams are disabled for offer or answer
> +    // then perform SDP checking based on which stream disabled
> +    bool isaudiooffer = (offeraudio == false) ? offeraudio : answeraudio;

Isn't this a more complex way of saying
bool isaudioOffer = (offeraudio && answeraudio)

@@ +548,4 @@
>      pObserver->state = TestObserver::stateNoResponse;
>      ASSERT_EQ(pc->CreateAnswer(constraints), NS_OK);
>      ASSERT_TRUE_WAIT(pObserver->state == TestObserver::stateSuccess, kDefaultTimeout);
> +    SDPSanityCheck(pObserver->lastString, isaudiooffer, isvideooffer, false);

I think I will file a new bug to change these booleans for audio/video/dc in SDPSanityCheck to integers and make it count exactly how many.  That way we will catch an extra m=video line in the unittest.

Comment 13

[enda mannion (:emannion)]

Attachment: Impoved handling of hints and more tests

Comment 14

[enda mannion (:emannion)]

Attachment: V7: Impoved handling of hints and more tests

Updated to build on m-c tip

Comment 15

[Ethan Hugg [:ehugg]]

Comment on attachment 676149 [details] [diff] [review]
V7: Impoved handling of hints and more tests

lgtm.  I'd like to get an r+ from EKR before we push.

Comment 16

[Eric Rescorla (:ekr)]

I will not get to this before tomorrow Europe time.

Comment 17

[Eric Rescorla (:ekr)]

Comment on attachment 676149 [details] [diff] [review]
V7: Impoved handling of hints and more tests

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

I'm pretty concerned about this stuff about fewer levels in the answer SDP.  SDP requires the same number of m-lines on each side,
even if they are null in the answer.

Can you please explain the logic here?

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +177,5 @@
>  
>    for (i=0; i<constraints->constraint_count; i++) {
>  
>      if (strcmp(constraints_table[OfferToReceiveAudio].name, constraints->constraints[i]->name) == 0) {
> +      if (cpr_strcasecmp("FALSE", constraints->constraints[i]->value) == 0 &&

This logic seems very confusing. Can you refactor it (a) into a function so you don't have to cut-and-paste the A/V
and (b) so it's not a huge if tree.

Isn't this logic effectively:

switch (direction) {
    case SDP_DIRECTION_SENDONLY:
        if (is_true(constraint) || is_empty(constraint)) 
            direction = SDP_DIRECTION_SENDRECV;
        break;
    case SDP_DIRECTION_SENDRECV:
        if (is_false(constraint)) 
            direction = SDP_DIRECTION_SENDONLY;
        break;
    case SDP_DIRECTION_RECVONLY:
        if (is_false(constraint)) 
            enabled = FALSE;
        break;
}

@@ +3993,5 @@
>       * Process each media line in the remote SDP
>       */
>      for (i = 1; i <= num_m_lines; i++) {
> +        /*
> +         * Sometimes the answer may have less m= lines that the offer

How can the answer have less m= lines? That's forbidden in SDP in general.

Comment 18

[enda mannion (:emannion)]

Ok, I will get to these today or tomorrow.  SO then if OfferToReceiveAudio is false and an audio stream is not added then we will add the m= line but just set the port to 0.

I understood from an email that this was not the case, that the m= line should not be added


                                |    AddStream(audio)   |    No AddStream(audio)
 -------------------------------------------------------------------------------
OfferToReceiveAudio unspecified |    m=audio, sendrecv  |    no m-line
OfferToReceiveAudio = no        |    m=audio, sendonly  |    no m-line
OfferToReceiveAudio = yes       |    m=audio, sendrecv  |    m=audio, recvonly

Comment 19

[Eric Rescorla (:ekr)]

That's about the off(In reply to enda mannion (:emannion) from comment #18)
> Ok, I will get to these today or tomorrow.  SO then if OfferToReceiveAudio
> is false and an audio stream is not added then we will add the m= line but
> just set the port to 0.
> 
> I understood from an email that this was not the case, that the m= line
> should not be added
> 
> 
>                                 |    AddStream(audio)   |    No
> AddStream(audio)
>  ----------------------------------------------------------------------------
> ---
> OfferToReceiveAudio unspecified |    m=audio, sendrecv  |    no m-line
> OfferToReceiveAudio = no        |    m=audio, sendonly  |    no m-line
> OfferToReceiveAudio = yes       |    m=audio, sendrecv  |    m=audio,
> recvonly

This is about the offer. The rules of SDP require the answerer to provide exactly as many m-lines as the offerer did.

Comment 20

[enda mannion (:emannion)]

Attachment: Impoved handling of hints and more tests

Comment 21

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #17)
> Comment on attachment 676149 [details] [diff] [review]
> Impoved handling of hints and more tests
> 
> Review of attachment 676149 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm pretty concerned about this stuff about fewer levels in the answer SDP. 
> SDP requires the same number of m-lines on each side,
> even if they are null in the answer.

This is now addressed.  See SDP example below.

> 
> Can you please explain the logic here?
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +177,5 @@
> >  
> >    for (i=0; i<constraints->constraint_count; i++) {
> >  
> >      if (strcmp(constraints_table[OfferToReceiveAudio].name, constraints->constraints[i]->name) == 0) {
> > +      if (cpr_strcasecmp("FALSE", constraints->constraints[i]->value) == 0 &&
> 
> This logic seems very confusing. Can you refactor it (a) into a function so
> you don't have to cut-and-paste the A/V
> and (b) so it's not a huge if tree.
> 
> Isn't this logic effectively:
> 
> switch (direction) {
>     case SDP_DIRECTION_SENDONLY:
>         if (is_true(constraint) || is_empty(constraint)) 
>             direction = SDP_DIRECTION_SENDRECV;
>         break;
>     case SDP_DIRECTION_SENDRECV:
>         if (is_false(constraint)) 
>             direction = SDP_DIRECTION_SENDONLY;
>         break;
>     case SDP_DIRECTION_RECVONLY:
>         if (is_false(constraint)) 
>             enabled = FALSE;
>         break;
> }

I did not get to refactor this, maybe this can be a separate bug.

> 
> @@ +3993,5 @@
> >       * Process each media line in the remote SDP
> >       */
> >      for (i = 1; i <= num_m_lines; i++) {
> > +        /*
> > +         * Sometimes the answer may have less m= lines that the offer
> 
> How can the answer have less m= lines? That's forbidden in SDP in general.

Done.

Comment 22

[enda mannion (:emannion)]

Sample SDP where No video stream is added on answer and offerToReceivevideo is false again for answer.

[ RUN      ] SignalingTest.OfferAnswerDontAddVideoStreamOnAnswerDontReceiveVideoOnAnswer
Init Complete
Init Complete
onCreateOfferSuccess = v=0
o=Mozilla-SIPUA 21107 0 IN IP4 0.0.0.0
s=SIP Call
t=0 0
a=ice-ufrag:4257457c
a=ice-pwd:6276d381162f17ee09ec5e32eab2589d
a=fingerprint:sha-256 B3:56:01:3D:46:CD:5C:F3:AE:8E:91:C2:5F:00:3C:AC:FF:03:C7:CE:0B:43:B9:DE:15:5E:00:2C:1B:AD:B3:FB
m=audio 49196 RTP/SAVPF 109 0 8 101
c=IN IP4 144.254.150.19
a=rtpmap:109 opus/48000
a=ptime:20
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 49196 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 64528 typ host
m=video 58489 RTP/SAVPF 120
c=IN IP4 144.254.150.19
a=rtpmap:120 VP8/90000
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 58489 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 57221 typ host
m=application 50265 SCTP/DTLS 5000 
c=IN IP4 144.254.150.19
a=fmtp:5000 protocol=webrtc-datachannel;streams=16
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 50265 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 59210 typ host

OnAddStream called hints=1
OnAddStream called hints=2
onCreateAnswerSuccess = v=0
o=Mozilla-SIPUA 6111 0 IN IP4 0.0.0.0
s=SIP Call
t=0 0
a=ice-ufrag:f33a5f6d
a=ice-pwd:58bd2683a21123b7d6008717b9aec25e
a=fingerprint:sha-256 0E:0B:30:84:13:BB:06:3E:33:B9:C7:66:8A:A9:81:08:8A:21:88:F4:E2:A6:41:E5:70:9B:AB:4B:12:88:04:ED
m=audio 49441 RTP/SAVPF 109 101
c=IN IP4 144.254.150.19
a=rtpmap:109 opus/48000
a=ptime:20
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 49441 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 61483 typ host
m=video 0 RTP/SAVPF 120
m=application 52066 SCTP/DTLS 5001 
c=IN IP4 144.254.150.19
a=fmtp:5001 protocol=webrtc-datachannel;streams=16
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 52066 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 63629 typ host

Close
Close
[       OK ] SignalingTest.OfferAnswerDontAddVideoStreamOnAnswerDontReceiveVideoOnAnswer (1969 ms)

Comment 23

[Eric Rescorla (:ekr)]

(In reply to enda mannion (:emannion) from comment #21)
> (In reply to Eric Rescorla from comment #17)
> > Comment on attachment 676149 [details] [diff] [review]
> > Impoved handling of hints and more tests
> > 
> > Review of attachment 676149 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm pretty concerned about this stuff about fewer levels in the answer SDP. 
> > SDP requires the same number of m-lines on each side,
> > even if they are null in the answer.
> 
> This is now addressed.  See SDP example below.

This appears to be correct.


> > 
> > Can you please explain the logic here?
> > 
> > ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> > @@ +177,5 @@
> > >  
> > >    for (i=0; i<constraints->constraint_count; i++) {
> > >  
> > >      if (strcmp(constraints_table[OfferToReceiveAudio].name, constraints->constraints[i]->name) == 0) {
> > > +      if (cpr_strcasecmp("FALSE", constraints->constraints[i]->value) == 0 &&
> > 
> > This logic seems very confusing. Can you refactor it (a) into a function so
> > you don't have to cut-and-paste the A/V
> > and (b) so it's not a huge if tree.
> > 
> > Isn't this logic effectively:
> > 
> > switch (direction) {
> >     case SDP_DIRECTION_SENDONLY:
> >         if (is_true(constraint) || is_empty(constraint)) 
> >             direction = SDP_DIRECTION_SENDRECV;
> >         break;
> >     case SDP_DIRECTION_SENDRECV:
> >         if (is_false(constraint)) 
> >             direction = SDP_DIRECTION_SENDONLY;
> >         break;
> >     case SDP_DIRECTION_RECVONLY:
> >         if (is_false(constraint)) 
> >             enabled = FALSE;
> >         break;
> > }
> 
> I did not get to refactor this, maybe this can be a separate bug.

I'd prefer it were done now. It's just way too hard to verify that the
existing code is correct.

 
> > 
> > @@ +3993,5 @@
> > >       * Process each media line in the remote SDP
> > >       */
> > >      for (i = 1; i <= num_m_lines; i++) {
> > > +        /*
> > > +         * Sometimes the answer may have less m= lines that the offer
> > 
> > How can the answer have less m= lines? That's forbidden in SDP in general.
> 
> Done.

Comment 24

[Eric Rescorla (:ekr)]

Comment on attachment 677359 [details] [diff] [review]
Impoved handling of hints and more tests

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

I think this still needs some work.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +222,5 @@
> +boolean gsmsdp_is_cap_enabled(fsmdef_dcb_t *dcb, sdp_media_e media_type) {
> +
> +    boolean result = FALSE;
> +
> +    if (SDP_MEDIA_AUDIO == media_type) {

A switch would seem better here.

@@ +4019,5 @@
>       */
>      for (i = 1; i <= num_m_lines; i++) {
> +        /*
> +         * Sometimes the answer may have less m= lines that the offer
> +         * k is used to store the answer m= line level

This comment is now irrelevant.

@@ +4691,5 @@
>          }
>  
> +        if (media->support_direction != SDP_DIRECTION_INACTIVE) {
> +          /*
> +           * Get the local RTP port. The src port will be set in the dcb

Why are you sending ICE parameters for an inactive line in an offer?

Comment 25

[enda mannion (:emannion)]

Attachment: revised: Impoved handling of hints and more tests

Comment 26

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #24)
> Comment on attachment 677359 [details] [diff] [review]
> Impoved handling of hints and more tests
> 
> Review of attachment 677359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this still needs some work.

No probs at all.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +222,5 @@
> > +boolean gsmsdp_is_cap_enabled(fsmdef_dcb_t *dcb, sdp_media_e media_type) {
> > +
> > +    boolean result = FALSE;
> > +
> > +    if (SDP_MEDIA_AUDIO == media_type) {
> 
> A switch would seem better here.
>

I removed this function as I no longer use it.
 
> @@ +4019,5 @@
> >       */
> >      for (i = 1; i <= num_m_lines; i++) {
> > +        /*
> > +         * Sometimes the answer may have less m= lines that the offer
> > +         * k is used to store the answer m= line level
> 
> This comment is now irrelevant.

Done.

> 
> @@ +4691,5 @@
> >          }
> >  
> > +        if (media->support_direction != SDP_DIRECTION_INACTIVE) {
> > +          /*
> > +           * Get the local RTP port. The src port will be set in the dcb
> 
> Why are you sending ICE parameters for an inactive line in an offer?

I am not sending ICE parameters for an inactive line in an offer?

Here is one of the tests. Note the video m= line on answer just has this:
m=video 0 RTP/SAVPF 120


[ RUN      ] SignalingTest.OfferAnswerDontAddVideoStreamOnAnswerDontReceiveVideoOnAnswer
Init Complete
Init Complete
onCreateOfferSuccess = v=0
o=Mozilla-SIPUA 12232 0 IN IP4 0.0.0.0
s=SIP Call
t=0 0
a=ice-ufrag:11885f4d
a=ice-pwd:17ea3b4b083c39295fac24fe2c2e9ffc
a=fingerprint:sha-256 E7:E1:F3:E9:11:24:A1:3A:B5:CA:81:3F:FC:3F:14:2A:07:D1:E1:55:D0:AD:77:E4:E7:0A:F0:87:1C:ED:6F:38
m=audio 52953 RTP/SAVPF 109 0 8 101
c=IN IP4 144.254.150.19
a=rtpmap:109 opus/48000
a=ptime:20
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 52953 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 65422 typ host
m=video 57368 RTP/SAVPF 120
c=IN IP4 144.254.150.19
a=rtpmap:120 VP8/90000
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 57368 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 60494 typ host
m=application 62176 SCTP/DTLS 5000 
c=IN IP4 144.254.150.19
a=fmtp:5000 protocol=webrtc-datachannel;streams=16
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 62176 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 54479 typ host

OnAddStream called hints=1
OnAddStream called hints=2
onCreateAnswerSuccess = v=0
o=Mozilla-SIPUA 21254 0 IN IP4 0.0.0.0
s=SIP Call
t=0 0
a=ice-ufrag:cfcdd0b1
a=ice-pwd:b67e317f7d2a756374348ed86bcf5799
a=fingerprint:sha-256 E5:6D:B9:8A:11:4F:8E:0E:89:F0:5E:7E:1B:88:78:1E:1B:E7:2E:21:5D:9C:0E:F8:C6:1E:10:05:11:A8:65:FD
m=audio 57622 RTP/SAVPF 109 101
c=IN IP4 144.254.150.19
a=rtpmap:109 opus/48000
a=ptime:20
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 57622 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 63650 typ host
m=video 0 RTP/SAVPF 120
m=application 56372 SCTP/DTLS 5001 
c=IN IP4 144.254.150.19
a=fmtp:5001 protocol=webrtc-datachannel;streams=16
a=sendrecv
a=candidate:0 1 UDP 2113667327 144.254.150.19 56372 typ host
a=candidate:0 2 UDP 2113667326 144.254.150.19 53336 typ host

Comment 27

[Eric Rescorla (:ekr)]

Enda, when do you plan to finish this?

Comment 28

[enda mannion (:emannion)]

I thought I had, what is left to do?  Is it the refactor of gsmsdp_process_cap_constraints   I was hoping to create a separate bug for that?

Comment 29

[Eric Rescorla (:ekr)]

No, please do it now. It's hard to review it in its current state.

Comment 30

[enda mannion (:emannion)]

I took a look at this and think it can also get messy refactoring.  This is because we still need an outer loop that interates through the media_caps, audio, video, data, then an inner loop in the is_true function is needed to loop through the constraints.  I think the way it is done is more efficient.   I will give it more time in the morning also to see if I can do some other refactor.

Comment 31

[Eric Rescorla (:ekr)]

Hmmm... I provided a candidate refactor.

Comment 32

[enda mannion (:emannion)]

You did

> Isn't this logic effectively:
> 
> switch (direction) {
>     case SDP_DIRECTION_SENDONLY:
>         if (is_true(constraint) || is_empty(constraint)) 
>             direction = SDP_DIRECTION_SENDRECV;
>         break;
>     case SDP_DIRECTION_SENDRECV:
>         if (is_false(constraint)) 
>             direction = SDP_DIRECTION_SENDONLY;
>         break;
>     case SDP_DIRECTION_RECVONLY:
>         if (is_false(constraint)) 
>             enabled = FALSE;
>         break;
> }
> 

But there is more needed and there will need to be a lot of loooping

..
code to get cap from table

for(i=0; i<constraints->constraint_count; i++) {

  for (cap = 0; cap < cap_max; cap++) {

    switch (media_cap->direction) {
       case SDP_DIRECTION_SENDONLY:
           if (is_true(constraint, cap_type) || is_empty(constraint, cap_type)) 
               direction = SDP_DIRECTION_SENDRECV;
           break;
       case SDP_DIRECTION_SENDRECV:
           if (is_false(constraint, cap_type)) 
               direction = SDP_DIRECTION_SENDONLY;
           break;
       case SDP_DIRECTION_RECVONLY:
           if (is_false(constraint, cap_type)) 
               enabled = FALSE;
           break;
     }
  }
}

new functions are needed
boolean is_true(constraint, cap_type) {

....
}

same for is_false  and is_empty

Do you still think this is better?

Comment 33

[Adam Roach [:abr]]

Attachment: Refactoring constraint handling to unify audio and video handling, make logic clearer.

Improving test cases to check media directionality.

Comment 34

[Eric Rescorla (:ekr)]

Enda, I asked Adam to take a look at this... Can you take a look at his approach?

Comment 35

[enda mannion (:emannion)]

Comment on attachment 679781 [details] [diff] [review]
Refactoring constraint handling to unify audio and video handling, make logic clearer.

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

Sorry I couldn't get to this refactor on time, thanks for doing it and a nice job.  I should be available for other odd jobs though.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +177,5 @@
> +  if (constraint[0] == 'F') {
> +    cap->support_direction &= ~SDP_DIRECTION_FLAG_RECV;
> +  } else if (constraint[0] == 'T') {
> +    cap->support_direction |= SDP_DIRECTION_FLAG_RECV;
> +  }

Provided setBooleanConstraint always uses upper case to set the value this check should suffice, which it does right now.

@@ +196,5 @@
> +                                    constraints->constraints[i]->value);
> +    } else if (strcmp(constraints_table[OfferToReceiveVideo].name,
> +               constraints->constraints[i]->name) == 0) {
> +      gsmsdp_process_cap_constraint(&dcb->media_cap_tbl->cap[CC_VIDEO_1],
> +                                    constraints->constraints[i]->value);

Look like you nailed the refactor, nice one!   Nothing yet here for mandatory vs non mandatory constraints but I guess I didn't handle this either.

Comment 36

[enda mannion (:emannion)]

I built this patch and all test seem to work correctly as before.

Comment 37

[Eric Rescorla (:ekr)]

Comment on attachment 679781 [details] [diff] [review]
Refactoring constraint handling to unify audio and video handling, make logic clearer.

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

one real question below.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3491,5 @@
>       * This is temporary code to allow configuration of the two
>       * default streams. When multiple streams > 2 are supported this
>       * will be re-implemented.
>       */
>      if (msg->data.track.media_type == VIDEO) {

Are the audio/video branches here backwards?

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +173,4 @@
>   */
> +void gsmsdp_process_cap_constraint(cc_media_cap_t *cap,
> +                                   const char *constraint) {
> +  /* Check constraint string for values "TRUE" or "FALSE" */

This seems pretty sketchy. What if it's FOOBAR?

Maybe we want strcmp?

@@ +3997,5 @@
> +         * have been created in the answer.
> +         */
> +        if (create_answer) {
> +
> +            /* Its best find the media by type rather than by may not be as uniform as SIPCC */

Can you rewrite this comment so it makes sense?

@@ +4146,5 @@
>              /*
>               * Negotiate RTP/SRTP. The result is the media transport
>               * which could be RTP/SRTP or fail.
>               */
>              transport = gsmsdp_negotiate_media_transport(dcb_p, sdp_p,

Why does this need an explicit argument of "i" now?

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c
@@ +953,5 @@
>  {
>      sdp_transport_e remote_transport;
>      sdp_transport_e negotiated_transport = SDP_TRANSPORT_INVALID;
>      void           *sdp_p = cc_sdp_p->dest_sdp;
> +    //uint16_t       level;

Remove this if it's going away.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +79,5 @@
> +  SHOULD_REJECT_AUDIO = 0x08,
> +  SHOULD_SEND_VIDEO = 0x10,
> +  SHOULD_RECV_VIDEO = 0x20,
> +  SHOULD_INACTIVE_VIDEO = 0x40,
> +  SHOULD_REJECT_VIDEO = 0x80,

Can I suggest phrasing these as 0x01 << 1, etc. so it's more obvious.

@@ +680,2 @@
>  
> +    if (flags & AUDIO_FLAGS && offer) {

Please use parentheses to make precedence clear here.

@@ +932,5 @@
>  {
> +  sipcc::MediaConstraints constraints;
> +  OfferAnswer(constraints, constraints, true, true, true, true, false,
> +              SHOULD_SENDRECV_AV, SHOULD_SENDRECV_AV);
> +  PR_Sleep(kDefaultTimeout * 2); // Wait for completion

Wny does this hav a timeouy?

@@ +1114,1 @@
>  TEST_F(SignalingTest, OfferAnswerReNegotiateOfferAnswerDontReceiveVideoNoVideoStream)

IMO all the renegogitate tests are questionable.

Comment 38

[Eric Rescorla (:ekr)]

Comment on attachment 679781 [details] [diff] [review]
Refactoring constraint handling to unify audio and video handling, make logic clearer.

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

one real question below.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3491,5 @@
>       * This is temporary code to allow configuration of the two
>       * default streams. When multiple streams > 2 are supported this
>       * will be re-implemented.
>       */
>      if (msg->data.track.media_type == VIDEO) {

Are the audio/video branches here backwards?

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +173,4 @@
>   */
> +void gsmsdp_process_cap_constraint(cc_media_cap_t *cap,
> +                                   const char *constraint) {
> +  /* Check constraint string for values "TRUE" or "FALSE" */

This seems pretty sketchy. What if it's FOOBAR?

Maybe we want strcmp?

@@ +3997,5 @@
> +         * have been created in the answer.
> +         */
> +        if (create_answer) {
> +
> +            /* Its best find the media by type rather than by may not be as uniform as SIPCC */

Can you rewrite this comment so it makes sense?

@@ +4146,5 @@
>              /*
>               * Negotiate RTP/SRTP. The result is the media transport
>               * which could be RTP/SRTP or fail.
>               */
>              transport = gsmsdp_negotiate_media_transport(dcb_p, sdp_p,

Why does this need an explicit argument of "i" now?

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c
@@ +953,5 @@
>  {
>      sdp_transport_e remote_transport;
>      sdp_transport_e negotiated_transport = SDP_TRANSPORT_INVALID;
>      void           *sdp_p = cc_sdp_p->dest_sdp;
> +    //uint16_t       level;

Remove this if it's going away.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +79,5 @@
> +  SHOULD_REJECT_AUDIO = 0x08,
> +  SHOULD_SEND_VIDEO = 0x10,
> +  SHOULD_RECV_VIDEO = 0x20,
> +  SHOULD_INACTIVE_VIDEO = 0x40,
> +  SHOULD_REJECT_VIDEO = 0x80,

Can I suggest phrasing these as 0x01 << 1, etc. so it's more obvious.

@@ +680,2 @@
>  
> +    if (flags & AUDIO_FLAGS && offer) {

Please use parentheses to make precedence clear here.

@@ +932,5 @@
>  {
> +  sipcc::MediaConstraints constraints;
> +  OfferAnswer(constraints, constraints, true, true, true, true, false,
> +              SHOULD_SENDRECV_AV, SHOULD_SENDRECV_AV);
> +  PR_Sleep(kDefaultTimeout * 2); // Wait for completion

Wny does this hav a timeouy?

@@ +1114,1 @@
>  TEST_F(SignalingTest, OfferAnswerReNegotiateOfferAnswerDontReceiveVideoNoVideoStream)

IMO all the renegogitate tests are questionable.

Comment 39

[Adam Roach [:abr]]

(In reply to Eric Rescorla from comment #38)
> 
> Are the audio/video branches here backwards?
> 

They are (as were the unit tests, which is why they all passed). I've switched the sense of both this function and the unit tests in the new patch I'm adding momentarily (I have a little bit of hg wrangling to get the patch to look right, but this shouldn't take long).

> 
> This seems pretty sketchy. What if it's FOOBAR?
>

This needs more work. See bug 811360.
 

> 
> Can you rewrite this comment so it makes sense?
> 

Done.

> @@ +4146,5 @@
> >              /*
> >               * Negotiate RTP/SRTP. The result is the media transport
> >               * which could be RTP/SRTP or fail.
> >               */
> >              transport = gsmsdp_negotiate_media_transport(dcb_p, sdp_p,
> 
> Why does this need an explicit argument of "i" now?
>

This is one of Enda's changes, so I'm only taking a guess, but it looks like the code previously found the proper media section by type alone. The use of an index into the SDP media sections would appear to be a step towards a generic, multiple-streams-per-type implementation, allowing clear indication of which stream is being negotiated.

> @@ +932,5 @@
> >  {
> > +  sipcc::MediaConstraints constraints;
> > +  OfferAnswer(constraints, constraints, true, true, true, true, false,
> > +              SHOULD_SENDRECV_AV, SHOULD_SENDRECV_AV);
> > +  PR_Sleep(kDefaultTimeout * 2); // Wait for completion
> 
> Wny does this hav a timeouy?

It appears to be unnecessary. I've removed it.

All other suggestions have been incorporated into the patch.

Comment 40

[Adam Roach [:abr]]

Attachment: Improved handling of constraints and more tests. Includes updates from code review.

Comment 41

[enda mannion (:emannion)]

Not sure if you want me to update the patch with comments, (In reply to Adam Roach [:abr] from comment #39)
> (In reply to Eric Rescorla from comment #38)
> > 
> > Are the audio/video branches here backwards?
> > 
> 
> They are (as were the unit tests, which is why they all passed). I've
> switched the sense of both this function and the unit tests in the new patch
> I'm adding momentarily (I have a little bit of hg wrangling to get the patch
> to look right, but this shouldn't take long).
> 
> > 
> > This seems pretty sketchy. What if it's FOOBAR?
> >
> 
> This needs more work. See bug 811360.
>  
> 
> > 
> > Can you rewrite this comment so it makes sense?
> > 
> 
> Done.
> 
> > @@ +4146,5 @@
> > >              /*
> > >               * Negotiate RTP/SRTP. The result is the media transport
> > >               * which could be RTP/SRTP or fail.
> > >               */
> > >              transport = gsmsdp_negotiate_media_transport(dcb_p, sdp_p,
> > 
> > Why does this need an explicit argument of "i" now?
> >
> 
> This is one of Enda's changes, so I'm only taking a guess, but it looks like
> the code previously found the proper media section by type alone. The use of
> an index into the SDP media sections would appear to be a step towards a
> generic, multiple-streams-per-type implementation, allowing clear indication
> of which stream is being negotiated.

Correct this is a step towards multi-stream work but cam into effect to solve the problem of having a media line disabled\removed thus the level stored in the media struct was not consistant so now I pass the correct level.


> 
> > @@ +932,5 @@
> > >  {
> > > +  sipcc::MediaConstraints constraints;
> > > +  OfferAnswer(constraints, constraints, true, true, true, true, false,
> > > +              SHOULD_SENDRECV_AV, SHOULD_SENDRECV_AV);
> > > +  PR_Sleep(kDefaultTimeout * 2); // Wait for completion
> > 
> > Wny does this hav a timeouy?
> 
> It appears to be unnecessary. I've removed it.
> 
> All other suggestions have been incorporated into the patch.

Comment 42

[Adam Roach [:abr]]

(In reply to enda mannion (:emannion) from comment #41)
> Not sure if you want me to update the patch with comments, (In reply to Adam
> Roach [:abr] from comment #39)

I already updated the patch. Thanks.

Comment 43

[Eric Rescorla (:ekr)]

Comment on attachment 681533 [details] [diff] [review]
Improved handling of constraints and more tests. Includes updates from code review.

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

lgtm

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4006,5 @@
> +            /* Since the incoming SDP might not be in the same order as
> +               our media, we find them by type rather than location
> +               for this check. Note that we're not checking for the
> +               value of any _particular_ m= section; we're just checking
> +               whether (at least) one of the specified type exists.  */

This is not the clearest comment ever, since m-lines are supposed to be in order.

Comment 44

[Adam Roach [:abr]]

(In reply to Eric Rescorla from comment #43)
> Comment on attachment 681533 [details] [diff] [review]
> Improved handling of constraints and more tests. Includes updates from code
> review.
> 
> Review of attachment 681533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +4006,5 @@
> > +            /* Since the incoming SDP might not be in the same order as
> > +               our media, we find them by type rather than location
> > +               for this check. Note that we're not checking for the
> > +               value of any _particular_ m= section; we're just checking
> > +               whether (at least) one of the specified type exists.  */
> 
> This is not the clearest comment ever, since m-lines are supposed to be in
> order.

I agree that m=lines are in order; but keep in mind that this is taking place during negotiation. We don't have any way to predict what order the m= lines will arrive in an initial offer. Without a time machine, we can't put our media structures in the same order before it arrives.

Now, I'm still just guessing at this, since I don't quite understand the relationship between media_list, src->src_sdp, and src->dest_sdp in the DCB.

(Aside -- in an IRC conversation with EKR, he indicated that he's good leaving the comment as-is for the time being. I suspect this code gets changed quite a bit when we do multiple m-lines per media type anyway, so we have a chance to revisit it later).

Comment 45

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4811930bb965

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4811930bb965

Comment 47

[enda mannion (:emannion)]

(In reply to Adam Roach [:abr] from comment #44)
> (In reply to Eric Rescorla from comment #43)
> > Comment on attachment 681533 [details] [diff] [review]
> > Improved handling of constraints and more tests. Includes updates from code
> > review.
> > 
> > Review of attachment 681533 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > lgtm
> > 
> > ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> > @@ +4006,5 @@
> > > +            /* Since the incoming SDP might not be in the same order as
> > > +               our media, we find them by type rather than location
> > > +               for this check. Note that we're not checking for the
> > > +               value of any _particular_ m= section; we're just checking
> > > +               whether (at least) one of the specified type exists.  */
> > 
> > This is not the clearest comment ever, since m-lines are supposed to be in
> > order.
> 
> I agree that m=lines are in order; but keep in mind that this is taking
> place during negotiation. We don't have any way to predict what order the m=
> lines will arrive in an initial offer. Without a time machine, we can't put
> our media structures in the same order before it arrives.
> 
> Now, I'm still just guessing at this, since I don't quite understand the
> relationship between media_list, src->src_sdp, and src->dest_sdp in the DCB.
> 
> (Aside -- in an IRC conversation with EKR, he indicated that he's good
> leaving the comment as-is for the time being. I suspect this code gets
> changed quite a bit when we do multiple m-lines per media type anyway, so we
> have a chance to revisit it later).

You are right, when multiple m-lines per media type gets implemented there will be an overhaul to the code that creates SDP (gsmsdp_create_local_sdp) and negotiates the SDP (gsmsdp_negotiate_media_lines). As well as fsmdef_ev_addstream and fsmdef_ev_removestream and a few other places.  


There is a relationship between src->src_sdp, and src->dest_sdp and the dcb->media_list, each fsmdef_media_t in the media_list is made up of data gleaned from both the src_sdp and dest_sdp through negotiation, plus each fsmdef_media_t gets populated with some fields when each one is created in gsmsdp_create_local_sdp. 

src->src_sdp (local), and src->dest_sdp (remote) are structural reperesentations of the SDP that can be read or built into a string for transport.

If you want to do a hangout on SIPCC sometime I am happy to meet up.