Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Filter and buffering ice candidates (#49) #50

Merged
merged 21 commits into from
Sep 16, 2019
Merged

Conversation

Kirguir
Copy link
Contributor

@Kirguir Kirguir commented Sep 4, 2019

Resolves #49

Synopsis

Receive error if add ICE candidate with empty candidate.

Solution

Filter ICE candidates with empty candidates on server.

Checklist

  • Created PR:
    • In draft mode
    • Name contains WIP: prefix
    • Name contains issue reference
    • Has k:: labels applied
    • Has assignee
  • Documentation is updated (if required)
  • Tests are updated (if required)
  • Changes conform code style
  • CHANGELOG entry is added (if required)
  • FCM (final commit message) is posted
    • and approved
  • Review is completed and changes are approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • WIP: prefix is removed
    • All temporary labels are removed

- filter ICE candidates with empty candidate
@Kirguir Kirguir added enhancement Improvement of existing features or bugfix k::design Related to overall design and/or architecture labels Sep 4, 2019
@Kirguir Kirguir self-assigned this Sep 4, 2019
@Kirguir
Copy link
Contributor Author

Kirguir commented Sep 5, 2019

FCM

Buffer ICE candidates received before remote description (#50, #49)

Additionally:
- filter ICE candidates with empty candidate

@Kirguir Kirguir requested a review from alexlapa September 5, 2019 12:30
Copy link
Collaborator

@alexlapa alexlapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kirguir ,

Все ок. Ждем #28 и #40. Когда сольем подтяните и на повторное ревью.

});

handle_ice_candidates(rx2, peer1_clone, 2)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kirguir ,

add_ice_candidate_before_precess_offer и add_ice_candidate_before_precess_answer завершаются не потому, что было получено N кандидатов, а потому, что дропаются пиры, содержащие tx'ы, что ведет к закрытию стрима в handle_ice_candidates.

Чинится мувом пиров в handle_ice_candidate(..).map(move |_| let _ = peer )

)
.map_err(|_| assert!(false)),
);
added += 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kirguir ,

Лучше инкрементить в add_ice_candidate().map(), дабы быть уверенным что этот промис успеет выполнится.

@Kirguir Kirguir requested a review from alexlapa September 11, 2019 15:29
# Conflicts:
#	.env
#	.travis.yml
#	Makefile
#	jason/src/peer/mod.rs
#	jason/tests/peer/mod.rs
#	jason/tests/web.rs
#	src/shutdown.rs
@alexlapa alexlapa marked this pull request as ready for review September 15, 2019 18:26
@alexlapa alexlapa requested a review from tyranron September 15, 2019 23:20
use futures::{
future::{self, IntoFuture},
sync::mpsc::{unbounded, UnboundedReceiver},
Future, Stream,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If trait doesn't used by name explicitly, you should not pollute the current namespace and import it via as _.


use futures::{
future::{self, IntoFuture},
sync::mpsc::{unbounded, UnboundedReceiver},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not hesitate to use mpsc::unbounded instead of just unbounded. It's still quite ergonomic while much more informative.

warn!("Empty candidate from Peer: {}, ignoring", peer_id);
let fut: Box<
dyn ActorFuture<Actor = _, Item = _, Error = _>,
> = Box::new(actix::fut::ok(()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a shorter alias above: ActFuture<I, E>.

@alexlapa alexlapa added this to the 0.2.0 milestone Sep 16, 2019
@alexlapa alexlapa changed the title WIP: Filter and buffering ice candidates (#49) Filter and buffering ice candidates (#49) Sep 16, 2019
@alexlapa alexlapa merged commit 5a4c58b into master Sep 16, 2019
@alexlapa alexlapa deleted the buffering_ice_candidates branch September 16, 2019 19:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvement of existing features or bugfix k::design Related to overall design and/or architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to execute 'addIceCandidate' on 'RTCPeerConnection': Error processing ICE candidate
3 participants