998546 - ontrack and onaddstream fire too late

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Nils Ohlmeier [:drno]]

The current version of the webrtc spec says:

onaddstream of type EventHandler,
    This event handler, of event handler event type addstream, MUST be fired by all objects implementing the RTCPeerConnection interface. It is called any time a MediaStream is added by the remote peer. This will be fired only as a result of setRemoteDescription. Onnaddstream happens as early as possible after the setRemoteDescription. This callback does not wait for a given media stream to be accepted or rejected via SDP negotiation.

But if you run one of our simple test cases you will see that onaddstream for the answer side (pcRemote in the test output) fires really late:

./mach mochitest-plain dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html  | egrep -i "(onaddstream|setRemoteDescription|SET_REMOTE_DESCRIPTION)"
 0:17.53 44 INFO TEST-INFO | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html | Run step: PC_REMOTE_SET_REMOTE_DESCRIPTION
 0:17.54 737435648[13082aa90]: [GSM Task|tnp] ui.c:1712: SIPCC-UI_API: 1/2, ui_set_remote_description: state=27 call_instance=0
 0:17.55 48 INFO TEST-PASS | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html | signalingState after remote setRemoteDescription is 'have-remote-offer'
 0:17.57 53 INFO TEST-INFO | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html | Run step: PC_LOCAL_SET_REMOTE_DESCRIPTION
 0:17.67 1990591248[1003342d0]: [main|PeerConnectionImpl] PeerConnectionImpl.cpp:255: Returning success for OnAddStream()
 0:17.67 737435648[13082aa90]: [GSM Task|tnp] ui.c:1712: SIPCC-UI_API: 1/1, ui_set_remote_description: state=27 call_instance=0
 0:17.67 56 INFO TEST-INFO | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html | PeerConnectionWrapper (pcLocal): 'onaddstream' event fired for {}
 0:17.68 61 INFO TEST-PASS | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html | signalingState after local setRemoteDescription is 'stable'
 0:17.73 1990591248[1003342d0]: [main|PeerConnectionImpl] PeerConnectionImpl.cpp:255: Returning success for OnAddStream()
 0:17.73 70 INFO TEST-INFO | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html | PeerConnectionWrapper (pcRemote): 'onaddstream' event fired for {}
 0:19.85     1.315054 |    1.066513 | Set Remote Description         | PeerConnectionImpl.cpp:1274  | SetRemoteDescription
 0:19.86     1.535206 |    0.054609 | Set Remote Description         | PeerConnectionImpl.cpp:1274  | SetRemoteDescription

If it helps to prove the point here I'll go ahead and implement the test case requested in Bug 966528.

Comment 1

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

Because our MediaStream implementation currently can't guarantee a stream has all its tracks until a while after it's created, we stall onaddstream until the tracks exist.

This bug will be about reworking MediaStreamGraph to have the tracks available earlier, perhaps not requiring that commands run through MSG before adding (incomplete) tracks to the stream.

This is a significant change, unfortunately.

Comment 2

[Nils Ohlmeier [:drno]]

To make matter even worse: in case there are not tracks we create a remote stream. But because there are not tracks in the stream the onaddstream callback never fires!
In a consistent world I would expect that the onaddstream callback fires immediately if there are no remote track we have to wait on.

This means we currently have no reliable way for implementers to find out what tracks they have/will have for a given PC!

Comment 3

[Martin Thomson [:mt:]]

I understand that the current status is a pre-bungle placeholder.  Once we have bundle, there will be support for nStreams != 1.

We should at least be consistent: either make the empty stream available in both places (onaddstream and getRemoteStreams) or don't show it in either place.  I prefer the latter: no media => no MediaStream.

Comment 5

[Nils Ohlmeier [:drno]]

As 33 landed already I assume this needs to be re-scheduled?

Comment 6

[Maire Reavy [:mreavy]]

Fx 33 is the currently Nightly.  It uplifts on July 21.

Comment 7

[Iñaki Baz Castillo]

Hi, is there any progress in this issue? I've realized that Firefox Nightly 37.0a1 (2014-12-22) implements BUNDLE (I've not tested it) Since this issue seems to depend on the architecture changes needed for adding BUNDLE, I wonder whether this issue is already addressed.

To summarize:
- onaddstream fires too late (this issue).
- PeerConnection MUST include all the remote tracks after SetRemoteDescriptionSuccessCallback (https://bugzilla.mozilla.org/show_bug.cgi?id=1019579)

Thanks for any comment.

Comment 8

[Maire Reavy [:mreavy]]

Byron - if this isn't fixed by other landings, can you update this with the current status so we can prioritize?  Thanks.

Comment 9

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

This is messing up negotiation, since people can't reject tracks by stopping them. Suggest up-prioritizing.

Comment 10

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

It also messes up tests that assume ontrack fires before "connected" state, such as https://github.com/webrtc/adapter/pull/262

Comment 11

[Iñaki Baz Castillo]

This bug and #1019579 (two clear bugs) were reported two years ago. Please developers, at least provide some feedback about the status of them.

Comment 12

[Maire Reavy [:mreavy]]

Given that the spec has changed, the impact on use cases people care about, and the progress on other features like renegotiation, we're making this a P1, and bwc is taking over the work.  If things go well, I expect this to land this quarter.

Comment 13

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 998546: Fire ontrack and onaddstream on time. r=drno

Review commit: https://reviewboard.mozilla.org/r/50937/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50937/

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8749417 [details]
MozReview Request: Bug 998546: Fire ontrack and onaddstream on time. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50937/diff/1-2/

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8749417 [details]
MozReview Request: Bug 998546: Fire ontrack and onaddstream on time. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50937/diff/2-3/

Comment 16

[Nils Ohlmeier [:drno]]

Comment on attachment 8749417 [details]
MozReview Request: Bug 998546: Fire ontrack and onaddstream on time. r=drno

https://reviewboard.mozilla.org/r/50937/#review47815

Sorry I got interrupted when reviewing r2. But the comment still applies, so I wanted to get this out, before doing a review of r3.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1861
(Diff revision 2)
> -                                  mHandle,
> -                                  aPco);
> -    info->GetMediaStream()->OnTracksAvailable(tracksAvailableCallback);
> -#else
> -    if (!numPreexistingTrackIds) {
>        aPco->OnAddStream(*info->GetMediaStream(), jrv);

As this is now the one and only OnAddStream call, you should probably copy the return code check from lines 257+ from the original function to here.

Strange, that this one was without a return code check so far.

Comment 17

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/50937/#review47815

Ok, I've fixed, but will wait to re-push.

Comment 18

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/50937/#review48161

I'm a little bit concerned about the amount of times the test now access |localMediaElements[0]| or |remoteMediaElements[0]| apparently with assumption of what to find in there. I would prefer it if the access to that array would get protected with some search function which you ask for the first remote video element or something like that. But I won't block the review on having this.

::: dom/media/tests/mochitest/head.js:251
(Diff revision 3)
> -  element = document.createElement(type === 'audio' ? 'audio' : 'video');
> +  // Even if this is just audio now, we might add video later.
> +  element = document.createElement('video');

I understand the use case, but it looks like through this change we no longer test with pure 'audio' element at all, or?
Could we add (if we don't have one) a simple test with just an audio element and a PC?

::: dom/media/tests/mochitest/pc.js:838
(Diff revision 3)
> +    var element = getMediaElement(this.label, direction, stream.id);
> +
> +    if (!element) {
> +      element = createMediaElement(this.label, direction, stream.id);
> +      // Store local media elements so that we can stop them when done.
> +      // Don't store remote ones because they should stop when the PC does.

This comment appears to be outdated now.

::: dom/media/tests/mochitest/pc.js:932
(Diff revision 3)
> -          };
> +        };
> +      this.ensureMediaElement(track, stream, "local");
> -      });
> +    });
> -    }
>  
> -    var element = createMediaElement(type, this.label + '_' + side + this.streams.length);
> +    return getMediaElement(this.label, "local", stream.id);

Does anyone ever use the return value from this function?
And before it was either not returning anything, or just a boolean... which is/was broken.

::: dom/media/tests/mochitest/pc.js:1477
(Diff revision 3)
> +      this._pc.getSenders().map(sender => this.waitForRtpFlow(sender.track)),
>        this._pc.getSenders().map(sender => this.waitForRtpFlow(sender.track)),

Why is this being checked now twice? :-)

::: dom/media/tests/mochitest/test_peerConnection_capturedVideo.html:64
(Diff revision 3)
>            var type = "";
>            if (hasAudio) { type += "audio"; }
>            if (hasVideo) { type += "video"; }

|type| appears to be unused now and should be removed.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1861
(Diff revision 3)
> -                                  mHandle,
> -                                  aPco);
> -    info->GetMediaStream()->OnTracksAvailable(tracksAvailableCallback);
> -#else
> -    if (!numPreexistingTrackIds) {
>        aPco->OnAddStream(*info->GetMediaStream(), jrv);

Please add error handling here (as per comment for revision 2).

Comment 19

[Byron Campen [:bwc]]

Comment on attachment 8749417 [details]
MozReview Request: Bug 998546: Fire ontrack and onaddstream on time. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50937/diff/3-4/

Comment 20

[Nils Ohlmeier [:drno]]

Comment on attachment 8749417 [details]
MozReview Request: Bug 998546: Fire ontrack and onaddstream on time. r=drno

https://reviewboard.mozilla.org/r/50937/#review48251

::: dom/media/tests/mochitest/head.js:240
(Diff revisions 3 - 4)
> - *        Type of media element to create ('audio' or 'video')
>   * @param {string} label
> - *        Description to use for the element
> + *        Prefix to use for the element
> + * @param {direction} "local" or "remote"
> + * @param {stream} A MediaStream id.
>   * @return {HTMLMediaElement} The created HTML media element

Now the comments are missing the new |audioOnly| parameter :-)

Comment 21

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/50937/#review48251

> Now the comments are missing the new |audioOnly| parameter :-)

I'll fix.

Comment 22

[Byron Campen [:bwc]]

Comment on attachment 8749417 [details]
MozReview Request: Bug 998546: Fire ontrack and onaddstream on time. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50937/diff/4-5/

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a70f5cb17432

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/a70f5cb17432