Closed Bug 1033833 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jib, Assigned: jib)

References

()

Details

(Whiteboard: [p=1])

Attachments

(4 files, 6 obsolete files)

8.73 KB, patch
jib
: review+
Details | Diff | Splinter Review
20.72 KB, patch
jib
: review+
Details | Diff | Splinter Review
165.01 KB, patch
jib
: review+
Details | Diff | Splinter Review
2.14 KB, patch
jib
: review+
Details | Diff | Splinter Review
This part of the code needs updating to match advancements in the spec (CreateOffer/Answer no longer takes constraints but a dictionary).
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → jib
Whiteboard: [p=1]
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.
Attachment #8458275 - Flags: review?(bugs)
Attachment #8458275 - Flags: review?(adam)
Attachment #8458275 - Attachment description: Update CreateOffer/Answer API to spec - no longer takes constraints but a dictionary → Part 1: Update CreateOffer/Answer API to spec - no longer takes constraints but a dictionary
Attachment #8458275 - Flags: review?(bugs) → review+
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"
Attachment #8458275 - Flags: review?(adam) → review-
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.
Attachment #8458279 - Flags: review?(adam) → review-
(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?
Incorporated feedback. Thanks!
Attachment #8458275 - Attachment is obsolete: true
Attachment #8458862 - Flags: review+
Attachment #8458862 - Flags: review?(adam)
Now disables tests as described in comment 5.
Attachment #8458279 - Attachment is obsolete: true
Attachment #8458875 - Flags: review?(adam)
Attachment #8458875 - Flags: review?(adam) → review+
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.")
Added comments. Carrying forward r=abr.
Attachment #8458875 - Attachment is obsolete: true
Attachment #8458976 - Flags: review+
Comment on attachment 8458966 [details] [diff] [review]
Part 3: finish plumbing offerToReceiveAudio|Video to long

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

Thanks.
Attachment #8458966 - Flags: review?(adam) → review+
Unbitrot. Carrying forward r=abr.
Attachment #8458966 - Attachment is obsolete: true
Attachment #8458977 - Flags: review+
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.
Attachment #8458862 - Flags: review?(adam) → review+
Hold on as sec. checking android crash.
Keywords: checkin-needed
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
Attachment #8459355 - Flags: review?(bzbarsky)
Comment on attachment 8459355 [details] [diff] [review]
Part 4: unbreak test_exceptions_from_jsimplemented.html

r=me
Attachment #8459355 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.