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

Add version-based visibility of shairport-sync endpoints #1831

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

kuzhylol
Copy link

@kuzhylol kuzhylol commented Mar 24, 2024

This patch introduces modification to AirPlay attributes allowing AirPlay clients seeing only appropriate Endpoints of specific version of AirPlay protocol.

[Test]
Run two instances of shairport-sync - built for version 1 and for version 2.

TuneBlade recognizes only AirPlay version 1 instance running.
MacOS/iOS recognizes only AirPlay version 2 instance running.

Without that patch, both MacOS/iOS detect two endpoints at the same time. The TuneBlade detects only AirPlay version 1 since it doesn't support AirPlay version 2.

@mikebrady
Copy link
Owner

Very interesting, thanks. Let me run a few checks.

This patch introduces modification to AirPlay attributes allowing AirPlay clients
seeing only appropriate Endpoints of specific version of AirPlay protocol.

[Test]
Run two instances of shairport-sync - built for version 1 and for version 2.

TuneBlade recognizes only AirPlay version 1 instance running.
MacOS/iOS recognizes only AirPlay version 2 instance running.

Without that patch, both MacOS/iOS detect two endpoints at the same time.
The TuneBlade detects only AirPlay version 1 since it doesn't support AirPlay version 2.
@kuzhylol
Copy link
Author

Hi @mikebrady, do you have any thoughts or concerns?

@mikebrady
Copy link
Owner

Apologies, Oleh -- I haven't yet run my checks; very busy IRL just now.

@mikebrady
Copy link
Owner

mikebrady commented Apr 1, 2024

Thanks again for this interesting PR, which I have tested out and which seems to work.

If you could make this target the development branch, rather than the master branch, if would be much preferred. This way, it can be further tested by advanced users before it becomes part of a full release.

@kuzhylol kuzhylol changed the base branch from master to development April 1, 2024 11:14
@kuzhylol
Copy link
Author

kuzhylol commented Apr 1, 2024

Hi @mikebrady,
Thank you for testing my patch.

I've moved the target branch to development.

@mikebrady
Copy link
Owner

Thanks for this. Let's give it a try!

@mikebrady mikebrady merged commit 6eba4fd into mikebrady:development Apr 1, 2024
9 checks passed
kuzhylol added a commit to kuzhylol/seamless-point-manifest that referenced this pull request Apr 1, 2024
Merge requests were merged into shairport-sync and alac repositories:
Shairport-sync: mikebrady/shairport-sync#1831
Alac: mikebrady/alac#12

These patches unlocks further development and allows removing local
patches.
kuzhylol added a commit to kuzhylol/seamless-point-manifest that referenced this pull request Apr 1, 2024
Pull requests were merged into shairport-sync and alac repositories:
Shairport-sync: mikebrady/shairport-sync#1831
Alac: mikebrady/alac#12

These patches unlocks further development and allows removing local
patches.
@mikebrady
Copy link
Owner

Hi @kuzhylol. It looks as if @poker335 is right, so an update has been pushed on the development branch with these changes reverted.

Thanks -- it was a nice idea, but unfortunately it doesn't seem to be a general solution. When you look at the Unofficial AirPlay Specification, in the section on "Audio codecs" it looks as if the strings removed specify the codecs used, and in that sense, removing them seems the wrong thing to do.

@kuzhylol
Copy link
Author

kuzhylol commented Aug 3, 2024

Hi @mikebrady, this is very unfortunate. Based on you experience, why removing "Audio codecs" section can split APv1 and APv2?

Perhaps you could propose more general solution?
What if we remove this section only for APv2?

@mikebrady
Copy link
Owner

Hi @mikebrady, this is very unfortunate. Based on you experience, why removing "Audio codecs" section can split APv1 and APv2?

Hi @kuzhylol. Since AirPlay specifications and protocols have not been published, we can only go by our experience and "common sense", which is always a bit unreliable. On a "common sense" basis, it looks like removing the advertisement of the audio codecs seems not to be very sensible. This seems to be confirmed by @poker335's experience, and my own, using Shairport Sync on real Apple sources.

Perhaps you could propose more general solution? What if we remove this section only for APv2?

This is not something I've spent a great deal of time considering, but, in the absence of documentation to guide us, any changes would have to be very thoroughly tested.

@kuzhylol
Copy link
Author

kuzhylol commented Aug 4, 2024

Hi @mikebrady, I've been thinking about the 'issue" happened on #1856

Isn't that an expected behavior to have AirPlay v1 not visible for AirPlay v2 "only" device?
The iOS 17.5 and iPadOS 17.5 looks a cutting edge firmware version. Why Airplay v1 is even needed there?

Having AirPlay v2 only for brand new devices -- That's what I actually wanted to improve in Shairport Sync 😄
This case just confirmed that patch works.


What do you think on that @mikebrady?

@mikebrady
Copy link
Owner

Thanks for your comments.

Isn't that an expected behavior to have AirPlay v1 not visible for AirPlay v2 "only" device?

AirPlay 2 devices are capable of classic AirPlay operation for compatibility with classic AirPlay devices. AirPlay 2 devices also need to be able to switch dynamically from AirPlay 2 to classic AirPlay operation.

The iOS 17.5 and iPadOS 17.5 looks a cutting edge firmware version. Why Airplay v1 is even needed there?

Apple continues to support classic AirPlay on these systems.

Having AirPlay v2 only for brand new devices -- That's what I actually wanted to improve in Shairport Sync 😄
This case just confirmed that patch works.

Classic AirPlay is supported by Apple on brand new devices, and it remains, for the present at least, the only lossless AirPlay. It would be a step backwards for Shairport Sync to drop support for Classic AirPlay on newer operating systems.

Generally, it looks as if Apple is careful to retain classic AirPlay compatibility years after AirPlay 2 was introduced and classic-AirPlay-only devices were discontinued. This may be because audio devices -- e.g. stereos and AV receivers -- have a much longer expected lifetime than computer-based products. It seems only logical that Shairport Sync should try to do the same thing.

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