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

Jump to bottom
Merged
merged 2 commits into from
Oct 26, 2024
Merged

Fix colormanagement broken in 1.3.0, #3929 #5213

merged 2 commits into from
Oct 26, 2024

Conversation

low-batt
Copy link
Contributor

This commit will:

  • Add a new method setRenderICCProfile to VideoLayer that sets MPV_RENDER_PARAM_ICC_PROFILE
  • Remove setting of the mpv option icc-profile from the VideoView method setICCProfile
  • Change VideoView to use icc-profile-auto instead of icc-profile to control use of an ICC profile

Using icc-profile-auto frees icc-profile for use by users as icc-profile overrides icc-profile-auto. IINA must set MPV_RENDER_PARAM_ICC_PROFILE in order for icc-profile-auto to work.

Description:

@low-batt low-batt marked this pull request as draft October 14, 2024 22:34
@low-batt
Copy link
Contributor Author

This is a draft because we need to update the libmpv on our download server with the latest patches.

@low-batt low-batt requested a review from uiryuu October 14, 2024 22:37
@low-batt low-batt linked an issue Oct 14, 2024 that may be closed by this pull request
Colormanagement broken in 1.3.0 #3929
Closed
uiryuu
uiryuu reviewed Oct 15, 2024
View reviewed changes
Copy link
Member
@uiryuu uiryuu left a comment

Choose a reason for hiding this comment

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

Tested with libmpv+latest patches, confirmed user can override ICC profile when "Load ICC Profile" checkbox is checked. (The recompiled libmpv hasn't been uploaded yet)

A few questions here, not blocking this PR tho:

  1. videoLayer's colorspace is always set to window's colorspace, even though the user provide their own ICC profile. Is this desired?
  2. The vcolorspaceField now will only show the color profile of the screen (system setting), even though the user provide their own ICC profile. Is this correct?

@low-batt
Copy link
Contributor Author

I don't know enough about color spaces to provide an authoritative answer. What I can say is that the mpv code does not take into account the icc-profile option when setting the color space of the layer.

@svobs
Copy link
Contributor
svobs commented Oct 15, 2024 •
edited
Loading

A few questions here, not blocking this PR tho:

  1. videoLayer's colorspace is always set to window's colorspace, even though the user provide their own ICC profile. Is this desired?
  2. The vcolorspaceField now will only show the color profile of the screen (system setting), even though the user provide their own ICC profile. Is this correct?
  1. Not that I can completely answer this, but I think I have some understanding...
    As I talked about in my analysis in this post, setting the layer's colorspace is a Mac-only extra layer of translation, albeit one that must be configured to enable [E|H]DR. Whereas icc-profile relates to the screen, cocoa-cb-output-csp relates to the layer. But mpv is already doing colorspace management, right? So you wouldn't want too much more color mapping being done between "layer" and "screen"...
    Well, according to the docs for cocoa-cb-output-csp: To get correct results, this needs to be set to the color primaries/transfer characteristics of the VO target. So when using icc-profile-autowith cocoa-cb-output-csp=auto (as this PR is doing), both will be set to the screen's colorspace, which should ensure good results.
    If some power user specifies a different icc-profile, presumably that user needs to also figur 8000 e out the relevant value for cocoa-cb-output-csp, as well as target-trc and target-prim as the doc also notes. If they are crazy enough to provide all of those, then they probably have enough time on their hands to do the tons of fine-tuning that will be needed...
  2. Sounds like the ICC profile information is missing from the Inspector and should be added? It sounds like both icc-profile & icc-profile-auto are fairly new options (relative to the Inspector) which could have been missed.

@low-batt
Copy link
Contributor Author

Too late here for me to be thinking, so just one comment. The cocoa-cb-output-csp mpv option was added after the mpv 0.38.0 release and is missing from the libmpv IINA is using.

@uiryuu
Copy link
Member
uiryuu commented Oct 15, 2024 •
edited
Loading

Update: libmpv is uploaded. Convert this PR to ready for review.

@uiryuu uiryuu marked this pull request as ready for review October 15, 2024 07:46
@svobs
Copy link
Contributor
svobs commented Oct 15, 2024

Too late here for me to be thinking, so just one comment. The cocoa-cb-output-csp mpv option was added after the mpv 0.38.0 release and is missing from the libmpv IINA is using.

To be clear, I am not suggesting that cocoa-cb-output-csp should be configurable in IINA's UI. I was just pointing out that IINA is in effect implementing cocoa-cb-output-csp=auto when it sets videoLayer.colorspace equal to the screen's colorspace.

@low-batt low-batt mentioned this pull request Oct 15, 2024
Closed
3 tasks
svobs
svobs reviewed Oct 17, 2024
View reviewed changes
iina/ViewLayer.swift
@@ -347,6 +347,40 @@ 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.

svobs
svobs reviewed Oct 17, 2024
View reviewed changes
iina/VideoView.swift Outdated
logHDR("Not using ICC profile due to user preference")
player.mpv.setFlag(MPVOption.GPURendererOptions.iccProfileAuto, false)
} else if let screenColorSpace {
logHDR("Using 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.

Might be good to change this message to "Configuring auto ICC profile" or something with "auto" in it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

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 updated the log message to include the name of the color space:

19:10:15.183 [hdr0][d] Using the ICC profile of the color space LG UltraFine

I didn't mention any thing about automatically, because IINA doesn't take advantage of that aspect of the option. IINA is manually turning icc-profile-auto on and off.

svobs
svobs reviewed Oct 17, 2024
View reviewed changes
iina/ViewLayer.swift
/// 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) {
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.

8000

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.

@low-batt low-batt marked this pull request as draft October 18, 2024 01:38
@low-batt
Copy link
Contributor Author

Changed to draft while I look into the crashes @svobs encountered.

@low-batt
Fix colormanagement broken in 1.3.0, #3929
cc768bf 
This commit will:
- Add a new method setRenderICCProfile to VideoLayer that sets
  MPV_RENDER_PARAM_ICC_PROFILE
- Remove setting of the mpv option icc-profile from the VideoView method
  setICCProfile
- Change VideoView to use icc-profile-auto instead of icc-profile to
  control use of an ICC profile

Using icc-profile-auto frees icc-profile for use by users as icc-profile
overrides icc-profile-auto. IINA must set MPV_RENDER_PARAM_ICC_PROFILE
in order for icc-profile-auto to work.
@low-batt
Fix colormanagement broken in 1.3.0, #3929
a45a5a0 
This commit will:
- Add a new method setRenderICCProfile to VideoLayer that sets
  MPV_RENDER_PARAM_ICC_PROFILE
- Remove setting of the mpv option icc-profile from the VideoView method
  setICCProfile
- Change VideoView to use icc-profile-auto instead of icc-profile to
  control use of an ICC profile

Using icc-profile-auto frees icc-profile for use by users as icc-profile
overrides icc-profile-auto. IINA must set MPV_RENDER_PARAM_ICC_PROFILE
in order for icc-profile-auto to work.
@low-batt low-batt marked this pull request as ready for review October 23, 2024 23:21
@lhc70000 lhc70000 merged commit 58d623b into develop Oct 26, 2024
2 checks passed
@lhc70000 lhc70000 deleted the icc-profile branch October 26, 2024 02:03
MouJieQin pushed a commit to MouJieQin/iina-for-varchive that referenced this pull request Oct 27, 2024
@low-batt @MouJieQin
Fix colormanagement broken in 1.3.0, iina#3929 (iina#5213)
a0a457d 
* Fix  colormanagement broken in 1.3.0, iina#3929

This commit will:
- Add a new method setRenderICCProfile to VideoLayer that sets
  MPV_RENDER_PARAM_ICC_PROFILE
- Remove setting of the mpv option icc-profile from the VideoView method
  setICCProfile
- Change VideoView to use icc-profile-auto instead of icc-profile to
  control use of an ICC profile

Using icc-profile-auto frees icc-profile for use by users as icc-profile
overrides icc-profile-auto. IINA must set MPV_RENDER_PARAM_ICC_PROFILE
in order for icc-profile-auto to work.

* Fix  colormanagement broken in 1.3.0, iina#3929

This commit will:
- Add a new method setRenderICCProfile to VideoLayer that sets
  MPV_RENDER_PARAM_ICC_PROFILE
- Remove setting of the mpv option icc-profile from the VideoView method
  setICCProfile
- Change VideoView to use icc-profile-auto instead of icc-profile to
  control use of an ICC profile

Using icc-profile-auto frees icc-profile for use by users as icc-profile
overrides icc-profile-auto. IINA must set MPV_RENDER_PARAM_ICC_PROFILE
in order for icc-profile-auto to work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svobs svobs svobs left review comments

@uiryuu uiryuu uiryuu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Colormanagement broken in 1.3.0
4 participants
@low-batt @svobs @uiryuu @lhc70000
0