Skip to content

Use Server Discovery from SDK#1533

Draft
JPKribs wants to merge 25 commits intojellyfin:mainfrom
JPKribs:networkDiscovery
Draft

Use Server Discovery from SDK#1533
JPKribs wants to merge 25 commits intojellyfin:mainfrom
JPKribs:networkDiscovery

Conversation

@JPKribs
Copy link
Member

@JPKribs JPKribs commented May 6, 2025

Summary

Resolves(?): #554
Relies on: jellyfin/jellyfin-sdk-swift#47


All of the work for this PR is done, but this branch currently uses my own branch with the ServerDiscovery changes. for the JellyfinSDK. When we have a final version of the ServerDiscovery changes, I will update this PR to use the updated 0.5.2(?) Swift SDK. For now, this version of the SDK and this PR can be used to validate the changes for the SDK changes.


The primary goal of this PR is to replace UDPBroadcast with Apple's Swift-NIO.

A secondary goal of this PR is to make the JellyfinDiscovery process more robust. None of these are huge issues but I thought I would address them while I was here:

  1. The previous deinit for ServerDiscovery was flaky. As a result, if you opened the ConnectToServerView then closed it, then reopened it, JellyfinDiscovery would break until the app restarted.
  2. Due to 1), I wanted to try and make my implementation "recover" on failure. I've thrown a lot at this and it appears to work how I expect so I feel this make it more robust and less likely to break.
  3. IF JellyfinDiscovery breaks, there is currently no error or notice to the user that is the case. I've added some more visible notice so the user can be alerted that JellyfinDiscovery is no longer proceeding.
  4. There was no visible indication that JellyfinDiscovery was running/searching. I just added this to the existing ProgressView so the user could see there was something running.
  5. This last item is very rare but if you duplicates a Jellyfin server the ServerID is the same between both devices. The result is that the first to respond shows up in the list since ServerDiscoveryResponse is Identifiable. I changed the ForEach to used \.Self instead to resolve this but the other route would be to make the id: serverID + address or something like that but \.Self was the easiest route in case we decided we didn't want this item. Realistically, 99% of people are never going to have this issue. I only have this because I cloned my server for a Dev server / testing EFCore.

New Error Message on Crash

The red "Error" is shown if there a problem then the localizedDescription is shown under. In this case, Channel not ready. Swiftfin then tries again in 12 seconds per the Discovery Broadcast Timer. I was considering a version of this with a timer.

Simulator Screenshot - iPhone 16 Pro - 2025-05-06 at 13 16 12

With Timer (Not in PR)

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-05-06.at.15.04.50.mov

Why NIO over BlueSocket?

I've selected SwiftNIO over BlueSocket for our UDP broadcast/multicast implementation primarily because it's backed by Apple, and I have more faith that this will get updates than other packages. When I looked at Bluesocket, I was concerned that the last release was in 2021.

The usage for NIO was a little more... manual(?) than I would have liked but it looks like NIO is very granular so future usage for things like WebSocketting shouldn't be roadblocked due to lack of functionality. In my brief research, I found that SwiftNIO is described as having "excellent" WebSocket support (Thinking forward to #408). I originally did this with the Network package but I decided NIO would be good to get the ball rolling with and learning so I could potentially look at #408 down the road. Probably after #1203 since I don't really want to touch playback code until it's settled haha.

@JPKribs JPKribs added enhancement New feature or request developer Alters the developer experience labels May 6, 2025
@JPKribs
Copy link
Member Author

JPKribs commented May 6, 2025

Functional for both IPv4 and IPv6. This recovers if there are issues and appropriately displays errors / loading.

The only thing I don't love.... I ran into this error: apple/swift-nio#1494. It didn't impact anything or cause issues but to get that error to go away I needed to add:

    private static func getDeviceIPv4Address() -> String? {
        var ptr: UnsafeMutablePointer<ifaddrs>?
        defer { ptr.flatMap(freeifaddrs) }
        guard getifaddrs(&ptr) == 0, let first = ptr else { return nil }

        for cur in sequence(first: first, next: { $0.pointee.ifa_next }) {
            let flags = Int32(cur.pointee.ifa_flags)
            guard flags & IFF_UP != 0, flags & IFF_LOOPBACK == 0 else { continue }
            guard cur.pointee.ifa_addr.pointee.sa_family == sa_family_t(AF_INET) else { continue }

            var addr = cur.pointee.ifa_addr.withMemoryRebound(
                to: sockaddr_in.self, capacity: 1
            ) { $0.pointee.sin_addr }
            var buf = [CChar](repeating: 0, count: Int(INET_ADDRSTRLEN))
            inet_ntop(AF_INET, &addr, &buf, socklen_t(INET_ADDRSTRLEN))
            return String(cString: buf)
        }
        return nil
    }

It's just so unnecessary since we kind of DO want to bind to all sources. For example, tvOS with Ethernet, it would make the most sense to bind to all sources at 0.0.0.0?

That's the only part of this that seems off. Otherwise, I am happy with this PR.

@JPKribs JPKribs marked this pull request as ready for review May 6, 2025 22:46
@JPKribs JPKribs added iOS Impacts iOS or iPadOS tvOS Impacts tvOS labels May 6, 2025
@sjorge
Copy link

sjorge commented May 8, 2025

Ran a build with this, not sure how to test though. I removed the server and it showed up as a discovered server I could add.

Is it only used for that?

@JPKribs
Copy link
Member Author

JPKribs commented May 8, 2025

Is it only used for that?

For now, yes. So, if i did this correctly, it should be unnoticeable. Hopefully a little more robust but I don’t think people sit on this screen and try to crash it like I did. So mostly, does network discovery still work.

This PR is more about switching packages so we can start looking at web socketing for things like Sync Play. We’re still a long ways from that point but this is a distant step 1

——

Edit: Actually, a good test is confirming this works on IPv6. I put in all the work for it but when I tested it I had to create a new IPv6 network so I have no idea if those were “normal” conditions.

@sjorge
Copy link

sjorge commented May 8, 2025

My network is dualstack, but it just lists it with the fqdn... is there a way to check without a packet trace to see if it was using IPv6?

@JPKribs
Copy link
Member Author

JPKribs commented May 8, 2025

I think that would require putting the JF server on only the IPv6? At least, that's what I was doing to test it. What I did was, I put the server on exclusively IPV6 and did the same for my laptop with the emulator.

IPv6 is frankly something I haven't really worked with a lot so I very well could have done it incorrectly to test it

@sjorge
Copy link

sjorge commented May 9, 2025

Still seems good with IPv4 disabled on the dashboard
Screenshot 2025-05-09 at 11 59 03

It still shows up at least.

@JPKribs
Copy link
Member Author

JPKribs commented May 9, 2025

That's awesome! Thank you for the test! There was no reason that shouldn't have worked, but I'm so green on V6 that it's just good to have a second set of eyes. I appreciate the help!

@LePips
Copy link
Member

LePips commented May 10, 2025

This is an amazing thing to upgrade. I've previously tried otherwise with Starscreen, NIO, or Bluesocet but gave up due to other priorities.

go in the SDK instead?

I actually would be more interested in this living in the SDK, like how we provide the QuickConnect object.

@JPKribs
Copy link
Member Author

JPKribs commented May 10, 2025

Opened: jellyfin/jellyfin-sdk-swift#47


This PR uses my temp branch but should be a good, easy spot for testing. Almost a pure copy from here into the SDK. Everything is working as expected. This branch is purely that migration to use the SDK and also an upgrade to the UI to better reflect loading / errors.

@JPKribs JPKribs changed the title Replace UDPBroadcast with NIO Use Server Discovery from SDK May 13, 2025
@JPKribs JPKribs marked this pull request as draft May 13, 2025 00:25
@JPKribs JPKribs removed the enhancement New feature or request label Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

developer Alters the developer experience iOS Impacts iOS or iPadOS tvOS Impacts tvOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants