866513 - Accessing the label property on a media stream track for gUM-based Media Streams always returns an empty string

Status: RESOLVED FIXED

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

Description

[Jason Smith [:jsmith]]

Attachment: gUM Test Case

STR:

1. Load attached test case
2. Inspect the console log

Expected:

See http://dev.w3.org/2011/webrtc/editor/getusermedia.html#widl-MediaStreamTrack-label. Example - I'd expect in the case of requesting access to my internal mic and camera that the label would indicate that the one audio track came from my internal mic and my video track came from my internal camera.

Actual:

The label property on any media stream track from gUM media stream is always returning an empty string.

Comment 1

[Jason Smith [:jsmith]]

Don't know how important of a compatibility issue this is, but I definitely can confirm Chrome is doing the expected behavior here.

Comment 2

[Jason Smith [:jsmith]]

Per WebRTC meeting, not a critical compatibility issue, so not a blocker.

Comment 3

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

Tied to "implement full API for MediaStreams"

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Proposed patch (v1)

I'd want to take this!

This is my initial approach, please let me know if I'm on the correct track :)

Comment 7

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

Comment on attachment 8708628 [details] [diff] [review]
Proposed patch (v1)

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

Great! This looks good!

Maybe add a test? E.g. either a new test or expand on http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_enumerateDevices.html to enumerate each device, then call getUserMedia with it's deviceId and check that the track.label matches the deviceInfo.label ?

::: dom/media/DOMMediaStream.h
@@ +480,5 @@
> +
> +  MediaStreamTrack* CreateOwnDOMTrack(TrackID aTrackID, MediaSegment::Type aType)
> +  {
> +    nsString noLabel;
> +    return CreateOwnDOMTrack(aTrackID, aType, noLabel);

Nit: maybe leave this stub out, and have callers of CreateOwnDOMTrack provide an empty label instead, to encourage providing one? I don't think we want to make it too easy to ignore it.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Proposed patch cleaned up (v2)

Had to add a "NotSupportedError" case to the test in order to pass it locally, let me know if this is expected, or if I'm doing anything wrong.

Removed the stub as suggested.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Also, we only fill the `label` in `MediaDeviceInfo` under some circumstances (see https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDevices.cpp#100), so if I'm not wrong, in that test after `enumerateDevices` all the labels will be empty unless we have persistent permissions (since there's no gUM stream active yet).

Note that I might be wrong, since it's my second patch ever, and my first patch to this part of the codebase.

Comment 10

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

Comment on attachment 8708681 [details] [diff] [review]
Proposed patch cleaned up (v2)

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

The test needs some fixes to not break test's promise chain, so I would like to see it again.

::: dom/media/DOMMediaStream.cpp
@@ +128,5 @@
>      NS_WARN_IF_FALSE(!mStream->mTracks.IsEmpty(),
>                       "A new track was detected on the input stream; creating a corresponding MediaStreamTrack. "
>                       "Initial tracks should be added manually to immediately and synchronously be available to JS.");
> +    nsString noLabel;
> +    mStream->CreateOwnDOMTrack(aTrackId, aType, noLabel);

It might be less laborious to do:

    mStream->CreateOwnDOMTrack(aTrackId, aType, noLabel, nsString());

throughout, now that we're hitting more places.

::: dom/media/tests/mochitest/test_enumerateDevices.html
@@ +37,5 @@
>        is(d.deviceId.length, 44, "Correct device id length");
>        ok(d.label.length !== undefined, "Device label: " + d.label);
>        is(d.groupId, "", "Don't support groupId yet");
> +      navigator.mediaDevices.getUserMedia({ deviceId: d.deviceId })
> +        .then(stream => {

These gUM calls are happening in parallel, which may be OK, except they create new detached promise-chains (which are not returned or properly terminated with a .catch - you want to do one or the other, not both - and this suppresses any coding-errors in the code directly below - The reason is: any (coding) error in an onSuccess callback triggers subsequent onFailure callbacks only, and NOT the corresponding onFailure callback in the same .then(onSuccess, onFailure) pair - but this is all a side-issue).

The big issue is these separate chains need to be brought into the main promise-chain of this test, otherwise there is nothing keeping the test from ending prematurely.

We may also want to do these sequentially to simplify the reading of logs if/when something breaks this test in the future. Something like:

We'll have to modify the devices forEach loop here for this, to build and return a promise chain that includes and launches all actions sequentially. Something like:

    let p = Promise.resolve();
    devices.forEach(d => {
      ok(etc...);
      p.then(() => navigator.mediaDevices.getUserMedia({ deviceId: d.deviceId })
      .then(stream => { etc... });
    });
    return p;

For bonus points there's also a reduce pattern instead of forEach that may simplify further.
See http://stackoverflow.com/questions/28134271/loop-with-native-promises/28145039#28145039

@@ +43,5 @@
> +            is(typeof track.label, "string", "Label is a string")
> +            is(track.label, d.label, "Label is the device label")
> +          })
> +        },
> +        e => is(e.name, "NotSupportedError", "GetUserMedia with deviceId " + d.deviceId + "  failed: " + e.message));

NotSupportedError should not happen, so we need to figure out what's causing that. Let me know if it still happens after these changes.

Comment 11

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

Sorry, that should have been: p = p.then(() ...

Comment 12

[Emilio Cobos Álvarez [:emilio]]

> It might be less laborious to do:
> 
>     mStream->CreateOwnDOMTrack(aTrackId, aType, noLabel, nsString());
> 
> throughout, now that we're hitting more places.

I guess you refer to CreateOwnDOMTrack(aTrackId, aType, nsString()).

Agreed, I used `noLabel` to be explicit about what that argument meant,
but the method signature could be checked for that.

> We'll have to modify the devices forEach loop here for this, to build and
> return a promise chain that includes and launches all actions sequentially.
> Something like:
> 
>     let p = Promise.resolve();
>     devices.forEach(d => {
>       ok(etc...);
>       p.then(() => navigator.mediaDevices.getUserMedia({ deviceId:
> d.deviceId })
>       .then(stream => { etc... });
>     });
>     return p;
> 
> For bonus points there's also a reduce pattern instead of forEach that may
> simplify further.
> See
> http://stackoverflow.com/questions/28134271/loop-with-native-promises/
> 28145039#28145039
 
Got it, I'll do it ASAP.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Attachment: Proposed patch (v3)

This should do it.

Forget about the "NotSupportedError", it was just that I'm stupid, and I wasn't passing the `audio` and `video` parameters to gUM, sorry. The test is passing now.

Comment 14

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

Comment on attachment 8708702 [details] [diff] [review]
Proposed patch (v3)

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

Fix up the constraints and this looks good to go. Thanks!

::: dom/media/tests/mochitest/test_enumerateDevices.html
@@ +40,5 @@
>        is(d.deviceId.length, 44, "Correct device id length");
>        ok(d.label.length !== undefined, "Device label: " + d.label);
>        is(d.groupId, "", "Don't support groupId yet");
> +      return p
> +        .then(() => navigator.mediaDevices.getUserMedia({ audio: d.kind === "audioinput", video: d.kind === "videoinput", deviceId: d.deviceId }))

The constraints go inside audio and video. E.g. { video: { deviceId: d.deviceId } }, which raises a concern perhaps that these tests pass anyway because there's only one fake device of each kind as of now, but little we can do about that. We should be correct though, so maybe something like:

    { audio: (d.kind == "audioinput") && { deviceId: d.deviceId }, 

if you can stand that sort of thing.

Nits: wrap to 80ish characters, and in JS, == is preferred to ===. [1]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Attachment: Proposed patch (v4)

Cool! Also parenthesized the typeof(track.label).

I'll have into account the coding style guide for the next time :P

I finally went with:

var c = {};
c[d.kind == "videoinput" ? "video" : "audio"] = { deviceId: d.deviceId };

Wich is easier to read IMHO.

Hope it's all fine now :-)

Comment 16

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

Comment on attachment 8708715 [details] [diff] [review]
Proposed patch (v4)

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

lgtm.

Comment 17

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

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=123557519dc0

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Try seems to be green (modulo intermittents and all that stuff), is there something else I should do?

Comment 19

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

Looks good, marking it checking-needed.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76ba09e7504c

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76ba09e7504c