jellyfin-web
jellyfin-web copied to clipboard
Adding DOVI support on webOS
-- resubmitting fresh/clean PR --
This change will block non-mp4 containers from direct playing DOVI media on webOS browsers with DOVI support. This, paired with the "Prefer fMP4-HLS container" client playback setting will enable DOVI Profile 5 playback on webOS.
For DOVI Profile 8 (dovi with hdr10/hlg fallback), I think we will require some changes in the server side or in ffprobe. Please see jellyfin/jellyfin#10468#issuecomment-1780029285
Changes Where browser is webOS and has DOVI support, push a CodecProfile that says containers other than mp4 cannot direct play DOVI.
Issues jellyfin/jellyfin-webos#159
-- I've incorporated the latest review comments from dmitrylyzo from the old PR in this one, and happy to know your thoughts here. Should we wait for clarity on dovi-8 related issues before merging this?
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells
No Coverage information
0.0% Duplication
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells
No Coverage information
0.0% Duplication
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.
I decided to try this today - I have an android player and don't need it but I know some people on webos are struggling.
I can't confirm it's working for sure. What I can say is that when playing mkv with DV and HDR layer, it'll play the HDR straight from mkv.
I would expect this to play the DV layer as a preference always. Maybe if that's not covered in this PR, it should be.
@Schaka, this PR enables playback of DV media that does not have HDR fallback (DV profile 5) by forcing a transmux, like you describe in the recently linked issue. For profile 8, things have to change on the server side (please see linked issue in OP).
@dmitrylyzo hey, I think this PR can be merged in it's current state. While it does not fully enable all dolby vision playback, it will resolve the green-purple issue which still helps a considerable chunk of users.
If a fix for full support is implemented in server side without introducing new videorangetypes, there will be no further changes required in web. If new videorangetypes are introduced in server (like in https://github.com/jellyfin/jellyfin/pull/10469), they will have to be added to the below lines https://github.com/jellyfin/jellyfin-web/blob/713ac0e130bfd452debf1c2a1b25a64b5bcaa01f/src/scripts/browserDeviceProfile.js#L930 https://github.com/jellyfin/jellyfin-web/blob/713ac0e130bfd452debf1c2a1b25a64b5bcaa01f/src/scripts/browserDeviceProfile.js#L936
In any case, the changes tagged to this PR are good to go.
Do you have a sample DV5 file I could use to test this? If you guys still need confirmation, that is. On the I'm on the C1.
There is one here https://kodi.wiki/view/Samples Dolby Vision Mystery Box (Profile 5) Remember to enable fMP4-HLS in playback settings!
I tested this PR with the DV5 (in mkv container) and it works perfectly, just transmuxing as expected.
If I wanted to hackfix DV playback for profile 7 and 8, I'd have to remove HDR, HDR10 and HLG from mkv, so that it would force transmux to mp4 as well?
Edit: I actually tried. It will play the HDR10 or HDR fallback when transmuxing to mp4, if I adjust the browserProfile for -mp4
(non-mp4 as I understand) to only accept SDR.
Am I correct in understanding that as of now, transmuxing P7 or P8 loses DV information and thus will only play back HDR/HLG/whatever fallback?
Am I correct in understanding that as of now, transmuxing P7 or P8 loses DV information and thus will only play back HDR/HLG/whatever fallback?
With jellyfin-ffmpeg5, I am able to remux to MP4 keeping DoVi8. So I suppose the DV info should remain when remuxing.
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.
I'll try to have a look here tomorrow, see if I can help out somehow.
Just in case. This PR can be considered as "feature complete" - no need to add more.
My suggestion of using NotEquals
is not really necessary, but it might be useful when more video ranges are added in the future.
On the other hand, it doesn't work if multiple video ranges are required - need another negation condition that supports a list of values.
DOVI+HDR10/HLG should be implemented in the new PR as it requires changes to the server.
Awesome. @GeorgeH005, do you want to take that one up?
@viktory36 Will try to, I will link the pull request here when I open one. Would appreciate your help, given your previous work on the web codebase.
No problem - we will just need the new types added in server also added here: https://github.com/jellyfin/jellyfin-web/blob/713ac0e130bfd452debf1c2a1b25a64b5bcaa01f/src/scripts/browserDeviceProfile.js#L930 https://github.com/jellyfin/jellyfin-web/blob/713ac0e130bfd452debf1c2a1b25a64b5bcaa01f/src/scripts/browserDeviceProfile.js#L936
So we will have 3 prs total. This one with no more changes, another one needed in jellyfin-web that deals with with DOVI+HDR10/HLG implementation that modifies above lines + others if necessary, and the one in server that introduces the new types.
With these 3 PRs merged, we will end up with full DOVI support on WebOS, and all other clients will now see "DOVIWithHDR10" in playback info instead of just "HDR10" when they're playing DOVI media.
No problem - we will just need the new types added in server also added here:
https://github.com/jellyfin/jellyfin-web/blob/713ac0e130bfd452debf1c2a1b25a64b5bcaa01f/src/scripts/browserDeviceProfile.js#L930
https://github.com/jellyfin/jellyfin-web/blob/713ac0e130bfd452debf1c2a1b25a64b5bcaa01f/src/scripts/browserDeviceProfile.js#L936
My thoughts exactly! I was waiting in order to make sure I wasn't missing anything else that needed changing (frankly I am still checking, but so far so good).
Here is the relevant pull request: jellyfin/jellyfin-web#5282
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.
Shouldn't this have already been merged? I will have to make changes to the types mentioned here in #5282, and I suppose it would be better to make them after this has been merged, rather than here directly
The PR currently has merge conflicts. I believe it should be good to go once those are resolved.
@viktory36 Rebase please?
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
No data about Coverage
0.3% Duplication on New Code
@dmitrylyzo @thornbill resolved the conflict. Merge, please?
wow this would be great ! would this work on 10.9 then?
@iwatch-x Probably in the beta following its merging.
Cloudflare Pages deployment
Latest commit | 3659355 |
---|---|
Status | ✅ Deployed! |
Preview URL | https://8325b07a.jellyfin-web.pages.dev |
Type | 🔀 Preview |