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

[Android] Fix HDR static metadata primaries order #19852

Merged
merged 1 commit into from Jun 13, 2021

Conversation

arthur-liberman
Copy link
Contributor

@arthur-liberman arthur-liberman commented Jun 5, 2021

Description

Fix the order of the primaries coordinates in the HDR static metadata in Android

Motivation and context

The primaries coordinates of the HDR static metadata in Android are wrong

How has this been tested?

Can't test it, but the new order fits when looking at existing kernel outputs. (See screenshots)
In addition, this fix fits with the FFmpeg and google documentation of the metadata structure.

What is the effect on users?

Fixes incorrect colors in HDR content in Android.

Screenshots (if appropriate):

image

  • Note that the coordinates in the pdata->config output are in the wrong positions.

Types of change

  • 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)
  • 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

@arthur-liberman
Copy link
Contributor Author

@peak3d

@fuzzard fuzzard added this to the N* 20.0 Alpha 1 milestone Jun 6, 2021
@fuzzard fuzzard requested a review from peak3d June 6, 2021 00:07
@fuzzard fuzzard added Platform: Android v20 Nexus Type: Fix non-breaking change which fixes an issue labels Jun 6, 2021
@CastagnaIT
Copy link
Collaborator

CastagnaIT commented Jun 6, 2021

could this problem be the cause of incorrect colours with DolbyVision with some devices?
xbmc/inputstream.adaptive#557

@arthur-liberman
Copy link
Contributor Author

Sadly I'm not really familiar with how DV works. But I've seen that incorrect primaries can lead to the image looking not-quite-right even in "plain" HDR10.
I would say that for someone who has a way to build Kodi for Android and test it, should be quite an easy check.

@arthur-liberman
Copy link
Contributor Author

arthur-liberman commented Jun 6, 2021

@CastagnaIT If I understand the jenkins process correctly, you can grab
https://jenkins.kodi.tv/job/Android-ARM64/18596/artifact/kodi-20210605-39921858-PR19852-merge-arm64-v8a.apk
or
https://jenkins.kodi.tv/job/Android-ARM/21643/artifact/kodi-20210605-39921858-PR19852-merge-armeabi-v7a.apk
depending on your device, sideload it and see if it fixes the issue.

@CastagnaIT
Copy link
Collaborator

@CiNcH83 have you time to test if this PR resolve your DV color problem?
download apk above ^^

@CiNcH83
Copy link

CiNcH83 commented Jun 7, 2021

@CastagnaIT

Yes. I already have it on my TODO list. Will hopefully be able to check today.

@CiNcH83
Copy link

CiNcH83 commented Jun 7, 2021

Unfortunately can't find the build artifact. The above link does not work.

@CastagnaIT
Copy link
Collaborator

maybe @fuzzard can make start the build of android artifact

@arthur-liberman
Copy link
Contributor Author

jenkins build this please

@arthur-liberman
Copy link
Contributor Author

arthur-liberman commented Jun 7, 2021

One more thing we found here is that some content does not have a valid content light metadata. In such case, if I understand the way things work in Android, the code will not trigger HDR at all.

And now that I think of it, this line is probably incorrect as well:

data[9] = static_cast<short>(av_q2d(m_hints.masteringMetadata->min_luminance) + 0.5);

Because the minimum luminance in content usually ranges in the 0.00x range. Adding 0.5 to 0.005 and casting the result to short, which is an integer value, will result in a minimum luminance of 0.
If we are to check the CTA+861.3-A standard, we see that it is also quite possible that using 5,000 for max chromaticity is also incorrect. The document states that the 2-byte (short) values for primaries + white point are coded in units of 0.00002, which translates to 50,000 if you perform the following operation: 1/0.00002.
Sadly, I don't have any way of reading the metadata output by the actual device, so it's a little difficult for me to verify any of this. Also, I only have Amlogic devices, and after looking at the kernel code used by Android on those devices, this code does not work correctly with it anyway.
I will keep digging, maybe I will come up with something useful.

Edit: The Amlogic kernel can dump the metadata it's currently using into dmesg. This is what I see as the output when playing an HEVC video, where the metadata is extracted from the content, and not passed the same way as it is for VP9. And I'm pretty sure I'm correct about the max chromaticity multiplier. It should 50,000.

[12108.078837@2] hdmitx: primaries:
[12108.078838@2] hdmitx: 13250, 
[12108.078839@2] hdmitx: 34500, 
[12108.078840@2] hdmitx: 
[12108.078841@2] hdmitx: 7500, 
[12108.078842@2] hdmitx: 3000, 
[12108.078843@2] hdmitx: 
[12108.078844@2] hdmitx: 34000, 
[12108.078845@2] hdmitx: 16000, 
[12108.078846@2] hdmitx: 
[12108.078847@2] hdmitx: white_point: 
[12108.078848@2] hdmitx: 15635, 
[12108.078849@2] hdmitx: 16450, 
[12108.078849@2] hdmitx: 
[12108.078850@2] hdmitx: luminance: 
[12108.078851@2] hdmitx: 1000, 
[12108.078852@2] hdmitx: 1, 
[12108.078853@2] hdmitx: 
[12108.078854@2] hdmitx: max_content: 0, 
[12108.078855@2] hdmitx: max_frame_average: 0

@arthur-liberman
Copy link
Contributor Author

I pushed a couple extra fixes. Please review, and try the resulting binaries to see if there's any improvement.
Once we finalize things, we can squash the commits.
It would be very useful to have someone with a HDFury Vertex or something similar that can read the metadata from the HDMI signal of a player to verify this type of changes.

@CiNcH83
Copy link

CiNcH83 commented Jun 8, 2021

I downloaded the latest build from Jenkins. Will test this afternoon.

@CiNcH83
Copy link

CiNcH83 commented Jun 8, 2021

Purple tinting with Dolby Vision unfortunately is still present on BRAVIA.

@kszaq
Copy link
Contributor

kszaq commented Jun 8, 2021

DV does not use static metadata, #19099 might help.

@CiNcH83
Copy link

CiNcH83 commented Jun 8, 2021

#19099 might help

Won't help either. I am using Inputstream Adaptive to play Netflix which doesn't use ffmpeg. Plus my BRAVIA does not use the MediaTek chipset to decode DV (OMX.MTK) but the Sony FPGA. The correct decoder is being instantiated (OMX.dolby.vision.dvhe.stn.decoder.secure).

@arthur-liberman
Copy link
Contributor Author

I didn't really expect this PR to fix anything with DV, although it would've been neat if it has.

@arthur-liberman
Copy link
Contributor Author

I set up a build env for Android and was able to test this PR.

  • Primaries are now correct.
  • Min luminance is now correct.
  • Content without CLL metadata now triggers HDR and plays correctly.

@fritsch
Copy link
Member

fritsch commented Jun 12, 2021

So then, please make Jenkins happy and final review can happen. Thank you very much.

@arthur-liberman
Copy link
Contributor Author

Done

@arthur-liberman arthur-liberman force-pushed the android-fix-hdr-primaries branch 2 times, most recently from 44dd6f9 to b6afe92 Compare June 12, 2021 14:37
@arthur-liberman
Copy link
Contributor Author

Should I squash all of the commits?

@fritsch
Copy link
Member

fritsch commented Jun 13, 2021

Yes, please.

Fix primaries order and chromaticity, fix minimum display luminance
Populate HDR static metadata even when CLL is missing
Use 'unsigned short' instead of 'short'.
It will make the data readable in case of debugging or logging
@arthur-liberman
Copy link
Contributor Author

Ok, done

@fritsch
Copy link
Member

fritsch commented Jun 13, 2021

Thanks again. I or anybody else coming by can ousnfrom my side after Jenkins is happy.

@arthur-liberman
Copy link
Contributor Author

I see that the Android builds failed, is this just Jenkins acting up or did I break something?

@fuzzard
Copy link
Contributor

fuzzard commented Jun 13, 2021

Jenkins playing up, close and reopen the PR usually fixes it.

@fritsch fritsch merged commit 2ce666e into xbmc:master Jun 13, 2021
@arthur-liberman
Copy link
Contributor Author

I also think it's worth to backport it Matrix, once it's verified by others as not causing any unexpected issues.

@CiNcH83
Copy link

CiNcH83 commented Jun 13, 2021

I played several HDR10 and HLG HEVC and VP9.2 files. All fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: Android Type: Fix non-breaking change which fixes an issue v20 Nexus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants