Use float OpenGL buffer and extended linear colorspaces for HDR playback #4174

OceanS2000 · 2023-01-11T15:34:49Z

I have read CONTRIBUTING.md
This implements/fixes issue Luminance is incorrect for HDR videos taken on iPhone #3772.

Description:
This WWDC 2021 presentation suggested to use floating point OpenGL buffers (see 20:56) and extended linear colorspaces (see 18:08) for EDR content.

One may also note that original mpv mac output code also use floating point OpenGL buffers for 10-bit playback, which format are most HDR videos in. Further, the official Apple Documentation notes that itur_2020_PQ colorspaces are deprecated.

It turns out that these are all we need to match QuickTime.app's HDR output.

Device: Macbook Pro 14" with M1 pro
Test clip: download

Left: IINA.app current develop branch
Middle: this PR
Right: QuickTime.app

OceanS2000 · 2023-01-11T16:57:47Z

Added a nasty hack to workaround a weird behavior about set colorspace to nil.

Basically once colorspace was set to extended linear, setting it back to nil would cause falling back to kCGColorSpaceExtendedLinearSRGB which is definitely not what we want. Without manually setting it to a SDR colorspace the color would be completely broken.
Is it a bug with macOS?

svobs · 2023-01-12T02:10:28Z

+1!

I pulled this PR and tested with some of my own HDR videos and will confirm they look 10x better, especially on my MacBook's built-in display, but also on my HDR display. Hopefully the maintainers can take a look at it soon.

CC @CarterLi

CarterLi · 2023-01-12T02:42:58Z

iina/VideoView.swift

      videoLayer.wantsExtendedDynamicRangeContent = false
+      // It seems that once there is a layer with colorspace set to something extended linear,
+      // then setting colorspace to nil will cause falling back to ExtendedLinearsRGB
+      videoLayer.colorspace = CGColorSpaceCreateDeviceRGB()


colorspace is nil by default. Disabling HDR mode should revert all changes to the default value as HDR was never enabled. In addition, since we are testing videoLayer.colorspace != nil in Line 286, setting videoLayer.colorspace to non-nil makes the resetting code be executed every time when setICCProfile being called

The problem was, as I mentioned in the commit, once videoLayer.colorspace was set to CGColorspace.extendedLinear* before, then setting it to nil would not clear it, instead, it would fallback to kCGColorspaceExtendedLinearSRGB and completely throw off SDR color. I'm not sure whether it is the intended behavior.

Setting it to CGColorspaceCreateDeviceSRGB() should also turn off color management on the buffer like nil. I do agree condition should be updated, though.

Attached is the screenshot trying to turn off HDR without the last commit d943877.

It turns out that it is kCGLPFAColorFloat, kCGLPFAColorSize, CGLPixelFormatAttribute(64), that makes nil fallback to kCGColorSpaceExtendedLinearSRGB. You may remove these code and try again

static let SRGB = CGColorSpaceCreateDeviceRGB()

videoLayer.colorspace = VideoView.SRGB

if videoLayer.colorspace != VideoView.SRGB { videoLayer.colorspace = VideoView.SRGB ...

In addition, videoLayer.colorspace = VideoView.SRGB should be set in init()

It turns out that it is kCGLPFAColorFloat, kCGLPFAColorSize, CGLPixelFormatAttribute(64), that makes nil fallback to kCGColorSpaceExtendedLinearSRGB. You may remove these code and try again

The problem is output of 10-bit color depth videos would be incorrect without them. These ensure pixel format of OpenGL buffer be 16-bit float-point instead of default 8-bit.

The following test pattern Mehanik HDR10 test patterns/03. Grayscale/04. 10bit test/01. test_10-bit_23.976.mp4 from https://www.avsforum.com/threads/hdr10-test-patterns-set.2943380/ will demonstrate the problem. Unfortunately, because all screenshots I could take would be clamped to 8-bit color depth, I cannot directly provide a picture here.
Direct download link: test-10bit.mp4

I have a lot of HDR test videos.

I meant that it was not once videoLayer.colorspace was set to CGColorspace.extendedLinear* before caused the problem

You're right indeed, the comment and problematic commit message were removed.

iina/VideoView.swift

CarterLi · 2023-01-12T02:50:57Z

itur_2020_PQ was deprecated ( and unsupported by newer macOS ) because it was replaced by itur_2100_PQ. itur_2100_PQ was not deprecated.

OceanS2000 · 2023-01-12T05:36:03Z

itur_2020_PQ was deprecated ( and unsupported by newer macOS ) because it was replaced by itur_2100_PQ. itur_2100_PQ was not deprecated.

Yes, that's right. So current code should work, but it just doesn't (so we have #3772 and probably #4075). In that term I agree with @low-batt and believe switch to MoltenVK and Metal would be the long-term solution. But now at least we can try to make current result a little better.

CarterLi · 2023-01-14T12:06:22Z

Tested. While I didn't see much difference for PQ, HLG was improved greatly. However there are several issues should be addressed.

low-batt · 2023-01-16T00:35:18Z

Compare IINA 1.3.1 to Quicktime playing New York 8k | 4k HDR specifically looking at the clock in the beginning. Playing that video IINA's image of the clock face looks pretty close to that displayed by Quicktime. When that video is played with IINA built with the changes in this PR the video is darker than before and definitely looks worse than IINA 1.3.1.

When I play the video shown at the start of this PR with IINA 1.3.1 I see the blown out highlights shown in the screenshots. Playing that video with IINA built with the changes in this PR the video is significantly improved. But it does not match Quicktime. The picture is now slightly dimmer than Quicktime. This is especially visible when comparing the light bulb in the lamp. From the comments in the PR I was expecting the picture to match?

I'm testing on a MacBookPro18,2 with the XDR display running macOS 13.1.

I believe @CarterLi has added this fix to IINA+, so I also tested with IINA+. I had to build it myself since builds are failing reporting:

Node.js 12 actions are deprecated. Please update the following actions to use Node.js 16: actions/checkout@v2. For more information see: https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/.

With IINA+ the test video is darker still. Possibly the difference is that IINA+ is setting target-peak?:

18:23:07.250 [hdr][d] Will enable tone mapping target-peak=1600

CarterLi · 2023-01-17T10:29:20Z

Can we apply this patch only to HLG videos and keep PQ videos as it was? So that PQ videos can be displayed without any conversion.

OceanS2000 · 2023-01-17T12:51:43Z

Can we apply this patch only to HLG videos and keep PQ videos as it was? So that PQ videos can be displayed without any conversion.

That's a great idea. I did some experiments and found that actually mapping everything to PQ seeming to also solve HLG problem. And this approach can avoid the mess for getting mpv output linear values.

low-batt · 2023-01-17T21:31:41Z

Comparing IINA built with the latest version of this PR to Quicktime when playing New York 8k | 4k HDR the results are pretty close. If stop the video at the beginning and look at the shutters above the clock you can see the picture displayed by Quicktime is slightly brighter.

When I play the video shown at the start of this PR the results are again pretty close. It also looks to me that Quicktime is just slightly brighter.

There are open mpv issues with trying to match Quicktime behavior. We may be up against those issues. This is definitely an improvement.

@CarterLi do you approve of these changes or still have concerns?

CarterLi · 2023-01-18T08:40:55Z

LGTM

CarterLi · 2023-01-18T08:47:54Z

I suggest that iina should adopt iina-plus's manual tone-mapping setting feature, for low max-brightness monitors ( < 1000 nit )

CarterLi@91938e2

low-batt · 2023-01-18T19:58:15Z

On adopting iina-plus's manual tone-mapping setting, I've been hoping we'd figure out a way to automatically determine a display's brightness.

By the way, the IINA+ builds are still failing. The latest one seems to be stuck in the queued state.

low-batt

Pulled PR locally, built and tested. Definitely an improvement over the current behavior with the HDR videos I tested.

CarterLi · 2023-03-01T09:43:23Z

FYI: #4075 (comment)

uiryuu

Just a small change, other parts LGTM

iina/InspectorWindowController.swift

Use float pixel format for OpenGL buffer

5c62be4

CarterLi reviewed Jan 12, 2023

View reviewed changes

iina/VideoView.swift Outdated Show resolved Hide resolved

CarterLi reviewed Jan 12, 2023

View reviewed changes

iina/VideoView.swift Outdated Show resolved Hide resolved

CarterLi added a commit to CarterLi/iina that referenced this pull request Jan 14, 2023

HDR: iina#4174

b6388f4

CarterLi added a commit to CarterLi/iina that referenced this pull request Jan 14, 2023

HDR: iina#4174

6250dad

OceanS2000 force-pushed the float-pixformat branch 2 times, most recently from 7751179 to 1406085 Compare January 14, 2023 15:23

low-batt linked an issue Jan 15, 2023 that may be closed by this pull request

Luminance is incorrect for HDR videos taken on iPhone #3772

Open

1 task

OceanS2000 force-pushed the float-pixformat branch 3 times, most recently from 6dd3bcf to c04649f Compare January 17, 2023 12:07

Use _PQ colorspaces exclusively for HDR playback

ad84162

OceanS2000 force-pushed the float-pixformat branch from c04649f to ad84162 Compare January 17, 2023 12:53

CarterLi added a commit to CarterLi/iina that referenced this pull request Jan 18, 2023

HDR: iina#4174

00745d6

low-batt approved these changes Jan 18, 2023

View reviewed changes

low-batt requested a review from saagarjha March 15, 2023 02:30

uiryuu requested changes Mar 31, 2023

View reviewed changes

iina/InspectorWindowController.swift Outdated Show resolved Hide resolved

Update iina/InspectorWindowController.swift

a812fba

uiryuu merged commit 763700d into iina:develop Apr 1, 2023

low-batt mentioned this pull request Apr 17, 2023

HDR bug on M2/M3 chip. #4312

Open

1 task

CarterLi mentioned this pull request Oct 10, 2024

Inaccurate Colorspace handling for SDR/HDR detection #5200

Closed

1 task

Uh oh!

Use float OpenGL buffer and extended linear colorspaces for HDR playback #4174

Use float OpenGL buffer and extended linear colorspaces for HDR playback #4174

Uh oh!

Conversation

OceanS2000 commented Jan 11, 2023

Uh oh!

OceanS2000 commented Jan 11, 2023

Uh oh!

svobs commented Jan 12, 2023

Uh oh!

CarterLi Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

OceanS2000 Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

CarterLi Jan 14, 2023

Choose a reason for hiding this comment

Uh oh!

CarterLi Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OceanS2000 Jan 14, 2023

Choose a reason for hiding this comment

Uh oh!

CarterLi Jan 14, 2023

Choose a reason for hiding this comment

Uh oh!

OceanS2000 Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CarterLi commented Jan 12, 2023

Uh oh!

OceanS2000 commented Jan 12, 2023

Uh oh!

CarterLi commented Jan 14, 2023

Uh oh!

low-batt commented Jan 16, 2023

Uh oh!

CarterLi commented Jan 17, 2023

Uh oh!

OceanS2000 commented Jan 17, 2023

Uh oh!

low-batt commented Jan 17, 2023

Uh oh!

CarterLi commented Jan 18, 2023

Uh oh!

CarterLi commented Jan 18, 2023

Uh oh!

low-batt commented Jan 18, 2023

Uh oh!

low-batt left a comment

Choose a reason for hiding this comment

Uh oh!

CarterLi commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uiryuu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CarterLi Jan 14, 2023 •

edited

Loading

CarterLi commented Mar 1, 2023 •

edited

Loading