-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Basic HDR support #3539
Basic HDR support #3539
Conversation
Comparation, upper IINA with HDR support, lower mpv while screenshot won't show any difference (only IINA is much lighter) Video source (downloaded with youtube-dl): https://www.youtube.com/watch?v=WO2b03Zdu4Q |
Can you please provide the video source? You may cut the video if it's too big: https://stackoverflow.com/questions/18444194/cutting-the-videos-based-on-start-and-end-time-using-ffmpeg Can you please test this video? And compare to Safari or Chrome ( but not Firefox which doesn't support HDR playback ) |
Your video seems working well in Chrome, I'm not able to open it in Safari. Here's the video cut cut.mp4: |
What about playing in IINA? |
In IINA it works. Your version looks much brighter. |
Not only brighter, but also the texture should look more clear |
Your video provides no mastering display side data, which is used to determine the color space should be used by IINA video layer.
I'll test it tomorrow as I have only a Windows machine which has no HDR support now |
Try my latest build @spittix |
That looks good :) Brightness is at the expected level (left upper is test version, left lower is IINA 1.2.0, right upper is Infuse) If you compare the yellow between test version an Infuse, there's a small difference, which leads to tiny less contrast I guess. Another point is, that this activated HDR mode is nowhere shown. So can you do me a favour and add a status for that? |
Sounds that Infuse was using different colorspace ( bt.2020 I suppose ). Since the video source didnt suggest which colorspace should be used, any colorspace other than bt.709 was not wrong. I chose Display P3 simply because bt.2020 was deprecated in macOS 11+ |
Understood and agree :). |
In console log ;)
TBO I have very little knowledge with cocoa programming. Any contributions are welcome. There are other things to do:
|
Will it display HDR content on non-HDR displays as on MacBooks, courtesy of EDR? |
Yes. Basically IINA and mpv do the same thing on HDR mode. The only difference is that mpv provides some colorspace switches for users to choose, but IINA tries to detect it from the video source. |
If possible, could someone test on this clip? https://drive.google.com/file/d/1FXmRj-wN_GP65AQHzRShNSKojPopZJcF/view?usp=drivesdk infuse displays this incorrectly and over brightens, so wanted to make sure it works here. Additionally playing it through iOS apps on macOS like nPlayer or Plex from the App Store it doesn’t look correct either (not sure why, the colors are less saturated). Have been trying to find a player that plays it correctly, like QuickTime. Update: Nevermind, tried second test build and it looks good 👍 Second Update: Doesn't seem to be working correctly, might be color profile, will try to find a sample file I'm able to share. Great example here: https://4kmedia.org/sony-swordsmith-hdr-uhd-4k-demo/ Via Quicktime, you can see the entire background of the flame at the two second marker, via IINA all you can see is the flame. |
The file was too big to download due to the network issue in my country. Can you cut the file into smaller one as said in |
It's not as noticeable as I initially thought, but the hue of the flame looks different comparing quicktime and IINA in fullscreen, and additionally the background seems darker in the IINA version. Quicktime looks far more orangeish. https://drive.google.com/file/d/1CS4MtuCQZEhJ42_wWdVX4Dkp33rfRIH9/view?usp=sharing (Hopefully this is small enough) |
This file doesn't seem to be well encoded.
Did Quicktime really do the correct thing? |
I’m not sure if it did the “correct” thing, it just did it differently than this implementation. To my eyes I prefer what QuickTime does, the difference is quite noticeable in a lot of HDR content I’ve tested with. You can test QuickTime on your end too I believe? Should be preinstalled on all MacBooks.
As a second note - another test file with correct luminance/metadata, however still includes the metadata at the stream level doesn't play correctly due to incorrect color space (at least I believe). The color hues look off slightly, however the luminance appears the same. https://drive.google.com/file/d/1W5Drhe0Jstvp4a4Ae-oJkSxyPr3CjQ7w/view?usp=sharing Source: https://ff.de/hdr10plus-metadata-test/ I think most (if not all) HDR content is sent out in BT.2020, so that may be a better default rather than P3. |
Why? It's a suggestion for the whole movie, isn't it?
Please try my latest binary: https://github.com/CarterLi/iina/releases/tag/v1.2.0-5
It suggests Display P3 too
But it was deprecated by Apple. It would be better if we provide an option for user to choose. I'm no expert at cocoa. Can you contribute? |
There are enough reason to use IINA.
|
Unfortunately I'm still seeing the same issue with the flame file compared to quicktime, the hue is off (I assume this is due to assuming P3 instead of Rec2020, not sure why as you're looking at the individual frames). Will make sure this is the case tomorrow when I can run it via XCode and view the logs (I believe this is the only way to view NSLogs). Sorry I have no experience at all with MacOS development, but hopefully can figure some of this stuff out.
It is, but mp4/mkv support multiple video streams, so I'm guessing the metadata is per stream for that reason?
I don't think it does? From mediainfo (Color Primaries are labeled as BT2020).
Agreed.
Unfortunately I've never worked with Cocoa or Swift (which is why I haven't contributed anything or tried to tackle this myself), but I can try and sit down and figure it out tomorrow. |
It's about
|
This makes sense, I was misinterpreting those values. I'm unsure on why the QuickTime hue's don't match IINA's then (I don't think it's purely luminance level like you mentioned as the orange hue is pretty noticeably different). Potentially Quicktime is interpreting the color space incorrectly then? Do you notice the same hue difference that I do when comparing Quicktime and IINA on the flame file? |
Did you change anything? I see the same build number as 2 days ago.
A good idea in my opinion.
Unfortunately I've no experience with cocoa. |
Changelog can be find here: https://github.com/CarterLi/iina/commits/develop
We can't do anything if no one is familiar with cocoa. |
@CarterLi oh great to see my code lives on! Thanks for augmenting it. Have you guys tried the HDR binary that I made a year ago from my original HDR commit? https://github.com/anastasiuspernat/iina/releases/tag/1.1.2.HDR Btw I implemented same fix for mpv back then. I hardly remember the details, doesn't it work on M1? |
Firsthttps://github.com/anastasiuspernat/iina/blob/develop/iina/FFmpegController.m#L489 This logic was wrong. Should be Since the logic was wrong, you would never find the correct primaries code and default to dcip3. And with your special case, primaries will be set as Display P3, always. SecondYou didn't specify And to be honest. The code in FFmpegController was full of mess. Lots of unnecessary code, unused variables, memory / resource leaks... But still, your code was really a good start. This PR won't exist without your code. Thanks for your work. |
Update on this - flame file matches Quicktime hue exactly when setting the video layer to the itur_2020_PQ color space. Are you sure it's correct behavior to set the video layer to the mastering display color primaries? Seems odd that Quicktime does this differently. Second update - noticed a bug in grabbing mastering display color primaries from video frame, will submit pull request shortly. I think matching Quicktime's behaviour would make sense - using the color primaries metadata tag for both As an example, iPhone videos captured in HLG using HDR that have no Mastering Display metadata are displayed with an itur_2020_pq colorspace in quicktime as well, with the current implementation using P3 the video looks off in IINA. EDIT: All of this is fixed in latest updates - using mastering display color space for |
https://github.com/CarterLi/iina-hdr/releases/tag/v1.2.0-7 Changelog:
One major issue I found: Currently HDR mode was mapped to display instead of video source. So if you open an SDR video, close it, then open an HDR video, HDR mode won't be enabled. What we should do:
Don't have a good way to resolve it yet. |
Top : IINA (latest from your repo/releases) Files from here. |
That's awful. Thanks for reporting. Edit: Seems that the screen was warming up. If you pause for some time, the image gets normal. I don't know how to fix it. As long as I enable HDR mode, the screen gets over bright, regardless what primaries is used. Let's pause the player a while for now |
I have paused it for a while but EDR fails to be kicking in. To be noted is that Low Power Mode is enabled and dimmed display. Does that impact EDR ? Update: It seems EDR kicks in instantly no matter the settings/power options but color is messed up as show above. |
Suspect that the lighting differences are due to not being able to set |
I closed this PR because I was bored resolving conflicts between my working branch and this PR branch, but I'll keep releasing new binaries in my fork repo when any changes are made ( for example cherry-picking @low-batt's fixes ). HDR for IINA is considered feature complete though. Any issues, feature requests should be filed in my fork repo |
So just to be clear, when does your very cool HDR improvements go into the main release. HDR on the MacBook Pro M1 is literally brighter than the sun. Nice work! It does not look that different on my SDR display but it is very noticeable. The one on the right looks blown out on an SDR display but is incredible on the mini-LEd. Thanks again. I presume I just have to keep copying DMGs from iina-plus then? |
Description:
This was based on the work of @anastasiuspernat. Although his implementation was not correct, it was a very good start.
Changes:
The original code of @anastasiuspernat can be found at: https://github.com/anastasiuspernat/iina
Since this repo seems dead already, If anyone want to test this feature, I have released a binary here: https://github.com/CarterLi/iina/releases ( arm64 only )