Skip to content

Protocol cleanup (8): binary protocol refactoring #2932

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.

Sign up for GitHub

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

Jump to bottom
Merged
zmerp merged 3 commits into from
Jul 31, 2025
Merged

Protocol cleanup (8): binary protocol refactoring #2932

zmerp merged 3 commits into from
Jul 31, 2025

Conversation

@zmerp
Copy link
Member

@zmerp zmerp commented Jul 22, 2025 •
edited
Loading

  • Upgrade to to Bincode 2
  • Make prefix data little-endian
  • Make packet length contain itself (add 4 bytes to the count)
  • Remove extra 4 bytes from the max shard length
  • Improve performance of send functions (control socket and stream socket) by avoiding double serialization at steady state
  • Do not store separately the buffer size where applicable

Initially I wanted to use the new Bincode Encode and Decode traits in place of serde, given that they seem largely preferred by bincode. This is too cumbersome since we use glam in protocol packets which only implement serde Serialize and Deserialize. I know that Decode is more powerful than Deserialize because it allows for custom allocators, but apart from that I'm still unsure if there is any other change we could benefit from, like better performance.

Tested on Quest 3/Windows

@zmerp zmerp force-pushed the protocol-cleanup branch from 0264b7f to aa644de Compare July 22, 2025 18:16
The-personified-devil
The-personified-devil requested changes Jul 22, 2025
View reviewed changes
Copy link
Collaborator

@The-personified-devil The-personified-devil left a comment

Choose a reason for hiding this comment

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

I should probably give the files a through review of all the existing code some time in the future, because it seems to be partly operate on some pretty wrong assumptions, that don't seem to cause bugs but substantially complicate the code for no good reason.

@zmerp zmerp force-pushed the protocol-cleanup branch from 06ef4a8 to 0cc5a3d Compare July 29, 2025 13:09
@zmerp
Copy link
Member Author

zmerp commented Jul 29, 2025

I've looked for any place where storing the length/size of the buffer was redundant. All tested and working

The-personified-devil
The-personified-devil requested changes Jul 29, 2025
View reviewed changes
Copy link
Collaborator

@The-personified-devil The-personified-devil left a comment

Choose a reason for hiding this comment

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

Found some stuff, but my brain is mush today so this isn't a proper review.

alvr/sockets/src/control_socket.rs Outdated

pub fn send<S: Serialize>(&mut self, packet: &S) -> Result<()> {
framed_send(&mut self.inner, &mut vec![], packet)
framed_send(&mut self.inner, &mut vec![0; FRAMED_PREFIX_LENGTH], packet)
Copy link
Collaborator

@The-personified-devil The-personified-devil Jul 29, 2025

Choose a reason for hiding this comment

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

This isn't really an optimization, I think it's mainly just confusing cuz it leads you to try and figure out why it's that way, probably just turn it back into vec![].

Copy link
Collaborator

@The-personified-devil The-personified-devil Jul 31, 2025

Choose a reason for hiding this comment

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

This is still present in like one or two spots, but honestly I don't care anymore, just fix it in the next pr, that'll change that enough anyway.

@zmerp zmerp force-pushed the protocol-cleanup branch from 2c396ab to 903f600 Compare July 30, 2025 04:45
@zmerp zmerp merged commit a67c4e3 into master Jul 31, 2025
9 checks passed
@The-personified-devil The-personified-devil deleted the protocol-cleanup branch August 1, 2025 01:12
Jimmy32767255 added a commit to Jimmy32767255/ALVR-CN that referenced this pull request Aug 28, 2025
@Jimmy32767255
Squashed commit of the following:
d3bf935 
commit f14fbfc
Author: zmerp <merp.zee@gmail.com>
Date:   Tue Aug 26 09:09:46 2025 +1000

    refactor: :arrow_up: Update dependencies (alvr-org#2986)

commit 452452e
Author: zmerp <merp.zee@gmail.com>
Date:   Thu Aug 21 17:12:56 2025 +1000

    refactor(common): ♻️ Move popup logic out of alvr_common (alvr-org#2987)

commit c3ec94b
Author: zmerp <merp.zee@gmail.com>
Date:   Tue Aug 19 06:39:27 2025 +1000

    fix(server_core): :bug: Fix locked webserver during client handshake (alvr-org#2988)

commit 7589bb7
Author: zmerp <merp.zee@gmail.com>
Date:   Mon Aug 18 00:32:25 2025 +1000

    refactor(graphics): ⬆️ Update wgpu tp v25 (alvr-org#2982)

commit 343618b
Author: zmerp <merp.zee@gmail.com>
Date:   Wed Aug 13 03:05:49 2025 +1000

    feat(dashboard): :sparkles: Add "Clear all" button in Logs tab (alvr-org#2983)

commit f0c0516
Author: Leonhard Saam <54042101+The-personified-devil@users.noreply.github.com>
Date:   Sun Aug 10 04:05:40 2025 +0200

    ci: fully fix merge queues (alvr-org#2980)

    - disable the checks on push as the merge queue ones do their job fully

commit 93c5b43
Author: Leonhard Saam <54042101+The-personified-devil@users.noreply.github.com>
Date:   Sat Aug 9 16:46:45 2025 +0200

    ci: fix merge-queue actions (alvr-org#2978)

commit 12e6589
Author: Leonhard Saam <54042101+The-personified-devil@users.noreply.github.com>
Date:   Fri Aug 8 15:18:43 2025 +0200

    fix: bump cpal+rodio, fix os error 11 timeout (alvr-org#2944)

    - fix cpal device enumeration being extremely slow on v0.15
    - actually respect virtual audio cable search ordering
    - don't store is_output property
    - compare devices based on id
    - remove AudioDevice struct

commit 5d017a1
Author: Leonhard Saam <54042101+The-personified-devil@users.noreply.github.com>
Date:   Fri Aug 8 15:16:04 2025 +0200

    ci: make rarely triggering checks run on merge queue only (alvr-org#2973)

commit a211bad
Author: zmerp <merp.zee@gmail.com>
Date:   Fri Aug 8 03:33:32 2025 +0200

    refactor: :rotating_light: Fix Rust v1.89 lints (alvr-org#2972)

commit 7757a8b
Author: micsthepick <11528421+micsthepick@users.noreply.github.com>
Date:   Wed Aug 6 11:00:21 2025 +1000

    fix client stream.wgsl shader (alvr-org#2962)

commit 33a90ab
Author: Meister1593 <leruop@Gmail.com>
Date:   Sat Aug 2 02:43:58 2025 +0000

    fix (dashboard): Fix incorrect AV1 gpu in note alvr-org#2331, add more information about headsets (alvr-org#2949)

commit ec947a0
Author: zmerp <merp.zee@gmail.com>
Date:   Fri Aug 1 17:30:14 2025 +0200

    fix(xtask): :pushpin: Pin cargo-ndk to previous version (alvr-org#2954)

commit a67c4e3
Author: zmerp <merp.zee@gmail.com>
Date:   Thu Jul 31 23:24:52 2025 +0200

    Protocol cleanup (8): binary protocol refactoring (alvr-org#2932)

commit 6db106f
Author: zmerp <merp.zee@gmail.com>
Date:   Tue Jul 22 19:14:15 2025 +0200

    fix(dashboard): :bug: Fix unexpected version popup (alvr-org#2929)

commit 94ea7db
Author: zmerp <merp.zee@gmail.com>
Date:   Tue Jul 22 15:52:38 2025 +0200

    Protocol cleanup (7): Reserved to normal packet variants (alvr-org#2915)

commit 1fb60d1
Author: Meister1593 <leruop@Gmail.com>
Date:   Sat Jul 19 20:20:46 2025 +0000

    Pipewire implementation refactor (alvr-org#2902)

    * refactor(linux,audio): Refactor pipewire audio, exit early if pipewire is not found and inform user

    * refactor(linux,audio): simplify logic, add check for pipewire before starting receiving or sending data

    * refactor(linux,audio): leftover small refactors, remove clippy macros

    * refactor(linux,audio): add flatpak note, try initing more pipewire functions to trigger error, moved try pipewire earlier to abort all audio processing early

    * refactor(linux,audio,flatpak) Added check for pipewire early in dashboard launch, reformatting output

    * refactor(linux,audio): reduce nesting

    * revert unnecessary change

    * remove unnecessary nesting

    * reformat string

    * reformat string

    * clippy, format, macos fixes

    * clippy linux gate fixes

    * chore: Refactor pipewire impl some more

    * Address review comments; simplify pw flatpak check

    * fix: fix bug in pw playback + speed up buffer filling

    * reverse pw socket check

    * add check for pipewire being a socket or an empty file

    * light refactor to simplify

    * address comments in pr: invert logic for socket check again and simplify code, add comment related to state changes in pipewire causing cut out

    * format fix

    * fix typo

    ---------

    Co-authored-by: Leonhard Saam <leonhard.saam@yahoo.com>

commit 222ae53
Author: Dagg <dagg@kyzune.com>
Date:   Mon Jul 14 23:33:37 2025 -0700

    Implement high fidelity IOBT (XR_META_body_tracking_fidelity) for Quest 3 (alvr-org#2917)

    * Sync Files

    * Implement xrRequestBodyTrackingFidelityMETA

    * Formatting

    * Fix warnings

    * Removed maxenum value
    Probably not necessary now that i think about it

    * Cleanup and renaming

    * Add config option

    * Fix formatting

    * Explicitly set fidelity mode based on config value, don't bail on mode set fail.

    * Swap closure for if let, and_then

    * Bail if request method is missing, force Ok from mode set.

    ---------

    Co-authored-by: GitHub Action <action@github.com>

commit 80796e2
Author: Riccardo Zaglia <riccardo.zaglia5@gmail.com>
Date:   Tue Jul 15 02:19:14 2025 +1000

    fix(ci): :green_heart: Fix CI release workflow

commit 8c72354
Author: zmerp <merp.zee@gmail.com>
Date:   Sun Jul 13 16:51:54 2025 +0200

    fix(dashboard): :bug: Remove devices from Headset speaker dropdown (alvr-org#2918)

commit 8c0a815
Author: zmerp <merp.zee@gmail.com>
Date:   Fri Jul 11 15:54:48 2025 +0200

    Protocol cleanup (6) (alvr-org#2912)

    * Protocol cleanup (6)

    * Don't impl Default for BodyTrackingBDConfig

commit 6fa03e9
Author: zmerp <merp.zee@gmail.com>
Date:   Fri Jul 11 06:32:06 2025 +0200

    fix(audio): :bug: Fix input/output audio device handling (alvr-org#2911)

commit 4e08e0e
Author: zmerp <merp.zee@gmail.com>
Date:   Thu Jul 10 20:39:05 2025 +0200

    Protocol cleanup (5) (alvr-org#2907)

# Conflicts:
#	alvr/dashboard/src/dashboard/components/settings_controls/presets/builtin_schema.rs   resolved by origin/master(远端) version
#	alvr/session/src/settings.rs   resolved by origin/master(远端) version
#	wiki/Installation-guide.md   resolved by origin/master(远端) version
#	wiki/Installing-ALVR-and-using-SteamVR-on-Linux-through-Flatpak.md   resolved by origin/master(远端) version
#	wiki/Other-resources.md   resolved by origin/master(远端) version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@The-personified-devil The-personified-devil The-personified-devil approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zmerp @The-personified-devil