815002 - Provide a capability for automation to be able to fake multiple devices on a machine

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P3)

Description

[Jason Smith [:jsmith]]

In order to build automation that simulates different devices on a machine, we need an ability to fake devices on that machine. This tracks to work to be able to allow mochitest automation to be able to manage fake devices on a machine for use in test automation.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Adding whiteboard entry so we can track the needs.

Comment 2

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

This needs to simulate actual devices at least closer to the hardware level (either in or preferably below the webrtc.org capture code), so that the tests can cover the real paths for GUM.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This will be the central part of our meeting today. CC'ing folks who will be involved here.

Comment 4

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

Result in the meeting was that ted would investigate options for driver-level fake audio/video devices.  We believe they exist for all OS's and likely could be installed on the test machines.

Comment 5

[Jason Smith [:jsmith]]

Ted/Clint - If a detailed requirements spec would help for driver-level fake audio/video devices, let me know. I could hack one up together if need be.

Comment 6

[(not currently active) Ted Mielczarek]

I've put all the info on bug 826265 for setting up fake webcam/audio input on the Linux test slaves. I'll look at other platforms in a bit.

Comment 7

[Jason Smith [:jsmith]]

Although this blocks a certain set of automation I don't think we'll be able to get this done in time for FF 20. Push comes to shove, I think we can ship without this. We should still work on this however.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> I've put all the info on bug 826265 for setting up fake webcam/audio input
> on the Linux test slaves. I'll look at other platforms in a bit.

Ted, do you have any update for us regarding other platforms? Will you have the time to work on that?

Comment 9

[(not currently active) Ted Mielczarek]

I haven't done anything regarding other platforms beyond our initial survey of what software was available. I could probably make some time for this, although I'm not excited about the prospect.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: allow using loopback devices in WebRTC mochitests on Linux when available

This is not perfect but it's a start. This adds a bunch of code to the Mochitest head.js to determine if we have loopback devices available, and if so makes use of them in getUserMedia. It also adds code for feeding data to the loobpack devices (by shelling out to gstreamer or speaker-test).

This passes all the Mochitests in dom/media/tests/mochitest on my local Ubuntu 12.04 with audio/video loopback drivers installed. There are two issues with it:
1) The speaker-test process (from startFakeAudio) doesn't seem to get killed reliably (I might have just missed replacing a few instances of SimpleTest.finish).
2) The version of v4l2loopback installed on the test slaves (the one that ships in Ubuntu 12.04) has a bug that causes occasional kernel panics, I'll file a bug to get it updated to a newer version.

Comment 11

[(not currently active) Ted Mielczarek]

Filed bug 864951 on getting a newer v4l2loopback driver on the test slaves.

Comment 12

[Jason Smith [:jsmith]]

Comment on attachment 740984 [details] [diff] [review]
allow using loopback devices in WebRTC mochitests on Linux when available

Also adding whimboo for feedback to see what he thinks. I'll take a look in a sec.

Comment 13

[Jason Smith [:jsmith]]

Comment on attachment 740984 [details] [diff] [review]
allow using loopback devices in WebRTC mochitests on Linux when available

Also adding Randell given that this altering internals for getUserMediaDevices.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 740984 [details] [diff] [review]
allow using loopback devices in WebRTC mochitests on Linux when available

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

Thank you for the patch, Ted! I have taken a quick look into it and it's a great start, even it's getting complicated to follow the execution logic now. Anyway, having the functionality in place should be the first goal. I made a couple of inline comments for code which was not clear to me. Please have a look at it.

::: dom/media/tests/mochitest/head.js
@@ +11,5 @@
> +// Specifies whether we are using loopback devices
> +var LOOPBACK_ENABLED = false;
> +// Loopback devices available on the test machines
> +var video_device_name = null;
> +var audio_device_name = null;

How would this work given that we have a local and remote peer by default in our tests? Can we use a faked device for each of them or is it only for one side?

@@ +26,5 @@
> +  // ALSA, snd-aloop, recording device.
> +  audio_device_name = "plughw:CARD=Loopback,DEV=1";
> +
> +  function startFakeVideo(opts) {
> +    opts = opts || {"pattern": "green"};

Is there a way to get a rotating pattern or can you only define a hardcoded color?

@@ +31,5 @@
> +    var process = Cc["@mozilla.org/process/util;1"].
> +      createInstance(Ci.nsIProcess);
> +    var file = Cc["@mozilla.org/file/local;1"].
> +      createInstance(Ci.nsILocalFile);
> +    file.initWithPath("/usr/bin/gst-launch-0.10");

Which dependencies do we have here, and how do we handle systems which do not have this tool available? We should gracefully fallback to faked streams.

@@ +63,5 @@
> +    startFakeAudio.proc = process;
> +  }
> +
> +  //TODO: make this automatic
> +  function stopFakeAudio() {

Looks like a general method in mochitests to have as a clean-up method like we have for browser chrome tests would do a perfect job here. Otherwise we will have to be very careful to really call this method with all of the different paths we have in those tests.

@@ +80,5 @@
> +    var audioid = "audio" in opts ? opts["audio"] : null;
> +    var videoid = "video" in opts ? opts["video"] : null;
> +    var audiodevice = null;
> +    var videodevice = null;
> +    for (var d of devices) {

I have never seen `of` in such a construct. Did you meant `in`?

@@ +100,5 @@
> +      newopts.video = true;
> +      newopts.videoDevice = SpecialPowers.unwrap(videodevice);
> +    }
> +    callback(newopts);
> +  }, function(err){ callback({}, err); });

To be in sync with other methods we might want to have a separate onError callback.

@@ +110,5 @@
> + */
> +function _initLoopbackDevices(aCallback) {
> +  if (!FAKE_ENABLED) {
> +    aCallback();
> +  }

Shouldn't it get a return after `aCallback()`?

Comment 15

[(not currently active) Ted Mielczarek]

Thanks for the feedback!

(In reply to Henrik Skupin (:whimboo) from comment #14)
> How would this work given that we have a local and remote peer by default in
> our tests? Can we use a faked device for each of them or is it only for one
> side?

I don't think it's a problem to use the same device more than once, I can run gum_test.html in multiple tabs using the same loopback device for audio or video. All the existing mochitests pass using this code, so it seems to work alright.

> Is there a way to get a rotating pattern or can you only define a hardcoded
> color?

There are a bunch of built-in options for gstreamer's videotestsrc:
http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-plugins/html/gst-plugins-base-plugins-videotestsrc.html#GstVideoTestSrcPattern

I used green here just to make it simple (I figured this would be fairly easy if we actually wanted to test the video data). Note that as implemented this also only writes a single frame of data which the driver will return. If we need constant video input we can rework this to act more like startFakeAudio, with a continuously-running process.

> Which dependencies do we have here, and how do we handle systems which do
> not have this tool available? We should gracefully fallback to faked streams.

I should check that both of these programs are available up in _initLoopbackDevices, and not set LOOPBACK_ENABLED if they're missing. speaker-test comes from alsa-utils, and gst-launch-0.10 comes from gstreamer0.10-tools. These are both available on the test slaves, which is what I was mostly concerned with. (I should also write up a wiki doc somewhere on how to configure loopback devices on Linux.)

> Looks like a general method in mochitests to have as a clean-up method like
> we have for browser chrome tests would do a perfect job here. Otherwise we
> will have to be very careful to really call this method with all of the
> different paths we have in those tests.

Fully agreed. I might just bite the bullet and do that.

> > +    for (var d of devices) {
> 
> I have never seen `of` in such a construct. Did you meant `in`?

Nope, this is a new ES6 feature:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for...of

> To be in sync with other methods we might want to have a separate onError
> callback.

This makes sense.

> Shouldn't it get a return after `aCallback()`?

Yes, good catch.

Comment 16

[Jason Smith [:jsmith]]

Comment on attachment 740984 [details] [diff] [review]
allow using loopback devices in WebRTC mochitests on Linux when available

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

On the right track. There's a lot of inherent complexity exposed in the global namespace here. Can we create a wrapper or class here that external callers use such that we simplify the call logic here?

Also, what happens if you try to run this patch in a try run right now? I'm curious how well this does in try.

Note - This initial review is based on code only. I'll probably try testing this myself later.

::: dom/media/tests/mochitest/head.js
@@ +7,5 @@
>  var Cr = SpecialPowers.Cr;
>  
>  // Specifies whether we are using fake streams to run this automation
>  var FAKE_ENABLED = true;
> +// Specifies whether we are using loopback devices

Given the general complexity around the fake devices logic, we might want to pull all of this logic into a class that callers can reference to reduce complexity here. I think we should pull a lot of the complexity here out of the global namespace.

@@ +11,5 @@
> +// Specifies whether we are using loopback devices
> +var LOOPBACK_ENABLED = false;
> +// Loopback devices available on the test machines
> +var video_device_name = null;
> +var audio_device_name = null;

FWIW, the same device should be able to be used multiple times on the same page with gUM.

@@ +18,5 @@
> +function stopFakeVideo() {}
> +function startFakeAudio(opts) {}
> +function stopFakeAudio() {}
> +
> +if (navigator.userAgent.indexOf("Linux") != -1) {

Per Henrik's point below, the check here I think needs to be more than checking the user agent. You need to check really if the "fake devices" is available to be executed on the machine. If it isn't, fallback to the fake streams we already have.

Note - this complexity issue might be solved if you pull the logic into a class.

@@ +62,5 @@
> +    process.run(false, args, args.length);
> +    startFakeAudio.proc = process;
> +  }
> +
> +  //TODO: make this automatic

If this is part of the runTest logic, you might be able to make this automatic.

@@ +111,5 @@
> +function _initLoopbackDevices(aCallback) {
> +  if (!FAKE_ENABLED) {
> +    aCallback();
> +  }
> +  if (video_device_name == null && audio_device_name == null) {

This really should be the demorgan of this over the other logic below, and the aCallback logic executes after it. Meaning:

if(video_device_name !== null || audio_device_name !== null) {
    getMediaDevice logic
}

try aCallback logic here

@@ +228,5 @@
> +  if (!LOOPBACK_ENABLED) {
> +    navigator.mozGetUserMedia(constraints, onSuccess, onError);
> +  }
> +  else {
> +    if (constraints.video) {

This feels strange doing the rewrite of an existing variable. Can we reduce the complexity here?

@@ +286,2 @@
>        }, function () {
> +        _initLoopbackDevices(aCallback);

The loopback device callback here feels strange. Maybe the naming is throwing me off here?

Comment 17

[(not currently active) Ted Mielczarek]

Thanks for the feedback!

(In reply to Jason Smith [:jsmith] from comment #16)
> On the right track. There's a lot of inherent complexity exposed in the
> global namespace here. Can we create a wrapper or class here that external
> callers use such that we simplify the call logic here?

I realized the same thing while writing this. As a first pass I wanted to be minimally invasive to verify that things worked. It would definitely be better to wrap the non-public-API bits of this somehow.

> Also, what happens if you try to run this patch in a try run right now? I'm
> curious how well this does in try.

I was going to give that a shot today actually. I was waiting on a few last-minute changes I asked for from RelEng, but the last of those got fixed yesterday.

> Note - This initial review is based on code only. I'll probably try testing
> this myself later.

Ping me if you need info on how to set up your environment, since I haven't documented this particularly well yet.

> Given the general complexity around the fake devices logic, we might want to
> pull all of this logic into a class that callers can reference to reduce
> complexity here. I think we should pull a lot of the complexity here out of
> the global namespace.

Yeah, I agree. Once I've verified that this doesn't blow up on try I will do some refactoring.

> > +if (navigator.userAgent.indexOf("Linux") != -1) {
> 
> Per Henrik's point below, the check here I think needs to be more than
> checking the user agent. You need to check really if the "fake devices" is
> available to be executed on the machine. If it isn't, fallback to the fake
> streams we already have.

There are two different concerns here:
* Are we on a platform where we could use loopback devices (which is what this is testing)--this controls the selection of device names and the functionality of startFake{Video,Audio}
* Do we have loopback devices available? If we're on a supported platform we still need to verify that the loopback devices are present to use them for tests. This is what _initLoopbackDevices does.

If you have suggestions for how to make that clearer I am open to ideas.

> > +  //TODO: make this automatic
> 
> If this is part of the runTest logic, you might be able to make this
> automatic.

What that comment really means is "we should fix the Mochitest harness to allow providing cleanup functions to run at the end of a test."

> > +  if (video_device_name == null && audio_device_name == null) {
> 
> This really should be the demorgan of this over the other logic below, and
> the aCallback logic executes after it. Meaning:
> 
> if(video_device_name !== null || audio_device_name !== null) {
>     getMediaDevice logic
> }
> 
> try aCallback logic here

You're right, reordering things would probably make this clearer.

> This feels strange doing the rewrite of an existing variable. Can we reduce
> the complexity here?

I wanted to avoid losing other constraints that were passed in, but yeah, this wound up not making much sense. (I had a set of test functions I was using and an API in my head before I looked at head.js, and they just didn't match really well.)

> The loopback device callback here feels strange. Maybe the naming is
> throwing me off here?

Essentially I just wanted to do some extra setup (verify whether we have loopback devices) before starting the test. I guess realistically we could push that off until the first time getUserMedia is called, since we don't need them until that point.

Comment 18

[(not currently active) Ted Mielczarek]

Attachment: allow using loopback devices in WebRTC mochitests on Linux when available

I started refactoring this a bit, I think I've simplified a lot of it. I merged _getMediaDevices into _initLoopbackDevices which removed a lot of complexity. I think that was mostly a leftover from the API I was imagining as I tested things. This fits better into the existing structure of head.js. It could still stand some better encapsulation, I'll look at that in a bit.

I pushed this (along with some prereqs) to try to see what happens:
https://tbpl.mozilla.org/?tree=Try&rev=e791e48cef90

Comment 19

[(not currently active) Ted Mielczarek]

This is very green on mochitest-3 on Linux{32,64} {opt,debug}.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> > Is there a way to get a rotating pattern or can you only define a hardcoded
> > color?
> 
> There are a bunch of built-in options for gstreamer's videotestsrc:
> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-
> plugins/html/gst-plugins-base-plugins-videotestsrc.
> html#GstVideoTestSrcPattern
> 
> I used green here just to make it simple (I figured this would be fairly
> easy if we actually wanted to test the video data). Note that as implemented
> this also only writes a single frame of data which the driver will return.
> If we need constant video input we can rework this to act more like
> startFakeAudio, with a continuously-running process.

I think that would be better. That's what we currently have with faked streams. We have a rotating pattern through colors. 

> > Looks like a general method in mochitests to have as a clean-up method like
> > we have for browser chrome tests would do a perfect job here. Otherwise we
> > will have to be very careful to really call this method with all of the
> > different paths we have in those tests.
> 
> Fully agreed. I might just bite the bullet and do that.

How difficult would it be to port the registerCleanupFunction method from browser chrome to mochitest? See:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#602

> 
> > > +    for (var d of devices) {
> > 
> > I have never seen `of` in such a construct. Did you meant `in`?
> 
> Nope, this is a new ES6 feature:
> https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for.
> ..of

Lovely. What I have been waiting for. Thanks a ton for the reference.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> > > Looks like a general method in mochitests to have as a clean-up method like
> > > we have for browser chrome tests would do a perfect job here. Otherwise we
> > > will have to be very careful to really call this method with all of the
> > > different paths we have in those tests.
> > 
> > Fully agreed. I might just bite the bullet and do that.
> 
> How difficult would it be to port the registerCleanupFunction method from
> browser chrome to mochitest? See:
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.
> js#602

I filed bug 866641 for it.

Comment 22

[(not currently active) Ted Mielczarek]

Attachment: allow using loopback devices in WebRTC mochitests on Linux when available

This still isn't perfect, but I'm running out of energy to do any more cleanup. I'll push this to try again and see what it looks like, it passes all my tests locally.

Comment 23

[(not currently active) Ted Mielczarek]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> I think that would be better. That's what we currently have with faked
> streams. We have a rotating pattern through colors. 

This turns out to be a pain to do so I punted on it. We'll get solid green for now. You can feed almost anything to gstreamer, so it should be straightforward to pass in a video file or a set of images if you need to test something specific. You can also just use the same code that I already have there to change the solid color if you need a simple test.

> How difficult would it be to port the registerCleanupFunction method from
> browser chrome to mochitest? See:
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.
> js#602

I patched this in bug 866641.

I wrapped all the implementation details in an anonymous function to hide them from tests, but I don't really have the motivation to do further refactoring. I'm happy to hand this off to someone else if you care to do that, but otherwise I think we should just get this reviewed and land it.

Comment 24

[(not currently active) Ted Mielczarek]

I pushed this to try, and it passed on opt but failed a test on debug (different tests on Linux32/64):
https://tbpl.mozilla.org/?tree=Try&rev=1caa45282b44

It looks like it just gets unlucky and hits a GC in the middle of a test, which makes it run slowly enough that it fails to get the stream data in time. I'll test and see if raising the timeout slightly fixes this.

Comment 25

[(not currently active) Ted Mielczarek]

Pushed to try with a change to make it double the timeout only on debug builds:
https://tbpl.mozilla.org/?tree=Try&rev=ddde3685a43f

Comment 26

[(not currently active) Ted Mielczarek]

This try push had one intermittent failure on the linux opt mochitest-3. It looks like the speaker-test app failed to run properly, but I can't figure out why. I'm a little leery of pushing something that can add more intermittent failures, we certainly have enough of those already. I could wallpaper over this, since it's failing when it tries to shutdown the process, but that would make it ignore cases where the process failed to start.

Comment 27

[(not currently active) Ted Mielczarek]

Attachment: allow using loopback devices in WebRTC mochitests on Linux when available

I rewrote this patch to build on the patch from bug 934667. It works well in local testing, but I need to verify that everything works on the test machines. I'll push this to try and give it a shot.

On the plus side this vastly simplifies the changes to the Mochitests--they simply check if the prefs are set and stop passing {fake:true} to gUM. There's still some complexity of configuring the media devices, but I pushed that down into the Python harness so it only has to be done once, on startup.

Comment 28

[(not currently active) Ted Mielczarek]

Attachment: allow using loopback devices in WebRTC mochitests on Linux when available

I tweaked this a bit and it works on Try, but we're hitting some weird crashes on opt builds:
https://tbpl.mozilla.org/?tree=Try&rev=678452c3b3f0

Specifically we're hitting a pure virtual function call and exiting, which is weird because Breakpad ought to be catching those.

Comment 29

[(not currently active) Ted Mielczarek]

Apparently that crash is already filed: bug 999289.

Comment 30

[(not currently active) Ted Mielczarek]

Attachment: allow using loopback devices in WebRTC mochitests on Linux when available

Okay, since jesup is already working on fixing the crash that shows up on try I might as well get this reviewed.

Jason: the JavaScript bits are pretty straightforward. We're just looking to see if both of the loopback media prefs are set, and if so setting FAKE_ENABLED=false.

Joel: the Python bits here are a little hairy. I use some ctypes to do ioctls on the video devices, it's mostly a straight copy of the C code in v4l2-ctl:
https://github.com/koradlow/v4l2-rds-ctl/blob/master/utils/v4l2-ctl/v4l2-ctl.cpp#L892

The special v4l2loopback ioctls are described in the driver:
https://github.com/umlaeute/v4l2loopback/blob/fd822cf0faaccdf5f548cddd9a5a3dcebb6d584d/v4l2loopback.c#L131

The alternative here would be to require v4l2-ctl to be installed on the test machines. If you think this is too terrible we could do that instead.

We also run gst-launch (gstreamer commandline) to feed a single frame of video into the loopback video device, which it will continuously transmit (from the settings we fed it via ioctl).

Then we use pactl (the PulseAudio command line control tool) to find and load 'module-sine-source', which provides an audio device that emits a sine wave.

Comment 31

[Jason Smith [:jsmith]]

Comment on attachment 8418040 [details] [diff] [review]
allow using loopback devices in WebRTC mochitests on Linux when available

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

::: dom/media/tests/mochitest/head.js
@@ +9,5 @@
>  // Specifies whether we are using fake streams to run this automation
>  var FAKE_ENABLED = true;
> +try {
> +  var a = SpecialPowers.getCharPref('media.audio_loopback_dev');
> +  var v = SpecialPowers.getCharPref('media.video_loopback_dev');

Nit - can we use descriptive variable names here?

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8418040 [details] [diff] [review]
allow using loopback devices in WebRTC mochitests on Linux when available

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

I don't like assumptions about tools, and would like to have more understanding about what fake means as I am confused.

::: dom/media/tests/mochitest/head.js
@@ +9,5 @@
>  // Specifies whether we are using fake streams to run this automation
>  var FAKE_ENABLED = true;
> +try {
> +  var a = SpecialPowers.getCharPref('media.audio_loopback_dev');
> +  var v = SpecialPowers.getCharPref('media.video_loopback_dev');

this is confusing because if we add --use-fake-media-devices we set these preferences and then set FAKE_ENABLED=false.  I am not sure what is fake and what is not after reading this patch.

@@ +14,5 @@
> +  dump('Using loopback devices:\n');
> +  dump('audio: ' + a + '\nvideo: ' + v + '\n');
> +  FAKE_ENABLED = false;
> +} catch (e) {
> +  dump('No loopback devices\n');

nit: I would prefer more descriptions in the dump statements: i.e.:
no loopback devices found in preferences media.audio_loopback_dev or media.video_loopback_dev

::: testing/mochitest/runtests.py
@@ +845,5 @@
> +
> +  # Feed it a frame of output so it has something to display
> +  subprocess.check_call(['/usr/bin/gst-launch-0.10', 'videotestsrc',
> +                         'pattern=green', 'num-buffers=1', '!',
> +                         'v4l2sink', 'device=%s' % device])

we assume this gst-launch-0.10 is installed, this could fail locally.  do we depend on the output here?

@@ +850,5 @@
> +  info['video'] = name
> +
> +  # Use pactl to see if the PulseAudio module-sine-source module is loaded.
> +  def sine_source_loaded():
> +    o = subprocess.check_output(['pactl', 'list', 'short', 'modules'])

another assumption about a tool

Comment 33

[Jason Smith [:jsmith]]

Comment on attachment 8418040 [details] [diff] [review]
allow using loopback devices in WebRTC mochitests on Linux when available

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

::: dom/media/tests/mochitest/head.js
@@ +9,5 @@
>  // Specifies whether we are using fake streams to run this automation
>  var FAKE_ENABLED = true;
> +try {
> +  var a = SpecialPowers.getCharPref('media.audio_loopback_dev');
> +  var v = SpecialPowers.getCharPref('media.video_loopback_dev');

We could rename the constant to FAKE_STREAMS_ENABLED it that would help clarify the code here a bit better.

Comment 34

[(not currently active) Ted Mielczarek]

(In reply to Joel Maher (:jmaher) from comment #32)
> this is confusing because if we add --use-fake-media-devices we set these
> preferences and then set FAKE_ENABLED=false.  I am not sure what is fake and
> what is not after reading this patch.

Upon reflection, yes, I named things terribly. There are basically three categories of possibility here:
1) Real audio/video devices (microphone, webcam)
2) The devices I'm configuring with this patch, which are real drivers but don't represent real devices (loopback video driver, sine wave audio generator)
3) Fake streams from the DOM code, which are synthesized within the browser.

I agree that calling category 2 "fake' is confusing because category 3 is already called that. I had originally called them "loopback" in an earlier patch when I was using loopback devices for both, but since I switched the audio device to not actually be a loopback device that felt like a misnomer. I can probably come up with a less confusing name.
> ::: testing/mochitest/runtests.py
> @@ +845,5 @@
> > +
> > +  # Feed it a frame of output so it has something to display
> > +  subprocess.check_call(['/usr/bin/gst-launch-0.10', 'videotestsrc',
> > +                         'pattern=green', 'num-buffers=1', '!',
> > +                         'v4l2sink', 'device=%s' % device])
> 
> we assume this gst-launch-0.10 is installed, this could fail locally.  do we
> depend on the output here?
> 
> @@ +850,5 @@
> > +  info['video'] = name
> > +
> > +  # Use pactl to see if the PulseAudio module-sine-source module is loaded.
> > +  def sine_source_loaded():
> > +    o = subprocess.check_output(['pactl', 'list', 'short', 'modules'])
> 
> another assumption about a tool

If I make verifyOptions check for the presence of both tools and error early does that make you feel better? I agree that we ought to at least present a reasonable error if a required tool is missing.

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

verifyOptions would be a good place for this.

Comment 36

[(not currently active) Ted Mielczarek]

Attachment: allow using loopback devices in WebRTC mochitests on Linux when available.

I added checks in verifyOptions to make sure we have all the binaries we need, and I renamed --use-fake-media-devices to --use-test-media-devices. It's sort of a bland name, but less confusing perhaps.

Comment 37

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8418214 [details] [diff] [review]
allow using loopback devices in WebRTC mochitests on Linux when available.

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

this is much better.

Comment 38

[(not currently active) Ted Mielczarek]

Attachment: Enable loopback devices on Linux Mochitests

This is the mozharness config patch to actually enable this for Linux Mochitests. This needs to wait on the outstanding dep to land so it doesn't cause shutdown crashes.

Comment 39

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/575f54b18f5d

Waiting on bug 999289 to land the mozharness patch.

Comment 40

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

https://hg.mozilla.org/mozilla-central/rev/575f54b18f5d

Comment 41

[Maire Reavy [:mreavy]]

Just updating the points to reflect the status of the bug.

Comment 42

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

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #39)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/575f54b18f5d
> 
> Waiting on bug 999289 to land the mozharness patch.

Ok, that's in.  Are we good to land this?

Comment 43

[(not currently active) Ted Mielczarek]

Yep, a try build with your patches + this patch looked good. I'll land today.

Comment 44

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a499c7e9ab6

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux mochitest orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c823c4af278

https://tbpl.mozilla.org/php/getParsedLog.php?id=40636554&tree=Mozilla-Inbound
06:55:06     INFO -  Couldn't find a v4l2loopback video device
06:55:06     INFO -  Could not find test media devices to use

https://tbpl.mozilla.org/php/getParsedLog.php?id=40637582&tree=Mozilla-Inbound
06:55:09  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_devices_get_user_media.js | Test timed out

What platforms did you run on Try?

Comment 46

[(not currently active) Ted Mielczarek]

I think there are actually two problems here. One of them is bug 1017634, which is a RelEng issue. The other is that browser-chrome failure, which I didn't see because I wasn't running browser-chrome tests because I had no idea we were testing getUserMedia in browser-chrome tests. I'll fix that separately.

Comment 47

[(not currently active) Ted Mielczarek]

Attachment: fix browser_device_get_user_media.js to work with --use-test-media-devices

This patch fixes browser_device_get_user_media.js to use the same logic as the plain gUM mochitests for determining whether to use fake streams or actual loopback devices.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

These failures were also occurring frequently with this patch landed:
https://tbpl.mozilla.org/php/getParsedLog.php?id=40640399&tree=Mozilla-Inbound

Comment 49

[(not currently active) Ted Mielczarek]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #48)
> These failures were also occurring frequently with this patch landed:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=40640399&tree=Mozilla-
> Inbound

Thanks, filed bug 1017774 on that.

Comment 50

[Florian Quèze [:florian]]

Comment on attachment 8430888 [details] [diff] [review]
fix browser_device_get_user_media.js to work with --use-test-media-devices

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

This code change seems reasonable, although I don't understand why specifying "fake: true" in the gUM options would ever cause the tests to fail. Could you explain this, or point me to a previous comment here that explains this?

::: browser/base/content/test/general/get_user_media.html
@@ +4,5 @@
>  <body>
>  <div id="message"></div>
>  <script>
> +// Specifies whether we are using fake streams to run this automation
> +var FAKE_ENABLED = true;

FAKE_ENABLED isn't a very explicit name, would useFakeStreams be better?

@@ +6,5 @@
>  <script>
> +// Specifies whether we are using fake streams to run this automation
> +var FAKE_ENABLED = true;
> +try {
> +  var audioDevice = SpecialPowers.getCharPref('media.audio_loopback_dev');

Nit: The rest of this test, and I think most of the files in this folder use double quotes for strings.

@@ +10,5 @@
> +  var audioDevice = SpecialPowers.getCharPref('media.audio_loopback_dev');
> +  var videoDevice = SpecialPowers.getCharPref('media.video_loopback_dev');
> +  dump('TEST DEVICES: Using media devices:\n');
> +  dump('audio: ' + audioDevice + '\nvideo: ' + videoDevice + '\n');
> +  FAKE_ENABLED = false;

I assume you are sure that the prefs will always either have a valid value, or not exist at all (so that getCharPref throws). If this assumption isn't correct, you may want to null check audioDevice and videoDevice before setting FAKE_ENABLED to false.

@@ +33,1 @@
>                                     function(stream) {

nit: You no longer need the line break before "function(stream)".

Comment 51

[(not currently active) Ted Mielczarek]

(In reply to Florian Quèze [:florian] [:flo] (Away until June 2nd) from comment #50)
> Comment on attachment 8430888 [details] [diff] [review]
> fix browser_device_get_user_media.js to work with --use-test-media-devices
> 
> Review of attachment 8430888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This code change seems reasonable, although I don't understand why
> specifying "fake: true" in the gUM options would ever cause the tests to
> fail. Could you explain this, or point me to a previous comment here that
> explains this?

It's unexpected fallout from attachment 8418214 [details] [diff] [review] + attachment 8419557 [details] [diff] [review]. With those two patches applied, on Linux the Mochitest harness will set the media.{audio,video}_loopback_dev preferences, which will cause the WebRTC backend to hand those devices back as the only option. In that situation if you pass fake:true, the backend tells you it doesn't have a matching device so these tests hang.

We could probably fix the backend to support this case a little better, but using this patch has the nice side-effect that you wind up testing with "real" (as in actually provided by the OS) devices, so it tests more of the actual code we're shipping to users.


> FAKE_ENABLED isn't a very explicit name, would useFakeStreams be better?

I'm happy to use whatever you want. This is just copied verbatim from dom/media/tests/mochitest/head.js.

> I assume you are sure that the prefs will always either have a valid value,
> or not exist at all (so that getCharPref throws). If this assumption isn't
> correct, you may want to null check audioDevice and videoDevice before
> setting FAKE_ENABLED to false.

Yes, these prefs aren't set by default and we explicitly set them in the Python Mochitest harness, so they'll either be missing or have a useful value.

Thanks for the review!

Comment 52

[(not currently active) Ted Mielczarek]

Looking nice and green on Try:
https://tbpl.mozilla.org/?tree=Try&rev=89a1d204ad9b

Comment 53

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05ddfd9f4f09

Comment 54

[(not currently active) Ted Mielczarek]

I'm going to file followup bugs for making this work on Mac and Windows.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Backed out for hitting bug 1017634 again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/38d3a91774e2

https://tbpl.mozilla.org/php/getParsedLog.php?id=40894987&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=40895363&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=40895391&tree=Mozilla-Inbound
etc...

Comment 56

[Rail Aliiev [:rail]]

The slaves listed in the logs above are still the "old style" slaves, with puppet enabled on boot. I just checked the slaves and all of them have v4l2loopback module loaded and cltbld in the "video" group. Any other ideas why it may happen?

[[e-mail] ~]# lsmod
Module                  Size  Used by
binfmt_misc            17292  1 
snd_aloop              18892  3 
snd_pcm                80845  2 snd_aloop
snd_seq_midi           13132  0 
snd_rawmidi            25424  1 snd_seq_midi
snd_seq_midi_event     14475  1 snd_seq_midi
snd_seq                51567  2 snd_seq_midi,snd_seq_midi_event
snd_timer              28931  2 snd_pcm,snd_seq
snd_seq_device         14172  3 snd_seq_midi,snd_rawmidi,snd_seq
snd                    62064  11 snd_aloop,snd_pcm,snd_rawmidi,snd_seq,snd_timer,snd_seq_device
soundcore              14635  1 snd
snd_page_alloc         14108  1 snd_pcm
v4l2loopback           28853  0 
videodev               86588  1 v4l2loopback

Comment 57

[Rail Aliiev [:rail]]

# id cltbld
uid=1000(cltbld) gid=1000(cltbld) groups=1000(cltbld),29(audio),44(video)

Comment 58

[Rail Aliiev [:rail]]

http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#793
fd = libc.open(device, O_RDWR)
this one returns -1

Comment 59

[Rail Aliiev [:rail]]

# ls -l /dev/video0
crw-rw---- 1 root video 81, 0 Jun  2 15:44 /dev/video0

Comment 60

[Rail Aliiev [:rail]]

hmm, on another instance it gives me 
(True, 'Dummy video device')
permissions on /dev/video0 and groups are the same...

Comment 61

[Rail Aliiev [:rail]]

oops, my bad, I was trying to open /dev/video instead of /dev/video0. The mystery is not resolved yet...

Comment 62

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

https://hg.mozilla.org/mozilla-central/rev/cb7eb7fcf67b
https://hg.mozilla.org/mozilla-central/rev/05ddfd9f4f09

Comment 63

[Rail Aliiev [:rail]]

I think I found the issue. In bug 1017634 we changed the way how we load the modules. The new approach may have a race condition when the modules are not loaded on first run (module packages installed, but /etc/modules is not populated yet). I believe the patch attached will fix this issue.

Reopening the bug since the patch was backed out.

Comment 64

[Carsten Book [:Tomcat]]

(In reply to Wes Kocher (:KWierso) from comment #62)
> https://hg.mozilla.org/mozilla-central/rev/05ddfd9f4f09

backed this also now out from m-c (and will merge it to the other trees like fx-team) to fix the errors there

Comment 65

[Rail Aliiev [:rail]]

Take 2? The underlying issue should be fixed now.

Comment 66

[(not currently active) Ted Mielczarek]

Third time's the charm:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6148fa86eddf

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6148fa86eddf