Skip to content

Conversation

@Aircoookie
Copy link

This is an initial draft of the proposed visualizer type specification, including the visualizer_support JSON object, as well as the binary message data type definitions.

Feedback is highly appreciated!

Updated the visualizer support object structure and binary type details.
Copy link
Member

@maximmaxim345 maximmaxim345 left a comment

Choose a reason for hiding this comment

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

Hi @Aircoookie!

Thanks for the PR, sorry for taking so long to respond to you.

I have some suggestions on the architecture:

Splitting into multiple roles instead of tag-value encoding

Instead of a single visualizer role with tag-value encoded binary messages, I think it would be cleaner to split this into separate roles: visualize_beats, visualize_loudness, visualize_highest_amplitude, visualize_spectrum.

Benefits:

  • Each role would have its own binary message type (we have 44 reserved message IDs for new roles)
  • No need for tag-value encoding - the message format would be self-describing based on the message type
  • Clients can still subscribe only to the specific visualizations they need
  • Future-proofing: with PR #42, each visualization type can be versioned independently (e.g., visualize_spectrum@v2) without affecting the others

The only drawback I can think of is a slight overhead from receiving more WebSocket messages instead of bundled data - maybe resulting in at most 100 extra bytes per second, which isn't worth worrying about IMO.

Redundant bin count in Spectrum data

For the Spectrum/FFT data, the leading byte containing the bin count n seems redundant:

  • The client already knows n_disp_bins from the stream/start message
  • With separate roles, the client can calculate it from the WebSocket message length

Other than that, looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants