Closed Bug 1246310 Opened 8 years ago Closed 8 years ago

adding an audio track to a video-only stream during renegotiation blocks video

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

46 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: philipp, Assigned: pehrsons)

Details

Attachments

(9 files, 1 obsolete file)

Attached file unified.html (obsolete) —
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/48.0.2564.82 Chrome/48.0.2564.82 Safari/537.36

Steps to reproduce:

run the attached sample for renegotiation. Currently requires FF46+ until the distributed adapter.js shims ontrack.

Note that I am munging the SDP to present a single stream to the receiver. This is probably different from what the existing tests do.


Actual results:

video freezes after the audio track is added. RTP still seems to be flowing from looking at about:webrtc


Expected results:

audio starts playing.

Note that adding audio to a video track works. At least in FF47. FF44 with a custom adapter.js that shims ontrack never starts playing the video but continues playing audio.
note that this is caused by the attempt to simulate a single stream for the receiver. Not replacing the stream id before calling setRemoteDescription works as expected.
(At first, I thought this was because you're using getTracks() instead of getAudioTracks(), but that was a red herring.)

Seems to reproduce without munging with this add-audio fiddle: https://jsfiddle.net/r0ybaphz/

I get this shell output in Nightly debug:

> ...
> [Child 9129] WARNING: NS_ENSURE_TRUE(startupCache) failed: file /Users/Jan/moz/mozilla-central/dom/xbl/nsXBLDocumentInfo.cpp, line 267
> [Parent 9127] WARNING: Image width or height is non-positive: file /Users/Jan/moz/mozilla-central/layout/base/nsLayoutUtils.cpp, line 6500
> [Child 9129] WARNING: 'NS_FAILED(branch->GetBoolPref("media.video.test_latency", &mVideoLatencyTestEnable))', file /Users/Jan/moz/mozilla-central/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp, line 277
> [Child 9129] WARNING: 'NS_FAILED(branch->GetBoolPref("media.video.test_latency", &mVideoLatencyTestEnable))', file /Users/Jan/moz/mozilla-central/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp, line 277
> [Child 9129] WARNING: 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.: '!mStream->mTracks.IsEmpty()', file /Users/Jan/moz/mozilla-central/dom/media/DOMMediaStream.cpp, line 130
> WARNING: no real random source present!
> [Child 9129] WARNING: Adding track to StreamBuffer that should have no more tracks: file /Users/Jan/moz/mozilla-central/dom/media/StreamBuffer.h, line 223
> [Child 9129] ###!!! ASSERTION: Buffer underran: 'bufferEnd >= mProcessedTime', file /Users/Jan/moz/mozilla-central/dom/media/MediaStreamGraph.cpp, line 315

The last two lines happen after I hit "Add Audio", and the video freezes then.

Oddly, the other way works: add-video fiddle https://jsfiddle.net/gwbuL3xj/ but only in 45+ :(
attached a wrong version of the repro. unified_videoaudio.html shows what I actually intended to show, adding audio to a video stream
Attachment #8716525 - Attachment is obsolete: true
NSPR_LOG_MODULES=MediaStream:5,MediaStreamGraph:4 should help here.
Attached file nspr log from FF44
I get nightly to stop displaying video with jib's fiddle.
Attached file from my sample, FF44
Attached file from my sample, FF47
My question is: how important is this? I take it this affects 44 to 47 (maybe not all with the same symptoms)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jib)
Flags: needinfo?(fippo)
After chatting with mreavy, prioritizing analysis of our options.  jib - What do you know so far?  I can take that and deep-dive into the MediaStreamGraph code as needed.
Rank: 10
Flags: needinfo?(fippo)
Priority: -- → P1
This is not a regression afaict, as mozregression shows it dating back to 38 - the introduction of renegotiation - at least, and that's only because it crashes 37. (if you want to try, you need to call pc.onnegotiationneeded() manually in those early versions: https://jsfiddle.net/oxmc9drb/ )

Back to nightly, this is 100% reproducible for me. From comment 2, this is in StreamBuffer::AddTrack:

> [Child 9129] WARNING: Adding track to StreamBuffer that should have no more tracks: file /Users/Jan/moz/mozilla-central/dom/media/StreamBuffer.h, line 223

because mTracksKnownTime == STREAM_TIME_MAX.

mTracksKnownTime is set to STREAM_TIME_MAX in RemoteSourceStreamInfo::StartReceiving() [1] so this looks like it's just not implemented.

[1] http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp?rev=a71b79b9c3ef&mark=1352-1352#1335
Flags: needinfo?(jib)
possibly related: audio->audio/video (the much more relevant use-case) does not work (does not show the video) in 44 but works in 45+.
correction of #12: I have seen this working in interop scenarios, also when running two instances of firefox 44.0.2 so that failure seems to be timing related.
Rank: 10 → 19
https://github.com/opentok/upgrade-test/ -- the other way round (audio->audio+video) now comes with tests for your convenience :-)
Looks like we add an audio track to the existing stream on the receiving side - but something's up with the time stamps.

Video (pull based) is flowing but the stream does not progress. This, I believe, is because the audio track (push based in PullNotify.. i.e., on pull it takes what's available and pushes it, seems sensitive to timing mismatch) is constantly a bit behind.
Confirmed with an assert in NotifyPull.
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Component: WebRTC → Audio/Video: MediaStreamGraph
AddAudioTrack() has this comment:
>    * Like AddTrack, but resamples audio from aRate to the graph rate.

Even so it would only resample the AudioSegments added through AppendToTrack.
Not the initial one, provided to AddAudioTrack itself, even though
MediaPipelineReceiveAudio depends on this functionality.

Review commit: https://reviewboard.mozilla.org/r/46757/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46757/
Attachment #8741795 - Flags: review?(padenot)
Attachment #8741796 - Flags: review?(rjesup)
We create the tracks with a segment lasting from 0 to current time,
so there's no need to offset them with the same amount again.

Review commit: https://reviewboard.mozilla.org/r/46759/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46759/
Comment on attachment 8741796 [details]
MozReview Request: Bug 1246310 - Let MediaPipelineReceive tracks start at 0. r?jesup

https://reviewboard.mozilla.org/r/46759/#review43345
Attachment #8741796 - Flags: review?(rjesup) → review+
Comment on attachment 8741795 [details]
MozReview Request: Bug 1246310 - Resample the AudioSegment in SourceMediaStream::AddAudioTrack. r?padenot

https://reviewboard.mozilla.org/r/46757/#review43593
Attachment #8741795 - Flags: review?(padenot) → review+
Comment on attachment 8741795 [details]
MozReview Request: Bug 1246310 - Resample the AudioSegment in SourceMediaStream::AddAudioTrack. r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46757/diff/1-2/
Comment on attachment 8741796 [details]
MozReview Request: Bug 1246310 - Let MediaPipelineReceive tracks start at 0. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46759/diff/1-2/
Fix-only try push caused an intermittent leak to be more frequent I think: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b30abc44a8c

New try push with test (which I confirmed fails without the fix locally): https://treeherder.mozilla.org/#/jobs?repo=try&revision=466705dbecf6
Comment on attachment 8742808 [details]
MozReview Request: Bug 1246310 - Add a method for attaching local tracks rather than full streams. r?drno

https://reviewboard.mozilla.org/r/47457/#review44341

Looks good. Just fix the inconsistent return value.

::: dom/media/tests/mochitest/pc.js:837
(Diff revision 1)
> +   *        MediaStreamTrack to handle
> +   * @param {MediaStream} stream
> +   *        MediaStream to use as container for `track` on remote side
> +   */
> +  attachLocalTrack : function(track, stream) {
> +    info("Get a local " + track.kind + " track");

s/Get/Got/

::: dom/media/tests/mochitest/pc.js:850
(Diff revision 1)
> +    var mappedStream =
> +      this.streamsForLocalTracks.find(o => o.fromStreamId == stream.id);

As you are paranoid about the track.id a few lines above, how about adding the same safety check for stream.id before this as well?

::: dom/media/tests/mochitest/pc.js:855
(Diff revision 1)
> +    var mappedStream =
> +      this.streamsForLocalTracks.find(o => o.fromStreamId == stream.id);
> +
> +    if (mappedStream) {
> +      mappedStream.stream.addTrack(track);
> +      return;

Why are you returning nothing here, but the |observedNegotiationNeeded| at the end?
Could also help to clarify the return type in the comment at the top.
Attachment #8742808 - Flags: review?(drno) → review+
Attachment #8742809 - Flags: review?(drno) → review+
Comment on attachment 8742809 [details]
MozReview Request: Bug 1246310 - Mochitest for adding audio track to video-only PeerConnection. r?drno

https://reviewboard.mozilla.org/r/47459/#review44345

Just minor suggestions.

::: dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html:16
(Diff revision 1)
> +    title: "Renegotiation: add audio track to existing video-only stream",
> +  });
> +
> +  runNetworkTest(function (options) {
> +    var test = new PeerConnectionTest(options);
> +    var stream;

Relying on the fact that nothing in between creates any object called |stream| is ... 
How about something like |test.original_gum_stream| instead?

::: dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html:19
(Diff revision 1)
> +  runNetworkTest(function (options) {
> +    var test = new PeerConnectionTest(options);
> +    var stream;
> +    test.chain.replace("PC_LOCAL_GUM",
> +      [
> +        function PC_LOCAL_GUM_AV(test) {

GUM_AV does not sounds very descriptive to me. Maybe something like PC_LOCAL_GUM_ATTACH_VIDEO_ONLY or something like that?
https://reviewboard.mozilla.org/r/47457/#review44341

> Why are you returning nothing here, but the |observedNegotiationNeeded| at the end?
> Could also help to clarify the return type in the comment at the top.

Fixed by returning `this.observedNegotiationNeeded` here.

I wish I knew why I'm returning that value. I'm mimicing `attachMedia()` and it also doesn't have a comment for this.
https://reviewboard.mozilla.org/r/47459/#review44345

> Relying on the fact that nothing in between creates any object called |stream| is ... 
> How about something like |test.original_gum_stream| instead?

Done.

> GUM_AV does not sounds very descriptive to me. Maybe something like PC_LOCAL_GUM_ATTACH_VIDEO_ONLY or something like that?

Done. Also changed PC_LOCAL_ADD_SECOND_TRACK to PC_LOCAL_ATTACH_SECOND_TRACK_AUDIO to be a bit consistent.
You need to log in before you can comment on or make changes to this bug.