Lots for reviewers to criticize. See my comment in the issue for details about why this change is needed.

I replaced setting of isOpaque with setting of backgroundColor because that is what mpv does. But reading the doc for isOpaque it seemed like that is the better choice. I finally decided IINA should match up with mpv, but I am unsure about this change.

The modification to parse_doc.rb is another change that should be looked it. I put the enum type for the option value, CocoaCbSwRenderer, in MPVOption because possibly parse_doc.rb could be enhanced to generate that.

Another change I forgot to mention. This commit adds the CGL pixel format attribute kCGLPFABackingStore as mpv includes this attribute. From OpenGL Programming Guide for Mac:

When your application uses a double-buffered context, it displays the rendered image by calling a function to flush the image to the screen— theNSOpenGLContext class’s flushBuffer method or the CGL function CGLFlushDrawable. When the image is displayed, the contents of the back buffer are not preserved. The next time your application wants to update the back buffer, it must completely redraw the scene.

Your application can add a backing store attribute (NSOpenGLPFABackingStore or kCGLPFABackingStore) to preserve the contents of the buffer after the back buffer is flushed. Adding this attribute disables some optimizations that the system can perform, which may impact the performance of your application.

Is mpv doing partial draws?

And we need to discuss cocoa-cb-10bit-context:

Creates a 10bit capable pixel format for the context creation (default: yes). Instead of 8bit integer framebuffer a 16bit half-float framebuffer is requested.

macOS and cocoa-cb only.

Do we need to add UI for setting this? Is there a way to programmatically obtain this? Notes on the net indicate the old ways of doing so no longer work.

On this:

I replaced setting of isOpaque with setting of backgroundColor because that is what mpv does. But reading the doc for isOpaque it seemed like that is the better choice. I finally decided IINA should match up with mpv, but I am unsure about this change.

Not entirely clear what isOpaque is supposed to do, since there is already an opacity property which is more powerful. It looks very old. In theory it might at one point have provided a performance boost if set to true. But nowadays AppKit has many optimizations in place which check to see what is visible , and do not draw if the layer is hidden, off screen, transparent, occluded, and possibly other reasons. I have found that even if the layer is hidden or has 0% opacity, the player window is still 100% opaque and its background color is black, so a black background will be shown which is trivial for AppKit to draw.

Setting the layer's backgroundColor is also unnecessary because VideoView's entire frame is mapped with video which IINA is completely in charge of drawing. That drawing process never consults the layer's background, so the background will never be seen. And because the window is opaque, there is no need for AppKit to composite any layers under it, which means no real performance to be gained by setting it.

mpv render code for vo=gpu does not do partial draws, so it is more performant to not set kCGLPFABackingStore.

But I actually am curious if kCGLPFABackingStore even does anything, CAOpenGLLayer is not double-buffered in the traditional sense. I will run a test and see.

isOpaque is different from opacity. IsOpaque on views/layers skips drawRect (or compositing for CALayer trees?) for anything behind the layer/view marked as opaque. It used to be considered good practice to always mark a view bound to NSOpenGLContext as opaque. https://cocoadev.github.io/NSOpenGLView/.

However you seem to be correct that a lot of it is redundant now. It used to be the case that to create a transparent window you have to set [NSWindow setOpaque: NO]. Now it is no longer needed, possibly because as of 10.15 or something the root view is implicitly layer backed

Edit: I tested NSOpenGLPFABackingStore does not have any effect on a CAOpenGLLayer, you get a new FBO each time.

Btw is it correct to always set wantsExtendedDynamicRangeContent even if the video does not make use of EDR headroom?

Out of the things you mentioned, which one was responsible for the color differences that were mentioned on SDR video? I don't think 10-bit vs 8-bit backbuffer would matter much for color shifts, so I was assuming that the main difference was in how/whether the ICC profile was set.

On this:

It used to be the case that to create a transparent window you have to set [NSWindow setOpaque: NO]. Now it is no longer needed, possibly because as of 10.15 or something the root view is implicitly layer backed

Confusing. The default of CALayer.isOpaque is false. From the Apple documentation:

The default value of this property is false. If your app draws completely opaque content that fills the layer’s bounds, setting this property to true lets the system optimize the rendering behavior for the layer. Specifically, when the layer creates the backing store for your drawing commands, Core Animation omits the alpha channel of that backing store. Doing so can improve the performance of compositing operations. If you set the value of this property to true, you must fill the layer’s bounds with opaque content.

Seemed safer to set this to true. I only removed the IINA statement setting this to match up with mpv.

On this:

I tested NSOpenGLPFABackingStore does not have any effect on a CAOpenGLLayer, you get a new FBO each time.

Should I remove setting kCGLPFABackingStore with a comment explaining mpv sets it, but IINA does not think it is needed? I'm having a hard time making the call between matching mpv and not matching because we think mpv should not need to set something.

On this:

Out of the things you mentioned, which one was responsible for the color differences that were mentioned on SDR video? I don't think 10-bit vs 8-bit backbuffer would matter much for color shifts, so I was assuming that the main difference was in how/whether the ICC profile was set.

The color space of the layer was the primary difference by far. But with only that change the color of the blue sky still looked to my eyes slightly different compared to mpv. I just set cocoa-cb-10bit-context to no in advanced settings and ran the build for this PR. I definitely see a difference in the blue sky on the left side of the picture, but it is subtle.

The default of CALayer.isOpaque is false

Hm interesting, by default the contentView of an NSWindow does not have a layer property. Yet at the same time Apple documented somewhere in AppKit release notes that as of 10.15 (or maybe it was 10.12) all views are layer backed by default. Not sure how to square the two. Probably there is some hidden layer attribute not accessible via public API.

Seemed safer to set this to true.

It's probably more performant to set it to true, but there shouldn't really be any downside if leaving it as false. There is an edge-case where mpv technically supports displaying content with an alpha channel (e.g. transparent png) but upstream cocoa-cb mode doesn't support this properly either.

with a comment explaining mpv sets it, but IINA does not think it is needed

Up to you, from my perspective whether it is set or not has zero effect, from what I can see it's simply ignored by CAOpenGLLayer just like the double-buffering attribute.

On this line:

Another minor (not related to this PR) issue is that, I noticed that the entire setICCProfile function is just load the ICC profile for the current display, whereas there is already a mpv option --icc-profile-auto which

Automatically select the ICC display profile currently specified by the display settings of the operating system.

Could we just use this option instead?

I dug into this quite a bit and I have thoughts.

So, an ICC profile ≈ a ColorSync profile, yeah? And they aren't very useful unless you're converting from an input profile to an output profile.

I stared at this function for a long time (pasted below):

    override func updateICCProfile() {
        guard let colorSpace = window?.screen?.colorSpace else {
            log.warning("Couldn't update ICC Profile, no color space available")
            return
        }

        libmpv.setRenderICCProfile(colorSpace) // 1
        layer?.colorspace = getColorSpace()    // 2
    }

Took me a while to realize that in the last 2 lines, mpv is doing 2 separate things (although I am still super new to this, so welcome corrections):

1. Extracts the ICC profile for the "screen output" from window?.screen and uses it to supply the MPV_RENDER_PARAM_ICC_PROFILE to the mpv GL renderer.
2. Sets the video layer's ICC profile to a custom ICC profile from --cocoa-cb-output-csp if it was supplied. Otherwise it falls back to the screen's ICC profile.

It looks like item 1 needs to be reproduced in IINA, because the above code won't run when using libmpv.

Indeed the documentation for --icc-profile-auto says:

Applications using libmpv with the render API need to provide the ICC profile via MPV_RENDER_PARAM_ICC_PROFILE.

So to answer @uiryuu's question, this new code is essentially implementing --icc-profile-auto for item 1 above, because libmpv won't have the needed info because it has no NSWindow object. This is why setting --icc-profile explicitly is needed.

I think, if --icc-profile={profile_name} is supplied by the user in either IINA Advanced Settings, or via the command line, the user value should override this new logic. Could be a follow-on PR.

Item 2 above seems to be unique to Mac OS, and is represented by the --cocoa-cb-output-csp option. The docs say:

--cocoa-cb-output-csp=<csp>
This sets the color space of the layer to activate the macOS color transformation. Depending on the color space used the system's EDR (HDR) support will be activated. To get correct results, this needs to be set to the color primaries/transfer characteristics of the VO target.
[…]
macOS and cocoa-cb only.

From this comment this param seems to be a bit half-hearted (and also sheds some light on how much TLC is being given to cocoa-cb nowadays):

with the recent merge of the vulkan macOS backend for gpu(-next) (and making it default) this is already included with --target-colorspace-hint, eg through libplacebo and vulkan the proper path-through parameter/hints are already set automatically, and MoltenVK maps those settings to the right colorspaces, edr data and dynamic ranges.
[…]
implementing this feature, or rather merging mpv-player/mpv#8485, will not work in the vulkan case, since it will interfere with libplacebo/vulkan/moltenvk and actually breaks HDR output. so it shouldn't be added to the metal layer.

that would only leave the old opengl cocoa-cb backend that is more or less deprecated. i could cleanup my PR and add it to cocoa-cb, though it is more or less just a makeshift solution and will never be as good/automatic as --target-colorspace-hint and is only a platform specific solution.

@uiryuu's other point is interesting:

I assume that this results in always setting CAOpenGLLayer's color space to the screen ICC profile. However we have a setting in the Video/Audio tab which lets the user to choose whether or not load ICC profile. You said that in the testing, you disabled load ICC profile to match mpv's behavior, but will enabling load ICC profile change the color on this branch?

I don't see a good reason for that checkbox to exist, TBH. Until now I didn't understand it, so I left it unchecked. But now it seems that the correct behavior is to always leave it checked. If anything, a much more useful feature would be to have a popup selection of ICC profiles to choose from. But if the checkbox is left as-is, how about: "if checked, use the screen's profile; if unchecked, always use generic RGB. Unless HDR is enabled for the video; in which case the checkbox setting is ignored entirely & the HDR settings are used instead."

Running out of time tonight. But need to check what this does exactly, because it's not documented:

player.mpv.setString(MPVOption.GPURendererOptions.iccProfile, "")

I was worried about fully updating IINA in one huge PR. So for this PR I tried to be conservative and focused only on addressing #5124. I was going to follow up with changes related to the ICC profile. @svobs pointed out the key change I left out, setting MPV_RENDER_PARAM_ICC_PROFILE which is needed for --icc-profile-auto. And yes, once that option is working probably the IINA setting should be changed to use it.

Should I go ahead and add the ICC profile changes to this PR?

And yes, once that option is working probably the IINA setting should be changed to use it.

Oh yes, for others who are reading this, I forgot to note that IINA needs to update to libmpv 0.39.0 because of this fix in client-api (among other fixes…):

--- mpv 0.39.0 ---
2.4    - mpv_render_param with the MPV_RENDER_PARAM_ICC_PROFILE argument no
         longer has incorrect assumptions about memory allocation and can be
         correctly used.

Should I go ahead and add the ICC profile changes to this PR?

Not totally clear on which part you mean. Ideally here are all the changes I can think of to fully flesh out handling of ICC profiles, along with my perceived importance of each:

1. Add code to favor --icc-profile when the user includes it in Advanced > Additional mpv options or via command line arg. I suppose we can push this to a future PR.
2. Change Load ICC profile from checkbox to ICC profile popup, defaulting to Auto or Same As Screen. Future PR.
3. Add support for custom ICC profile for video layer via --cocoa-cb-output-csp - No, don't think we need this anytime soon, if ever.
4. Set MPV_RENDER_PARAM_ICC_PROFILE & ensure the param is working - Needs upgrade to libmpv 0.39.0 as prerequisite, as noted above. Maybe fine to include in future PR, but should do it as soon as feasible.
5. Yet another thing I forgot to mention: the mappings between ICC profile name string (mpv) and CGColorSpace in VideoView.requestEdrMode have fallen out-of-date with respect to mpv's getColorSpace logic, which includes mappings up to Mac OS 12. These should be reviewed and updated. Definitely fine for a future PR.

In short, I suppose I'm fine with leaving this PR as-is if you want to break out the remaining ICC stuff into a separate PR, given that some of this stuff is still bleeding edge and dependencies need to be updated first.

I see this PR has been merged. I will start working on the changes related to ICC profiles.

IINA prefers to use a pure version of official mpv and FFmpeg releases, but that doesn't always work out. As can be seen here we have been applying patches to mpv 0.38.0, including the fix for MPV_RENDER_PARAM_ICC_PROFILE. Last I heard we still need to rebuild our libmpv to pickup the latest of those patches.

If mpv had fixed the AVFoundation audio problems, we'd consider either patching or updating libmpv. But mpv has not fixed the new driver as far as I know. At this point we want to postpone upgrading to mpv 0.39.0 and focus on getting this release out the door.

The issue of IINA stepping on user's mpv settings extends beyond --icc-profile. The user might want to control the other settings IINA started using with the addition of HDR support. But it is not enough for IINA to avoid setting options if they appear in IINA's Advanced settings. That does not cover the user using mpv.conf. The mpv.conf file supports using Conditional auto profiles which is desirable for settings like color. We either need settings to suppress IINA's use of specific mpv options, or a general setting that allows users to add mpv option names to suppress.

In the case at hand, if IINA can switch to using --icc-profile-auto then the user can override IINA's use by using --icc-profile since that option overrides --icc-profile-auto.

I will start working on the ICC profile changes.

The issue of IINA stepping on user's mpv settings extends beyond --icc-profile. The user might want to control the other settings IINA started using with the addition of HDR support. But it is not enough for IINA to avoid setting options if they appear in IINA's Advanced settings. That does not cover the user using mpv.conf. The mpv.conf file supports using Conditional auto profiles which is desirable for settings like color. We either need settings to suppress IINA's use of specific mpv options, or a general setting that allows users to add mpv option names to suppress.

This definitely deserves its own discussion. I've been thinking about that issue as well. Specifically, I think it would be great if the Additional mpv options table in Advanced Settings could be modified to include additional columns which show the set of properties which were parsed from the command line and the properties generated from prefs in MPVController.mpvInit. By showing them side-by-side in a table and graying out / striking out the properties which were overridden, this would allow power users to get insight about how properties are handled, as well as general troubleshooting. Unfortunately I still don't have a good plan for handling profiles & other data from mpv.conf. As of now I am inclined to say that the best solution might be to patch libmpv to expose its conf parsing functionality as public APIs, which IINA can call over the C bridge. But even after that, there is probably a lot of work to do to interpret profile data.