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

Add stricter checks for DolbyVision in HEVC #5184

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

nyanmisaka
Copy link
Member

Changes

  • Add stricter checks for DolbyVision in HEVC

Using ios and osx version checking is not reliable. Also Apple has adopted AV1 but hasn't enabled it in HLS yet, so we may need to add checks for DolbyVision in AV1 in the future.

@nyanmisaka nyanmisaka requested a review from a team as a code owner February 6, 2024 14:25
Copy link

sonarqubecloud bot commented Feb 7, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@thornbill thornbill added this to the v10.9.0 milestone Feb 7, 2024
@thornbill thornbill added the enhancement Improve existing functionality or small fixes label Feb 7, 2024
@thornbill thornbill enabled auto-merge February 7, 2024 13:43
@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit 6de6f44
Status ✅ Deployed!
Preview URL https://f75c9fa9.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs
View bot logs

@thornbill thornbill merged commit 33d9cfd into jellyfin:master Feb 7, 2024
20 of 21 checks passed
@@ -930,7 +938,7 @@ export default function (options) {
av1VideoRangeTypes += '|HLG';
}

if (supportsDolbyVision(options)) {
if (supportsDolbyVision(options) && canPlayDolbyVisionHevc(videoTestElement)) {
Copy link
Contributor

@dmitrylyzo dmitrylyzo Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iirc, canPlayType doesn't always work correctly on some platforms. So we probably shouldn't call canPlayDolbyVisionHevc on Tizen(it doesn't support DV) and webOS.

We need someone to test this on real webOS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC Samsung only seems to be focused on promoting their HDR10 Plus, if they enable DolbyVision in the future we could add it back.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some discussion about LG TV that they do have DolbyVision working.

And the mp4box of properly encoded progressive and fMP4 HLS streams should have similar metadata for the player to recognize and trigger DolbyVision.

HLS.js currently does not seem to be able to correctly handle the relationship between dvh1 and hvc1, and will fail if WebOS does not support native HLS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some discussion about LG TV that they do have DolbyVision working.

My concern is that canPlayType is not reliable. If it works properly on modern webOS (older ones hardly support DV), then no problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how the TV client utilizes this boolean options.supportsDolbyVision? Is there a button I didn't notice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, just chiming in to confirm this works fine on my 2022 webos.

@nyanmisaka nyanmisaka deleted the strict-dovi-hevc branch February 7, 2024 13:56
@legosoff
Copy link

legosoff commented Mar 29, 2024

@Shadowghost
Copy link
Contributor

This change so currently not available in stable branch and therefore not in the We is app.
It will be part of 10.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality or small fixes
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants