1003695 - Allow DOMMediaStream to handle multiple audio tracks and video tracks

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Shelly Lin [:shelly]]

Currently, we are hard coding TrackID = 1 as audio track and TrackID = 2 as video track everywhere when MediaStream deals with its tracks. 

We want to extend the ability of handling multi-tracks. For example, when adding a new track to the SourceMediaStream, we could do:

SourceMediaStream::AddTrack(){
  mID = GenUniqueTrackId();
  ...
}

Instead of passing in 1 or 2 to set the mID.

This bug will not modify the current behavior of playback, which still plays the first audio track and the first video track.

Comment 1

[Shelly Lin [:shelly]]

After further analysis, this is not a blocker of project Stingray, remove the keyword of stingray so that it might sound more appealing.

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Drop this bug since bug 938034 no longer depends on it.

Comment 3

[Shelly Lin [:shelly]]

Okay, I'm too lazy to create a new bug, the idea is very similar but not as scary as what it is in comment 0. We only enable the support of multiple media tracks in DOMMediaStream, our playback pipeline remains unchanged.

Comment 4

[Shelly Lin [:shelly]]

Attachment: Support multi-tracks in DOMMediaStream

Hi roc, this is from the requirements of TV project. Since bug 998872 (TVManager API) and bug 1002823 (OverlayImage) have landed, implementation of "DOMTVMediaStream"(bug 987498) is soon in the roadmap. Could you review the patch at your convenience? Thanks! (I'll include a test later.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8517364 [details] [diff] [review]
Support multi-tracks in DOMMediaStream

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

The rest looks fine.

::: dom/media/DOMMediaStream.h
@@ -278,5 @@
>    // MediaStream is owned by the graph, but we tell it when to die, and it won't
>    // die until we let it.
>    MediaStream* mStream;
>  
> -  nsAutoTArray<nsRefPtr<MediaStreamTrack>,2> mTracks;

Why are you changing this?

Comment 6

[Shelly Lin [:shelly]]

Attachment: Support multi-tracks in DOMMediaStream (v2)

Patch updated per our discussion over irc. Thank you :)

Comment 7

[Shelly Lin [:shelly]]

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0612ba79462e

Comment 8

[Shelly Lin [:shelly]]

Attachment: Test case

Hi roc, I'd like to add a test case for the minimum multi-tracks support, this patch might look a bit hacky, but I don't have a better solution...

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8526664 [details] [diff] [review]
Test case

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

::: dom/webidl/MediaStream.webidl
@@ +17,5 @@
>      (boolean or MediaTrackConstraints) audio = false;
>      (boolean or MediaTrackConstraints) video = false;
>      boolean picture = false; // Mozilla legacy
>      boolean fake = false;    // for testing
> +    boolean fakeTracks = false; // for testing

Please document here exactly what 'fake' and 'fakeTracks' do (in terms of behavior visible to scripts).

Comment 10

[Shelly Lin [:shelly]]

Attachment: Support multi-tracks in DOMMediaStream (w/ test case)

Hi Roc, this patch carries the previous r+ and updated with a test case.

Comment 11

[Shelly Lin [:shelly]]

Attachment: Support multi-tracks in DOMMediaStream (w/ test case)

Remove the whitespaces.

Comment 12

[Shelly Lin [:shelly]]

Comment on attachment 8533651 [details] [diff] [review]
Support multi-tracks in DOMMediaStream (w/ test case)

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

::: dom/media/DOMMediaStream.cpp
@@ +361,5 @@
>    switch (aType) {
>    case MediaSegment::AUDIO: {
>      for (size_t i = 0; i < mTracks.Length(); ++i) {
>        track = mTracks[i]->AsAudioStreamTrack();
> +      if (track && track->GetTrackID() == aTrackID) {

After debugging the test case fail of "test_peerConnection_capturedVideo.html", I found out my logic here break the essential idea of "BindDOMTrack", however, the original logic of BindDOMTrack breaks the ability of adding multiple audio/video tracks into a MediaStream. Say we have a MediaStream with one audio track, id 1, and we want to add another audio track with id 2, BindDOMTrack will return the previous existed audio track, and overwrite its id from 1 to 2. Result in the second audio track with id 2 is never created, see:
http://dxr.mozilla.org/mozilla-central/source/dom/media/DOMMediaStream.cpp#52

I'd suggest adding a flag of "needBind" for the case that needs "BindDOMTrack".

Comment 13

[Shelly Lin [:shelly]]

Hi jib, I think the logic of "BindDOMTrack" breaks the ability of adding multiple audio/video tracks into a media stream, but I'm not sure what BindDOMTrack is for, please find more details in comment 12, thanks.

Comment 14

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

Hi Shelly, BindDOMTrack tries to connect up a real track - when it arrives - with it's dummy placeholder that now often exists in DOMMediaStream in its place up-to this point. The dummy placeholders are an extension of "hints", and were added to appease content JS which may interrogate a DOMMediaStream it's created and expect to find tracks in it. Basically, the spec says the tracks should be there at stream creation time in a lot of cases, yet in reality - as I'm sure you know - it often takes a bit of time for the underlying tracks to actually be ready.

WebRTC moving toward a track-based API, was one of the reasons we had to address this.

Basically, where you see BindDOMTrack called is where we used to call CreateDOMTrack, and CreateDOMTrack is now called at stream-creation where hints are set.

Your patch looks good and it probably breaks some assumption in my patch that should be made explicit about hard-coded track number. I'll try to figure out what that is.

Comment 15

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

Comment on attachment 8533651 [details] [diff] [review]
Support multi-tracks in DOMMediaStream (w/ test case)

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

::: dom/media/DOMMediaStream.cpp
@@ +366,1 @@
>          track->BindTrackID(aTrackID);

Note that this is really just "SetTrackId" so since the id's already match you no longer need to call it here. The function itself can probably be removed as well.

Comment 16

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

I'm running with your second patch and I'm not seeing any failures from test_peerConnection_capturedVideo.html. Could you link me to some logs or give me steps to reproduce?

Comment 17

[Shelly Lin [:shelly]]

Attachment: Support multi-tracks in DOMMediaStream (v3, w/ test case)

Hi jib, thanks for your time and the clear explanation. After re-basing to the latest m-c, the test passed magically! (bizarre..but yeah!) Could you review the part of removing function |BindTrackID|? Thanks!

Comment 18

[Shelly Lin [:shelly]]

Comment on attachment 8534789 [details] [diff] [review]
Support multi-tracks in DOMMediaStream (v3, w/ test case)

Hi roc, could you look at the comments in MediaStream.webidl? Thank you very much!

Comment 19

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

Comment on attachment 8534789 [details] [diff] [review]
Support multi-tracks in DOMMediaStream (v3, w/ test case)

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

LGTM. You'll need a DOM reviewer for the MediaStream.webidl change though.

::: dom/media/DOMMediaStream.cpp
@@ +387,5 @@
>    }
>    default:
>      MOZ_CRASH("Unhandled track type");
>    }
> +  if (track && bindSuccess) {

Nit: track can never be null here so testing bindSuccess is sufficient. Another track ptr may be simpler than a boolean here.

::: dom/media/test/mochitest.ini
@@ +428,5 @@
>  [test_metadata.html]
>  [test_mixed_principals.html]
>  skip-if = true # bug 567954 and intermittent leaks
>  [test_mozHasAudio.html]
> +[test_multiple_mediastreamtrack.html]

tracks plural?

::: dom/media/test/test_multiple_mediastreamtrack.html
@@ +9,5 @@
> +<body>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +function startTest() {
> +  navigator.mozGetUserMedia({audio:true, video:true, fake:true, fakeTracks:true},

You might want to use the new one here - navigator.mediaDevices.getUserMedia() - as it has better error handling if the callback ever were to throw for some reason.

@@ +33,5 @@
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv(
> +  {
> +    "set": [
> +      ["media.track.enabled", true]

Do you need this?

::: dom/media/webrtc/MediaEngineDefault.h
@@ +159,5 @@
> +  : mHasFakeTracks(false)
> +  , mMutex("mozilla::MediaEngineDefault")
> +  {}
> +
> +  MediaEngineDefault(bool aHasFakeTracks)

A matter of style, but I would add a = false default to the aHasFakeTracks parameter here instead of having two constructors.

You probably also want to make the constructor explicit to avoid implicit conversion from a boolean.

Comment 20

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

(In reply to Shelly Lin [:shelly] from comment #17)
> After re-basing to the latest m-c, the test passed magically!

By chance do you remember what the error was? Just curious in case it is intermittent.

Comment 21

[Shelly Lin [:shelly]]

Sure, full log here:
https://tbpl.mozilla.org/php/getParsedLog.php?id=54252982&tree=Try&full=1

Summary:
535 INFO TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_capturedVideo.html | PeerConnectionWrapper (pcLocal) has 2 local audio tracks - got 2, expected 1
536 INFO TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_capturedVideo.html | PeerConnectionWrapper (pcLocal) has 2 local video tracks - got 2, expected 1

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8534789 [details] [diff] [review]
Support multi-tracks in DOMMediaStream (v3, w/ test case)

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

::: dom/webidl/MediaStream.webidl
@@ +16,5 @@
>  dictionary MediaStreamConstraints {
>      (boolean or MediaTrackConstraints) audio = false;
>      (boolean or MediaTrackConstraints) video = false;
>      boolean picture = false; // Mozilla legacy
> +    boolean fake = false; // For testing purpose, enable the fake attribute

add a trailing "."

@@ +17,5 @@
>      (boolean or MediaTrackConstraints) audio = false;
>      (boolean or MediaTrackConstraints) video = false;
>      boolean picture = false; // Mozilla legacy
> +    boolean fake = false; // For testing purpose, enable the fake attribute
> +                          // generatres frames of solid colors if video is

"Generates"

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Byron, you should be aware of this.

Comment 24

[Shelly Lin [:shelly]]

Attachment: Support multi-tracks in DOMMediaStream (nit fixed)

Patch updated with comments addressed.

Hi Byron, could you take a look at the change in MediaStream.webidl? The attribute is just for testing purpose. Thanks!

Comment 25

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

Comment on attachment 8534826 [details] [diff] [review]
Support multi-tracks in DOMMediaStream (nit fixed)

Byrin may want to look at this, but you need an r= from a DOM peer on this list or your patch will bounce: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1001106&attachment=8412616 

(it's my cheatsheet and is probably outdated, but I don't know where it's kept these days)

Comment 26

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

s/Byrin/Byron/ sorry ;-)

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8534826 [details] [diff] [review]
Support multi-tracks in DOMMediaStream (nit fixed)

It might be nice to separate those Mozilla specific properties to a different
dictionary and make that inherit MediaStreamConstraints.

I assume using fake or fakeTracks don't do anything by default on release builds.

Comment 28

[Shelly Lin [:shelly]]

(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8534826 [details] [diff] [review]
> Support multi-tracks in DOMMediaStream (nit fixed)
> 
> It might be nice to separate those Mozilla specific properties to a different
> dictionary and make that inherit MediaStreamConstraints.
> 
> I assume using fake or fakeTracks don't do anything by default on release
> builds.

jib, thanks for the referral, and smaug, thanks for your time.

I think putting those Mozilla specific properties into another dictionary inherits from MediaStreamConstraints sounds like a good idea, however, MediaStreamConstraints is used in many places and MediaManager uses mFake and mFakeTracks to differentiate the creation of device backend, it's probably better to leave it as a separate bug for improvement.

Comment 29

[Shelly Lin [:shelly]]

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a39e44ef7c63

Comment 30

[Shelly Lin [:shelly]]

Attachment: Support multi-tracks in DOMMediaStream

Carry r+ from roc, jib, and smaug.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4992ad7c89

Comment 32

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

(In reply to Shelly Lin [:shelly] from comment #28)
> I think putting those Mozilla specific properties into another dictionary
> inherits from MediaStreamConstraints sounds like a good idea, however,
> MediaStreamConstraints is used in many places and MediaManager uses mFake
> and mFakeTracks to differentiate the creation of device backend, it's
> probably better to leave it as a separate bug for improvement.

Yeah I don't think dictionary inheritance really works, because it's static. If we did this, then - as Shelly points out - we'd have to change all APIs that need to use this testing feature from MediaStreamConstraints to DerivedMediaStreamConstraintsWithFakeStuff, which looks ugly, and is bug-prone unless all handlers use the new dictionary. It also confuses many people - including spec people who think peerConnection.getStats can return a base dictionary with a plethora of info - Not trying to start a debate here, but I don't think we should open another bug.

Comment 33

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

https://hg.mozilla.org/mozilla-central/rev/5c4992ad7c89