Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sink pad ordering #213

Merged
merged 2 commits into from Mar 2, 2019

Conversation

Projects
None yet
4 participants
@Manishearth
Copy link
Member

Manishearth commented Feb 27, 2019

Fixes #210

Based on #208

This implements a version of the fix detailed in #210 (comment)

Instead of unlinking/relinking, we simply keep new streams unlinked until we decide to trigger an offer, at which point we link them to new request pads. The answerer side keeps them unlinked until it gets a remote offer, at which point it also requests pads and links them to the correctly numbered request pads.

A missing piece was that the payload number also has to match, so we keep track of that too.

r? @jdm @slomo (last two commits only)

@Manishearth Manishearth force-pushed the Manishearth:sink-pad-ordering branch from c413cf5 to 957a4cd Feb 27, 2019

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Feb 27, 2019

I'm done for the day, but I do need to write some more comments for this. Will do tomorrow.

//
// An alternate fix is to sort pending_streams according to the m-line index
// and just do it in order. This also seems brittle.
webrtc.get_request_pad("sink_%u").unwrap();

This comment has been minimized.

@Manishearth

Manishearth Feb 27, 2019

Author Member

I feel like there's an underlying bug here, this should be unnecessary

cc @slomo

@Manishearth Manishearth force-pushed the Manishearth:sink-pad-ordering branch 3 times, most recently from 378076b to 1e15743 Feb 28, 2019

@@ -21,7 +21,7 @@ lazy_static! {
&[
("media", &"video"),
("encoding-name", &"VP8"),
("payload", &(96i32)),
// ("payload", &(96i32)),

This comment has been minimized.

@jdm

jdm Mar 1, 2019

Member

Remove these commented versions?

thread: WebRtcThread,
signaller: Box<WebRtcSignaller>,
/// All the streams that are connected to the

This comment has been minimized.

@jdm

jdm Mar 1, 2019

Member

to the ???

///
/// When we create an offer, we're controlling the pad order, so we set request_new_pads
/// to true and forcefully link all pending streams before generating the offer.
fn link_stream(&mut self, mut boxed_stream: Box<MediaStream>, request_new_pads: bool) {

This comment has been minimized.

@jdm

jdm Mar 1, 2019

Member

Can we use an enum instead of a boolean here?

@@ -90,6 +90,7 @@ pub enum InternalEvent {
OnNegotiationNeeded,
OnIceCandidate(IceCandidate),
OnAddStream(Box<MediaStream>),
DescriptionAdded(SendBoxFnOnce<'static, ()>, bool, SdpType),

This comment has been minimized.

@jdm

jdm Mar 1, 2019

Member

Maybe use an enum instead of a bool here?

let element = stream.src_element();

if let Some(idx) = idx {
if idx >= self.request_pad_counter {

This comment has been minimized.

@jdm

jdm Mar 1, 2019

Member

Is there any risk that we could call link_stream with !request_new_pads but end up running this code that requests pads? Would that be bad? I find the logic here a little bit obtuse.

This comment has been minimized.

@Manishearth

Manishearth Mar 2, 2019

Author Member

Yes, this code requesting new pads is fine, request_new_pads is about requesting new pads when there is no remote m-line: in that case we normally want to just buffer it up for later, except for when we're making an offer, at which point we want to flush all buffered streams.

The behavior of this function with !request_new_pads is "request pads only if they have been set up by the remote already", which is too long to fit in a variable. I can document this more explicitly.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

☔️ The latest upstream changes (presumably #209) made this pull request unmergeable. Please resolve the merge conflicts.

@Manishearth Manishearth force-pushed the Manishearth:sink-pad-ordering branch 3 times, most recently from 4835ab0 to cec05f0 Mar 2, 2019

@jdm

jdm approved these changes Mar 2, 2019

@Manishearth Manishearth force-pushed the Manishearth:sink-pad-ordering branch from cec05f0 to f0e7554 Mar 2, 2019

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Mar 2, 2019

@bors-servo r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 2, 2019

📌 Commit f0e7554 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 2, 2019

⌛️ Testing commit f0e7554 with merge fa92926...

bors-servo added a commit that referenced this pull request Mar 2, 2019

Auto merge of #213 - Manishearth:sink-pad-ordering, r=jdm
Fix sink pad ordering

Fixes #210

Based on #208

This implements a version of the fix detailed in #210 (comment)

Instead of unlinking/relinking, we simply keep new streams unlinked until we decide to trigger an offer, at which point we link them to new request pads. The answerer side keeps them unlinked until it gets a remote offer, at which point it also requests pads and links them to the correctly numbered request pads.

A missing piece was that the payload number also has to match, so we keep track of that too.

r? @jdm @slomo (last two commits only)
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 2, 2019

☀️ Test successful - checks-travis
Approved by: jdm
Pushing fa92926 to master...

@bors-servo bors-servo merged commit f0e7554 into servo:master Mar 2, 2019

1 check passed

homu Test successful
Details
@slomo

This comment has been minimized.

Copy link

slomo commented Mar 4, 2019

@Manishearth You are mentioning the wrong guy in your PR.

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Mar 4, 2019

Oh, sorry about that!

@Manishearth Manishearth deleted the Manishearth:sink-pad-ordering branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.