-
-
Notifications
You must be signed in to change notification settings - Fork 281
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
Customizable Device Profiles #1169
Conversation
Should swiftfin not just change the profile it uses automatically based on iOS version/hardware ? With maybe a force transcode option. |
It should, and that should always be the goal, but there may be experimental features that users want to enable using this. My best example is the mixed AV1 support where AV1 isn't supported on hardware for Apple TV 4K gen 3 but it's able to play it back via software decoding. Meanwhile, if I were to try and play the same AV1 file via software decoding using an Apple TV 4K gen 2, it would stutter constantly. This feature isn't a replacement for good hardware detection but more of a fallback option. The video/audio stream gets tricky as well because Apple devices may support something via hardware but VLKit/AVFoundation doesn't allow playback. I don't want to say there will be gaps but my goal with this PR is to build in a process so any potential gaps can be circumvented instead of being a showstopper. IMO, by default, Swiftfin should also err on the side of compatibility. This feature would allow you to enable direct play of something like AV1 when your device would have to software decode it. |
Update It looks like currently we have the VideoPlayer compatibilities as as the Device Profiles. I've kept this logic but moved it to the VideoPlayerType enum. So, instead of building the Device Profiles in code, it can just be pulled in as:
This should mean all the VideoPlayer logic lives together in one spot and should be easier to maintain as we potentially look at using MPV. Personally, I like the idea of maintaining the Native player in addition to the custom player so I hope both these logics can live side by side.
I hope that moving this logic to the enums is easier to maintain. It makes more sense to me but I'm open to move these around as needed! Updated: August 6th, 2024 |
I haven't had a chance to look at this very thoroughly, but I might as well add a few notes in case they are helpful. I love the idea of a custom device profile. I had previously tried to update the device profile in #519, and I know it was not perfect. I am surprised you would have trouble with any HEVC videos; I am not sure why video transcoding would be required for them, and I would be interested to know why it wasn't working. I don't remember if I tested AV1 or not - I think my initial assumption was that VLCKit would support the same codecs as the Jellyfin server would for transcoding (since they both rely on ffmpeg), but I think that is not a great assumption going forward as the server is likely updated more frequently than VLCKit - or at least the VLCKit dependency in this app. The main issues I see with the isSupported check:
In case it is helpful, I had tested many other video codecs which VLCKit seems to support (and as far as I can recall, worked in the simulator): |
Hey @holow29 thank you for taking a look! I remember before #519 and it's honestly been night and day. Plus, adding the Custom Profile part was almost a pure cut and paste of what you had in there. I appreciate the great foundation you laid! The conversion from string to enum is 100% just to make the custom profile easier to make settings around. I was realizing about 5 minutes in that the enums probably triple the total number of lines compared to the original Device Profiles. As for issues I ran into personally, 100% of them fall under the Native Player. Having now seen the profiles, I have a hunch it's Apple's more precise "X works but only if you're using Y and Z" formatting between containers and codecs that cause the headaches. I just use AirPlay a lot for travel so I wanted to try and figure this out. As for this PR, I was operating with the assuming the existing Device Profiles were based on the player/software compatibility. Was I correct with that assumption?
I'm hoping to borrow an older iPad from a family member to test this as well. I'm purely just checking for VideoToolBox and seeing if the number of decoders for that device > 0. Seeing AV1 on iPhone XS, I'm certain it's both HW and SW. I'd like to think even this helps filter something out for the profiles but if it ends up not benefiting I can pull that part and just keep the enums with the original logic. My concern I want to test are examples just like this where there are technically software decoders for a codec but it's not quality enough for stable streaming. I agree with the static vs dynamic profiles. Primarily, I want to use the dynamic route to filter down the static route. Opposed to using the dynamic generation fully. If we end up finding this dynamic isSupported method doesn't return what we need it to, I'm 100% fine pulling that part out. I've personally seen some new successes but my range of devices is so small. I could very easily be missing something. |
Since there are some outstanding discussions about the decoder check, I've decided to remove that from this PR. Re-adding it would not be difficult but I think that warrants a lot more discussion. I realize that I should try and keep this as close to 1 change per PR as I can get so that item may get moved to another PR at a later date. I've updated the main post to reflect the final state of this PR. None of the Device Profiles should be functionally different vs Main. The primary difference is they now live in an enum for centralized editing and can be called from the VideoPlayerType. The Custom Profile changes are still present. |
This is quite impressive and it will take me a while to get through this. I have been thinking about what I've been wanting to get into the iOS 15 cut off and think this would be great to have soon. |
Yes! Just to be clear: above you said that this PR (maybe with the isSupported filter?) solved some issues with your AV1 and HEVC files. I want to make sure we are on the same page because you also say it now doesn't change the profiles functionally, and I haven't heard of any problems (save one with AV1 and opus that I have yet to investigate). If you could be more clear about the issues and how they were solved, that would be helpful - maybe create an issue if now unrelated to this PR? |
I can make a separate issue. With the isSupported logic removed, I am back to having this issue on the native player. I'll post the media details and maybe you can make some sense from it. I understand that Native Player doesn't support AV1 Apologies, I just double checked and Jellyfin does not attempt to transcode this file. Issue linked here with full media details: #1170. @holow29 Forcing it to transcode, it looks like the issue is Native Player doesn't want to play Opus? I don't have a lot of Opus files to test. That seems to correspond with your comment:
I think you have a lot more knowledge in this realm. Should Native AVKit be able to play Opus? Should we remove it from the Native Player profiles? |
Thanks for all the clarification.
It can play 2 ch Opus, but not 5.1 at least. I am not sure about 1 channel/mono, but I threw together a codecProfile that I hope will fix it. Let's move discussion of that to the issue you created - and maybe you can include that codecProfile in this PR if it works (I haven't tested). |
Sounds good. This PR should be static now. Everything should functionally mirror exactly what was there before with the addition of the custom profiles. Doing some digging, it does appear as though my playback issues seem to exclusively exist with Opus 5.1. As we figure out what that issue is, we can make that change on a different PR. |
f7e1b03
to
32f23ff
Compare
…w.swift fix merge
…ew change to use the Device Profile as a Transcoding Profile.
I've re-based on Main as of today (August 13th, 2024). All screenshots are up to date with the tvOS menu updates from #1163. Pending any review changes, this should be ready to merge! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great and I probably should have looked at this prior to working on my video player refactor 😅
Most comments are architectural but I think we can also mainstream this feature a bit. I commented on the issue instead so that the details are split from the idea.
Shared/Extensions/JellyfinAPI/DeviceProfile/DeviceProfile+CustomProfile.swift
Outdated
Show resolved
Hide resolved
AFAIK the values can be empty (meaning all allowed) - for exampe, right now, the |
I was testing this and it looks like @holow29 is right. Leaving a section blank appears to mean 'Anything Goes'. This includes the empty Container. That being said, I think requiring that the custom device profiles specify each component is probably safer. The current implementation on https://github.com/LePips/SwiftFin/tree/custom-device-builder requires that something exists for all profile components on creation but on edit they can be removed. Do we want this to be disabled on both creation & edit or available on both creation and edit? |
Thank you for testing that. I just didn't finish the validation on edit but can do that on iOS. |
@JPKribs My branch has been updated for validation on profile creation and editing on iOS, but not validation for replace + no profiles. I actually am unsure what to do in that case but will have to think about it. I briefly took a look at fixing tvOS however will be unable to look at this again until after this (US Labor Day) weekend. I have also made changes to the ordered selector view since I didn't take a deeper look on its creation. I think that for validation for tvOS that we will have to override the back press and then prompt an alert to:
At the same time, since tvOS is (long, long ... long) ways away, I'm okay to get this in for the static profile building refactor and iOS and have tvOS for later (keep and fix current changes in this PR/branch to get an MVP and polish later). I think I will have to take a look at the entire tvOS navigation situation so that we can reuse as much code as possible without creating other coordinators just for the platform. I know that just adds to the pile of stuff to do, but I think it would be better to get this moving forward into the iOS TestFlight and fix issues for both platforms if any are reported. |
@LePips If you have this implemented on iOS in a completed fashion, I would not be offended at all if you wanted to just use your branch to merge this in. I could open a new PR and base my on Main to implement this on tvOS. That way we get this into Swiftfin off your finished branch and I can work on the tvOS implementation of this separately. Let me know if that's what you want to do! |
If you haven't made many changes since I created my branch I would prefer if you merged mine into yours and have this PR go forward, there's a lot of conversation I don't want to get "lost" in a "Closed" PR. |
I had some tvOS work done but I'm happy to move that to another branch. Realistically, I want to take what you did for iOS and base anything new on that. Let me try that merge now! |
@LePips I have your changes merged in and tvOS implemented. tvOS basically mirrors iOS. Instead of swipe to delete I put it in a context menu: I think the only issue that I have on tvOS is that creating a profile validates and doesn't save until you click save but editing a profile let's you edit without saving on the way out. That's not a major issue and it's something I can work on in another PR. That way we can get this one out the door for testflight for iOS and I can address this on tvOS before we start getting tvOS updated again. Let me know if that works or if you want me to make sure that gets knocked out first! Edit:I've updated all of the screenshots on this PR to reflect its current state |
@JPKribs I had to fix the edit functionality and reverted the changes for I may have to make some more changes/cleanup, but this is probably in a good state to be merged soon. |
Sounds good! Thanks for all your help getting this over the finish line. Is there anything else from my end I can help do for this? |
I've made some final cleanup changes and will approve so that I can continue on my video manager refactoring. @holow29 In response to how the direct profile should work by omitting codec/container values, I haven't found that to be an issue in my testing. If it does however transcode when it shouldn't, that's easily fixable by just ignoring the transcoding URL from the server. Regarding about how technically a codec mixture might be reported to be supported but still has issues due to software decoding/other things, I wonder if it may be beneficial to "recommend" pre-built custom profiles dynamically based on device either in-app or through a documentation page on the repo. Lastly, I have a few thoughts left on this feature that I will leave on the issue feature request instead of here. Thank you to both of you for the work you have put into the entire device profile builder and its testing. |
Issue
#1168
Summary
The majority of this change refactors the existing Device Profile Logic. Previously, this logic was hard coded into two functions for Native Profiles and Swiftfin Profiles. Instead, I moved all the Video Player logic and moved it to the VideoPlayerTypes enum. There, the Player Specific logic is available and stored as a var based on the VideoPlayerTypes selection. This should give a centralized location for updating this in the future.
In addition to moving the logic from the 2 funcs data into the enum, I created enums for:
I replaced all of the string versions of this in the Device Profile with these enums instead. This should result in the same configuration as before. The enums should assist in ensuring that there are no misspellings and work as a validation that all codecs exist prior to their usage.
This PR also creates a new DeviceProfile Array that can either be added to the existing default DeviceProfiles or completely replace them. This allows a user to either add a more modern, unsupported Device Profile to enable more DirectPlay ability or they can completely remove all DirectPlay profiles to force more content to Transcode for older devices. There is an optional checkbox to utilize this this Device Profile as the Transcode profile as well.
In addition to this, I have added a compatibility mode that looks like this:
This results in everything being transcoded into the most compatible format.
I've combined this and Max Bitrate into a single Playback Quality settings page. Please find the Screenshots below.
Screenshots
iOS
Maximum Bitrate -> Playback Quality
Profile Management Page
Profile Editor Page
Media Component Selector
Example - Video:tvOS
Maximum Bitrate -> Playback Quality
Profile Management Page
Profile Editor Page
Media Component Selector
Example - Audio: