1468721 - Add new mochitest argument (--use-real-media-devices)

Status: NEW

(Core :: WebRTC, enhancement, P3)

Description

[Roxana Robotin]

Add a new argument to the ./mach mochitest command: --use-real-media-devices
It will be used to run test with real devices by default. 
Tests run with this command ./mach mochitest --use-real-media-devices [test_name] will not need new implementation in order to user real devices. The default device for video or audio will be used

Comment 1

[Alex Chronopoulos [:achronop]]

BTW that's the case when we use no argument at all.

Comment 2

[Andreas Pehrson [:pehrsons]]

Could be useful to allow passing the names of these devices in with the command.

Comment 3

[Roxana Robotin]

(In reply to Alex Chronopoulos [:achronop] from comment #1)
> BTW that's the case when we use no argument at all.

There is always a message that displays that fake devices are being used: 
"1:05.92 GECKO(30896) TEST DEVICES: No test device found in media.audio_loopback_dev, using fake audio streams.
1:05.92 GECKO(30896) TEST DEVICES: No test device found in media.video_loopback_dev, using fake video streams."
Real devices seem to be bypassed.

Comment 4

[Alex Chronopoulos [:achronop]]

Yeah that's right, my bad. It has been changed recently to make use of either fake or loopback. The argument makes sense. Keep in your mind that there are tests that force the use of fake devices like [1] so you might want to change that. 

[1] https://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/test_getUserMedia_constraints.html#105

Comment 5

[Roxana Robotin]

(In reply to Andreas Pehrson [:pehrsons] from comment #2)
> Could be useful to allow passing the names of these devices in with the
> command.

This is a good suggestion. I will look into it, thank you

Comment 6

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

(In reply to Andreas Pehrson [:pehrsons] from comment #2)
> Could be useful to allow passing the names of these devices in with the
> command.

Wouldn't the name be specific to the system you run the test on? What's the use-case for that? We have a fairly rich JS api for enumerating devices.

Comment 7

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #6)
> (In reply to Andreas Pehrson [:pehrsons] from comment #2)
> > Could be useful to allow passing the names of these devices in with the
> > command.
> 
> Wouldn't the name be specific to the system you run the test on? What's the
> use-case for that? We have a fairly rich JS api for enumerating devices.

I'm talking about the command line and giving them as arguments there.

The reason for that is simple; it avoids magically choosing the right devices.

Also, it makes the implementation quite simple, as we currently for loopback devices (with --use-test-media-devices) pass them through to tests at [1]. What we pass to tests is the device names. These names are known to the test runner because 1) it's always linux, and 2) it's always the same virtual devices.

For real devices, the names won't be known to the runner anymore so the runner will either have to be smart and pick the ones the user (the person running the mach command) wanted to test with (magic), or pass in the ones explicitly given by the user.

Admittedly it can be hard for the user to know the exact strings, so allowing for regexes is probably more sensible.


With this we can maintain all current code for choosing devices.

We probably want to define an extra flag in the test runner though, so that test manifests can skip tests that are not expected to pass with real devices.
`skip-if = real-devices` or some such.


[1] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/testing/mochitest/runtests.py#1928-1929

Comment 8

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

Right now I suspect the first use-case for this command line will be to execute mochitests manually by hand. As such, I don't think we should require the name on the command line, for the reason you mention that it can be hard to get right on the different systems people might end up running it on.

It seems to me the simplest thing is for --use-test-media-devices alone to turn off fake devices and use the default devices like what would normally happen in Firefox. That would seem to produce good results simply in most cases.

That said, being able to add the name optionally might be useful. It might let people run all mochitests against specific USB devices they have plugged in on laptops that come with built-in devices.

> We probably want to define an extra flag in the test runner though, so that test manifests can skip tests that are not expected to pass with real devices.

Yes, I agree we want the behavior to limit tests to specific configurations, including real devices. Don't we have tests already that only run for loopback devices? Whether by pref or flag we should probably use the same mechanism for both cases.

Being able to write new tests specifically for (manual for now) running on machines with real devices, is a specific goal I believe. E.g. this should let us write mochitests for constraints and other things we don't have automation for today. 

Long-term if we're ever able to set up test machines with real devices, even off-tree, if softvision wants to run them, these automated tests would be ready to go.

Comment 9

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #8)
> Right now I suspect the first use-case for this command line will be to
> execute mochitests manually by hand. As such, I don't think we should
> require the name on the command line, for the reason you mention that it can
> be hard to get right on the different systems people might end up running it
> on.

Well, it needs to be clearly documented which devices get chosen in mach then.


> It seems to me the simplest thing is for --use-test-media-devices alone to
> turn off fake devices and use the default devices like what would normally
> happen in Firefox. That would seem to produce good results simply in most
> cases.

Let's not use --use-test-media-devices for running with non-test (i.e., real) devices. Many tests don't show the video from the camera so it will be confusing to figure out what's going on, and to understand what is meant to be going on.

They'll then also be sending various sine tones to speakers which will greatly shock and annoy the user (been there, volume was high).


> That said, being able to add the name optionally might be useful. It might
> let people run all mochitests against specific USB devices they have plugged
> in on laptops that come with built-in devices.
> 
> > We probably want to define an extra flag in the test runner though, so that test manifests can skip tests that are not expected to pass with real devices.
> 
> Yes, I agree we want the behavior to limit tests to specific configurations,
> including real devices. Don't we have tests already that only run for
> loopback devices? Whether by pref or flag we should probably use the same
> mechanism for both cases.

There is one afaik and it just returns early if there's no loopback audio device present. Optimally it should be skipped in those cases instead. 


> Being able to write new tests specifically for (manual for now) running on
> machines with real devices, is a specific goal I believe. E.g. this should
> let us write mochitests for constraints and other things we don't have
> automation for today. 
> 
> Long-term if we're ever able to set up test machines with real devices, even
> off-tree, if softvision wants to run them, these automated tests would be
> ready to go.



Perhaps we should all agree on the interface we want this work to have before writing code in the (potentially) wrong direction. Is there a design doc or such available for this?

Comment 10

[Roxana Robotin]

> Perhaps we should all agree on the interface we want this work to have
> before writing code in the (potentially) wrong direction. Is there a design
> doc or such available for this?

There is no document on this, but I will create one, based on this discussions and add it here

Comment 11

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

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #8)
> It seems to me the simplest thing is for --use-test-media-devices alone to
> turn off fake devices and use the default devices like what would normally
> happen in Firefox. That would seem to produce good results simply in most
> cases.

I meant --use-real-media-devices here. Sorry for the confusion.

Comment 12

[Roxana Robotin]

Attachment: Bug_1468721_Add_new_mochitest_argument.txt

First commit on this task (added new command argument and a new test using real devices [dom/media/tests/mochitest/test_realDevices_getUserMedia_active_autoplay.html] ). Please review and let me know if you need more information

Comment 13

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

Comment on attachment 9006476 [details] [diff] [review]
Bug_1468721_Add_new_mochitest_argument.txt

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

Hi, sorry for the delay in reviewing.

::: testing/mochitest/runtests.py
@@ +53,4 @@
>      tags,
>  )
>  
> +from subprocess import call, check_output

call() and check_output() appead unused. Can this be removed?

@@ +844,5 @@
> +    name = 'Microsoft Corp. LifeCam HD-3000'
> +    audio = 'LifeCam HD-3000 Analog Mono'
> +    info['video'] = name
> +    info['audio'] = audio
> +    return info

This function appears to hardcode strings for a specific system. We probably shouldn't rely on that.

@@ +1946,5 @@
>              prefs['media.cubeb.output_device'] = "Null Output"
>              prefs['media.volume_scale'] = "1.0"
>  
> +        # See if we should use REAL media devices.
> +        if options.useRealMediaDevices:

As you mentioned offline, this produces the following error when I run it:

    AttributeError: 'Namespace' object has no attribute 'useRealMediaDevices'

What's missing is an entry modeled after useTestMediaDevices here:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/testing/mochitest/mochitest_options.py#485

When I add that, it spins around. Hope that helps.

@@ +1947,5 @@
>              prefs['media.volume_scale'] = "1.0"
>  
> +        # See if we should use REAL media devices.
> +        if options.useRealMediaDevices:
> +            prefs['media.navigator.permission.real'] = True;

I'm assuming more is coming here.

Comment 14

[Roxana Robotin]

Attachment: Bug_1468721_Add_new_mochitest_argument_partII.txt

Second commit on this task (added missing files). Please review and let me know if you need more information

Comment 15

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Jan-Ivar, is this still on the radar?

Comment 16

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

Patch needs to be rebased basically. I haven't had time to review and test it unfortunately. It's lingering in my review queue atm.

Comment 17

[Paul Adenot (:padenot)]

ping, this is still showing in triage queries.

Comment 18

[Michael Froman [:mjf]]

re-ping. Any movement here?

Comment 19

[Alex Chronopoulos [:achronop]]

Jib, I can help to rebase and land this one. Due to migration to phabricator all comments are lost. Could you give me a small description what is left to be done?

Comment 20

[Alex Chronopoulos [:achronop]]

I am resetting the ping to Jib on my question above.

Comment 21

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

Hi Alex, sorry I didn't catch the ni? change. Thanks for looking at this!

I don't recall there being any outstanding review issues. It's mostly about rebasing the patch and testing it. Testing may reveal issues of course. Feel free to hit me up on slack if you run into issues.

Comment 22

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

As above, pinging this because it's showing up in triage. Should we remove the reviewer until the patch is reworked so that folks do not keep having to pop in?

Comment 23

[Alex Chronopoulos [:achronop]]

This patch looks like it has been left in the middle. The device names are hardcoded, the test contains wait() methods. Also, the gUM request contains a constraint realDevices: true which I doubt we support. I don't think that simple rebasing and testing is the case here.

Comment 24

[Alex Chronopoulos [:achronop]]

I am leaving that for now since it requires some work and it's not a priority.