873888 - Media Stream returned from onaddstream should only return when all media tracks are available

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

Attachment: Test Case

Similar bug to the getUserMedia case that we recently fixed. I thought this was working recently, but apparently it's no longer working.

STR:

1. Load test case
2. Analyze console log

Expected

Analysis of the streams returned from onaddstream should have 1 video track and 1 audio track.

Actual

Analysis of the streams returned from onaddstream has 0 video/audio tracks. Analyzing it later after a few seconds reveals that the stream ends up having 1 video track and 1 audio track.

Comment 1

[Alex Keybl [:akeybl]]

Does this have user impact or only developer?

Comment 2

[Jason Smith [:jsmith]]

(In reply to Alex Keybl [:akeybl] from comment #1)
> Does this have user impact or only developer?

It's a developer impacting bug for compatibility with Chrome when building interoperable WebRTC applications.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Will track for now, we want to have interoperability, but any speculative fix needs to be ready by Beta 4 for FF22 to ship with WebRTC implementation otherwise we'll track for followup fix in 23.

Comment 4

[Ethan Hugg [:ehugg]]

Attachment: Wait for construction of tracks before returning onAddStream

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 752954 [details] [diff] [review]
Wait for construction of tracks before returning onAddStream

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

Uploading this as work-in-progress - still needs some more testing and cleanup.  DOMMediaStream has an OnTracksAvailableCallback that will notify when all of the tracks are ready.  However, it requires us to know what tracks we are expecting.  This is handled in GUM with TrackTypeHints so I brought some of that here to remote streams.  mTrackTypeHints may appear redundant to mTypes in RemoteSourceStreamInfo, but mTypes is not populated at the time we need to do the onAddStream callback.  We are populating the hints straight from the SDP code that initiates the streams and tracks.

Comment 6

[Ethan Hugg [:ehugg]]

Attachment: Wait for construction of tracks before returning onAddStream

Comment 7

[Ethan Hugg [:ehugg]]

Comment on attachment 752969 [details] [diff] [review]
Wait for construction of tracks before returning onAddStream


Try - https://tbpl.mozilla.org/?tree=Try&rev=d80c5345eec1

Comment 8

[Ethan Hugg [:ehugg]]

From the TRY results it appears that I bricked a refcount.  If you like this approach I can probably fix that up tomorrow.

Comment 9

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

Comment on attachment 752969 [details] [diff] [review]
Wait for construction of tracks before returning onAddStream

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

r+ with nits addressed

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +161,5 @@
> +      // returned.
> +      aStream->SetLogicalStreamStartTime(aStream->GetStream()->GetCurrentTime());
> +
> +      // This is safe since we're on main-thread, and the windowlist can only
> +      // be invalidated from the main-thread (see OnNavigation)

This comment is cloned from getUserMedia, and thus doesn't apply.  If this is safe (and it likely is), please include a comment indicating why it's safe for this case.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +6744,5 @@
> +  if (media->type == SDP_MEDIA_VIDEO) {
> +    vcm_ret = vcmAddRemoteStreamHint(dcb_p->peerconnection, idx, TRUE);
> +  } else if (media->type == SDP_MEDIA_AUDIO) {
> +    vcm_ret = vcmAddRemoteStreamHint(dcb_p->peerconnection, idx, FALSE);
> +  }

What if (for some reason in the future) it's neither?  It should either return quietly, or complain, but it should explicitly do one.

::: media/webrtc/signaling/test/FakeMediaStreams.h
@@ +212,3 @@
>    enum {
>      HINT_CONTENTS_AUDIO = 0x00000001U,
>      HINT_CONTENTS_VIDEO = 0x00000002U

If TrackTypeHints is a uint8_t, HINT_CONTENTS defines should be 0x01 and 0x02 to minimize confusion, even if an enum is int-sized.

Comment 10

[Ethan Hugg [:ehugg]]

>This comment is cloned from getUserMedia, and thus doesn't apply.  If this is safe >(and it likely is), please include a comment indicating why it's safe for this >case.

I do think we'll be safe on the main thread here and I'll change this comment.  However, the mochitest-3 failures may point to a problem with this.  If I change the threading on this I'll put it up for r? again.

>What if (for some reason in the future) it's neither?  It should either return >quietly, or complain, but it should explicitly do one.

There are other SDP_MEDIA_ types defined, but we're not going to add hints for them.  If the hints ends up zero then the callback returns immediately on stream creation.  I'll add an else clause to this with a comment and a vcm_ret=0;.

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: Wait for construction of tracks before returning onAddStream

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 753537 [details] [diff] [review]
Wait for construction of tracks before returning onAddStream


Rebased on latest M-C and addressed review comments

Not ready for checkin - still investigating mochitest errors.

bringing forward r+ from Randell.

Comment 13

[Ethan Hugg [:ehugg]]

Attachment: Wait for construction of tracks before returning onAddStream

Comment 14

[Ethan Hugg [:ehugg]]

Comment on attachment 753952 [details] [diff] [review]
Wait for construction of tracks before returning onAddStream


I determined that I don't need to hold on to the stream in TracksAvailableCallback because it's already being held in RemoteStreamInfo.  Doing so also causes problems with order of destruction.

Bringing forward r+ from Randell.  Full Try is here:

https://tbpl.mozilla.org/?tree=Try&rev=e2f48d72da9d

Comment 15

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/243533ca7769

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/243533ca7769

Comment 17

[Jason Smith [:jsmith]]

lgtm on trunk on 5/27 build

Comment 18

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

Comment on attachment 753952 [details] [diff] [review]
Wait for construction of tracks before returning onAddStream

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 834835 (866514 is the getUserMedia-side equivalent to this bug, landed on beta)

User impact if declined: Incoming channels won't have all their tracks when onaddstream fires, and so the application won't know whether the incoming data is audio/video or just audio (or just video).  This will encourage UA-sniffing and/or semi-random/fallible heuristics to guess when to check. 

Testing completed (on m-c, etc.): on m-c, verified by QA

Risk to taking this patch (and alternatives if risky): Low risk; mostly boiler-plate equivalents to roc's patch for 866514).

String or IDL/UUID changes made by this patch: none

Comment 19

[Ethan Hugg [:ehugg]]

Attachment: Wait for construction of tracks before returning onAddStream

Comment 20

[Ethan Hugg [:ehugg]]

Comment on attachment 755610 [details] [diff] [review]
Wait for construction of tracks before returning onAddStream


This version of the patch is modified to apply to Mozilla-Aurora.  In particular the OnAddStream() call has a different signature.  I also incorporated the warning fix from bug 876766

bringing forward r+ from jesup.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

*sigh*, so I'd already pushed and mid-aired with that.

https://hg.mozilla.org/releases/mozilla-aurora/rev/217559e93747
https://hg.mozilla.org/releases/mozilla-beta/rev/2b16cefa20c9

Backed out and re-landed on Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/14ae853dd478

Backed out from beta for bustage:
https://hg.mozilla.org/releases/mozilla-beta/rev/3e01c800e116

Comment 22

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

https://hg.mozilla.org/releases/mozilla-beta/rev/6df9cf4c25c2

Comment 23

[Simona Badau, Desktop QA]

(In reply to Jason Smith [:jsmith] from comment #0)
> STR:
> 
> 1. Load test case
> 2. Analyze console log
> 
> Expected
> 
> Analysis of the streams returned from onaddstream should have 1 video track
> and 1 audio track.
> 
> Actual
> 
> Analysis of the streams returned from onaddstream has 0 video/audio tracks.
> Analyzing it later after a few seconds reveals that the stream ends up
> having 1 video track and 1 audio track.

By console, do you mean the web console (Ctrl+Shift+K)? I couldn't reproduce the initial issue, is there something I need to set prior step 1?

Comment 24

[Jason Smith [:jsmith]]

Console = CTRL+SHIFT+K yes. Don't know why you can't reproduce the original issue.

Comment 25

[Iñaki Baz Castillo]

I wonder how much this fixed issue is related to #1019579.