Experimental HDR support on macOS using colorSpace approach by anastasiuspernat · Pull Request #8404 · mpv-player/mpv

anastasiuspernat · 2020-12-17T00:20:11Z

Enabling support for HDR monitors (in manual mode).

Main discussion is here: #7341

Added a new switch called --macos_hdr_csp (similar to d3d color space approach at #5804). It specifies the extended HDR color space. Doesn't require any shader or video render pipeline modification. Auto-activates EDR/HDR mode on the video layer.

Currently supported color spaces (passed to --macos_hdr_csp switch):

displayP3_PQ_EOTF DCI P3 primaries, PQ transfer function - that's the most common color profile for most videos
displayP3_HLG DCI P3 primaries, and the HLG gamma
extendedLinearDisplayP3 P3 primaries and linear transfer function
itur_2020 BT.2020
itur_2020_PQ_EOTF BT.2020 with PQ transfer function

This can (and probably must) be set together with target-trc switch. Both values must correspond to video's color primaries/transfer characteristics in order to get the correct color reproduction.

Tested on varios video footage on macOS Catalina 10.15.5. Produces same results as QuickTime player on the same footage.

video/out/cocoa_cb_common.swift

osdep/macosx_application.m

video/out/mac/gl_layer.swift

video/out/mac/view.swift

video/out/cocoa_cb_common.swift

video/out/mac/gl_layer.swift

Akemi · 2020-12-19T13:36:35Z

i added a few comments. it would also be nice to have some mpv option examples and some screenshots with before, now and quicktime. mostly out of curiosity and for future reference.

osdep/macosx_application.m

anastasiuspernat · 2020-12-22T03:05:14Z

I hope I fixed it! Updated everything by your comments and added new info to options.rst.

Also here are reference images from my tests. They don't fully show the dynamic range of the image (this part is done on the monitor side) but they do give some idea:

P.S. Unfortunately I couldn't find a way to compare the exact same frames from QuickTime and mpv so they look a little bit different because they come from slightly different timecodes.

avih · 2020-12-23T08:11:02Z

The commits add (and update) .DS_Store, is that intentional? (should we add it to .gitignore?)

anastasiuspernat · 2020-12-23T08:15:20Z

Sorry about .DS_Stores. No it's not intentional, they were auto-added by Finder. When I develop on Mac I usually add .DS_Store to .gitignore. I think I removed them all in the final commit, no?

avih · 2020-12-23T08:16:58Z

I think I removed them all in the final commit, no?

I didn't look at the commits one by one, but assuming you intend to keep the commits incremental without squashing, then removing them only at the last commit is not enough - you'll need to remove them from all commits (might be slightly painful, but not too much, though the pain would also make it easier to avoid next time :p).

anastasiuspernat · 2020-12-23T08:25:54Z

No I don't intend to keep all the commits. Would be happy if you could help me to squash them the right way if that's necessary. I think only the current final stage is important, thank you.

avih · 2020-12-23T08:33:12Z

Not necessary to squash them right away, depends on how you want to present it - incremental changes or one commit which changes over time. But you can do that it you prefer to work only on one commit.

This is git territory, but you can squash commits locally by running git rebase -i and replace pick with s (for squash) for all but the first new commit, then save the file and quit the editor, edit the combined commit message, and save and quit again. If you're happy with the result - git push -f would update the PR with the squashed commit.

For fllowup changes, you can either do the same with new commits before you push them, or just amend the existing commit, and again force push will update the PR. Force (-f) is required because it rewrites the history - acceptable in a PR but generally highly discouraged elsewhere.

anastasiuspernat · 2020-12-23T08:41:04Z

OK great! Thanks for the details. One commit per major followup would definitely look better. Will squash it on next iteration.

avih · 2020-12-23T08:52:29Z

There are no hard rules, but the convention is that you can force push in a PR (change existing commits), but preferably keep a set of commits which is intended to be pushed individually, where each commit does one "thing". Deciding what a "thing" is is subjective, but if your code does two unrelated things for instance, then it's preferable to keep those in two distinct commits.

I just briefly looked at the combined diff of this PR, and indeed I agree it should probably end up as one commit.

DOCS/man/options.rst

video/out/mac/common.swift

Akemi · 2020-12-27T12:07:05Z

video/out/mac/common.swift

+// HDR is supported fully only on macOS 10.15 and higher
+// some of the HDR color spaces are available on 10.14 though
+#if HAVE_MACOS_10_15_FEATURES
+                switch (mpv?.macOpts.macos_hdr_csp)


the indentation is wrong here.

Fixed it! Also some color spaces are declared as supported by 10.14.X SDKs, like 10.14.6 etc but seems HAVE_MACOS_10_14_FEATURES checking for 14.0 and Travis won't let me to go through if I put them inside HAVE_MACOS_10_14_FEATURES.

Here's PR: 427d81c
Travis build log: https://travis-ci.org/github/mpv-player/mpv/jobs/751898275
(the error is "'CGColorSpace' has no member 'displayP3_HLG'", with the check for #available(macOS 10.14.6...).

In any case only macOS Catalina 10.15 supports HDR output and it's better be compiled against this version. So most of those are now inside HAVE_MACOS_10_15_FEATURES.

no those color spaces are specifically defined to be available on 10.14.6 and not 10.14.X/any 10.14 SDK. it needs a specific minor configure check too.

HAVE_MACOS_10_14_FEATURES only checks for 10.14(.0) features but not for 10.14.6. this needs a separate configure check.

Akemi · 2020-12-27T12:08:29Z

video/out/mac/common.swift

+                (view?.layer as! CAOpenGLLayer).colorspace = CGColorSpace(name: name)
+            } else
+            {
+                (view?.layer as! CAOpenGLLayer).colorspace = windowColorSpace.cgColorSpace


no forced casts. i still dislike the else case here.

Need some help here. How else can I set color space on the view.layer? It's CALayer, right? Doesn't have colorspace property.

Also what's wrong with else, you mean the curly bracket?

typecasting is okay, but you should not force typecasting by any means, eg as? instead of as!. the latter case will crash when failing. see https://docs.swift.org/swift-book/LanguageGuide/TypeCasting.html

the else is unnecessary there.

colorSpcace = defaultColorSpace if someCondtion == true { colorSpcace = differentColorSpace }

Unfortunately this way typecasting doesn't work. With as? I receive a compilation error about unwrapping CAOpenGLLayer, probably view.layer has a wrong type definition?

something like this should haves worked. though i would need to see the specific error.

guard let newlayer = layer as? CAOpenGLLayer else { return } // access newlayer here

video/out/mac/common.swift

Akemi · 2020-12-27T12:12:21Z

i am not sure if what we see on those screenshots is 'intended' or more likely a 'coincidence' that it looks somewhat similar/okay'ish.

Akemi · 2020-12-27T12:19:23Z

video/out/mac/common.swift


    var cursorVisibilityWanted: Bool = true

+    var colorSpace: NSColorSpace?


i believe this variable is not needed.

It's used in updateICCProfile for setRenderICCProfile in cocoa_cb_common.swift.

what i meant here, is that we don't need a variable that is available to the whole object of that class but just a function local variable would have done perfectly fine.

Experimental HDR support on macOS using colorSpace approach Experimental HDR support on macOS using colorSpace approach Experimental HDR support on macOS using colorSpace approach Experimental HDR support on macOS using colorSpace approach Experimental HDR support on macOS using colorSpace approach Experimental HDR support on macOS using colorSpace approach Experimental HDR support on macOS using colorSpace approach Added macOS version check Added macOS version check for Catalina Stricter check for macOS Catalina features Switched from runtime check to compile time check of macOS features Made macOS HDR implementation more consistent Made macOS HDR implementation more consistent Made macOS HDR implementation more consistent Made macOS HDR implementation more consistent Made macOS HDR implementation more consistent Improved macOS HDR-related code by PR comments Improved macOS HDR-related code by PR comments Improved macOS HDR-related code by PR comments Improved macOS HDR-related code by PR comments Improved macOS HDR-related code by PR comments

anastasiuspernat · 2020-12-28T02:03:09Z

i am not sure if what we see on those screenshots is 'intended' or more likely a 'coincidence' that it looks somewhat similar/okay'ish.

@Akemi
They do look similar. The HDR is hardware-reproduced by monitor, there's no way on the software side to take screenshots that precisely show extended dynamic range. Probably the screenshots should be taken as pictures from the monitor using a good camera. I have one, but I don't have enough time, I'll see what I can do about it!

Also squashed the PR and updated code by our conversation. Answered on the rest of the issues.

skrew · 2020-12-29T02:01:33Z

Please don't forget iOS version for the HDR support, i think it's almost the same as MacOS

Akemi · 2020-12-29T06:37:37Z

Please don't forget iOS version for the HDR support, i think it's almost the same as MacOS

this has nothing to do with iOS though. there is no iOS backend in mpv. on iOS you will use libmpv and there the layer is handled by the libmpv user. so it's entirely in the hands of the one using libmpv on iOS to support this.

anastasiuspernat · 2021-01-15T19:14:41Z

@Akemi
Are there still any problems? Do I need to fix anything? Thank you and Happy New Year!

Akemi · 2021-01-15T19:32:26Z

i just didn't get around looking at it again.

Akemi · 2021-01-19T16:24:20Z

i opened my own PR #8485. i don't mean this to be disrespectful towards you and i hope it isn't seen as this. but what i had in mind and what you did here was a bit too far apart for me to explain everything i wanted. it would be nice if you could also test my PR.

nevertheless i will give you some more feedback on this PR for the future.

Akemi · 2021-01-19T16:32:34Z

osdep/macosx_application.m

+        {"macos-output-csp", OPT_CHOICE(macos_output_csp,
+            {"auto", -1}, {"displayP3_HLG", 0}, {"displayP3_PQ_EOTF", 1}, 
+            {"extendedLinearDisplayP3", 2}, {"itur_2020", 3}, {"itur_2020_HLG", 4}, 
+            {"itur_2020_PQ_EOTF", 5}, {"extendedSRGB",6}, {"extendedLinearSRGB",7}


the extended color spaces expect component values below 0 and above 1.0. that's what mpv can't do atm and with that those color spaces are a bit useless atm.

Akemi · 2021-01-19T16:34:00Z

video/out/cocoa_cb_common.swift

-            layer?.colorspace = colorSpace.cgColorSpace
-        }
+        super.updateICCProfile()    
+        libmpv.setRenderICCProfile(colorSpace!)


never use a forced unwrapping in swift if not necessary. always use a guard, if let or ? instead of !. otherwise it will only end up in hard crashes.

Akemi · 2021-01-19T16:34:27Z

video/out/mac/common.swift

+
+        if (mpv?.macOpts.macos_output_csp != -1) {
+            (view.layer as! CAOpenGLLayer).wantsExtendedDynamicRangeContent = true
+            view.wantsExtendedDynamicRangeOpenGLSurface = true;


semicolon not needed

Akemi · 2021-01-19T16:36:33Z

video/out/mac/common.swift

+            if #available(macOS 10.12, *) {
+                switch (mpv?.macOpts.macos_output_csp)
+                {
+                    case 6: name = CGColorSpace.extendedSRGB; break


breaks are not necessary on swift switch case. it will only fall through if an explicit fallthrough keyword is used. also the semicolons.

anastasiuspernat · 2021-01-19T18:19:44Z

@Akemi

i opened my own PR #8485. i don't mean this to be disrespectful towards you and i hope it isn't seen as this. but what i had in mind and what you did here was a bit too far apart for me to explain everything i wanted. it would be nice if you could also test my PR.

nevertheless i will give you some more feedback on this PR for the future.

No worries haha! You better know your codebase, also Swift is a new language to me.

In any case my main intention was to make IINA (mpv-based video player) play with my HDR monitor on macOS. I thought mpv is the main culprit but now I see it's not. My fork of IINA now plays HDR videos perfectly so my main goal is achieved and I just wanted to give back my findings (which are based on your knowledge) to the world.

I rarely take part in opensource and it was a lot of fun.

I'll try to test and answer when I have time! Thanks again for the guidance.

DavidSchechter · 2021-03-20T03:14:30Z

@Akemi @anastasiuspernat any progress on this?

JulyIghor · 2022-10-08T06:27:01Z

any progress on this?

Akemi · 2024-04-19T23:07:06Z

sorry for stalling this for so long. i am going to close this one i favour of #8485. i will also revisit #8485 and see how we will proceed with it. also see the last comment und the linked issue for discussion (#7341 (comment)).

Experimental HDR support on macOS using colorSpace approach

1be802b

Akemi reviewed Dec 19, 2020

View reviewed changes

osdep/macosx_application.m Outdated Show resolved Hide resolved

anastasiuspernat requested a review from Akemi December 22, 2020 03:05

Akemi reviewed Dec 27, 2020

View reviewed changes

anastasiuspernat force-pushed the macoshdr branch 3 times, most recently from d53d47c to 266f583 Compare December 28, 2020 00:51

Anastasy and others added 2 commits December 27, 2020 17:10

Improved macOS HDR-related code by PR comments

8cd7d2d

anastasiuspernat force-pushed the macoshdr branch from 87d52f8 to 8cd7d2d Compare December 28, 2020 01:11

anastasiuspernat requested a review from Akemi December 28, 2020 02:03

anastasiuspernat added 2 commits December 28, 2020 15:50

More precise test for macOS-version specific color spaces

427d81c

More broad test for macOS-version specific color spaces

b7df6d9

Akemi mentioned this pull request Jan 14, 2021

HDR not working on iOS #8467

Closed

Akemi mentioned this pull request Jan 19, 2021

[WIP] mac: add support to set color space for layer and macOS transformation (EDR/HDR) #8485

Closed

Akemi reviewed Jan 19, 2021

View reviewed changes

sfan5 added the priority:stalled label Sep 20, 2022

Akemi added the os:mac label Mar 16, 2024

Akemi closed this Apr 19, 2024


		var cursorVisibilityWanted: Bool = true

		var colorSpace: NSColorSpace?

Conversation

anastasiuspernat commented Dec 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Akemi commented Dec 19, 2020

Uh oh!

Uh oh!

anastasiuspernat commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avih commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anastasiuspernat commented Dec 23, 2020

Uh oh!

avih commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anastasiuspernat commented Dec 23, 2020

Uh oh!

avih commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anastasiuspernat commented Dec 23, 2020

Uh oh!

avih commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anastasiuspernat Dec 28, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Akemi commented Dec 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anastasiuspernat commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skrew commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Akemi commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anastasiuspernat commented Dec 22, 2020 •

edited

Loading

avih commented Dec 23, 2020 •

edited

Loading

avih commented Dec 23, 2020 •

edited

Loading

avih commented Dec 23, 2020 •

edited

Loading

avih commented Dec 23, 2020 •

edited

Loading

anastasiuspernat Dec 28, 2020 •

edited

Loading

anastasiuspernat commented Dec 28, 2020 •

edited

Loading

skrew commented Dec 29, 2020 •

edited

Loading

Akemi commented Dec 29, 2020 •

edited

Loading

Akemi Jan 19, 2021 •

edited

Loading

anastasiuspernat commented Jan 19, 2021 •

edited

Loading