Skip to content

[Sample] [Graphic] - Implementation of "Displaying UltraHDR" Sample #51

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jul 10, 2023
Merged

[Sample] [Graphic] - Implementation of "Displaying UltraHDR" Sample #51

merged 11 commits into from
Jul 10, 2023

Conversation

madebymozart
Copy link
Contributor

@madebymozart madebymozart commented Jul 5, 2023 •
edited
Loading

Description

This CL contains sample code for Displaying UltraHDR Images on API 34 and above.

How Has This Been Tested?

Test Configuration #1 - Pixel 7 Pro API 34 Beta (Android 14 Beta)

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Demo

Note: You will not see this effect properly until running the platform app on a device with API34. The demo is for illustrative purposes)
https://github.com/android/platform-samples/assets/5649571/1ac8ea89-c4c2-4c54-90a6-06001e3680d6

Screenshots

Screenshot_20230705_191647

HDR_ON

@madebymozart madebymozart added enhancement New feature or request new sample This denotes the creation of a new sample in the platform samples app labels Jul 5, 2023
@madebymozart madebymozart requested a review from marcelpinto July 5, 2023 23:30
@madebymozart madebymozart self-assigned this Jul 5, 2023
@madebymozart madebymozart changed the title Media displaying ultrahdr Media/ Graphics - Displaying an UltraHDR Image Sample Implementation Jul 5, 2023
@madebymozart madebymozart changed the title Media/ Graphics - Displaying an UltraHDR Image Sample Implementation Media / Graphics - Displaying an UltraHDR Image Sample Implementation Jul 5, 2023
marcelpinto
marcelpinto reviewed Jul 6, 2023
View reviewed changes
Copy link
Contributor

@marcelpinto marcelpinto left a comment

Choose a reason for hiding this comment

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

LGTM just few comments

samples/media/ultrahdr/README.md Outdated
@@ -0,0 +1,4 @@
# DisplayingUltraHDRSample samples

// TODO: provide minimal instructions
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you provide a quick readme?

  • What the sample does
  • Link to docs

...media/ultrahdr/src/main/java/com/example/platform/media/ultrahdr/common/ColorModeControls.kt Outdated
override fun onAttachedToWindow() {
super.onAttachedToWindow()
display?.run {
registerHdrSdrRatioChangedListener({ it.run() }, hdrSdrRatioListener)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: for readability move this into a variable called executor. Otherwise it's not clear what is it without going into the javadocs

...media/ultrahdr/src/main/java/com/example/platform/media/ultrahdr/common/ColorModeControls.kt Outdated
}
}

override fun onDetachedFromWindow() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: this happens when the activity is destroyed or the view is removed from the viewgroup, but if the app goes onPause this won't happen. Is it okay to still have the listener registered and HDR mode on? Or is it best practice to turn on/off onresume/onpause?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, you are not updating the mode on detach. Shouldn't you do it? Otherwise the activity will be kept as HDR even if the view is not there anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, makes sense

@madebymozart madebymozart changed the title Media / Graphics - Displaying an UltraHDR Image Sample Implementation Displaying an UltraHDR Image Sample Implementation Jul 6, 2023
@madebymozart madebymozart mentioned this pull request Jul 7, 2023
Merged
3 tasks
@madebymozart madebymozart changed the title Displaying an UltraHDR Image Sample Implementation Graphics - Implementation if "Displaying UltraHDR" Sample Jul 7, 2023
@madebymozart madebymozart requested a review from marcelpinto July 7, 2023 00:27
@madebymozart
Copy link
Contributor Author

@marcelpinto I've moved this to the graphic package as displaying UltraHDR is more of a graphic related topic

@madebymozart madebymozart changed the title Graphics - Implementation if "Displaying UltraHDR" Sample Graphics - Implementation of "Displaying UltraHDR" Sample Jul 7, 2023
@madebymozart madebymozart changed the title Graphics - Implementation of "Displaying UltraHDR" Sample [Sample] [Graphic] - Implementation of "Displaying UltraHDR" Sample Jul 7, 2023
@madebymozart madebymozart merged commit de598b8 into main Jul 10, 2023
@marcelpinto marcelpinto deleted the media-displaying-ultrahdr branch July 14, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelpinto marcelpinto marcelpinto approved these changes

Assignees

@madebymozart madebymozart

Labels
enhancement New feature or request new sample This denotes the creation of a new sample in the platform samples app
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@madebymozart @marcelpinto