1425618 - {offerToReceiveAudio: false} and {offerToReceiveVideo: false} stopped working.

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

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

A recent spec change [1] let's us support this legacy case after all. 

We should get this in to 59 to avoid regressing support. 

[1] https://github.com/w3c/webrtc-pc/pull/1693
[2] https://github.com/webrtc/adapter/pull/729/files

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 59, as long as we can land the fix(es) by Jan 10 it should be ok for 59.

Comment 2

[Byron Campen [:bwc]]

Attachment: Bug 1425618 - Part 1: Test-cases for offerToReceiveX: false.

Review commit: https://reviewboard.mozilla.org/r/208504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208504/

Comment 3

[Byron Campen [:bwc]]

Attachment: Bug 1425618 - Part 2: Make offerToReceiveX: false unset the send bit on transceivers.

Review commit: https://reviewboard.mozilla.org/r/208506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208506/

Comment 4

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

Comment on attachment 8937816 [details]
Bug 1425618 - Part 1: Test-cases for offerToReceiveX: false.

https://reviewboard.mozilla.org/r/208504/#review214692

::: dom/media/tests/mochitest/test_peerConnection_transceivers.html:369
(Diff revision 1)
> +    for (let kind of kinds) {
> +      if (kind == "audio") {
> +        options.offerToReceiveAudio = false;
> +      } else if (kind == "video") {
> +        options.offerToReceiveVideo = false;
> +      }
> +    }

Idea: Only because you have this pattern twice in this function, we could do:

    let keys = {audio: "offerToReceiveAudio", video: "offerToReceiveVideo"};

    // Test offerToReceive: true
    for (let kind of kinds) {
      options[keys[kind]] = true;
    }

    // Test offerToReceive: false
    for (let kind of kinds) {
      options[keys[kind]] = false;
    }

Seems safer against typos.

Comment 5

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

Comment on attachment 8937817 [details]
Bug 1425618 - Part 2: Make offerToReceiveX: false unset the send bit on transceivers.

https://reviewboard.mozilla.org/r/208506/#review214694

::: dom/media/PeerConnection.js:836
(Diff revision 1)
> -    if (options.offerToReceiveAudio === false) {
> -      this.logWarning("offerToReceiveAudio: false is ignored now. If you " +
> -                      "want to disallow a recv track, use " +
> -                      "RTCRtpTransceiver.direction");
> +    this._transceivers
> +      .filter(transceiver => {
> +        return (options.offerToReceiveVideo === false &&
> +                transceiver.receiver.track.kind == "video") ||
> +               (options.offerToReceiveAudio === false &&
> +                transceiver.receiver.track.kind == "audio");
> +      })
> +      .forEach(transceiver => {

Nit: For simple x => expression transforms, it seems more readable to omit return and {}:

    this._transceivers
      .filter(transceiver =>
        (options.offerToReceiveVideo === false &&
         transceiver.receiver.track.kind == "video") ||
        (options.offerToReceiveAudio === false &&
         transceiver.receiver.track.kind == "audio"))
      .forEach(transceiver => {

or with destructuring:

    this._transceivers
      .filter(({receiver: {track: kind}}) =>
        (options.offerToReceiveVideo === false && kind == "video") ||
        (options.offerToReceiveAudio === false && kind == "audio"))
      .forEach(transceiver => {

Comment 6

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

Comment on attachment 8937817 [details]
Bug 1425618 - Part 2: Make offerToReceiveX: false unset the send bit on transceivers.

https://reviewboard.mozilla.org/r/208506/#review214694

> Nit: For simple x => expression transforms, it seems more readable to omit return and {}:
> 
>     this._transceivers
>       .filter(transceiver =>
>         (options.offerToReceiveVideo === false &&
>          transceiver.receiver.track.kind == "video") ||
>         (options.offerToReceiveAudio === false &&
>          transceiver.receiver.track.kind == "audio"))
>       .forEach(transceiver => {
> 
> or with destructuring:
> 
>     this._transceivers
>       .filter(({receiver: {track: kind}}) =>
>         (options.offerToReceiveVideo === false && kind == "video") ||
>         (options.offerToReceiveAudio === false && kind == "audio"))
>       .forEach(transceiver => {

or

    this._transceivers
      .filter(({receiver: {track: kind}}) =>
        (options[{audio: "offerToReceiveAudio", video: "offerToReceiveVideo"}[kind]] === false))
      .forEach(transceiver => {

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8937817 [details]
Bug 1425618 - Part 2: Make offerToReceiveX: false unset the send bit on transceivers.

https://reviewboard.mozilla.org/r/208506/#review214694

> or
> 
>     this._transceivers
>       .filter(({receiver: {track: kind}}) =>
>         (options[{audio: "offerToReceiveAudio", video: "offerToReceiveVideo"}[kind]] === false))
>       .forEach(transceiver => {

Personally, I have a harder time reading those, but I guess I'm not accustomed to destructuring yet.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8937816 [details]
Bug 1425618 - Part 1: Test-cases for offerToReceiveX: false.

https://reviewboard.mozilla.org/r/208504/#review214692

> Idea: Only because you have this pattern twice in this function, we could do:
> 
>     let keys = {audio: "offerToReceiveAudio", video: "offerToReceiveVideo"};
> 
>     // Test offerToReceive: true
>     for (let kind of kinds) {
>       options[keys[kind]] = true;
>     }
> 
>     // Test offerToReceive: false
>     for (let kind of kinds) {
>       options[keys[kind]] = false;
>     }
> 
> Seems safer against typos.

I've tweaked this to be better.

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8937816 [details]
Bug 1425618 - Part 1: Test-cases for offerToReceiveX: false.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208504/diff/1-2/

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8937817 [details]
Bug 1425618 - Part 2: Make offerToReceiveX: false unset the send bit on transceivers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208506/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50619d5809b4
Part 1: Test-cases for offerToReceiveX: false. r=jib
https://hg.mozilla.org/integration/autoland/rev/403655330301
Part 2: Make offerToReceiveX: false unset the send bit on transceivers. r=jib

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/50619d5809b4
https://hg.mozilla.org/mozilla-central/rev/403655330301

Comment 13

[Nils Ohlmeier [:drno]]

According to this https://bugzilla.mozilla.org/show_bug.cgi?id=1433953#c6 we haven't fixed this properly.

Fiddle to repro: https://jsfiddle.net/jib1/1h8mr8fw/

Comment 14

[Byron Campen [:bwc]]

I think I know what is going on here...

Comment 15

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2341b113b630a801f41993e564990adb4292aae

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 59, so that we make sure to follow up with uplift to beta if this is fixed & verified in 60.

Comment 17

[Nils Ohlmeier [:drno]]

The fix for the problem landed over in bug 1435013, because mozreview is a pain for re-opened bugs. Sorry for not updating the status properly here.

Comment 18

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed with Firefox 59 beta 10 and latest Nightly 60.0a1 under Win 10 64-bit, Ubuntu 14.04032-bit and Mac OS X 10.13.

Comment 19

[Eric Shepherd [:sheppy]]

Since this was uplifted to 59, there's nothing to report in Firefox 60 for developers -- however, I did add these properties to the docs for createOffer()'s options, since they were missing.