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

feat: Makes the use of a component socket optional. #147

Merged
merged 2 commits into from Oct 5, 2018

Conversation

Projects
None yet
3 participants
@bgrozev
Member

bgrozev commented Oct 3, 2018

No description provided.

@bbaldino

I worry that the experience of using ice4j without the merging socket with these changes may have lots of "gotchas" (and perhaps the jvb diff illustrates that? I need to take another pass). Most of these (dumb?) questions will probably be answered as I go through the jvb diff, but to help my memory:

  1. How will the user of the ice4j lib know which specific candidate to use directly (I think there's an event/notification of successful candidates, is that right? Will the user of the lib need to manually read from all successful candidates?)
  2. When a current candidate fails (and therefore the user needs to start using a different one) is there another event/notification? How will the user of the lib know which next candidate to start using?
  3. Will the transitions between candidates (e.g. when one fails) result in the loss of any packets? (not necessarily on the network, but packets maybe left in ice4j buffers)

I totally get these changes as something quick to re-run some tests, but if we find that we need to stick with this solution, do you think it's worth reconsidering ice4j a bit to find a better way to integrate this? Or do you think this is as good as it's going to get without significant effort?

}
}
return localCandidate.getCandidateIceSocketWrapper();

This comment has been minimized.

@bbaldino

bbaldino Oct 5, 2018

Member

how does it make sense to have a wrapper but with no remote candidate attached? will it be updated automatically if/once a remote candidate is determined?

@bbaldino

bbaldino Oct 5, 2018

Member

how does it make sense to have a wrapper but with no remote candidate attached? will it be updated automatically if/once a remote candidate is determined?

This comment has been minimized.

@bgrozev

bgrozev Oct 5, 2018

Member

I'm not quite sure (this is a rollback from a previous version). But I think if you are using the dynamic ports that you just bind to a local port and it's ready for reading regardless of the remote candidate (though you will need the target if you want to send packets).

@bgrozev

bgrozev Oct 5, 2018

Member

I'm not quite sure (this is a rollback from a previous version). But I think if you are using the dynamic ports that you just bind to a local port and it's ready for reading regardless of the remote candidate (though you will need the target if you want to send packets).

@bbaldino

This comment has been minimized.

Show comment
Hide comment
@bbaldino

bbaldino Oct 5, 2018

Member

Went through the bridge and the changes there don't seem terrible, actually, and I think they'd be even cleaner once the rewrite hits. I'm still curious about the answers to the questions above, but this doesn't seem as bad after looking at jvb.

Member

bbaldino commented Oct 5, 2018

Went through the bridge and the changes there don't seem terrible, actually, and I think they'd be even cleaner once the rewrite hits. I'm still curious about the answers to the questions above, but this doesn't seem as bad after looking at jvb.

int minPort,
int maxPort,
KeepAliveStrategy keepAliveStrategy,
boolean useComponentSocket)

This comment has been minimized.

@ibauersachs

ibauersachs Oct 5, 2018

Member

You might want to keep the existing constructor (with a default that resembles the previous behavior). Otherwise this needs a new API version.

@ibauersachs

ibauersachs Oct 5, 2018

Member

You might want to keep the existing constructor (with a default that resembles the previous behavior). Otherwise this needs a new API version.

IceMediaStream mediaStream,
KeepAliveStrategy keepAliveStrategy)
KeepAliveStrategy keepAliveStrategy,
boolean useComponentSocket)

This comment has been minimized.

@ibauersachs

ibauersachs Oct 5, 2018

Member

Same here with keeping the existing constructor and the version.

@ibauersachs

ibauersachs Oct 5, 2018

Member

Same here with keeping the existing constructor and the version.

This comment has been minimized.

@bgrozev

bgrozev Oct 5, 2018

Member

This one is protected. Theoretically, people might extend Component, but I highly doubt anyone is doing it. Are there any automated tools that check the API compatibility? Do you still think it's necessary to re-add the constructor with the previous signature?

@bgrozev

bgrozev Oct 5, 2018

Member

This one is protected. Theoretically, people might extend Component, but I highly doubt anyone is doing it. Are there any automated tools that check the API compatibility? Do you still think it's necessary to re-add the constructor with the previous signature?

This comment has been minimized.

@bgrozev

bgrozev Oct 5, 2018

Member

I added it, it doesn't hurt

@bgrozev

bgrozev Oct 5, 2018

Member

I added it, it doesn't hurt

This comment has been minimized.

@ibauersachs

ibauersachs Oct 5, 2018

Member

Part of the Maven build should actually check the API compatibility. However, since we never actually released 2.0 officially, it might not detect this (the API changed from 1.1 anyway).

@ibauersachs

ibauersachs Oct 5, 2018

Member

Part of the Maven build should actually check the API compatibility. However, since we never actually released 2.0 officially, it might not detect this (the API changed from 1.1 anyway).

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Oct 5, 2018

Member

1: Without the "component socket" it falls back to the previous ice4j API and behavior. That is, one candidate pair is selected and nominated, and once this is done (we move to the COMPLETED state) you can not change to another pair. You can access the selected pair through Component.getSelectedPair(), and that's where you get your socket.

2 & 3: This is not supported. ICE needs to run again.

I'll do the API changes that Ingo suggested which should also fix the tests.

Member

bgrozev commented Oct 5, 2018

1: Without the "component socket" it falls back to the previous ice4j API and behavior. That is, one candidate pair is selected and nominated, and once this is done (we move to the COMPLETED state) you can not change to another pair. You can access the selected pair through Component.getSelectedPair(), and that's where you get your socket.

2 & 3: This is not supported. ICE needs to run again.

I'll do the API changes that Ingo suggested which should also fix the tests.

@bgrozev bgrozev merged commit 7d2ad8f into jitsi:master Oct 5, 2018

1 check passed

default 799 tests run, 646 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment