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

LinuxRendererGLES: enable HDR passthrough for HDR videos with incomplete(?) metadata info #24160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smp79
Copy link
Contributor

@smp79 smp79 commented Nov 29, 2023

Description

This fixes #8322. This also fixed HDR passthrough for all of my HDR videos that didn't switch my TV into HDR mode. Even the HLG sample now switches the TV to HLG HDR mode.

How has this been tested?

Tested on Intel TGL + Samsung 4K TV.

Types of change

  • [X ] Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

Copy link
Contributor

@lrusak lrusak left a comment

Choose a reason for hiding this comment

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

I

@@ -132,7 +132,6 @@ bool CLinuxRendererGLES::Configure(const VideoPicture &picture, float fps, unsig
// setup the background colour
m_clearColour = CServiceBroker::GetWinSystem()->UseLimitedColor() ? (16.0f / 0xff) : 0.0f;

if (picture.hasDisplayMetadata && picture.hasLightMetadata)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this "fixes" the problem. Allowing incomplete metadata may result in a worse image as the MaxFALL and MaxCLL provide info about the HDR content.

This means that if the encoding contains a valid HDR10 (pq) or HLG flag in the color_transfer then it can switch HDR mode on without any other metadata.

Simply seeing the HDR logo light up on the tv doesn't mean the picture is correct.

Ref:

Static metadata: SMPTE ST 2086 (mastering display color volume), MaxFALL (maximum frame-average light level), and MaxCLL (maximum content light level)

https://en.wikipedia.org/wiki/HDR10

And

MaxFALL/MaxCLL is static metadata required for HDR10 content. 

https://professionalsupport.dolby.com/s/article/Calculation-of-MaxFALL-and-MaxCLL-metadata?language=en_US

Copy link
Contributor Author

@smp79 smp79 Nov 29, 2023

Choose a reason for hiding this comment

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

It can't look worse than any of the tone mapping methods. IMO we'd still want to switch the TV to HDR/BT.2020/HLG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not to mention that DRM PRIME platforms, eg. RPI4 allow switching to HDR mode with those "incomplete metadata" videos. Why this should be different for Intel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even the TV's internal player would switch the TV to HDR mode with those videos. But on Intel PC they are tone mapped to SDR and look horrible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This video, for example, doesn't even trigger the tone mapping and it is absolutely unwatchable. With this PR it would switch the TV to HDR mode and looks perfectly fine, same as with TV's internal player.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not to mention that DRM PRIME platforms, eg. RPI4 allow switching to HDR mode with those "incomplete metadata" videos. Why this should be different for Intel?

If the behaviour is inconsistent across platforms we should check how other platforms and applications handle it.

If it's expected to be able to just ignore any metadata then so be it but we should investigate more about what the correct behaviour should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of HLG streams - they don't even use MaxFALL/MaxCLL so those metadata checks would never allow the TV to switch to HDR. IMO the correct behaviour is to copy the behaviour of TV's internal player which is allow switching to HDR mode without MaxFALL/MaxCLL metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also removed metadata checks for tonemapping. HLG streams and HDR10 videos with missing MaxFALL/MaxCLL didn't trigger tonemapping for SDR TVs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to suggest removing the check altogether if you hadn't done it already. For Windows, HDR mode is triggered by color_transfer + primaries only. Metadata, when present, is forwarded as is to the TV, it hasn't caused issues that I know of. But it's possible that Windows is interfering in the process.

HLG streams typically don't come with metadata and that's valid. Windows has special handling though because of missing HLG passthrough support, Instead it's converted to PQ with shader and metadata is made up with 1000nits max, value found in many places of HLG spec.

@smp79 smp79 force-pushed the gles-hdr branch 4 times, most recently from c00aad5 to fc04d6c Compare November 30, 2023 22:06
@quietvoid
Copy link
Contributor

It's very strange to me that the changes remove HDR10 metadata passthrough altogether.
Is it not possible to only set it conditionally?

@smp79
Copy link
Contributor Author

smp79 commented Dec 2, 2023

It's very strange to me that the changes remove HDR10 metadata passthrough altogether.

The changes only remove unneeded checks and the leftover code that was used only for those checks.

@ilikenwf
Copy link

ilikenwf commented Dec 16, 2023

I know it is a stretch but could this have anything to do with my issue, in which youtube HDR videos refuse to play until I disable HDR? I'm using an intel Arc card...though if I disable use of the Arc, it does play, albeit poorly.

@smp79
Copy link
Contributor Author

smp79 commented Dec 16, 2023

I know it is a stretch but could this have anything to do with my issue

All I can say is that with this PR VP9 HDR videos that I downloaded from youtube (like this one) would switch the TV to HDR mode. Without this PR they are tonemapped to SDR. Whether or not it has anything to do with your issue I have no idea.

@ilikenwf
Copy link

ilikenwf commented Dec 16, 2023

My current struggle seems more to potentially be either with buliding and running Kodi on Arch, or with my intel arc...

I ran Libreelec live and managed to get it to switch to HDR, for one video but then no others...lol not sure how to handle that especially since my goal is avoiding the use of the privacy unfriendly google tv install that my new bravia has.

I did a build with this patch and it didn't solve my issues, unfortunately.

#23957

@ilikenwf
Copy link

So youtube HDR playback is still fully broken for me (though I wonder if it's not broken for everyone and everyone just watches youtube on Kodi in SDR), but I found arch is packaging kodi with the gl instead of gles backend...it appears that some if not all vp9 videos, at least when in mkv containers, refuse to playback in HDR mode for me, as well...

@ilikenwf
Copy link

ilikenwf commented Jan 6, 2024

@smp79 you think this will ever get merged? I've been building with it locally for a while now, as it fixes all of my trouble with HDR save for issues with youtube and my Arc card...so for vp9 i have VAAPI disabled right now.

@ilikenwf
Copy link

Any thoughts on merging this?

@smp79
Copy link
Contributor Author

smp79 commented Mar 11, 2024

@smp79 you think this will ever get merged?

No idea.
IMO this is a proper fix. We should not disable HDR passthrough if the video does not have MaxFALL/MaxCLL present. This is especially relevant for HLG DVB streams since HLG does not use MaxFALL/MaxCLL by design.

@ilikenwf
Copy link

@smp79 you think this will ever get merged?

No idea. IMO this is a proper fix. We should not disable HDR passthrough if the video does not have MaxFALL/MaxCLL present. This is especially relevant for HLG DVB streams since HLG does not use MaxFALL/MaxCLL by design.

Agree 100%.

@harment
Copy link

harment commented Mar 16, 2024

can add this patch for kodi20 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Intel NUC 13 Pro doesn't switch to HDR with my Sony A95L[BUG]
6 participants