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

RESOLVED FIXED in Firefox 39

Status

Hello (Loop)
Client
P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dmose, Assigned: standard8)

Tracking

unspecified
mozilla39
Points:
2
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [tech-debt])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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.
(Reporter)

Updated

3 years ago
Summary: make functional tests use fake video & audio tDiscoo avoid requiring real cameras and make cropping issues obvious → make functional tests use fake video & audio to avoid requiring real cameras and make cropping issues obvious
backlog: --- → Fx38?

Updated

3 years ago
backlog: Fx38? → tech-debt
Priority: -- → P2
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) {
(Reporter)

Comment 3

3 years ago
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...
(Assignee)

Comment 4

3 years ago
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
(Assignee)

Comment 5

3 years ago
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?
Flags: needinfo?(adam)
(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

3 years ago
(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".
Flags: needinfo?(adam)

Comment 8

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

Comment 9

3 years ago
FWIW, the Chrome means of doing this is a "--use-fake-device-for-media-stream" commandline flag.
(Assignee)

Comment 10

3 years ago
Created attachment 8568182 [details] [diff] [review]
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).
Attachment #8568182 - Flags: review?(adam)
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!
Attachment #8568182 - Flags: feedback+

Comment 12

3 years ago
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.
Attachment #8568182 - Flags: review?(adam) → feedback+

Comment 13

3 years ago
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
Attachment #8568182 - Flags: review+
(Assignee)

Updated

3 years ago
Assignee: nobody → standard8
Iteration: --- → 39.1 - 9 Mar
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
OS: Mac OS X → All
Hardware: x86 → All

Updated

3 years ago
Rank: 25
(Assignee)

Comment 14

3 years ago
Created attachment 8568772 [details] [diff] [review]
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.
Attachment #8568772 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8568772 - Flags: review?(bugs) → review+
(Assignee)

Comment 15

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/49fbb330d910
Target Milestone: --- → mozilla39
https://hg.mozilla.org/mozilla-central/rev/49fbb330d910
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 17

3 years ago
Nils, now we've resolved this, can you try removing your hacks from the test server?
Flags: needinfo?(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...
Works!
I removed the patching of the SDK from the test setup.
Flags: needinfo?(drno)
You need to log in before you can comment on or make changes to this bug.