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

BBA/BuiltIn: Add SSDP multicast support #10920

Merged
merged 3 commits into from Aug 28, 2022

Conversation

sepalani
Copy link
Contributor

@sepalani sepalani commented Jul 29, 2022

This PR implements the SSDP multicast support properly. It fixes one issue I had where my Windows host, its Linux VM and my smartphone couldn't see each other joining/leaving the LAN consistently. This method also seems firewall friendly, I don't see packets being dropped anymore on Wireshark.

The UDP port not opening issue isn't fixed in this PR. If the Windows discovery service is enabled, the VM and its hosts can't see each other consistently (shouldn't matter as I don't expect many people to use Dolphin in a VM to play LANs/online).

Ready to be reviewed & tested.


This change is Reviewable

@JMC47
Copy link
Contributor

JMC47 commented Jul 29, 2022

This PR causes a crash on android for my device (Pixel 3a) but apparently no one else has the problem.

@sepalani
Copy link
Contributor Author

After some testing, here's what's worth mentioning for Windows users:

  • When the SSDP discovery service is disabled, it makes the console detection (using the SSDP protocol) more stable.
  • Disabling Windows firewall also makes matchmaking/negotiation (UDP) more consistent and less likely to fail (due to the firewall dropping the UDP packet.

@JosJuice
Copy link
Member

For reference, so that it isn't lost: Here is a log from JMC reproducing the crash. No symbols for the native code, unfortunately. https://pastebin.com/BFpmnxby

@lioncash
Copy link
Member

lioncash commented Aug 3, 2022

Needs a rebase

@sepalani
Copy link
Contributor Author

sepalani commented Aug 4, 2022

@JMC47
I chose another approach that doesn't rely on the JNI code. It might not be 100% accurate but it's still better than introducing a JNI crash. Can you confirm that this new build doesn't crash on Android anymore?

@JMC47
Copy link
Contributor

JMC47 commented Aug 6, 2022

I somehow missed this comment. My bad. Testing now

@JMC47
Copy link
Contributor

JMC47 commented Aug 6, 2022

@sepalani this works and does not crash on Android.

@sepalani
Copy link
Contributor Author

@iwubcode
Done. I wrote a more detailed comment regarding network interface and I improved the error logs to add the reason of failure.

This PR was rebased to use Common::DecodeNetworkError, I'll test it again later this week to make sure it didn't introduce regressions.

@JMC47
Copy link
Contributor

JMC47 commented Aug 28, 2022

Is this all done now? I'd like to get the BBA stuff squared away before the Progress Report and Beta build.

@sepalani
Copy link
Contributor Author

@JMC47
Yes, I'm done here and with #10985. Tested on MKDD and Kirby Air Ride, the changes are working as intended on my end.

@JMC47 JMC47 merged commit e65d56a into dolphin-emu:master Aug 28, 2022
11 checks passed
@sepalani sepalani deleted the bba-multicast branch August 28, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants