Close add HDR tone mapping settings, #4358 #4360

low-batt · 2023-04-18T02:44:37Z

This commit will:

Add the new settings enableToneMapping, toneMappingTargetPeak and toneMappingAlgorithm
Update the Audio/Video settings panel to allow the user to control these settings
Update VideoView.refreshEdrMode to support these new settings
Add the CoreDisplay framework to the Xcode project
Add support to VideoView.refreshEdrMode to automatically determine the target peak setting based on the peak brightness of the display using the CoreDisplay framework
Add reporting of the pixel format to the Inspector window
Enhance HDR detection in InspectorWindowController.updateInfo
Correct the path to the PiP framework in the Xcode project

These change provide the ability to mpv's tone mapping when playing HDR videos.

The main author of these changes is Carter Li. This is a merge of changes that have been tested in his IINA fork. A few minor changes were made during the merge.

I have read CONTRIBUTING.md
This implements/fixes issue Add HDR tone mapping settings #4358.

Description:

low-batt · 2023-04-18T02:50:26Z

Notes for Reviewers

A few things of note…

The changes add use of the CoreDisplay framework
Display information returned by macOS is different in older versions
The changes require the planned mpv upgrade as they will try and set tone-mapping to auto which is not available in mpv 0.34.1. Nothing drastically bad happens, you just need to change the setting to avoid mpv complaining.
The Target peak setting only accepts a number, it does not accept auto as the mpv target-peak does

Changes Made During Merge

I made some changes during the merge. Hopefully they seem reasonable, if not object!

Audio/Video Settings Panel

I indented the controls for Target peak and Algorithm as they are disabled when enable tone mapping is disabled. That makes them a part of a "hierarchy of settings". The HIG indicates indentation should be used to communicate that. Other similar IINA settings use indentation.

I greatly simplified the descriptions of the new settings to match the existing "minimal" style. Of note the original descriptions contain URLs pointing to the relevant section of the mpv manual:

Maximum luminance supported by your screen
if set to 0, IINA will try to detect the maximum luminance of current monitor
Ref: https://mpv.io/manual/master/#options-target-peak

That is an interesting idea. Of course these should be clickable links and we'd have to discuss whether they should point to a version specific manual rather than master. Since this is not done for other settings I thought it best to consider such a change for the feature release and update the descriptions of many settings.

I this particular case referencing the mpv manual might make you think you could use the value auto, but as noted above that is not accepted.

I also tried to correct constraint warnings reported by Xcode. @svobs, If you have time please take a look and see if I'm adding regressions with respects to layout constraints.

Logging

To avoid the large log message that results from logging the full display info dictionary I changed the logging in VideoView.requestEdrMode to only log the dictionary if we are unable to find either of the two keys.

Preference Handling

I changed the code in VideoView.requestEdrMode that obtains the algorithm from the preference setting to fall back to the default value if the preference value could not be converted into a mpv string. This matches up with how other code uses the HardwareDecoderOption preference enumeration. This prevents a crash if a user does something like this:

defaults write com.colliderli.iina.plist toneMappingAlgorithm 99

In other words even though users should not be messing with the preference file behind IINA's back we treat it as external input and provide some protection against invalid values.

Screen Shots

With the changes in this PR:

The original code with detailed descriptions of the settings:

CarterLi · 2023-04-18T06:21:40Z

iina/VideoView.swift

    mpv.setString(MPVOption.GPURendererOptions.targetPrim, primaries)
+    // PQ videos will be display as it was, HLG videos will be converted to PQ
+    mpv.setString(MPVOption.GPURendererOptions.targetTrc, "pq")
+    mpv.setFlag(MPVOption.Screenshot.screenshotTagColorspace, true)


This was used for screenshot in jxl format.

Since (newer) mpv actually doesn't support ppm / pgm ..., I suggest merging my screenshot code too.

I had not noticed the old screenshot formats. They were removed from the UI in commit 994d622 back in 2017. I guess they did not remove the values from the enum to preserve compatibility with old plists. Probably safe to remove them now. I found the IINA+ commit that adds the new formats. I will prepare a separate PR for that change. Thanks for pointing this out.

CarterLi · 2023-04-18T06:25:42Z

I like the style change in preferences window. Good work!

However, as tone-mapping algorithm is hard to understand for most people, I still prefer keeping the link to MPV document.

low-batt · 2023-04-19T03:55:16Z

I do like the idea of linking to the mpv manual. If the other reviewers approve I can make that change. I think they need to be clickable links. I'm thinking they should link to the stable manual and not master. Thoughts reviewers?

uiryuu · 2023-04-19T04:06:19Z

@low-batt Can you rebase this PR? I'm having a hard time building this PR since we've already updated our deps.

We should align the new options with previous options. I understand you are trying to align the texts, but this seems off to me. The original design looks better to me in this sense.

As for link to the mpv manual, we can definitely do that, but adding a clickable button to the link of mpv manual is better, just like we previously did:

low-batt · 2023-04-19T04:20:46Z

I will rebase and fix the merge conflicts. I will add ? links to mpv manual.

Late here. I will get this done tomorrow.

This commit will: - Add the new settings enableToneMapping, toneMappingTargetPeak and toneMappingAlgorithm - Update the Audio/Video settings panel to allow the user to control these settings - Update VideoView.refreshEdrMode to support these new settings - Add the CoreDisplay framework to the Xcode project - Add support to VideoView.refreshEdrMode to automatically determine the target peak setting based on the peak brightness of the display using the CoreDisplay framework - Add reporting of the pixel format to the Inspector window - Enhance HDR detection in InspectorWindowController.updateInfo - Correct the path to the PiP framework in the Xcode project These change provide the ability to mpv's tone mapping when playing HDR videos. The main author of these changes is Carter Li. This is a merge of changes that have been tested in his IINA fork. A few minor changes were made during the merge. Co-authored-by: CarterLi <carter.li@eoitek.com>

This commit will: - Add the new settings enableToneMapping, toneMappingTargetPeak and toneMappingAlgorithm - Update the Audio/Video settings panel to allow the user to control these settings - Add help buttons that link to Wikipedia and the appropriate sections of the mpv manual - Update VideoView.refreshEdrMode to support these new settings - Add the CoreDisplay framework to the Xcode project - Add support to VideoView.refreshEdrMode to automatically determine the target peak setting based on the peak brightness of the display using the CoreDisplay framework - Add reporting of the pixel format to the Inspector window - Enhance HDR detection in InspectorWindowController.updateInfo - Correct the path to the PiP framework in the Xcode project These change provide the ability to mpv's tone mapping when playing HDR videos. The main author of these changes is Carter Li. This is a merge of changes that have been tested in his IINA fork. A few minor changes were made during the merge. Co-authored-by: CarterLi <carter.li@eoitek.com>

low-batt · 2023-04-19T20:24:12Z

I rebased with develop and corrected the merge conflicts.

I added help buttons that link to:

The panel now looks like:

uiryuu

Should we call refreshEdrMode when the display is changed? A quick search shows that we are not doing that currently, but I might have missed that.

From a UI point of view, I think the text field under "Enable tone mapping" and the text field under the algorithm drop down do not provide useful information, given that we already provide the help link to the users. If they are confused by the settings, these text fields would not help.

For the text field under "Target peak", should we also inform the users that if we cannot detect automatically, the nits value will fallback to 400 nits?

uiryuu · 2023-04-22T01:09:57Z

iina/PrefCodecViewController.swift

@@ -99,4 +103,21 @@ class PrefCodecViewController: PreferenceViewController, PreferenceWindowEmbedda
    let hwdec: Preference.HardwareDecoderOption = Preference.enum(for: .hardwareDecoder)
    hwdecDescriptionTextField.stringValue = hwdec.localizedDescription
  }
+
+  @IBAction func updateEnableToneMapping(_ sender: NSButton) {


Can you do this in xib by bindings?

Yes, I'm changing this to binding enabling of the two controls to the checkbox in the XIB.

uiryuu · 2023-04-22T01:14:17Z

iina/VideoView.swift

+      player.mpv.setString(MPVOption.GPURendererOptions.targetPeak, "auto")
+      player.mpv.setString(MPVOption.GPURendererOptions.toneMapping, "auto")
+      player.mpv.setString(MPVOption.GPURendererOptions.toneMappingParam, "default")
+      player.mpv.setString(MPVOption.GPURendererOptions.toneMapping, "")


Why did you set this option twice? On line 319

player.mpv.setString(MPVOption.GPURendererOptions.toneMapping, "auto")

Because it is that way in the code in the other repository and I failed to notice that when preparing the merge. GOOD CATCH!

The "auto" for toneMapping was a recent add by mpv, so maybe the empty string setting was supposed to be removed after upgrading to 035.1?

@CarterLi Which is the correct value to use?

I'm going to guess "auto" and remove the setting to the empty string.

Just use the default value of MPV

The default is "auto", so I removed this line that is in IINA+

uiryuu · 2023-04-22T01:21:16Z

iina/VideoView.swift

+            targetPeak = hdrLuminance
+          } else if let hdrLuminance = displayInfo["DisplayBacklight"] as? Int {
+            // We know macOS Catalina uses this key.
+            Logger.log("Found DisplayBacklight: \(hdrLuminance)", subsystem: hdrSubsystem);


I did a search in this source and removed a number of semicolons.

uiryuu · 2023-04-22T01:21:19Z

iina/VideoView.swift

+          Logger.log("Successfully obtained information about the display", subsystem: hdrSubsystem)
+          // Prefer ReferencePeakHDRLuminance, which is reported by newer macOS versions.
+          if let hdrLuminance = displayInfo["ReferencePeakHDRLuminance"] as? Int {
+            Logger.log("Found ReferencePeakHDRLuminance: \(hdrLuminance)", subsystem: hdrSubsystem);


uiryuu · 2023-04-22T01:21:26Z

iina/VideoView.swift

+            Logger.log("Found DisplayBacklight: \(hdrLuminance)", subsystem: hdrSubsystem);
+            targetPeak = hdrLuminance
+          } else {
+            Logger.log("Didn't find ReferencePeakHDRLuminance or DisplayBacklight, assuming HDR400", subsystem: hdrSubsystem);


uiryuu · 2023-04-22T01:21:32Z

iina/VideoView.swift

+            targetPeak = 400
+          }
+        } else {
+          Logger.log("Unable to obtain display information", level: .warning, subsystem: hdrSubsystem);


uiryuu · 2023-04-22T01:21:40Z

iina/VideoView.swift

+      let algorithm = Preference.ToneMappingAlgorithmOption(rawValue: Preference.integer(for: .toneMappingAlgorithm))?.mpvString
+        ?? Preference.ToneMappingAlgorithmOption.defaultValue.mpvString
+
+      Logger.log("Will enable tone mapping target-peak=\(targetPeak) algorithm=\(algorithm)", subsystem: hdrSubsystem);


uiryuu · 2023-04-22T01:24:32Z

iina/VideoView.swift

+            targetPeak = 400
+          }
+        } else {
+          Logger.log("Unable to obtain display information", level: .warning, subsystem: hdrSubsystem);


We should also fallback the target peak to 400 if the display info cannot be obtained. Otherwise if we cannot obtain the display info, MPVOption.GPURendererOptions.targetPeak will be set to 0 later.

I've updated this to also default to 400.

This commit will: - Add the new settings enableToneMapping, toneMappingTargetPeak and toneMappingAlgorithm - Update the Audio/Video settings panel to allow the user to control these settings - Update VideoView.refreshEdrMode to support these new settings - Add the CoreDisplay framework to the Xcode project - Add support to VideoView.refreshEdrMode to automatically determine the target peak setting based on the peak brightness of the display using the CoreDisplay framework - Add reporting of the pixel format to the Inspector window - Enhance HDR detection in InspectorWindowController.updateInfo - Correct the path to the PiP framework in the Xcode project These change provide the ability to mpv's tone mapping when playing HDR videos. The main author of these changes is Carter Li. This is a merge of changes that have been tested in his IINA fork. A few minor changes were made during the merge. Co-authored-by: CarterLi <carter.li@eoitek.com>

low-batt · 2023-04-23T01:20:53Z

This sequence shows the window starting out on my external LG display which does not support HDR. Then moving to the internal XDR display which does support HDR. Then moving back. IINA enabled and disabled HDR appropriately:

IINA log:

21:01:01.361 [iina][d] Refreshing HDR for player0 @ display3: "LG UltraFine" visible frame (0.0, 0.0, 2560.0, 1415.0) EDR: {supports=false maxPotential=1.0 maxCurrent=1.0}
21:01:01.361 [hdr][d] HDR gamma=pq, primaries=bt.2020, sig_peak=4.926108360290527
21:01:01.361 [hdr][d] HDR video was found but the display does not support EDR mode
21:01:01.361 [hdr][d] Loading ICC profile
21:01:05.189 [iina][d] Falling back to nominal display refresh rate: 120.0 from 59.9990747912984
21:01:05.189 [iina][d] Refreshing HDR for player0 @ display1: "Built-in Retina Display" visible frame (458.0, -1056.0, 1728.0, 1018.0) EDR: {supports=true maxPotential=16.0 maxCurrent=1.0}
21:01:05.189 [hdr][d] HDR gamma=pq, primaries=bt.2020, sig_peak=4.926108360290527
21:01:05.190 [hdr][d] Will activate HDR color space instead of using ICC profile
21:01:05.191 [hdr][d] Successfully obtained information about the display
21:01:05.191 [hdr][d] Found ReferencePeakHDRLuminance: 1000
21:01:05.191 [hdr][d] Will enable tone mapping target-peak=1000 algorithm=auto
21:01:21.269 [iina][d] Falling back to nominal display refresh rate: 59.99925000937488 from 120.25120635180502
21:01:21.270 [iina][d] Refreshing HDR for player0 @ display3: "LG UltraFine" visible frame (0.0, 0.0, 2560.0, 1415.0) EDR: {supports=false maxPotential=1.0 maxCurrent=1.0}
21:01:21.270 [hdr][d] HDR gamma=pq, primaries=bt.2020, sig_peak=4.926108360290527
21:01:21.270 [hdr][d] HDR video was found but the display does not support EDR mode
21:01:21.270 [hdr][d] Loading ICC profile

I removed the two unhelpful text fields and changed the remaining one to indicate the fall back to 400. The panel now looks like:

low-batt requested a review from uiryuu April 18, 2023 02:50

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

Add HDR tone mapping settings #4358

Closed

CarterLi reviewed Apr 18, 2023

View reviewed changes

low-batt and others added 2 commits April 19, 2023 13:31

low-batt force-pushed the close-4358 branch from 855a8cf to a544038 Compare April 19, 2023 20:16

uiryuu requested changes Apr 22, 2023

View reviewed changes

uiryuu mentioned this pull request Apr 22, 2023

Release Schedule for IINA 1.3.2 #4379

Closed

23 tasks

Add a period

bdd36df

uiryuu merged commit 52708e7 into develop Apr 23, 2023

uiryuu deleted the close-4358 branch April 23, 2023 06:19

Uh oh!

Close add HDR tone mapping settings, #4358 #4360

Close add HDR tone mapping settings, #4358 #4360

Uh oh!

Conversation

low-batt commented Apr 18, 2023

Uh oh!

low-batt commented Apr 18, 2023

Notes for Reviewers

Changes Made During Merge

Audio/Video Settings Panel

Logging

Preference Handling

Screen Shots

Uh oh!

CarterLi Apr 18, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

low-batt commented Apr 19, 2023

Uh oh!

uiryuu commented Apr 19, 2023

Uh oh!

low-batt commented Apr 19, 2023

Uh oh!

low-batt commented Apr 19, 2023

Uh oh!

uiryuu left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

low-batt commented Apr 23, 2023

Uh oh!

Uh oh!

CarterLi Apr 18, 2023 •

edited

Loading

CarterLi commented Apr 18, 2023 •

edited

Loading