dolphin icon indicating copy to clipboard operation
dolphin copied to clipboard

BBA/HLE: Add missing PSH flag

Open sepalani opened this issue 1 year ago • 5 comments

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 avatar Jan 26 '24 22:01 sepalani

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.

sepalani avatar Feb 20 '24 17:02 sepalani

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

AdmiralCurtiss avatar Feb 20 '24 19:02 AdmiralCurtiss

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 https://github.com/dolphin-emu/dolphin/pull/12560 to make sure they don't introduce regressions.

sepalani avatar Feb 22 '24 18:02 sepalani

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

JMC47 avatar Mar 09 '24 21:03 JMC47

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

AdmiralCurtiss avatar Mar 09 '24 23:03 AdmiralCurtiss

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.

ShiftaDeband avatar Mar 12 '24 01:03 ShiftaDeband

Good enough for me.

AdmiralCurtiss avatar Mar 12 '24 02:03 AdmiralCurtiss