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

Limit transcoding profiles with maximum resolution #3343

Merged
merged 3 commits into from Mar 25, 2022

Conversation

dmitrylyzo
Copy link
Contributor

@dmitrylyzo dmitrylyzo commented Jan 16, 2022

Save some CPU cycles and network bandwidth when transcoding 🤞

Changes

  • Limit transcoding profiles with maximum resolution
  • Limit quality settings with maximum resolution
    ⚠️ If the resolution is in between FHD and UHD, the 4K options are disabled.
    Allow next higher resolution

⚠️ Google Cast needs testing.

Requires jellyfin/jellyfin#7197 and jellyfin/jellyfin#7198

@joshuaboniface joshuaboniface added this to Active PRs in Release 10.8.0 via automation Mar 10, 2022
@thornbill thornbill added the backend Requires work on the server to finish label Mar 10, 2022
@dmitrylyzo dmitrylyzo marked this pull request as ready for review March 12, 2022 17:30
@dmitrylyzo dmitrylyzo force-pushed the transcode-resolution branch 4 times, most recently from c2d2cf4 to 55c7ff7 Compare March 15, 2022 18:12
@dmitrylyzo
Copy link
Contributor Author

dmitrylyzo commented Mar 15, 2022

The current implementation requires a dependency loop (appHost -> browserDeviceProfile -> appHost) to enable the limit in apps (except TVs) without any modification.

Alternatively, we can post-process the profile: dmitrylyzo@e502200
As a bonus, this will allow to add the limit to a profile that was not generated by browserDeviceProfile.

@thornbill
Copy link
Member

@dmitrylyzo Could the max width be passed to the profile builder as an "option"? https://github.com/jellyfin/jellyfin-web/blob/master/src/components/apphost.js#L38-L39

@dmitrylyzo
Copy link
Contributor Author

@dmitrylyzo Could the max width be passed to the profile builder as an "option"? https://github.com/jellyfin/jellyfin-web/blob/master/src/components/apphost.js#L38-L39

Yes, it is possible (it was my 2nd option), but in this case the resolution limiting will only work in the browser - apps will need to update their getDeviceProfile.

The 3rd option (in the comment above) allows post-processing of any profile, but the app has no options to control that (not a problem to add though).

@thornbill
Copy link
Member

👍 I think it's reasonable to expect clients that provide their own profiles to also be responsible for handling the resolution limits. In theory they should be able to determine resolution more accurately anyway.

@dmitrylyzo
Copy link
Contributor Author

dmitrylyzo commented Mar 17, 2022

In theory they should be able to determine resolution more accurately anyway.

There is a new AppHost.screen function for this (can be overrided). It is used for the profile and the list of quality options.

As for options, I think we should leave the next higher resolution, i.e. if the screen width greater than 1930 (+10 for sure) add 4K options.

We also need to support portrait oriented videos, but we currently lock the screen in landscape.

@Shadowghost
Copy link
Contributor

Why exactly do we allow the next bigger resolution too? IMHO exactly that should not be done because afaik it leads to Auto choosing 4K when transcoding is required on a local network without bandwith restriction.

@dmitrylyzo
Copy link
Contributor Author

Why exactly do we allow the next bigger resolution too? IMHO exactly that should not be done because afaik it leads to Auto choosing 4K when transcoding is required on a local network without bandwith restriction.

Quality options don't affect the transcoding resolution limit. The next bigger resolution is for:

If the resolution is in between FHD and UHD, the 4K options are disabled.

For example, you have 2K display, but only 1080p is allowed.

@dmitrylyzo
Copy link
Contributor Author

Personally, I lean toward the post-processing version:

  • to eliminate the dependency loop.
  • to re-use the AppHost.screen function instead of duplicating it (and the limiting logic) in apps.
  • to add a resolution limit to webui apps "out of the box".
  • to fix/trim/post-process the profiles even if they were not generated by browserDeviceProfile.

@sonarcloud
Copy link

sonarcloud bot commented Mar 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Release 10.8.0 automation moved this from Active PRs to Approved PRs Mar 23, 2022
@joshuaboniface joshuaboniface merged commit a993a80 into jellyfin:master Mar 25, 2022
Release 10.8.0 automation moved this from Approved PRs to Completed PRs Mar 25, 2022
@dmitrylyzo dmitrylyzo deleted the transcode-resolution branch March 26, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Requires work on the server to finish
Projects
No open projects
Release 10.8.0
  
Completed PRs
Development

Successfully merging this pull request may close these issues.

None yet

4 participants