search

aws / amazon-chime-sdk-js

A JavaScript client library for integrating multi-party communications powered by the Amazon Chime service.
Apache License 2.0
699 stars 473 forks source link

Fixed packets received check on Safari 17.3 and below #2868

Closed hensmi-amazon closed 3 months ago

hensmi-amazon commented 3 months ago

Issue #: None

Description of changes: Safari only has bytes received on 17.3 and below, so switching to that should be sufficient. Ping-pong check doesn't work in these cases to trigger reconnect itself even though websocket is snapped, but will fix that another day. I made a new minor version since I am just going to hotfix this out.

This original change was released in 3.20, and lead to re-connections not occurring on network changes.

Testing: Tested on Safari 17.3 ios, 17.4 macos, firefox, chrome with forced toggle.

Can these tested using a demo application? Please provide reproducible step-by-step instructions.

  1. Join with Safari 17.3 browser
  2. Toggle WiFi off and on. You may need to delay how long you keep it off.
  3. Should reconnect after around 15 seconds

Checklist:

  1. Have you successfully run npm run build:release locally? y

  2. Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved? n

  3. Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved? n

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ltrung commented 3 months ago

Do we need to update here too ? https://github.com/aws/amazon-chime-sdk-js/blob/60eeb35745ec6e0869017edbfab1b032279ecdd0/src/meetingreadinesschecker/DefaultMeetingReadinessChecker.ts#L274 Not sure how this work before if Safari does not have packetReceived?

hensmi-amazon commented 3 months ago

Ping-pong check is also broken in these cases but will fix that another day

Isn't ping-pong broken would cause reconnect?

Different from Chrome (at least in my testing), in safari (though i don't have this issue with 17.4) the websocket seems to shut down in a different way, that leads to the ping/pong logic to be disabled. I'm not sure why we would disable that any time besides the end of call since the signaling connection should always be established, but the code is pretty old.

Do we need to update here too ?

This is looking at inbound-rtp stats reports which do have the packetsReceived metric. The candidate-pair (which includes RTCP so can be used when attendee caps is None) did not have packetsReceived