1110973 - make functional tests use fake video & audio to avoid requiring real cameras and make cropping issues obvious

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

The idea is

* we shouldn't need real cameras on machines that run the test
* we should be able to check that the video looks like what we expect
** eg that local video is not cropped, for privacy reasons

After a bunch of discussion starting at <http://logs.glob.uno/?c=mozilla%23media#c132981>, I think should be fairly simple to monkeypatch gUM to return a MediaStream (or maybe even a LocalMediaStream) containing the video from a <video>, as suggested by abr.  I'm not sure if the idea is to use mozSrcOjbect, or mozCaptureStream.  Jib linked to an example using the latter further down the log.

For the cropping thing, it should be simple enough to generate a video file with a 1-pixel border around the edges so that at least it's manually visible.  Automating this could be done either here or in another bug by simply grabbing a canvas from the stream and looking for the border on all the edges.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

The idea, then, would be to write a tiny library that does this sort of stubbing, and add some sort of config parameter to both standalone and desktop for a debug mode that would use fake video & audio instead of the real thing.

Comment 2

[Nils Ohlmeier [:drno]]

I have something like this implemented on the e2e test already. But in a hacky way:

I'm patching sdk.js like this:

@@ -17614,7 +17614,8 @@
   // The default constraints
   var defaultConstraints = {
     audio: true,
-    video: true
+    video: true,
+    fake: true
   };

As it turns out that the TokBox SDK rejects unknown constraints. By using this patched sdk.js in Firefox and on the standalone server the e2e test is currently using Firefox fake sources.

I think we should have a better way accomplishing this. Especially as this hack only works with Firefox (although it should be even easier with Chrome as we only need to start Chrome with special arguments to make it use fake sources as well - no contraints).

The last (undocumented) argument on this line in the sdk.js looks promising in terms of being able to monkeypatch/(=?)overwrite gUM:

  OT.$.getUserMedia = function(constraints, success, failure, accessDialogOpened,
    accessDialogClosed, accessDenied, customGetUserMedia) {

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

The ideal thing would be to get fake standardized and implemented cross browser, but after talking to mt, that's not currently underway.

In the meantime, perhaps TokBox would be interested in tweaking the SDK to let that specific constraint through to Firefox?

It would also be interesting to learn a bit more about the undocumented "customGetUserMedia" in the SDK.

Adding a few folks to the CC who might have thoughts here...

Comment 4

[Mark Banner (:standard8)]

I was pondering if it'd be possible for the functional tests to inject a gUM setup for fake streams, therefore not requiring us to modify the app at still. However, I haven't yet thought of a way to do that from Marionette

Comment 5

[Mark Banner (:standard8)]

I've been thinking about this a bit more, and I can't really think of a easy way we could inject something for functional tests, but not affect the browser set-up.

It then basically comes down to preference setting - i.e. we could just set a preference in the loop tests to turn on "fake" for Loop itself.

However, I'm also wondering if we shouldn't just do a global pref for gUM that can force fake media in the same way as passing the constraint does.

Adam, would that be a reasonable pref to add for gUM in the core code?

Comment 6

[Nils Ohlmeier [:drno]]

(In reply to Mark Banner (:standard8) from comment #5)
> However, I'm also wondering if we shouldn't just do a global pref for gUM
> that can force fake media in the same way as passing the constraint does.
> 
> Adam, would that be a reasonable pref to add for gUM in the core code?

A pref would work. I think a command line option like Google Chrome has it would be even easier from a testing point of view (as we would not have to worry about/how to set the prefs - although we have to do that for most of our tests any way, so more more setting does not matter).

Comment 7

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #5)
> I'm also wondering if we shouldn't just do a global pref for gUM
> that can force fake media in the same way as passing the constraint does.
> 
> Adam, would that be a reasonable pref to add for gUM in the core code?

I don't see anything particularly ugly in doing so. It's a very small change to MediaManager -- basically, find any place where MediaStreamConstraints::mFake is checked, and check for the new pref. Architecturally, I think it's fine: there are lots of nerd-knobs in this code to assist with testing, some of which break things pretty profoundly. This isn't qualitatively different from something like "media.navigator.video.enabled".

Comment 8

[Adam Roach [:abr]]

BTW, adding a border to the fake video to check for cropping would probably be a reasonable thing to do.

Comment 9

[Adam Roach [:abr]]

FWIW, the Chrome means of doing this is a "--use-fake-device-for-media-stream" commandline flag.

Comment 10

[Mark Banner (:standard8)]

Attachment: Add a preference for enabling fake streams for tests, and use it in the Loop functional tests.

Thanks for the hint Adam. Something like this? (feel free to redirect the review if appropriate).

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8568182 [details] [diff] [review]
Add a preference for enabling fake streams for tests, and use it in the Loop functional tests.

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

Two minor recommendations:
- I think media.navigator.streams.fake would be a little bit more consistent (not mixing . and _)
- I would recommend to one #define at the top of MediaManager.cpp for the name

But otherwise looks good!

Comment 12

[Adam Roach [:abr]]

Comment on attachment 8568182 [details] [diff] [review]
Add a preference for enabling fake streams for tests, and use it in the Loop functional tests.

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

This looks fine to me, but I'm not a peer for DOM. I'm in the process of finding someone to give an official r+

::: dom/media/MediaManager.cpp
@@ +1597,5 @@
>    if (!Preferences::GetBool("media.navigator.video.enabled", true)) {
>      c.mVideo.SetAsBoolean() = false;
>    }
> +  bool fake = true;
> +  if (!c.mFake && !Preferences::GetBool("media.navigator.fake_streams", false)) {

I think the convention in this code is to wrap at 80 characters.

Comment 13

[Adam Roach [:abr]]

Comment on attachment 8568182 [details] [diff] [review]
Add a preference for enabling fake streams for tests, and use it in the Loop functional tests.

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

r=smaug via IRC

Comment 14

[Mark Banner (:standard8)]

Attachment: Add a preference for enabling fake streams for tests, and use it in the Loop functional tests.

As I was getting ready to land this, I noticed a few tests weren't being run. Somehow the way I was running the tests was covering up an assertion failing due to trying to obtain preferences off the main thread. I suspect it'd have been picked up on landing.

In any case, this fixes the issue by passing the preference value into the thread, hence re-requesting review as non-trivial change.

Comment 15

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/49fbb330d910

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/49fbb330d910

Comment 17

[Mark Banner (:standard8)]

Nils, now we've resolved this, can you try removing your hacks from the test server?

Comment 18

[Nils Ohlmeier [:drno]]

Once the marionette issues is resolved (tomorrow after the next Nightly build I hope) I can try.
Lets see if that setting works in the conversation window and the standalone client...

Comment 19

[Nils Ohlmeier [:drno]]

Works!
I removed the patching of the SDK from the test setup.