Closed Bug 873888 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 + fixed
firefox23 + fixed
firefox24 + verified

People

(Reporter: jsmith, Assigned: ehugg)

References

Details

(Keywords: compat, Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked])

Attachments

(3 files, 3 obsolete files)

Attached file 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.
Keywords: compat
Whiteboard: [WebRTC][blocking-webrtc+]
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-automation-blocked]
Blocks: 834837
Does this have user impact or only developer?
(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.
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.
Assignee: nobody → rjesup
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.
Attachment #752954 - Attachment is obsolete: true
Comment on attachment 752969 [details] [diff] [review]
Wait for construction of tracks before returning onAddStream


Try - https://tbpl.mozilla.org/?tree=Try&rev=d80c5345eec1
Attachment #752969 - Flags: review?(rjesup)
From the TRY results it appears that I bricked a refcount.  If you like this approach I can probably fix that up tomorrow.
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.
Attachment #752969 - Flags: review?(rjesup) → review+

>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;.
Attachment #752969 - Attachment is obsolete: true
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.
Attachment #753537 - Flags: review+
Attachment #753537 - Attachment is obsolete: true
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
Attachment #753952 - Flags: review+
Assignee: rjesup → ethanhugg
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked] → [WebRTC][blocking-webrtc+][qa-automation-blocked][webrtc-uplift]
https://hg.mozilla.org/mozilla-central/rev/243533ca7769
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Keywords: verifyme
lgtm on trunk on 5/27 build
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 876766
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
Attachment #753952 - Flags: approval-mozilla-beta?
Attachment #753952 - Flags: approval-mozilla-aurora?
Attachment #753952 - Flags: approval-mozilla-beta?
Attachment #753952 - Flags: approval-mozilla-beta+
Attachment #753952 - Flags: approval-mozilla-aurora?
Attachment #753952 - Flags: approval-mozilla-aurora+
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.
Attachment #755610 - Flags: review+
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked][webrtc-uplift] → [WebRTC][blocking-webrtc+][qa-automation-blocked]
(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?
Console = CTRL+SHIFT+K yes. Don't know why you can't reproduce the original issue.
I wonder how much this fixed issue is related to #1019579.
You need to log in before you can comment on or make changes to this bug.