931903 - Add mochitest to verify that forwarding a MediaStream from one PeerConnection to another succeeds

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

Because the hints are missing, when forwarding a MediaStream from one PeerConnection to another createOffer never returns success or failure.

Reported and discussed on IRC: 

http://logbot.glob.com.au/?c=mozilla%23media&s=22+Oct+2013&e=22+Oct+2013

15:53	ehugg	abr: Sure. Earlier on we used the NotifyQueuedTrackChanges callback to figure out which tracks where there since we could not enumerate them.
15:54	ehugg	The problem was that it was not guaranteed to fire in time before we needed the info.
15:55	ehugg	There was no way to enumerate the tracks of a a stream and no way even if we had a track to determine its type.
15:56	jesup	ehugg: right; the stream didn't appear until some semi-random time later
15:57	jesup	or rather the track(s)
15:59	ehugg	Yes, we need either to change how the media stream works or to do a 'wait-for-preroll' hack to get these notifications.

Comment 1

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

Attachment: testcase

Modified version of pc_test with a second set of PC's that include a forwarded stream.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

the test case fails here https://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c#3219

Comment 3

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

See also bug 932845 (same thing with mozCaptureStreamUntilEnded())

Comment 4

[Andreas Pehrson [:pehrsons]]

Is this still the case?

I get a bunch of errors in the console when using the test page so can't tell.

Comment 5

[Maire Reavy [:mreavy]]

This is still an issue.  We solved it for mozCaptureStream by adding hints after metadataloaded, but that doesn't help us here.  The test has bit-rotted, but if you take pc_test.html and add a forward of one of the received streams from a (new) pc3 to a pc4, that should show the problem (effectively, that's what the bitrotted test was, but ICE/etc changes have been made since then).

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Forwarding test case - just press start!

Seems to work just fine for me on mac with 37 :)

Comment 7

[Maire Reavy [:mreavy]]

Oh, very cool!  It looks like jib's changes in bug 1070127 in combination with your changes in bug 1103848 fixed this.  So I'd like to change this then into "add a mochitest for forwarding" so that this functionality doesn't get accidentally broken.

Andreas -- Can you take this bug (given its new purpose)?

Comment 8

[Andreas Pehrson [:pehrsons]]

Sure!

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Add mochitest for forwarding a stream from one PC to another

I hope I understood the testing framework correctly. My first time diving into it ;-)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fd7de48ac9e

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Add mochitest for forwarding a stream from one PC to another

Ok, this should fix the shutdown procedure.

I'll be gone for a week from now so if this gets r+, feel free to push to try and land it.

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8564224 [details] [diff] [review]
Add mochitest for forwarding a stream from one PC to another

Review of attachment 8564224 [details] [diff] [review]:
-----------------------------------------------------------------

In general this looks good as seem to work (to my surprise).
But since Byron just landed multistream this new test is failing, because the new MSID verification fails. So this needs to be fixed before landing.

Comment 12

[Andreas Pehrson [:pehrsons]]

Attachment: Add mochitest for forwarding a stream from one PC to another (carries r=jesup)

Solved using PeerConnectionWrapper.attachMedia instead of manually adding tracks to the PeerConnection.

Comment 13

[Andreas Pehrson [:pehrsons]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e670d73a71d3

Comment 14

[Nils Ohlmeier [:drno]]

Comment on attachment 8567792 [details] [diff] [review]
Add mochitest for forwarding a stream from one PC to another (carries r=jesup)

Review of attachment 8567792 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM and works now :-)

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

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

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