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] Replace enum HDRTypes with Display.HdrCapabilities constants #23769

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

joseluismarti
Copy link
Contributor

Description

Avoid using the value of the constants directly in the code.

Depends on xbmc/libandroidjni#48

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

@quietvoid
Copy link
Contributor

quietvoid commented Sep 15, 2023

Is the SDK version dependent on the device's Android? Or is it something set at compile time depending on the SDK used when building?

If it's dependent on the device, then I think this could break HDR10+ detection on some devices.

@joseluismarti
Copy link
Contributor Author

The availability of methods and constants depend on the API level of the device.

HDR_TYPE_HDR10_PLUS constant was added at level 29.

@quietvoid
Copy link
Contributor

quietvoid commented Sep 15, 2023

The availability of methods and constants depend on the API level of the device.

HDR_TYPE_HDR10_PLUS constant was added at level 29.

Okay.
Here my Amazon FireTV Stick 4K Max says API level 28, yet it supports HDR10+.
I think then that the changes would make it not detect properly.

I should try testing on an actual HDR10+ display.

@joseluismarti
Copy link
Contributor Author

Please check in that device in System info > Video the value of Display Supported HDR types

@quietvoid
Copy link
Contributor

quietvoid commented Sep 15, 2023

Alright, connected to a HDR10+ display, I don't think it showed HDR10+ (for some reason I don't remember exactly, maybe I should look again).
But the log says:

debug <general>: CAndroidUtils: Display supported HDR types: Unknown Unknown

On Alpha 3 build:

debug <general>: CAndroidUtils: Display supported HDR types: HDR10 HLG

So even the current code doesn't seem to detect HDR10+.
In the device settings, the video diagnostics do show HDR10+/HLG.

And on Dolby Vision display:

 debug <general>: CAndroidUtils: Display supported HDR types: Unknown Unknown Unknown

But the system info does show properly all of them: HDR10, Dolby Vision, HLG.
So clearly the logging is broken with the changes, at least.

@joseluismarti
Copy link
Contributor Author

As expected, the getHdrCapabilities() method, both with the previous code and with this PR, does not return the HDR10+ type because it's a device with API level < 29.

This PR should not affect the detection of supported HDR types.

The Amazon FireTV Stick 4K Max device will have some codec tweaks to play HDR10+.

@quietvoid
Copy link
Contributor

quietvoid commented Sep 16, 2023

As expected, the getHdrCapabilities() method, both with the previous code and with this PR, does not return the HDR10+ type because it's a device with API level < 29.

FWIW I just wanted to confirm since FireOS doesn't always strictly adhere to the API.
For example in #23595, using the constant value as a fallback still works.

@joseluismarti
Copy link
Contributor Author

I've checked with FireTV and Chromecast that this PR does not modify the behaviour and would be safe to merge

@quietvoid
Copy link
Contributor

LGTM.

I'll revisit testing HDR10+ detection when I get the new 4K Max next week, since it'll be API level 30.

@fuzzard
Copy link
Contributor

fuzzard commented Sep 28, 2023

Have merged the libandroidjni. If you can update to that and squash/cleanup the extra commits thatd be great.

@fuzzard fuzzard modified the milestone: Omega 21.0 Beta 1 Sep 28, 2023
@fuzzard fuzzard merged commit 2d85e2f into xbmc:master Sep 28, 2023
2 checks passed
@joseluismarti joseluismarti deleted the HDRTypes branch September 28, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants