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
[Android] Adjust GUI SRD peak luminance when display is in HDR PQ mode #24756
Conversation
666e740
to
1348873
Compare
I can't say I'm too happy about this approach. It will result in heavy banding, having just ~7 bits (?) per color channel. Also, it introduces colour inaccuracies. It might also lengthen the execution time of the shaders, depending on the platform. Pretty much every SOC with HDR support comes with its dedicated mapping hardware, so that such hacks are not needed. For devices with lacking drivers, a slightly more reasonable approach would be a proper resolve pass at the end, preferably using pixel local storage or a framebuffer fetch, I think. |
Run some tests and it's not working for some Dolby Vision videos. Working:
Not working:
|
@thexai @Hitcher I also tested some files in Dolby Vision format. It seems this feature only works for DV files where mediainfo shows that the HDR format is HDR10 or HDR10+ compatible:
As soon as the file only shows Dolby Vision without HDR10 compatibility, it won't work:
|
1348873
to
01a19a3
Compare
Fixed not working with some Dolby Vision profiles |
@sarbes The most important thing here is to adjust the brightness intensity in OSD. The fact that there may be banding in the subtitles, which are generally solid white, or playback status bar I don't think is a problem. Keep in mind that when HDR10 playback ends and is returned to the GUI, this is disabled. |
01a19a3
to
f206468
Compare
Confirmed working, thanks. |
Why not implement the mapping correctly in the first place? At least the part we can control is worth to get right, isn't it? Then we only have the quantization loss. |
f206468
to
9915086
Compare
653e124
to
7287d17
Compare
I'm sorry, but I have to object merging the PR in this state, especially since you don't want to work on it in the future. The current execution is wrong, the texture shaders are not the right place for adjusting the intensity. Gamma correct scaling has to be done in a post processing pass, if you want to have any resemblance of color correctness. Such an implementation would be of similar scale. And the code complexity would be less, as it would be much cleaner overall. I want to add such a pass in order to have a HDR UI some day. I can offer you to implement a basic version which deals with the full- to limited-range conversion. From there, you could easily append your brightness mapping. If you like, we can talk about it on our next team call. |
Can you be more specific? What is bad? and for you what is "correct mapping" (suggested changes, etc.)
How a single float multiplication in shader code can slowdown execution?
8-bit SDR is 256 steps and in shaders is 0.0 to 1.0 range 10-bit HDR is 1024 steps and in shaders is same 0.0 to 1.0 range Even using only half of scale (0.5) there are 512 steps (double precise that SDR) With default setting 40% is used 0.0 to 0.58 range 1024 x 0.58 = 593 steps Where is quantization loss? And why 593 steps are not sufficient to render Kodi GUI that is SDR in origin (256 steps). I am not a shaders expert and you know much more about this, but I consider these to be very basic concepts although I may be wrong about some things and have misconceptions. |
I have a normal job of 9 hours a day and night shifts. I can't spend a lot of hours on this. |
Why can't we go with this PR and improve / replace this code once @sarbes (or somebody else) comes up with something real (a PR)? I tend to prefer to have something not 100% now (!) instead of getting something "perfect" in the future. What speaks against an iterative approach? Just my 2ct. |
Because I would need to rip out half of the code. It has no place being there (the texture shaders). The alternative I'm proposing would be much simpler for @thexai to implement (like 6 LOC). I don't want to discourage you. I think it is a bit unfortunate that we have not talked about it before you did this PR. |
I am not your employee (or anything like that). Here everyone works on what they like/interest. If you were interested in this (which you are not) you would have already implemented it. https://forum.kodi.tv/showthread.php?tid=376360&pid=3185207#pid3185207 |
See above.
The Mali-400 series can do one MAD per cycle. In the worst case, you would double the amount of cycles needed. Similar for VideoCore GPUs. In case of your PR, you pay the cycle cost per fragment. In case of a separate pass, you pay the cost only once.
You need to remember that the framebuffer is just RGBA8.
Me too, I can relate to that. |
7287d17
to
b6cc5eb
Compare
Rebased only, no code changes. |
xbmc/cores/VideoPlayer/VideoRenderers/HwDecRender/RendererMediaCodecSurface.cpp
Show resolved
Hide resolved
I think the luminance mapping is good enough because the multiplier is calculated using a linear equation in CWinSystemAndroid::GetGuiSdrPeakLuminance() and the multiplied data is in PQ, so it's perceptually linear. Unlike Windows where the multiplication is in linear space so the curve of the multiplier is exponential for a similar effect. Is the fact that the primaries are converted by Android reliable? Even if not I guess not being blinded is better that inaccurate colors. Doing custom conversion to be certain would obviously be much more expensive for the GPU (similar to the Windows shader code). |
Colors are totally correct (at least on Shield). |
I'm pretty vocal about my ideas and changes in our Slack, I would say. I also have quite a bunch of them written down as a project (https://github.com/orgs/xbmc/projects/5). But unfortunately, it seems that I'm mostly alone in both conversations. Aside from my current PRs, I do have quite a bunch of uncommitted code, which is waiting of the approval of the former. I'm always available to talk about changes falling in the GUI/GL/GLES domain.
Those are assumptions on your side. My assumption of you is that you don't grasp the base of my objection.
Make no mistake, this is not a fix but a workaround for broken devices/software. My "complaint" was never about improving the user experience of this release, but the execution. And I don't think that a lot of people are qualified to have remarks about the latter. So naturally, the pool of such people (thus the amount of "complaints") is small compared to our large and rather vocal user base.
It could be incorporated into 21. But we don't have many reviewers and it might be considered to big of a change for the beta. Personally, I'd be willing to risk it.
You have my vote as well, if you would consider working on it once the post processing pass gets merged. I don't have the setup and the time needed to qualify changes. My main objection was (is) mainly about the idea that I would need to take ownership about something I can't test. |
While I don't have a suitable setup to test, I would expect it to be right (for most systems). |
I will work on it keeping in mind that GL/GLES is not my area and I also have limited time. I will try to do the best I can. |
That's a statement I can perfectly live with. I'll approve the PR from the the GLES side of things. It's probably best to get the green light from an Android guy and the release folks before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the GLES changes.
I noticed the brightness adjustment is activated depending on content instead of display mode. That works as long as the Fire TV option "Dynamic range settings" is set to "Adaptive". With "Dynamic range settings" set to "Always HDR", Kodi UI is very bright until a HDR video is started. After stopping the video and returning to Kodi UI the full brightness is back. |
That's the idea. The adjustment is just for playback. Kodi has no HDR mode (or similar) for the UI. |
Yeah, you need to turn off 'Always HDR'. |
"Always HDR" seems a feature of Fire TV only: We could certainly add one more condition to activate "GUI in PQ mode" as long as Fire TV / Android provides an API to detect that this optional mode is active. In any case this does not need to be done in this PR as is "Fire TV" specific setting. |
That setting is present on the Chromecast as well: "Always DV/HDR"(depending on what another pref is set to) and "Match content". That device skipped Android 11, so I can't confirm whether it was implemented in v11 or v12, but I presume it's on all devices with clean(ish) Android v12+. However, the behavior outlined by @hugbug is par for the course on any and all apps with that setting turned on, so I don't think it needs to be addressed by Kodi, but rather by Android itself, which is a whole nother can of worms. All that beeing said, I would like to thank you for taking this on and hope that this gets merged in v21 as it's an absolute godsend. I've made a feature request for a separate subtitle color in HDR mode quite a while back, but this is the proper way to deal with all these issus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning was already cited (anonymously ;-))
@thexai Am I correct in assuming that the option does not work for playing DV streams through Input Stream Adaptive, such as with the Disney+ addon? |
No idea, but should work if video is currently played as Dolby Vision on TV: i.e. DV metadata is passed to MediaCodecSurface and DV capable display is identified inside Kodi settings information. |
Strange. I played an episode of Secret Invasion through the Disney+ addon and my TV definitely switched to DV mode. But the OSD and subtitles stayed on the SDR brightness level. Playing local DV content in MP4 or MKV format from my NAS works fine though. |
I can confirm the same behaviour playing a DV video in the D+ addon. |
Add-on thing will be investigated later. May be necessary modify something in The first step to continue working in this is merge this PR now... |
@thexai how hard would it be to add this to Linux as well? I use HDR with Kodi on Linux, my own builds, and can see how this would be a nice feature so as not to burn my eyes. |
I would appreciate this coming to Linux on x86_64 as well! |
Description
[Android] Adjust GUI SRD peak luminance when display is in HDR PQ mode
This the equivalent functionality of #18984 and #21973 for Android.
Motivation and context
This is requested a lot on the forums....
https://forum.kodi.tv/showthread.php?tid=376360
https://forum.kodi.tv/showthread.php?tid=357205
https://www.reddit.com/r/kodi/comments/1awb6h3/osd_hdr_too_bright_during_playback/
With this is not necessary set subtitles color dark grey or different color for SDR / HDR and also is adjusted OSD and all GUI elements.
Android already does basic tone mapping - there is no need for full tone mapping from BT.709 to BT.2020 or gamma to PQ transfer. Only is need to set the SDR peak to 100 or 200 nits because by default it seems to be set to 1000 nits (at least in Shiled) and this is why it looks much brighter than normal SDR.
When SDR peak is set to 100 - 200 nits then GUI has same appearance than in SDR and GUI white is regular SDR white.
The default setting is 40% and matches what we already have in Windows. That is, the result is consistent with Windows using the same setting value. Adjusting GUI setting to 100% gives you the maximum brightness (~1000 nits), which is the same as before this PR.
How has this been tested?
Runtime Shield Pro 2019
Forum users:
What is the effect on users?
No more tricks to adjust brightness of subtitles / OSD / GUI when playing HDR10 or Dolby Vision contents.
Screenshots (if appropriate):
Types of change
Checklist: