Align Device Profiles with what server expects - #5711
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
nielsvanvelzen
left a comment
nielsvanvelzen
left a comment
There was a problem hiding this comment.
Initial review, because you changed actual values and the way it's written it became a lot harder to read the diff so there will be more review passes to come.
| fun createDeviceProfile( | ||
| context: Context, | ||
| userPreferences: UserPreferences, | ||
| serverVersion: ServerVersion, |
There was a problem hiding this comment.
We deliberately kept the serverVersion parameter because we may need it again with a future server release, just like how we needed it to support both 10.10 and 10.11 at the same time for the 0.19 release branch.
Removing the context is probably fine.
| val supportsHevcDolbyVisionEL = mediaTest.supportsHevcDolbyVisionEL() | ||
| val supportsHevcHDR10 = mediaTest.supportsHevcHDR10() | ||
| val supportsHevcHDR10Plus = mediaTest.supportsHevcHDR10Plus() | ||
| val hevcDoviHdr10PlusBug = KnownDefects.hevcDoviHdr10PlusBug |
There was a problem hiding this comment.
No need to assign a variable to another variable really..
There was a problem hiding this comment.
Was more for consistency than anything else, I'll remove it and directly check the flag
| if (supportsAV1) Codec.Video.AV1 else null, | ||
| if (supportsVP9) Codec.Video.VP9 else null, |
There was a problem hiding this comment.
We deliberately do not support transcoding profiles for AV1 and VP9 as we've been told multiple times by the ffmpeg team that this is extremely inefficient and shouldn't be done.
There was a problem hiding this comment.
At least for AV1 it's a sever side setting anyway so shouldn't it be fine to keep? And for which cases did the ffmpeg decs say it is inefficient (are we talking server or client here?)
There was a problem hiding this comment.
From my understanding the problem is that the server will often not hit 1x speed when transcoding to av1/vp9 and that would be the main reason to discourage it. But I'm not aware of the specifics, just know I was told to not use it.
| } | ||
|
|
||
| // Little helper function to more easily define video codec profiles | ||
| private fun DeviceProfileBuilder.videoCodecProfile( |
There was a problem hiding this comment.
This helper function really has little gain, we already have a proper DSL in place
| protocol = MediaStreamProtocol.HLS | ||
|
|
||
| videoCodec(*hlsVideoCodecs) | ||
| videoCodec(*hlsFmp4VideoCodecs) |
3 participants
There are some issues with the announced device profiles.
Changes
Code assistance
Used Claude Opus 4.8 to get an idea of how the logic works and do some refactoring.
Issues
Fixes jellyfin/jellyfin#17444