Experimental HDR support on macOS using colorSpace approach

[anastasiuspernat]

Enabling support for HDR monitors (in manual mode).

Main discussion is here: https://github.com/mpv-player/mpv/issues/7341

Added a new switch called --macos_hdr_csp (similar to d3d color space approach at https://github.com/mpv-player/mpv/pull/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.

[Akemi]

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.

[anastasiuspernat]

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]

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

[anastasiuspernat]

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]

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]

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]

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]

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

[avih]

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.

[Akemi]

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.

[anastasiuspernat]

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]

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

[Akemi]

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]

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

[Akemi]

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

[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.

[anastasiuspernat]

@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]

@Akemi @anastasiuspernat any progress on this?

[ValZapod]

• DCI P3 primaries

You mean p3-d65. That is display-p3 in format and target-prim, dci-p3 uses dci whitepoint, not D65.

BTW, https://patchwork.ffmpeg.org/project/ffmpeg/patch/[email protected]/

[JulyIghor]

any progress on this?

[Akemi]

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 (https://github.com/mpv-player/mpv/issues/7341#issuecomment-1967674781).