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)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: philipp, Assigned: pehrsons)
Details
Attachments
(9 files, 1 obsolete file)
3.00 KB,
text/html
|
Details | |
126.35 KB,
text/plain
|
Details | |
19.16 KB,
text/plain
|
Details | |
160.79 KB,
text/plain
|
Details | |
119.51 KB,
text/plain
|
Details | |
58 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jesup
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
drno
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
drno
:
review+
|
Details |
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.
Reporter | ||
Comment 1•8 years ago
|
||
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.
Comment 2•8 years ago
|
||
(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+ :(
Reporter | ||
Comment 3•8 years ago
|
||
attached a wrong version of the repro. unified_videoaudio.html shows what I actually intended to show, adding audio to a video stream
Updated•8 years ago
|
Attachment #8716525 -
Attachment is obsolete: true
Assignee | ||
Comment 4•8 years ago
|
||
NSPR_LOG_MODULES=MediaStream:5,MediaStreamGraph:4 should help here.
Reporter | ||
Comment 5•8 years ago
|
||
Reporter | ||
Comment 6•8 years ago
|
||
I get nightly to stop displaying video with jib's fiddle.
Reporter | ||
Comment 7•8 years ago
|
||
Reporter | ||
Comment 8•8 years ago
|
||
Comment 9•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
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)
Reporter | ||
Comment 12•8 years ago
|
||
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+.
Reporter | ||
Comment 13•8 years ago
|
||
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.
Updated•8 years ago
|
Rank: 10 → 19
Reporter | ||
Comment 14•8 years ago
|
||
https://github.com/opentok/upgrade-test/ -- the other way round (audio->audio+video) now comes with tests for your convenience :-)
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
Confirmed with an assert in NotifyPull.
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Component: WebRTC → Audio/Video: MediaStreamGraph
Assignee | ||
Comment 17•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
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 19•8 years ago
|
||
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 20•8 years ago
|
||
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+
Assignee | ||
Comment 21•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47457/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47457/
Attachment #8742808 -
Flags: review?(drno)
Attachment #8742809 -
Flags: review?(drno)
Assignee | ||
Comment 22•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47459/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47459/
Assignee | ||
Comment 23•8 years ago
|
||
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/
Assignee | ||
Comment 24•8 years ago
|
||
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/
Assignee | ||
Comment 25•8 years ago
|
||
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 26•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8742809 -
Flags: review?(drno) → review+
Comment 27•8 years ago
|
||
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?
Assignee | ||
Comment 28•8 years ago
|
||
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.
Assignee | ||
Comment 29•8 years ago
|
||
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.
Comment 30•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/21fbd1600568 https://hg.mozilla.org/integration/mozilla-inbound/rev/cc01ccccdedc https://hg.mozilla.org/integration/mozilla-inbound/rev/34d7c1009ea8 https://hg.mozilla.org/integration/mozilla-inbound/rev/da9b8b7a0263
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/21fbd1600568 https://hg.mozilla.org/mozilla-central/rev/cc01ccccdedc https://hg.mozilla.org/mozilla-central/rev/34d7c1009ea8 https://hg.mozilla.org/mozilla-central/rev/da9b8b7a0263
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•