1033833 - Update CreateOffer/Answer API to spec - no longer takes constraints but a dictionary

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This part of the code needs updating to match advancements in the spec (CreateOffer/Answer no longer takes constraints but a dictionary).

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: Update CreateOffer/Answer API to spec - no longer takes constraints but a dictionary

Olli, if you could look at the DOM changes that'd be great. Mostly removing stuff.

Adam, if you could look at the rest that'd be great.

Some of the signaling unittests are failing, but then I noticed they were before as well so not sure. Will remove them as separate patch.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Remove signaling unittests for createAnswer options

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try - https://tbpl.mozilla.org/?tree=Try&rev=249b2b3c07c7

Comment 4

[Adam Roach [:abr]]

Comment on attachment 8458275 [details] [diff] [review]
Part 1: Update CreateOffer/Answer API to spec - no longer takes constraints but a dictionary

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

Still needs a bit more cleanup -- re-review should be fast, but I'd like to look at this again.

::: dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveAudio.html
@@ +17,5 @@
>    });
>  
>    runNetworkTest(function() {
>      var test = new PeerConnectionTest();
> +    test.setOfferConstraints({ offerToReceiveAudio: true });

It seems to me that we need to update pc.js to call this "setOfferOptions"

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +63,5 @@
> +  }
> +}
> +
> +static void
> +Apply(const Optional<int32_t> &aSrc, cc_boolean_option_t *aDst) {

Apply(const Optional<int32_t> &aSrc, cc_int32_option_t *aDst) {

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1173,2 @@
>  {
> +  return CreateOffer(SipccOfferOptions (aOptions));

I know it was here when you got here, but please remove the space between SipccOfferOptions and its opening paren.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +212,5 @@
>    PeerConnectionImpl(const mozilla::dom::GlobalObject* aGlobal = nullptr);
>  
>    enum Error {
>      kNoError                          = 0,
> +    kInvalidOptionsType               = 1,

I don't think this is possible.

@@ +218,5 @@
>      kInvalidMediastreamTrack          = 3,
>      kInvalidState                     = 4,
>      kInvalidSessionDescription        = 5,
>      kIncompatibleSessionDescription   = 6,
> +    kIncompatibleOptions              = 7,

This either. You should probably remove these two codes rather than renaming them.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +1099,5 @@
>      }
>  }
>  
>  /**
> + * This function deallocates a options structure

...an options...

@@ +3376,1 @@
>      }

Remove this -- there are no options associated with createAnswer.

@@ +4029,3 @@
>            dcb->media_cap_tbl->cap[cap_index].bundle_only =
> +            msg->data.track.options->moz_bundle_only.value;
> +        }

Remove this -- there are no options associated with addStream.

@@ +4033,5 @@
> +
> +    /* Free the options structure */
> +    if (msg->data.track.options) {
> +       fsmdef_free_options(msg->data.track.options);
> +       msg->data.track.options = 0;

Interesting that there isn't corresponding code in createOffer. Can you double-check that we're not leaking options structures in there?

::: media/webrtc/signaling/src/sipcc/core/includes/ccapi.h
@@ +833,5 @@
>  typedef struct cc_feature_data_track_t_ {
>    cc_media_stream_id_t     stream_id;
>    cc_media_track_id_t      track_id;
>    cc_media_type_t          media_type;
> +  cc_media_options_t       *options;

Remove this.

::: media/webrtc/signaling/src/sipcc/include/cc_constants.h
@@ +582,1 @@
>  

Also add:

  typedef struct {
    cc_boolean was_passed;
    int32 value;
  } cc_int32_option_t;

@@ +582,3 @@
>  
>  typedef struct {
> +  cc_boolean_option_t offer_to_receive_audio;

cc_int32_option_t

@@ +582,4 @@
>  
>  typedef struct {
> +  cc_boolean_option_t offer_to_receive_audio;
> +  cc_boolean_option_t offer_to_receive_video;

cc_int32_option_t

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +92,1 @@
>    }

need to add a setInt32Option() for offerToReceive{Audio,Video}

@@ +1730,5 @@
>                                 uint32_t hints, uint32_t sdpCheck) {
>      EnsureInit();
> +    sipcc::OfferOptions aoptions;
> +    aoptions.setBooleanOption("OfferToReceiveAudio", true);
> +    aoptions.setBooleanOption("OfferToReceiveVideo", true);

aoptions.setInt32Option("OfferToReceiveAudio", 1);
aoptions.setInt32Option("OfferToReceiveVideo", 1);

@@ +1965,5 @@
>  TEST_F(SignalingTest, CreateOfferNoVideoStreamRecvVideo)
>  {
> +  sipcc::OfferOptions options;
> +  options.setBooleanOption("OfferToReceiveAudio", true);
> +  options.setBooleanOption("OfferToReceiveVideo", true);

options.setInt32Option("OfferToReceiveAudio", 1);
options.setInt32Option("OfferToReceiveVideo", 1);

@@ +1974,5 @@
>  TEST_F(SignalingTest, CreateOfferNoAudioStreamRecvAudio)
>  {
> +  sipcc::OfferOptions options;
> +  options.setBooleanOption("OfferToReceiveAudio", true);
> +  options.setBooleanOption("OfferToReceiveVideo", true);

options.setInt32Option("OfferToReceiveAudio", 1);
options.setInt32Option("OfferToReceiveVideo", 1);

@@ +1983,5 @@
>  TEST_F(SignalingTest, CreateOfferNoVideoStream)
>  {
> +  sipcc::OfferOptions options;
> +  options.setBooleanOption("OfferToReceiveAudio", true);
> +  options.setBooleanOption("OfferToReceiveVideo", false);

options.setInt32Option("OfferToReceiveAudio", 1);

With the removal of the mandatory distinction, I think we don't need OfferToReceiveVideo in this test.

@@ +1992,5 @@
>  TEST_F(SignalingTest, CreateOfferNoAudioStream)
>  {
> +  sipcc::OfferOptions options;
> +  options.setBooleanOption("OfferToReceiveAudio", false);
> +  options.setBooleanOption("OfferToReceiveVideo", true);

options.setInt32Option("OfferToReceiveVideo", 1);

@@ +2001,5 @@
>  TEST_F(SignalingTest, CreateOfferDontReceiveAudio)
>  {
> +  sipcc::OfferOptions options;
> +  options.setBooleanOption("OfferToReceiveAudio", false);
> +  options.setBooleanOption("OfferToReceiveVideo", true);

options.setInt32Option("OfferToReceiveVideo", 1);

@@ +2010,5 @@
>  TEST_F(SignalingTest, CreateOfferDontReceiveVideo)
>  {
> +  sipcc::OfferOptions options;
> +  options.setBooleanOption("OfferToReceiveAudio", true);
> +  options.setBooleanOption("OfferToReceiveVideo", false);

options.setInt32Option("OfferToReceiveAudio", 1);

@@ +2020,5 @@
>  TEST_F(SignalingTest, DISABLED_CreateOfferRemoveAudioStream)
>  {
> +  sipcc::OfferOptions options;
> +  options.setBooleanOption("OfferToReceiveAudio", true);
> +  options.setBooleanOption("OfferToReceiveVideo", true);

options.setInt32Option("OfferToReceiveAudio", 1);
options.setInt32Option("OfferToReceiveVideo", 1);

@@ +2030,5 @@
>  TEST_F(SignalingTest, DISABLED_CreateOfferDontReceiveAudioRemoveAudioStream)
>  {
> +  sipcc::OfferOptions options;
> +  options.setBooleanOption("OfferToReceiveAudio", false);
> +  options.setBooleanOption("OfferToReceiveVideo", true);

options.setInt32Option("OfferToReceiveVideo", 1);

@@ +2040,5 @@
>  TEST_F(SignalingTest, DISABLED_CreateOfferDontReceiveVideoRemoveVideoStream)
>  {
> +  sipcc::OfferOptions options;
> +  options.setBooleanOption("OfferToReceiveAudio", true);
> +  options.setBooleanOption("OfferToReceiveVideo", false);

options.setInt32Option("OfferToReceiveAudio", 1);

@@ +2054,5 @@
>  }
>  
>  TEST_F(SignalingTest, OfferAnswerDontReceiveAudioOnOffer)
>  {
> +  sipcc::OfferOptions offeroptions;

s/offeroptions/options/ (here and below)

@@ +2056,5 @@
>  TEST_F(SignalingTest, OfferAnswerDontReceiveAudioOnOffer)
>  {
> +  sipcc::OfferOptions offeroptions;
> +  offeroptions.setBooleanOption("OfferToReceiveAudio", false);
> +  offeroptions.setBooleanOption("OfferToReceiveVideo", true);

options.setInt32Option("OfferToReceiveVideo", 1);

And so on. There are a lot of these -- I'm not going to tag any below here. :)

@@ +2311,5 @@
>    //ASSERT_GE(a1_->GetPacketsReceived(0), 40);
>    ASSERT_GE(a2_->GetPacketsReceived(0), 40);
>  }
>  
>  TEST_F(SignalingTest, FullCallAnswererRejectsVideo)

DISABLED_FullCallAnswererRejectsVideo

And leave in the old body (modulo the constraints-to-options changes) so that whoever fixes this knows what it did before.

@@ +2486,5 @@
>      "a=rtpmap:103 ISAC/16000\r\n"
>      "a=rtpmap:104 ISAC/32000\r\n"
>      // NOTE: the actual SDP that Chrome sends at the moment
>      // doesn't indicate two channels. I've amended their SDP
> +    // here, under the assumption that the options

Search and replace error -- this should still say "constraints"

Comment 5

[Adam Roach [:abr]]

Comment on attachment 8458279 [details] [diff] [review]
Part 2: Remove signaling unittests for createAnswer options

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

Any of the tests that previously relied on setting answer constraints to work should be disabled for now. We do not want to remove them, as we *do* want to re-implement them eventually, using the normal stream-rejection behavior (e.g., calling stop() on an offered stream before creating an answer), once that functionality is in place. So, rather than removing these tests, please simply prefix the following test names with "DISABLED_":

OfferAnswerDontReceiveAudioOnOffer
OfferAnswerDontReceiveVideoOnOffer
OfferAnswerDontReceiveAudioOnAnswer
OfferAnswerDontReceiveVideoOnAnswer
OfferAnswerDontAddAudioStreamOnOfferRecvAudio
OfferAnswerDontAddAudioStreamOnOffer
OfferAnswerDontAddVideoStreamOnOfferRecvVideo
OfferAnswerDontAddVideoStreamOnOffer
OfferAnswerDontAddAudioStreamOnAnswer
OfferAnswerDontAddVideoStreamOnAnswer
OfferAnswerDontAddVideoStreamOnAnswerDontReceiveVideoOnAnswer
OfferAnswerDontAddAudioStreamOnAnswerDontReceiveAudioOnAnswer
OfferAnswerDontAddAudioStreamOnOfferDontReceiveAudioOnOffer
OfferAnswerDontAddVideoStreamOnOfferDontReceiveVideoOnOffer
OfferAnswerDontReceiveAudioNoAudioStreamOnOfferDontReceiveVideoOnAnswer
FullCallAnswererRejectsVideo


If a test other than the ones I list above fails, then that's a separate problem that should not be addressed as part of this bug.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Adam Roach [:abr] from comment #4)
> > +Apply(const Optional<int32_t> &aSrc, cc_boolean_option_t *aDst) {
> 
> Apply(const Optional<int32_t> &aSrc, cc_int32_option_t *aDst) {

I actually meant to leave this as cc_boolean_option_t until we support more than one of each kind of stream in the lower-level code. I don't think the code needs to fail on numbers higher than 1 (the api is kinda hokey here if you ask me) although the spec is vague about it.

There is a bug in this function though, I meant to coerce the value to boolean.

> @@ +4033,5 @@
> > +
> > +    /* Free the options structure */
> > +    if (msg->data.track.options) {
> > +       fsmdef_free_options(msg->data.track.options);
> > +       msg->data.track.options = 0;
> 
> Interesting that there isn't corresponding code in createOffer. Can you
> double-check that we're not leaking options structures in there?

Looks like it's called much higher up in the function, though I see there are a couple of early returns above it... I don't know this code well enough to trigger those.

> Also add:
> 
>   typedef struct {
>     cc_boolean was_passed;
>     int32 value;
>   } cc_int32_option_t;

I was hoping we could defer changing this until we support multiple streams of each kind in the lower-level implementation, for a smaller patch.

> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> >  TEST_F(SignalingTest, CreateOfferNoVideoStream)
> >  {
> > +  sipcc::OfferOptions options;
> > +  options.setBooleanOption("OfferToReceiveAudio", true);
> > +  options.setBooleanOption("OfferToReceiveVideo", false);
> 
> options.setInt32Option("OfferToReceiveAudio", 1);

ditto these.

> With the removal of the mandatory distinction, I think we don't need
> OfferToReceiveVideo in this test.

I'm not following, these were all optional before. Can you elaborate?

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: Update CreateOffer/Answer API to spec - no longer takes constraints but a dictionary (2) r=smaug

Incorporated feedback. Thanks!

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Disable signaling unittests for createAnswer options (2)

Now disables tests as described in comment 5.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try - https://tbpl.mozilla.org/?tree=Try&rev=d17d40421eae

Comment 10

[Adam Roach [:abr]]

Comment on attachment 8458875 [details] [diff] [review]
Part 2: Disable signaling unittests for createAnswer options (2)

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

r+ on this, although it would be good form to indicate why these tests are disabled (e.g., "the way to reject streams has changed, and we need to implement the new way before -- calling stop on a received stream -- before these can be fixed.")

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: finish plumbing offerToReceiveAudio|Video to long

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Disable signaling unittests for createAnswer options (3) r=abr

Added comments. Carrying forward r=abr.

Comment 13

[Adam Roach [:abr]]

Comment on attachment 8458966 [details] [diff] [review]
Part 3: finish plumbing offerToReceiveAudio|Video to long

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

Thanks.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: finish plumbing offerToReceiveAudio|Video to long. r=abr

Unbitrot. Carrying forward r=abr.

Comment 15

[Adam Roach [:abr]]

Comment on attachment 8458862 [details] [diff] [review]
Part 1: Update CreateOffer/Answer API to spec - no longer takes constraints but a dictionary (2) r=smaug

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

Thanks. Looks good.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Hold on as sec. checking android crash.

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: unbreak test_exceptions_from_jsimplemented.html

With constraints gone from createAnswer, use different case to test this.

Hopefully a quick review. Passed locally.

Try - https://tbpl.mozilla.org/?tree=Try&rev=e7736e91b676

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8459355 [details] [diff] [review]
Part 4: unbreak test_exceptions_from_jsimplemented.html

r=me

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: Update CreateOffer/Answer API to spec - no longer takes constraints but a dictionary (3) r=smaug, abr

Unbitrot. Carrying forward r=smaug, abr

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: unbreak test_exceptions_from_jsimplemented.html (2) r=bz

Commit msg.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/870f07d03e2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c72ab9bc701
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd3b4021310
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca3ce71016b0

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/870f07d03e2a
https://hg.mozilla.org/mozilla-central/rev/4c72ab9bc701
https://hg.mozilla.org/mozilla-central/rev/2fd3b4021310
https://hg.mozilla.org/mozilla-central/rev/ca3ce71016b0