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

[iOS] Fix #1256 #1311

Closed
wants to merge 2 commits into from
Closed

[iOS] Fix #1256 #1311

wants to merge 2 commits into from

Conversation

Gondnat
Copy link

@Gondnat Gondnat commented Nov 11, 2024

This PR is used to fix "Can't select subtitles, it jumps back to none #1256"

There are two parts to fix it:

  1. The SubtitleProfile in request should be as below, the old profile will get broken DeliveryUrl for external subtitles.
{Format: "vtt", Method: "External"}

The broken URL lost the extension name ".vtt" after Stream.

Borken URL

"DeliveryUrl": "/Videos/7c4730fd-90f1-e75e-656c-fe7da8fb22aa/7c4730fd90f1e75e656cfe7da8fb22aa/Subtitles/0/0/Stream?api_key=953dccded7e2455ba0088f62cf322dfc",

Normal URL

"DeliveryUrl": "/Videos/7c4730fd-90f1-e75e-656c-fe7da8fb22aa/7c4730fd90f1e75e656cfe7da8fb22aa/Subtitles/0/0/Stream.vtt?api_key=953dccded7e2455ba0088f62cf322dfc",
  1. To make embedded subtitles work well, they should reduce as same as embedded audio in function adjustAudioForExternalSubtitles

@LePips
Copy link
Member

LePips commented Nov 21, 2024

Apologies for the long wait, I know this is a somewhat simple fix but so was #1219 and that didn't entirely fix the issue. There are a few edge cases for me to consider, mainly with transcoded streams + external subtitles, that I remember being wacky that may contribute to this issue that I will need to test with.

@JPKribs JPKribs added the bug Something isn't working label Nov 24, 2024
@JPKribs
Copy link
Member

JPKribs commented Dec 19, 2024

Hey all! I just had a chance to test this and unfortunately this does not appear to fully resolve the issue. On DirectPlay, external subtitles work correctly. When transcoding, this the issue is still persistent for external subtitles.

@JPKribs
Copy link
Member

JPKribs commented Dec 20, 2024

I've been screwing around with this for a bit. Does Jellyfin/Swiftfin downsample audio? I think our Transcode Subtitle Index is getting thrown off because the audio index is unreliable. It looks like the Audio selection also gets screwed up by being transcoded but it seems to be more likely to get screwed up with multiple audio/subtitle sources.

From testing, this PR seems to work very consistently with non-transcoded content. IMO, maybe we pas in the StreamType as a parameter then have this run for DirectPlay. Figuring out transcoded should still be part of the solution but this would be my take on this:

func adjustExternalSubtitleIndexes(streamType: StreamType, audioStreamCount: Int) -> [MediaStream] {
    guard allSatisfy({ $0.type == .subtitle }) else { return self }
    
    let embeddedSubtitleCount = filter { !($0.isExternal ?? false) }.count
    let externalSubtitleCount = filter { $0.isExternal ?? false }.count
    
    var mediaStreams = self
    
    switch streamType {
    case .direct:
        for (index, mediaStream) in mediaStreams.enumerated() {
            var updatedStream = mediaStream
            if updatedStream.isExternal ?? false {
                updatedStream.index = (updatedStream.index ?? 0) + 1 + embeddedSubtitleCount + audioStreamCount
            } else {
                updatedStream.index = (updatedStream.index ?? 0) + externalSubtitleCount
            }
            mediaStreams[index] = updatedStream
        }
    case .hls:
            *Whatever we need to do to get this to work for HLS?*
    case .transcode:
            *Whatever we need to do to get this to work for transcodes?*
    }
    return mediaStreams
}

As an FYI, for testing you can force transcoding by going to Settings > Playback Quality and either set Device Profile to Most Compatible if your content isn't already H.264 & AAC or you can lower the Maximum Bitrate to something under the bitrate of the original file.

@Gondnat
Copy link
Author

Gondnat commented Jan 15, 2025

I've been screwing around with this for a bit. Does Jellyfin/Swiftfin downsample audio? I think our Transcode Subtitle Index is getting thrown off because the audio index is unreliable. It looks like the Audio selection also gets screwed up by being transcoded but it seems to be more likely to get screwed up with multiple audio/subtitle sources.

From testing, this PR seems to work very consistently with non-transcoded content. IMO, maybe we pas in the StreamType as a parameter then have this run for DirectPlay. Figuring out transcoded should still be part of the solution but this would be my take on this:

func adjustExternalSubtitleIndexes(streamType: StreamType, audioStreamCount: Int) -> [MediaStream] {
    guard allSatisfy({ $0.type == .subtitle }) else { return self }
    
    let embeddedSubtitleCount = filter { !($0.isExternal ?? false) }.count
    let externalSubtitleCount = filter { $0.isExternal ?? false }.count
    
    var mediaStreams = self
    
    switch streamType {
    case .direct:
        for (index, mediaStream) in mediaStreams.enumerated() {
            var updatedStream = mediaStream
            if updatedStream.isExternal ?? false {
                updatedStream.index = (updatedStream.index ?? 0) + 1 + embeddedSubtitleCount + audioStreamCount
            } else {
                updatedStream.index = (updatedStream.index ?? 0) + externalSubtitleCount
            }
            mediaStreams[index] = updatedStream
        }
    case .hls:
            *Whatever we need to do to get this to work for HLS?*
    case .transcode:
            *Whatever we need to do to get this to work for transcodes?*
    }
    return mediaStreams
}

As an FYI, for testing you can force transcoding by going to Settings > Playback Quality and either set Device Profile to Most Compatible if your content isn't already H.264 & AAC or you can lower the Maximum Bitrate to something under the bitrate of the original file.

Through code review and some testing, I think this issue might be related to VLCKit.
VLCKit behaves differently in handling the order of subtitles, even when they are added in the same sequence using addPlaybackSlave. For non-transcoded content, external subtitles appear after embedded subtitles. However, with transcoded content, the external subtitles take precedence.
Still working on it.

ddrccw added a commit to ddrccw/Swiftfin that referenced this pull request Jan 26, 2025
@JPKribs
Copy link
Member

JPKribs commented Feb 24, 2025

I spent a lot of time today working and reworking this. I've gotten to the point where I have something that works for External subtitles for transcodes and nothing else. It's definitely just an index issue since I get each of the tracks I am testing with to show up and be visible but I cannot for the life of be figure out the pattern to how they're shown.

Basing on this current PR, this is the current lay of the land:

Type Internal Tracks External Subtitles External Audio
Transcode
Direct

This being said, the existence of any external tracks breaks both audio and subtitles. Even internal tracks. If you have enough tracks, you will see they're not 'broken' but more offset. So, I can select an earlier track in the list and get something further down the list.

Something I found is that external audio are also experiencing this same issue. Also, even in this current PR version, transcoding is throwing off the audio tracks as well if there is an external track. So, while the direct play external subtitles are working all of the internal audio tracks are offset by 1 from my testing. My guess is that's going to be a big part of our issue. Looking at the offset logic for audio I'm having trouble trusting it's doing the correct thing?

    func adjustAudioForExternalSubtitles(externalMediaStreamCount: Int) -> [MediaStream] {
        guard allSatisfy({ $0.type == .audio }) else { return self }

        var mediaStreams = self

        for (i, mediaStream) in mediaStreams.enumerated() {
            var copy = mediaStream
            copy.index = (copy.index ?? 0) - externalMediaStreamCount
            mediaStreams[i] = copy
        }

        return mediaStreams
    }

So, if there are 5 external subtitles my audio track would move from 1 to -4? I'm almost positive the solution needs to be an update to how both subtitles and audio track offsets are handled but in all my testing I wasn't able to land anything.

I'm back on the road for work coming up here so I wont have a chance to look at this again for a bit but if no one else has cracked it by the 8th I can keep testing.

@JPKribs
Copy link
Member

JPKribs commented Feb 25, 2025

Okay, I lied @Gondnat. I ended up looking at this a lot more. I've gotten to the following state on my branch:

Type Internal Subtitles Internal Audio External Subtitles External Audio
Transcode 🟡*
DirectPlay ✅*

On Transcode, the default/first audio track is selected by default but changing it to any other track will fallback to None. Subtitles, in my testing, work for both DirectPlay and Transcoding 🎉, resolving the original #1256 issue but I would love to figure out the audio track issues. External tracks work correct IF they can be DirectPlayed. If your external track requires Transcoding to play, it will default to None instead.

Fixing Audio Tracks on Transcodes is a much bigger ask and would interfere a lot with #1203 since we need to switch URLs for streams.


Next Steps

I would greatly appreciate additional testers. I don't personally like to use external tracks for either audio or subtitles so I had to manually create my test scenarios. I don't know if these accurately reflect what the average Jellyfin uses is doing. Any assistance testing would be greatly appreciated to ensure I am not dropping anything.

I cannot guarantee that I caught every edge case so please let me know if you found any issues / scenarios where this does not work along with details about the file(s) that you tested with.

The code can either be pulled directly from my branch here: https://github.com/JPKribs/Swiftfin/tree/trackSelection or can be pulled by updating this PR's branch with the Logic & Changes provided below.

Eventually, I want to figure out the Transcoded Audio Tracks but I'm going to be busy for the next couple weeks with work so if anyone else has any insights, it would be greatly appreciated!


Logic & Changes

I can't seem to submit a PR to this branch so please find my code below. It's way over-commented while I try to wrap my head around this. Hopefully those help explain my thinking but I think they're fine to be pulled from a final version.

All of my changes can be found on my branch here: https://github.com/JPKribs/Swiftfin/tree/trackSelection

Alternatively, the code that varies from this PR can be found below:


MediaStream.swift

The following code should replace adjustExternalSubtitleIndexes and adjustAudioForExternalSubtitles.

extension [MediaStream] {

    /// What is this?
    /// Jellyfin maintains a sequential indexing system across all tracks with external at the beginning
    /// VLCKit handles external tracks (added separately to the media) by appending them to the end of the track list
    /// These functions change the order of the track list to reflect VLCKit's expectated order

    /// Adjust track indexes based on the a count of external tracks
    func adjustedAudio(
        for streamType: StreamType,
        embeddedSubtitleCount: Int,
        externalSubtitleCount: Int
    ) -> [MediaStream] {
        guard allSatisfy({ $0.type == .audio }) else { return self }

        // Get the count for embedded/external audio tracks
        let embeddedAudioCount = filter { !($0.isExternal ?? false) }.count
        let externalAudioCount = filter { $0.isExternal ?? false }.count

        // Get the default audio tracks
        let defaultAudioTracks = filter { $0.isDefault ?? false }

        // Create working version of the [MediaStream]
        var mediaStreams = self

        for (i, mediaStream) in mediaStreams.enumerated() {
            // Create a working version of the Audio Media Streams
            var copy = mediaStream

            // Handle indexing based on the Stream Type
            switch streamType {
            case .direct, .hls:
                // For DirectPlay, migrate external tracks to the end
                if copy.isExternal ?? false {
                    // Move external tracks to the end
                    copy.index = (copy.index ?? 0) + embeddedSubtitleCount + embeddedAudioCount
                } else {
                    // Move internal tracks to the beginning
                    copy.index = (copy.index ?? 0) - (externalAudioCount + externalSubtitleCount)
                }
                // Update the working copy with updated audio track indexes
                mediaStreams[i] = copy
            default:
                // TODO: Figure out tracks.
                break
            }
        }
        return mediaStreams
    }

    /// Adjust track indexes based on the a count of external tracks
    func adjustedSubtitles(
        for streamType: StreamType,
        embeddedAudioCount: Int,
        externalAudioCount: Int
    ) -> [MediaStream] {
        guard allSatisfy({ $0.type == .subtitle }) else { return self }

        // Get the count for embedded/external subtitle tracks
        let embeddedSubtitleCount = filter { !($0.isExternal ?? false) }.count
        let externalSubtitleCount = filter { $0.isExternal ?? false }.count

        // Create working version of the [MediaStream]
        var mediaStreams = self

        // For transcode, use a counter for sequential subtitles
        var subtitleCounter = 0

        for (i, mediaStream) in mediaStreams.enumerated() {
            // Create a working version of the Audio Media Streams
            var copy = mediaStream

            // Handle indexing based on the Stream Type
            switch streamType {
            case .direct, .hls:
                if copy.isExternal ?? false {
                    // Move external tracks to the end
                    copy.index = (copy.index ?? 0) + embeddedSubtitleCount + embeddedAudioCount + externalAudioCount
                } else {
                    // Move internal tracks to the beginning
                    copy.index = (copy.index ?? 0) - (externalAudioCount + externalSubtitleCount)
                }
            default:
                // In transcoded streams, video is 0, audio is 1, and subtitles start at 2
                let subtitleBaseIndex = 2

                // Assign a sequential index for subtitles starting from subtitleBaseIndex
                copy.index = subtitleBaseIndex + subtitleCounter
                subtitleCounter += 1
            }
            // Update the working copy with updated subtitle track indexes
            mediaStreams[i] = copy
        }
        return mediaStreams
    }
    ... // The rest of the code

MediaStream.swift

The following code should replace the init.

    init(
        playbackURL: URL,
        item: BaseItemDto,
        mediaSource: MediaSourceInfo,
        playSessionID: String,
        videoStreams: [MediaStream],
        audioStreams: [MediaStream],
        subtitleStreams: [MediaStream],
        selectedAudioStreamIndex: Int,
        selectedSubtitleStreamIndex: Int,
        chapters: [ChapterInfo.FullInfo],
        streamType: StreamType
    ) {
        self.item = item
        self.mediaSource = mediaSource
        self.playSessionID = playSessionID
        self.playbackURL = playbackURL
        self.videoStreams = videoStreams

        // Adjust Audio Indexes based on StreamType & External Tracks
        self.audioStreams = audioStreams.adjustedAudio(
            for: streamType,
            embeddedSubtitleCount: subtitleStreams.filter { !($0.isExternal ?? false) }.count,
            externalSubtitleCount: subtitleStreams.filter { $0.isExternal ?? false }.count
        )

        // Adjust Subtitle Indexes based on StreamType & External Tracks
        self.subtitleStreams = subtitleStreams.adjustedSubtitles(
            for: streamType,
            embeddedAudioCount: audioStreams.filter { !($0.isExternal ?? false) }.count,
            externalAudioCount: audioStreams.filter { $0.isExternal ?? false }.count
        )

        self.selectedAudioStreamIndex = selectedAudioStreamIndex
        self.selectedSubtitleStreamIndex = selectedSubtitleStreamIndex
        self.chapters = chapters
        self.streamType = streamType
        super.init()
    }

@JPKribs
Copy link
Member

JPKribs commented Mar 3, 2025

Hey @Gondnat! Sorry for pestering. I just wanted to follow up on the solution I proposed above. I'm hoping we can get this issue resolved for our 1.3 release. If you're busy with other priorities, I'd be happy to take ownership of this issue and move it forward. Otherwise, if you want to carry forward, I'd be happy to assist with what I've found.

Let me know what works best for you!

@LePips
Copy link
Member

LePips commented Mar 3, 2025

Out of urgency for getting this fixed as well as getting 1.3 out, I'm okay with contributing to this branch with the proper fixes and not worried about overriding. Proper accreditation will still happen.

@JPKribs
Copy link
Member

JPKribs commented Mar 3, 2025

Out of urgency for getting this fixed as well as getting 1.3 out, I'm okay with contributing to this branch with the proper fixes and not worried about overriding. Proper accreditation will still happen.

Sounds good. I have opened this here: #1445.

@Gondnat
Copy link
Author

Gondnat commented Mar 4, 2025

Hey @Gondnat! Sorry for pestering. I just wanted to follow up on the solution I proposed above. I'm hoping we can get this issue resolved for our 1.3 release. If you're busy with other priorities, I'd be happy to take ownership of this issue and move it forward. Otherwise, if you want to carry forward, I'd be happy to assist with what I've found.

Let me know what works best for you!

Sorry for delay. I'm quite busy lately, but I'd be happy if someone could take over. @LePips @JPKribs

@Gondnat Gondnat closed this Mar 4, 2025
@Gondnat Gondnat deleted the 1256 branch March 4, 2025 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants