Closed Bug 1436523 Opened 6 years ago Closed 6 years ago

Need to allow fake camera and loopback audio at the same time.

Categories

(Core :: WebRTC: Audio/Video, enhancement, P2)

Product:
Core
Component:
WebRTC: Audio/Video
Version:
60 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: jib, Assigned: bryce)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 1 obsolete file)

Bug 1436523 - Rework media manager enumeration to prefer loopback to fake devices, allow mixing of fake and loopback.
59 bytes, text/x-review-board-request
achronop
: review+
Details
Bug 1436523 - Update dom/media/test/ tests to better handle loopback + gUM device config.
59 bytes, text/x-review-board-request
achronop
: review+
Details
Bug 1436523 - Update dom/media/tests/mochitest tests to better handle loopback devices.
59 bytes, text/x-review-board-request
achronop
: review+
Details
Bug 1436523 - Make WebRTC mochitest harness more granular with fake device setup.
59 bytes, text/x-review-board-request
achronop
: review+
Details
Bug 1436523 - Add enum to specify select type of devices to enumerate in MediaManager rather than use bools.
59 bytes, text/x-review-board-request
achronop
: review+
Details
Bug 1436523 - Make sure webrtc chrome mochitests don't use loopback devices.
59 bytes, text/x-review-board-request
achronop
: review+
Details
Bug 1436523 - Change DeviceEnumerationType to scoped enum, add extra logging.
59 bytes, text/x-review-board-request
achronop
: review+
Details
Bug 1436523 - Update head.js to better handle pref changes for fake/loopback devices during tests.
59 bytes, text/x-review-board-request
achronop
: review+
Details 
Currently, the

    "media.navigator.streams.fake;true"

pref trumps both the

    "media.video_loopback_dev" and
    "media.audio_loopback_dev"

prefs [1]. This makes it impossible to use the high-level fake device for video and loopback for audio, something we need to do on Windows where we only have loopback devices for audio atm.

We need to fix that.

[1] https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/dom/media/MediaManager.cpp#1859,1864,1867
Our current thinking is to make the loopback prefs trump "media.navigator.streams.fake;true".

That way, to get fake video and loopback audio you'd set "media.audio_loopback_dev" and "media.navigator.streams.fake;true".
One problem with the solution in comment 1 is we'd need to update some tests that currently rely on the pref (or {fake: true} until bug 1436424 is fixed) to trump the loopback devices that are currently on by default on linux.

These tests would now need to temporarily clear "media.audio_loopback_dev" and "media.video_loopback_dev".
Taking a look into this.

I think we still need the fake video devices on Windows if we have loopback audio (please correct me if I'm mistaken), otherwise tests are going to freak out, so marking this as blocking Bug 1412915.

The test harness for the WebRTC tests looks like it needs changes to make its setup of fake devices more granular. Right now both audio and video loopback devices need to be set in order to stop the harness setting up fakes[0].

Going to have a play and see if I can get this behaving nice locally.

[0] https://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/head.js#16
Assignee: nobody → bvandyk
Blocks: 1412915
Comment on attachment 8957579 [details]
Bug 1436523 - Add enum to specify select type of devices to enumerate in MediaManager rather than use bools.

https://reviewboard.mozilla.org/r/226466/#review232874

::: dom/media/MediaManager.cpp:2975
(Diff revision 1)
>  
> +      // If we fetched any real cameras or mics, remove the "default" part of
> +      // their IDs.
>        if (aVideoType == MediaSourceEnum::Camera &&
>            aAudioType == MediaSourceEnum::Microphone &&
> -          !aFake) {
> +          (aVideoEnumType != Fake || aAudioEnumType != Fake)) {

Don't we need `&&` here? What if audio is real and video is fake?
Attachment #8957579 - Flags: review?(achronop) → review+
Comment on attachment 8957579 [details]
Bug 1436523 - Add enum to specify select type of devices to enumerate in MediaManager rather than use bools.

https://reviewboard.mozilla.org/r/226466/#review232874

> Don't we need `&&` here? What if audio is real and video is fake?

My thinking is that if either is real, we still want to chop the "default" part off our results -- as we will have got back some real devices, likely with a default label on one of them. If we have an && then both mics and cams would need to be not fake in order to do the chopping. Let me know if that sounds sensible.
Comment on attachment 8957580 [details]
Bug 1436523 - Rework media manager enumeration to prefer loopback to fake devices, allow mixing of fake and loopback.

https://reviewboard.mozilla.org/r/226468/#review232880

::: dom/media/MediaManager.cpp:1859
(Diff revision 1)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(aVideoType != MediaSourceEnum::Other ||
>               aAudioType != MediaSourceEnum::Other);
> +  MOZ_ASSERT(aVideoEnumType != Fake || aVideoType == MediaSourceEnum::Camera,
> +             "If fake cams are requested video type should be camera!");

I do not see how the expression in the assert much the description. From the message I would expect something like:
```
MOZ_ASSERT(aVideoEnumType == Fake && aVideoType == MediaSourceEnum::Camera, ...)
```
Probably your intention is to assert for a wider range of events here but the message do not reflect that. 

Similar to the following asserts.

::: dom/media/MediaManager.cpp:1907
(Diff revision 1)
>      }
>  
>      auto result = MakeUnique<SourceSet>();
>  
>      if (hasVideo) {
>        nsTArray<RefPtr<MediaDevice>> videos;

Nit: I know it is out of the scope of your fix but it will benefit the reader if we use the typedef alias (SourceSet) instead of the full type in order to declare `videos` and `audios`.
Attachment #8957580 - Flags: review?(achronop) → review+
Comment on attachment 8957581 [details]
Bug 1436523 - Update dom/media/test/ tests to better handle loopback + gUM device config.

https://reviewboard.mozilla.org/r/226470/#review233114
Attachment #8957581 - Flags: review?(achronop) → review+
Comment on attachment 8957582 [details]
Bug 1436523 - Update dom/media/tests/mochitest tests to better handle loopback devices.

https://reviewboard.mozilla.org/r/226472/#review233136

::: dom/media/tests/mochitest/test_peerConnection_audioCodecs.html:23
(Diff revision 1)
>    async function testAudioCodec(options = {}, codec) {
>      // sdputils checks for opus as part of its sdp sanity test
>      options.opus = codec == "opus";
>  
>      let test = new PeerConnectionTest(options);
> -    test.setMediaConstraints([{audio: true, fake: true}], []);
> +    test.setMediaConstraints([{audio: true}], []);

Please note that fake prefs are not set here.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:74
(Diff revision 1)
> -  runNetworkTest(function () {
> +  runNetworkTest(async function () {
> +    // This test doesn't seem to play nice with loopback audio, so force fake
> +    // audio on
> +    await pushPrefs(
> +      ['media.audio_loopback_dev', ''],
> +      ['media.navigator.streams.fake', true]);

I do not see how we allow loopback device here. In case of Linux loopback device should be set by test harness. By clearing the loopback pref we force to use the fake device.

The same for the other PC tests that make use of audio constraints. Please note that those tests do not use fake constarints. The existing behavior is to make use of loopback device when is available and fake device on platforms that loopback does not exist. I think we force fake device in all cases now

::: dom/media/tests/mochitest/test_peerConnection_trackDisabling.html:27
(Diff revision 1)
> -
> -  // Always use fake tracks since we depend on video to be somewhat green and
> +    // Always use fake tracks since we depend on video to be somewhat green and
> -  // audio to have a large 1000Hz component (or 440Hz if using fake devices).
> +    // audio to have a large 1000Hz component (or 440Hz if using fake devices).
> -  test.setMediaConstraints([{audio: true, video: true, fake: true}], []);
> +    ['media.audio_loopback_dev', ''],
> +    ['media.video_loopback_dev', ''],
> +    ['media.navigator.streams.fake', true]);

Continuing from previous comment, here, for example, we want fake devices so clearing the loopback pref is valid.
Attachment #8957582 - Flags: review?(achronop) → review-
Comment on attachment 8957583 [details]
Bug 1436523 - Make WebRTC mochitest harness more granular with fake device setup.

https://reviewboard.mozilla.org/r/226474/#review233144

::: dom/media/tests/mochitest/head.js:35
(Diff revision 1)
>  }
>  
>  /**
>   *  Global flag to skip LoopbackTone
>   */
> -var DISABLE_LOOPBACK_TONE = false
> +var DISABLE_LOOPBACK_TONE = false;

We could also change `var` to `let` here.
Attachment #8957583 - Flags: review?(achronop) → review+
Please do not forget to do a full try run before landing.
Comment on attachment 8957580 [details]
Bug 1436523 - Rework media manager enumeration to prefer loopback to fake devices, allow mixing of fake and loopback.

https://reviewboard.mozilla.org/r/226468/#review232880

> I do not see how the expression in the assert much the description. From the message I would expect something like:
> ```
> MOZ_ASSERT(aVideoEnumType == Fake && aVideoType == MediaSourceEnum::Camera, ...)
> ```
> Probably your intention is to assert for a wider range of events here but the message do not reflect that. 
> 
> Similar to the following asserts.

I agree the check and the message are not clear. I *think* the check is correct for the message: since aVideoEnum can be Normal | Fake | Loopback checking for `aVideoEnumType == Fake && aVideoType == MediaSourceEnum::Camera` will fail if we have loopback or normal. So the check is doing an exclusion of sorts: it checks to see if we're not fake, and if we're not we'll short circut and it's okay, but if we are fake, the second check will happen, and then we need to be a camera.

I've added a comment to try and clarify this. Let me know if it reads okay.
Comment on attachment 8957582 [details]
Bug 1436523 - Update dom/media/tests/mochitest tests to better handle loopback devices.

https://reviewboard.mozilla.org/r/226472/#review233136

> Please note that fake prefs are not set here.

I wanted to remove instances of fake constraints being explicitly set where I could, then leave it to head.js to set as appropriate based on the system being run on. Since in cases where pc.js is included it brings in head.js, my understanding is we can leave the harness to do configuration unless we explicitly want fakes or loops.

> I do not see how we allow loopback device here. In case of Linux loopback device should be set by test harness. By clearing the loopback pref we force to use the fake device.
> 
> The same for the other PC tests that make use of audio constraints. Please note that those tests do not use fake constarints. The existing behavior is to make use of loopback device when is available and fake device on platforms that loopback does not exist. I think we force fake device in all cases now

I've reverted this change as I cannot reproduce issues. I agree with letting the harness decide if fakes or loops are used, but had marked tests that I saw failures in when testing.

> Continuing from previous comment, here, for example, we want fake devices so clearing the loopback pref is valid.

Roger. I think with the above issue resolved we're good for this issue. Please let me know if otherwise.
Comment on attachment 8957580 [details]
Bug 1436523 - Rework media manager enumeration to prefer loopback to fake devices, allow mixing of fake and loopback.

https://reviewboard.mozilla.org/r/226468/#review232880

> I agree the check and the message are not clear. I *think* the check is correct for the message: since aVideoEnum can be Normal | Fake | Loopback checking for `aVideoEnumType == Fake && aVideoType == MediaSourceEnum::Camera` will fail if we have loopback or normal. So the check is doing an exclusion of sorts: it checks to see if we're not fake, and if we're not we'll short circut and it's okay, but if we are fake, the second check will happen, and then we need to be a camera.
> 
> I've added a comment to try and clarify this. Let me know if it reads okay.

That's fine thanks
Comment on attachment 8957582 [details]
Bug 1436523 - Update dom/media/tests/mochitest tests to better handle loopback devices.

https://reviewboard.mozilla.org/r/226472/#review233136

> I wanted to remove instances of fake constraints being explicitly set where I could, then leave it to head.js to set as appropriate based on the system being run on. Since in cases where pc.js is included it brings in head.js, my understanding is we can leave the harness to do configuration unless we explicitly want fakes or loops.

That's fine, as soon as try is happy! Thanks, for the explanation.

> I've reverted this change as I cannot reproduce issues. I agree with letting the harness decide if fakes or loops are used, but had marked tests that I saw failures in when testing.

I am reopening this because the same applies to one more test: test_peerConnection_addAudioTrackToExistingVideoStream.html
I had a quick look, can you please verify that we haven't missed more tests. When you are ready, feel free to close the issue and continue.
Comment on attachment 8957582 [details]
Bug 1436523 - Update dom/media/tests/mochitest tests to better handle loopback devices.

https://reviewboard.mozilla.org/r/226472/#review233406

Thanks for the patches and the explanations. It looks good. Once again we need a full try run before landing.
Attachment #8957582 - Flags: review?(achronop) → review+
Attachment #8957579 - Attachment is obsolete: true
Looks like the phase got mangled on the first changeset. So I've reset it and resubmitted it. Should be no changes from approved version, but it's been reflagged as needing review.
Comment on attachment 8958967 [details]
Bug 1436523 - Add enum to specify select type of devices to enumerate in MediaManager rather than use bools.

https://reviewboard.mozilla.org/r/227818/#review233782
Attachment #8958967 - Flags: review?(achronop) → review+
Comment on attachment 8959153 [details]
Bug 1436523 - Make sure webrtc chrome mochitests don't use loopback devices.

https://reviewboard.mozilla.org/r/228020/#review236556

Looks good with one questions.

::: browser/base/content/test/webrtc/head.js:578
(Diff revision 3)
> +  let prefs = [
> +    [PREF_PERMISSION_FAKE, true],
> +    [PREF_AUDIO_LOOPBACK, ""],
> +    [PREF_VIDEO_LOOPBACK, ""],
> +    [PREF_FAKE_STREAMS, true]
> +  ];

Looks good, I wonder if we found out what is causing it.
Attachment #8959153 - Flags: review?(achronop) → review+
Comment on attachment 8960139 [details]
Bug 1436523 - Change DeviceEnumerationType to scoped enum, add extra logging.

https://reviewboard.mozilla.org/r/228940/#review236560

Looks good thanks
Attachment #8960139 - Flags: review?(achronop) → review+
Comment on attachment 8961753 [details]
Bug 1436523 - Update head.js to better handle pref changes for fake/loopback devices during tests.

https://reviewboard.mozilla.org/r/230602/#review236562

Looks good with one small question.

::: dom/media/tests/mochitest/head.js:39
(Diff revision 1)
> +    WANT_FAKE_VIDEO = true;
> -  dump("TEST DEVICES: No test device found in media.video_loopback_dev, using fake video streams.\n");
> +    dump("TEST DEVICES: No test device found in media.video_loopback_dev, using fake video streams.\n");
> -}
> +  }
> +}
> +
> +updateConfigFromFakeAndLoopbackPrefs();

Since you call it in `getUserMedia(constraints)` do you need to call it here. Do we use all these outside `getUserMedia(constraints)` and if not do we plan to use it in the future? I don't see a real problem on that I am discussing to understand it better.
Attachment #8961753 - Flags: review?(achronop) → review+
Comment on attachment 8961753 [details]
Bug 1436523 - Update head.js to better handle pref changes for fake/loopback devices during tests.

https://reviewboard.mozilla.org/r/230602/#review236562

> Since you call it in `getUserMedia(constraints)` do you need to call it here. Do we use all these outside `getUserMedia(constraints)` and if not do we plan to use it in the future? I don't see a real problem on that I am discussing to understand it better.

Right now we I think we still end up using the `WANT_FAKE_` vars before our first gUM call in order to do some of `setupEnvironment()`. We could rework so there's no global state (which I like the idea of), but I didn't want to alter the harness too much as part of this bug. Let me know if that's all good.
Comment on attachment 8961753 [details]
Bug 1436523 - Update head.js to better handle pref changes for fake/loopback devices during tests.

https://reviewboard.mozilla.org/r/230602/#review236562

> Right now we I think we still end up using the `WANT_FAKE_` vars before our first gUM call in order to do some of `setupEnvironment()`. We could rework so there's no global state (which I like the idea of), but I didn't want to alter the harness too much as part of this bug. Let me know if that's all good.

That's fine, thank you for the answer.
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b0daeb126de
Add enum to specify select type of devices to enumerate in MediaManager rather than use bools. r=achronop
https://hg.mozilla.org/integration/autoland/rev/c137d6b021d2
Rework media manager enumeration to prefer loopback to fake devices, allow mixing of fake and loopback. r=achronop
https://hg.mozilla.org/integration/autoland/rev/378575c7b837
Update dom/media/test/ tests to better handle loopback + gUM device config. r=achronop
https://hg.mozilla.org/integration/autoland/rev/8709ce7f1de1
Update dom/media/tests/mochitest tests to better handle loopback devices. r=achronop
https://hg.mozilla.org/integration/autoland/rev/b6cccecb4a6b
Make WebRTC mochitest harness more granular with fake device setup. r=achronop
https://hg.mozilla.org/integration/autoland/rev/d737101646e8
Make sure webrtc chrome mochitests don't use loopback devices. r=achronop
https://hg.mozilla.org/integration/autoland/rev/1f32dbbd3d13
Change DeviceEnumerationType to scoped enum, add extra logging. r=achronop
https://hg.mozilla.org/integration/autoland/rev/4bdd6d6050b6
Update head.js to better handle pref changes for fake/loopback devices during tests. r=achronop
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: