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/HLE: Add missing PSH flag #12533

Merged
merged 1 commit into from Mar 12, 2024
Merged

Conversation

sepalani
Copy link
Contributor

@sepalani sepalani commented Jan 26, 2024

It seems the BBA/HLE interface doesn't set the TCP push flag when receiving data. The data might not be processed immediately if the PSH flag isn't set. The following filter in Wireshark usually don't display packets on network dumps but surprisingly does with BBA/HLE network dumps: tcp.len > 0 && tcp.flags.ack == 1 && tcp.flags.push == 0.

I'll set it as a draft for the time being until it's tested and/or a hardware test written.

Here is a hardware test and the associated network dumps captured from dolphin without this PR, with this PR and Wireshark:
net_http_get_loop.zip

NB: The IP address and domain are hardcoded (93.184.216.34, www.example.com), so please make sure they are still valid before running the homebrew.

Ready to be reviewed & merged.

@sepalani sepalani marked this pull request as ready for review February 20, 2024 17:27
@sepalani
Copy link
Contributor Author

According to the original Wireshark dump, the host receives TCP packets with the PSH flag set (which we don't set before this PR). Apparently for HTTP streams, this flag is set by the server in the last TCP segment of a PDU.

This PR always set the PSH flag, though I don't think it will matter since we don't (can't?) know if the PSH flag is set or if the data sent was split across several segments.

This PR is ready to be reviewed & merged.

@AdmiralCurtiss
Copy link
Contributor

Seems reasonable to me. Should we test this in some game or something or should we just merge it?

@sepalani
Copy link
Contributor Author

I believe it can be merged as is. It doesn't seem to affect the BBA games I have but they don't go online like Homeland or PSO.

@schthack @nolrinale @fuzziqersoftware @ShiftaDeband
You might want to test this PR and/or #12560 to make sure they don't introduce regressions.

@JMC47
Copy link
Contributor

JMC47 commented Mar 9, 2024

Was this tested and is it ready to go? It looks good to me, can merge upon request.

@AdmiralCurtiss
Copy link
Contributor

It is ready but I'm not sure if anyone actually tested it.

@ShiftaDeband
Copy link
Contributor

ShiftaDeband commented Mar 12, 2024

Tested on MacOS (Universal build but on an M1-series chip) and Windows ARM64 and had no issues connecting, staying online, communicating with others, and disconnecting/reconnecting. Can test Windows x64 later tonight.

@AdmiralCurtiss
Copy link
Contributor

Good enough for me.

@AdmiralCurtiss AdmiralCurtiss merged commit cda008b into dolphin-emu:master Mar 12, 2024
11 checks passed
@sepalani sepalani deleted the bba-psh-ack branch March 16, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants