8000 Fix colormanagement broken in 1.3.0, #3929 by low-batt · Pull Request #5213 · iina/iina · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix colormanagement broken in 1.3.0, #3929 #5213

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 26, 2024
Merged

Fix colormanagement broken in 1.3.0, #3929 #5213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc768bf
Fix colormanagement broken in 1.3.0, #3929
low-batt Oct 14, 2024
a45a5a0
Fix colormanagement broken in 1.3.0, #3929
low-batt Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file 8000
Failed to load files.
Loading
Diff view
Diff view
43 changes: 11 additions & 32 deletions iina/VideoView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,39 +299,18 @@ class VideoView: NSView {
RunLoop.current.add(displayIdleTimer!, forMode: .default)
}

func setICCProfile(_ displayId: UInt32) {
private func setICCProfile() {
let screenColorSpace = player.mainWindow.window?.screen?.colorSpace
if !Preference.bool(for: .loadIccProfile) {
logHDR("Not using ICC due to user preference")
player.mpv.setString(MPVOption.GPURendererOptions.iccProfile, "")
} else {
logHDR("Loading ICC profile")
typealias ProfileData = (uuid: CFUUID, profileUrl: URL?)
guard let uuid = CGDisplayCreateUUIDFromDisplayID(displayId)?.takeRetainedValue() else { return }

var argResult: ProfileData = (uuid, nil)
withUnsafeMutablePointer(to: &argResult) { data in
ColorSyncIterateDeviceProfiles({ (dict: CFDictionary?, ptr: UnsafeMutableRawPointer?) -> Bool in
if let info = dict as? [String: Any], let current = info["DeviceProfileIsCurrent"] as? Int {
let deviceID = info["DeviceID"] as! CFUUID
let ptr = ptr!.bindMemory(to: ProfileData.self, capacity: 1)
let uuid = ptr.pointee.uuid

if current == 1, deviceID == uuid {
let profileURL = info["DeviceProfileURL"] as! URL
ptr.pointee.profileUrl = profileURL
return false
}
}
return true
}, data)
}

if let iccProfilePath = argResult.profileUrl?.path, FileManager.default.fileExists(atPath: iccProfilePath) {
player.mpv.setString(MPVOption.GPURendererOptions.iccProfile, iccProfilePath)
}
logHDR("Not using ICC profile due to user preference")
player.mpv.setFlag(MPVOption.GPURendererOptions.iccProfileAuto, false)
} else if let screenColorSpace {
let name = screenColorSpace.localizedName ?? "unnamed"
logHDR("Using the ICC profile of the color space \(name)")
player.mpv.setFlag(MPVOption.GPURendererOptions.iccProfileAuto, true)
videoLayer.setRenderICCProfile(screenColorSpace)
}

let screenColorSpace = player.mainWindow.window?.screen?.colorSpace
let sdrColorSpace = screenColorSpace?.cgColorSpace ?? VideoView.SRGB
if videoLayer.colorspace != sdrColorSpace {
let name: String = {
Expand Down Expand Up @@ -435,7 +414,7 @@ extension VideoView {
if player.info.hdrAvailable != edrAvailable {
player.mainWindow.quickSettingView.setHdrAvailability(to: edrAvailable)
}
if edrEnabled != true { setICCProfile(displayId) }
if edrEnabled != true { setICCProfile() }
}

func requestEdrMode() -> Bool? {
Expand Down Expand Up @@ -493,7 +472,7 @@ extension VideoView {
logHDR("Will activate HDR color space instead of using ICC profile")

videoLayer.colorspace = CGColorSpace(name: name!)
mpv.setString(MPVOption.GPURendererOptions.iccProfile, "")
mpv.setFlag(MPVOption.GPURendererOptions.iccProfileAuto, false)
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")
Expand Down
38 changes: 38 additions & 0 deletions iina/ViewLayer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,44 @@ class ViewLayer: CAOpenGLLayer {
return ctx
}

// MARK: - ICC Profile
Copy link
Contributor
@svobs svobs Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this put in ViewLayer? Looks like it would fit better in MPVController (or even just VideoView). You could pull out the videoView.$isUninited.withLock into the calling method to keep it inside VideoView and reduce level of nested code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm slow in responding. Been focused on the JPEG XL problems.

The plan is to put it in MPVRenderer, along with all the render related code currently in MPVController, similar to how mpv structures the code. But I got interrupted doing that refactoring. As we are now looking to freeze changes, test and release the first beta, this refactoring will have to wait for a future release. So this method is here temporarily.


/// Set an ICC profile for use with the mpv [icc-profile-auto](https://mpv.io/manual/stable/#options-icc-profile-auto)
/// option.
///
/// This method fulfills the mpv requirement that applications using libmpv with the render API provide the ICC profile via
/// `MPV_RENDER_PARAM_ICC_PROFILE` in order for the `--icc-profile-auto` option to work. The ICC profile data will not
/// be used by mpv unless the option is enabled.
///
/// The IINA `Load ICC profile` setting is tied to the `--icc-profile-auto` option. This allows users to override IINA using
/// the [--icc-profile](https://mpv.io/manual/stable/#options-icc-profile) option.
func setRenderICCProfile(_ profile: NSColorSpace) {
// The OpenGL context must always be locked before locking the isUninited lock to avoid
// deadlocks.
videoView.player.mpv.lockAndSetOpenGLContext()
defer { videoView.player.mpv.unlockOpenGLContext() }
videoView.$isUninited.withLock() { isUninited in
Copy link
Contributor
@svobs svobs Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged this PR into my own code yesterday and have since been getting frequent crashes at startup when using the Xcode debugger, coming from inside mpv_render_context_render. The last 2 I saved:

  1. Assertion failed: (tex->params.render_src), function update_uniform, file ra_gl.c, line 990.
  2. Assertion failed: (h->canary == CANARY), function ta_dbg_check_header, file ta.c, line 291.

I did some experiments and it looks like I fixed it by locking the OpenGL context around the isUninited lock here in setRenderICCProfile. I realize my code is different than develop but I do suspect the problem exists here but just may be more rare. I suspect mpv needs calls to mpv_render_context_set_parameter to be isolated from all other rendering-related operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make this a draft and look into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was it. I added the code to lock the OpenGL context.

guard !isUninited else { return }

guard let renderContext = videoView.player.mpv.mpvRenderContext else { return }
guard var iccData = profile.iccProfileData else {
let name = profile.localizedName ?? "unnamed"
Logger.log("Color space \(name) does not contain ICC profile data", level: .warning)
return
}
iccData.withUnsafeMutableBytes { (ptr: UnsafeMutableRawBufferPointer) in
guard let baseAddress = ptr.baseAddress, ptr.count > 0 else { return }

let u8Ptr = baseAddress.assumingMemoryBound(to: UInt8.self)
var icc = mpv_byte_array(data: u8Ptr, size: ptr.count)
withUnsafeMutableBytes(of: &icc) { (ptr: UnsafeMutableRawBufferPointer) in
let params = mpv_render_param(type: MPV_RENDER_PARAM_ICC_PROFILE, data: ptr.baseAddress)
mpv_render_context_set_parameter(renderContext, params)
}
}
}
}

// MARK: - Utils

/** Check OpenGL error (for debug only). */
Expand Down
0