Closed Bug 880366 Opened 7 years ago Closed 7 years ago

onaddstream is failing to fire post setRemoteDescription in media flow tests in bug 831789

Categories

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

defect
Not set

Tracking

()

RESOLVED INVALID

People

(Reporter: jsmith, Assigned: ehugg)

Details

(Whiteboard: [WebRTC][blocking-webrtc-][qa-automation-blocked])

When I originally wrote the patch in bug 831789, the tests ran to completion in all green colors. Now, they are all timing out waiting for onaddstream to fire. Ethan mentions this is related to bug 873888, as fake streams are not calling NotifyTracksAvailable.
Blocks: 831789
Assignee: nobody → ethanhugg
Whiteboard: [WebRTC][blocking-webrtc-][qa-automation-blocked]
Actually NotifyTracksAvailable does get called for fake streams.  This may be more complex than I thought.  Still investigating.
It looks like you are holding up the remote call to setLocal waiting for the oddAddStream to be called in setRemote, but the onAddStream callback won't hit until the other side does their set Local.  You need to check that onAddStream fired later in the process now that we are waiting for the tracks to be set up.

In other words if you replace the isFinished() call with a onSuccess() in the setRemoteDescription call like this:

pc.js:709
    this._pc.setRemoteDescription(desc, function () {
      info("Successfully set remote description for " + self.label);
      setRemoteDescriptionFinished = true;
      onSuccess();
    }, unexpectedCallbackAndFinish(new Error));

You will see the onAddStream callbacks come in.  The stream you get on the onaddstream callback will also now be guaranteed to have all it's tracks so you could query it instead of guessing like pc.js:704
(In reply to Ethan Hugg [:ehugg] from comment #2)
> It looks like you are holding up the remote call to setLocal waiting for the
> oddAddStream to be called in setRemote, but the onAddStream callback won't
> hit until the other side does their set Local.  You need to check that
> onAddStream fired later in the process now that we are waiting for the
> tracks to be set up.
> 
> In other words if you replace the isFinished() call with a onSuccess() in
> the setRemoteDescription call like this:
> 
> pc.js:709
>     this._pc.setRemoteDescription(desc, function () {
>       info("Successfully set remote description for " + self.label);
>       setRemoteDescriptionFinished = true;
>       onSuccess();
>     }, unexpectedCallbackAndFinish(new Error));
> 
> You will see the onAddStream callbacks come in.  The stream you get on the
> onaddstream callback will also now be guaranteed to have all it's tracks so
> you could query it instead of guessing like pc.js:704

Is that alignment with the W3C spec? I originally thought the rule we followed was that onaddstream fires post setRemoteDescription being called in this scenario.
W3C Spec says this:

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.

In Bug 873888 in was requested that we wait until the tracks are available which is what we have now.  We should probably ask someone closer to the standards process what they think it should do.
needinfo on Randell to address the standards question in comment 4
Flags: needinfo?(rjesup)
Yeah....

The need is to support streams being added later.  Waiting on the tracks for the stream to also be available is a reasonable thing, since firing onaddstream when a stream is only half-constructed can cause problems (the reason for bug 873888).

Bug 873888 does not seem to be mandated (nor really prohibited) by the spec, it's an oversight based on the assumption a track is born with an initial set of tracks.  The bug tries to match that in the case where a stream will take "a while" to get tracks.

So, firing onaddstream when the stream is really ready, with the initial tracks existing, makes sense.  Until then the object representing the remote stream isn't ready to give to an application.  Perhaps we can suggest wording to improve the spec to make it clear.
Flags: needinfo?(rjesup)
The design decision in bug 873888 I agree does make sense. But why is setLocalDescription on the remote peer required before onaddstream fires?

Also - for now, I'll update the patch in bug 831789 to not make an assumption of when onaddstream so that we can something landed.
Looks like we'll be changing the test not to expect onaddstream to callback right away, so closing this.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
No longer blocks: 831789
You need to log in before you can comment on or make changes to this bug.