Color Management (OS X): Use system configured display profile automatically

[UliZappe]

Note: I can argue for OS X only, but of course, the issue might be relevant for the other platforms, too.

Now that color management works correctly :-) , it would certainly make sense from a usage point of view to offer an option to automatically use the system configured ICC display profile automatically, instead of having to configure it explicitly for mpv.

I.e. something like :icc-profile=auto should automatically use whatever display profile the user has set in the OS X System Preferences.

I will provide the code to figure out the path of the system’s display profile below, but I have too little insight in mpv’s architecture to know how to integrate this functionality correctly.

I can see two major architectural questions:

1. Can the color management LUT table be changed during playback?

If yes, mpv could adjust its color if the user changes its display profile during playback. (It would register to get a notification whenever the user changes the display profile for a screen, which is easy to implement.)

This is probably not the most important feature, but it could be useful. For instance, I use two different display profiles for daylight and artificial light. If I start watching a movie during the day, pause it, and continue in the night, such intelligent adaptation would be useful. Again, not the most important feature, but one that would make the integration in the OS "perfect". (QuickTime Player Pro X does it.)

2. In a multi-display configuration, can the user change the display during playback?

If yes, then point 1.) – the possibility to change the display profile during playback – would basically become mandatory, since different displays will almost certainly have different display profiles. And if I understand the --screen and --fullscreen options correctly, this is indeed the case.

The specific API of the function that provides the path of the display profile will probably depend on the answer to these two questions.

If no change of the display profile during playback is required and only the display profile of the currently used display needs to be set at launch, the following function is sufficient:

 Boolean get_screen_profile(char *pathBuffer, size_t maxPathSize)
{
    CFUUIDRef displayUUID = CGDisplayCreateUUIDFromDisplayID(kCGDirectMainDisplay);
    if (CFGetTypeID(displayUUID) == CFNullGetTypeID()) {MP_ERR(s, "Cannot get display UUID.\n"); return false;}

    CFDictionaryRef displayInfo = ColorSyncDeviceCopyDeviceInfo(kColorSyncDisplayDeviceClass, displayUUID);
    if (!displayInfo) {MP_ERR(s, "Cannot get display info.\n"); CFRelease(displayUUID); return false;}
    CFRelease(displayUUID);
    CFDictionaryRef customProfileInfo = CFDictionaryGetValue(displayInfo, kColorSyncCustomProfiles);
    if (!customProfileInfo) {MP_ERR(s, "Cannot get display profile info.\n"); CFRelease(displayInfo); return false;}

    CFURLRef customProfileURLs[CFDictionaryGetCount(customProfileInfo)];
    CFDictionaryGetKeysAndValues(customProfileInfo, NULL, (const void **)customProfileURLs);
    if ((void*)customProfileURLs[0] == kCFNull) {MP_ERR(s, "Cannot get display profile URL.\n"); CFRelease(displayInfo); return false;}
    Boolean result = CFURLGetFileSystemRepresentation(customProfileURLs[0], true, (UInt8*)pathBuffer, maxPathSize);
    CFRelease(displayInfo);

    if (!result) MP_ERR(s, "Cannot get display profile path.\n");
    return result;
}

(Note that I’m unsure about the log levels you would consider adequate.)

You can directly insert this function as is in video/out/cocoa_common.m, or even in video/out/gl_lcms.c if you additionally add

#include <ApplicationServices/ApplicationServices.h>

You would call the function as follows:

char buffer[PATH_MAX];
Boolean result = get_screen_profile(buffer, PATH_MAX);

If successful, buffer will contain the path to the ICC display profile of the current display, which can then be used for the load_file() call in mp_load_icc().

However, if the profile must be changeable during playback, you’ll probably need an API similar to this in video/out/cocoa_common.m (EDIT: updated code for retrieving the display ID):

static Boolean get_screen_profile(struct vo *vo, Boolean fullscreen, char *pathBuffer, size_t maxPathSize)
{
    struct vo_cocoa_state *s = vo->cocoa;
    NSScreen *screen = fullscreen? s->fs_screen : s->current_screen;
    const char *screenstring = fullscreen? "full screen " : "";

    CGDirectDisplayID displayID = (CGDirectDisplayID)[[[screen deviceDescription] valueForKey:@"NSScreenNumber"] unsignedLongValue];
    CFUUIDRef displayUUID = CGDisplayCreateUUIDFromDisplayID(displayID);
    if (CFGetTypeID(displayUUID) == CFNullGetTypeID()) {MP_ERR(s, "Cannot get %sdisplay UUID.\n", screenstring); return false;}

    CFDictionaryRef displayInfo = ColorSyncDeviceCopyDeviceInfo(kColorSyncDisplayDeviceClass, displayUUID);
    if (!displayInfo) {MP_ERR(s, "Cannot get %sdisplay info.\n", screenstring); CFRelease(displayUUID); return false;}
    CFRelease(displayUUID);
    CFDictionaryRef customProfileInfo = CFDictionaryGetValue(displayInfo, kColorSyncCustomProfiles);
    if (!customProfileInfo) {MP_ERR(s, "Cannot get %sdisplay profile info.\n", screenstring); CFRelease(displayInfo); return false;}

    CFURLRef customProfileURLs[CFDictionaryGetCount(customProfileInfo)];
    CFDictionaryGetKeysAndValues(customProfileInfo, NULL, (const void **)customProfileURLs);
    if ((void*)customProfileURLs[0] == kCFNull) {MP_ERR(s, "Cannot get %sdisplay profile URL.\n", screenstring); CFRelease(displayInfo); return false;}
    Boolean result = CFURLGetFileSystemRepresentation(customProfileURLs[0], true, (UInt8*)pathBuffer, maxPathSize);
    CFRelease(displayInfo);

    if (!result) MP_ERR(s, "Cannot get %sdisplay profile path.\n", screenstring);
    return result;
}

A pseudo code example to call this function would be:

void vo_cocoa_update_screen_profile_info(struct vo *vo)
{
    char buffer[PATH_MAX];
    if (get_screen_profile(vo, false, buffer, PATH_MAX)) printf("current screen profile: %s\n", buffer);
    if (get_screen_profile(vo, true, buffer, PATH_MAX)) printf("current fullscreen profile: %s\n", buffer);
}

This code example simply prints out the ICC profile paths for the screen and the fullscreen display. I’m not too sure about how mpv uses the vo_cocoa_state struct and about the meaning of its fs_screen and current_screen members, and how an update of its values would be triggered during playback.

What do you think?

[pigoz]

I was working on this two days ago. Yesterday I got home really late because of RL issues and could not complete my code. I didn't know about ColorSyncDeviceCopyDeviceInfo so in my code I just iterated of all color profiles. I will probably have a working commit this evening or tomorrow.

[haasn]

Does anybody know what the situation is like on Linux? I don't know if X even has any sort of concept of display profiles, but surely even if it doesn't some standards must exist to combat this.

It would be a cool feature to get profile auto-detection for other platforms as well. I know Windows has a concept of display profiles, too.

Edit: This seems relevant: http://www.argyllcms.com/doc/ucmm.html

Looks like my system indeed has it set up already, due to the wonders of dispcalGUI. Do we already rely on XDG stuff?

Edit: This seems relevant, too: http://www.burtonini.com/computing/x-icc-profiles-spec-0.2.html

Less reliant on XDG, too.

[pigoz]

No idea, I discussed this a few days ago with wm4 and this will implemented as a VOCTL command where the backends can reply with a profile path.

The main problem is that changing the profile during playback, will probably require some sort of VO reinitialization (something that is not currently handled in mpv's architecture).

[haasn]

I think that is a separate issue and it would be nice to get profile autodetection working at all for now.

Edit: With argyll-dispwin, if you install the .icc profile with -I then the _ICC_PROFILE property of the root X window seems to hold the profile as intended, at least over here.

[ghost]

The main problem is that changing the profile during playback, will probably require some sort of VO reinitialization (something that is not currently handled in mpv's architecture).

I think switching between existing profiles wouldn't be too hard, but we need some kind of notification mechanism etc., so it would require figuring out how it should work. vo_opengl itself probably won't need a full reinit. It just has to recompute the lookup table, and then make gl_video.c to reinitialize somehow.

Another thing is that this would break the icc-cache. Maybe that's ok, since most will use that only when configuring the icc profile manually.

[ghost]

Edit: This seems relevant: http://www.argyllcms.com/doc/ucmm.html

Looks like my system indeed has it set up already, due to the wonders of dispcalGUI. Do we already rely on XDG stuff?

Edit: This seems relevant, too: http://www.burtonini.com/computing/x-icc-profiles-spec-0.2.html

Less reliant on XDG, too.

Both of these look pretty annoying. First we should find out which one is "more" standard.

PS: and of course, color management on Linux is a ghetto. Maybe there isn't even a standard behavior across the desktop environments.

Also, @giselher: do you know if Wayland has anything?

[UliZappe]

@wm4

I think switching between existing profiles wouldn't be too hard, but we need some kind of notification mechanism

In OS X, this would be NSScreenColorSpaceDidChangeNotification, but @pigoz is certainly aware of that.

If additional screens are added (e.g. a projector is connected to a laptop), OS X sends an NSApplicationDidChangeScreenParametersNotification; then, mpv would have to register for yet another NSScreenColorSpaceDidChangeNotification for this new screen.

Also useful might be [NScreen deepestScreen], which returns the display with the highest color quality (bit depth), so if e.g. a laptop display (usually lower bit depths and dithering) and a projector are in mirroring mode, the "deepest screen" would be the projector, and its color profile should be used.

[ghost]

Well, I mean mpv-internally. Basically an annoying design decision how to propagate the system notification.

[UliZappe]

I found an improvement for the code of the second function mentioned above:

 static Boolean get_screen_profile(struct vo *vo, Boolean fullscreen, char *pathBuffer, size_t maxPathSize)

I overlooked the direct way to get the display ID from an NSScreen, so I used an awkward piece of code which retrieves the display ID from the frame of the NSScreen:

CGDirectDisplayID displayID;
uint32_t count;
CGError error =  CGGetDisplaysWithRect(NSRectToCGRect([screen frame]), 1, &displayID, &count);
if (error) {MP_ERR(s, "Cannot get %sdisplay ID.\n", screenstring); return false;}

The correct replacement for this code is:

 CGDirectDisplayID displayID = (CGDirectDisplayID)[[[screen deviceDescription] valueForKey:@"NSScreenNumber"] unsignedLongValue];

I will update the original code above accordingly. Only the new version should be used.

[pigoz]

Sorry, I was really busy with RL stuff the past week. I have started working on it on my commute.

Why do you get the first element of the custom profiles dictionary values array? Is that always guaranteed to be the profile currently in use?

Apart from that, your code looks ok and I'm integrating it into mpv. On a first commit I will probably ignore the issues with regard to monitor switching and just use the current monitor. This is not only a color management issue, since it's also a problem with monitors with different pixel density (retina vs normal), color depth, etc.

[UliZappe]

Why do you get the first element of the custom profiles dictionary values array? Is that always guaranteed to be the profile currently in use?

It seems so. Unfortunately, there’s no documentation apart from the header files, which don’t say anything about that.

But in my tests, this has always worked. In fact, I don’t even think you can set more than 1 "custom" (= current) profile for a monitor (per user, but we only care about the current user). The usual GUI way, via System Preferences > Displays > Color, always replaces the previous custom profile by the new one (if you select the default profile from Apple, then this profile also becomes the current "custom" profile), and when you use ColorSync Utility > Devices to set the current profile, the Open panel with which you specify the path to the new custom profile only lets you select 1 profile which, again, replaces the old custom profile. This differs from printers, where (depending on the printer driver) you might have a custom profile for RGB printing, one for CMYK, one for gray etc.

[UliZappe]

I just checked again and I found one exception:

If you select the monitor default profile in ColorSync Utility > Devices, then (contrary to doing so in System Preferences) the "custom profile" becomes actually undefined and the code must fall back to the default profile instead. This is probably the state after a fresh OS X installation, so it certainly matters. I will update my code accordingly tonight.

[pigoz]

I think its just a matter of querying for kColorSyncFactoryProfiles as well. If that's the case I already am making the needed code.

[UliZappe]

True, but currently I am testing this and get strange behavior. I’ll try a bit more.

[UliZappe]

OK, I rewrote the whole thing. It’s a bit convoluted, but it’s now hopefully as "ColorSync official" as it gets. (It also removes the arbitrary selection of the first returned custom profile that you asked about.)

If you came to basically the same result, then obviously you need not care. :-)

static Boolean get_screen_profile(struct vo *vo, Boolean fullscreen, char *pathBuffer, size_t maxPathSize)
{
    Boolean result;
    Boolean useOnlyEntry = false;
    CFTypeRef valueArray[1];
    CFURLRef profileURL;

    struct vo_cocoa_state *s = vo->cocoa;
    NSScreen *screen = fullscreen? s->fs_screen : s->current_screen;
    const char *screenstring = fullscreen? "full screen " : "";

    CGDirectDisplayID displayID = (CGDirectDisplayID)[[[screen deviceDescription] valueForKey:@"NSScreenNumber"] unsignedLongValue];

    CFUUIDRef displayUUID = CGDisplayCreateUUIDFromDisplayID(displayID); // for the current display, use kCGDirectMainDisplay instead of displayID
    if (CFGetTypeID(displayUUID) == CFNullGetTypeID()) {MP_ERR(s, "Cannot get %sdisplay UUID.\n", screenstring); return false;}

    CFDictionaryRef deviceInfo = ColorSyncDeviceCopyDeviceInfo(kColorSyncDisplayDeviceClass, displayUUID);
    if (!deviceInfo) {MP_ERR(s, "Cannot get %sdisplay info.\n", screenstring); CFRelease(displayUUID); return false;}
    CFRelease(displayUUID);

    CFDictionaryRef factoryInfo = CFDictionaryGetValue(deviceInfo, kColorSyncFactoryProfiles);
    if (!factoryInfo) {MP_ERR(s, "Cannot get %sdisplay factory settings.\n", screenstring); CFRelease(deviceInfo); return false;}
    CFStringRef defaultProfileID = CFDictionaryGetValue(factoryInfo, kColorSyncDeviceDefaultProfileID);
    if (!defaultProfileID)  useOnlyEntry = true;        // if no profile ID is specified, the profile dictionaries are guaranteed to contain only one entry, which we will use

    CFDictionaryRef profileInfo = CFDictionaryGetValue(deviceInfo, kColorSyncCustomProfiles);
    if (profileInfo)
    {
        if (useOnlyEntry)
        {
            if (CFDictionaryGetCount(profileInfo) != 1) {MP_ERR(s, "Inconsistent %sdisplay profile setting.\n", screenstring); CFRelease(deviceInfo); return false;}
            CFDictionaryGetKeysAndValues(profileInfo, NULL, (const void **)valueArray);
            // If profileURL is kCFNull or NULL, the profile URL could not be retrieved although a custom profile was specified.
            // This points to a configuration error, so we should not fall back to the factory profile, but return an error instead.
            if ((void*)valueArray[0] == kCFNull) {MP_ERR(s, "Cannot get %sdisplay profile URL.\n", screenstring); CFRelease(deviceInfo); return false;}
            profileURL = valueArray[0];
        }
        else profileURL = CFDictionaryGetValue(profileInfo, defaultProfileID);
        if (!profileURL) {MP_ERR(s, "Cannot get %sdisplay profile URL.\n", screenstring); CFRelease(deviceInfo); return false;}
    }
    else    // No custom profile specified; try factory profile for the device
    {
        CFDictionaryRef defaultProfileInfo;

        if (useOnlyEntry)
        {
            if (CFDictionaryGetCount(factoryInfo) != 1) {MP_ERR(s, "Inconsistent %sdisplay factory profile setting.\n", screenstring); CFRelease(deviceInfo); return false;}
            CFDictionaryGetKeysAndValues(factoryInfo, NULL, (const void **)valueArray);
            if ((void*)valueArray[0] == kCFNull) {MP_ERR(s, "Cannot get %sdisplay profile info.\n", screenstring); CFRelease(deviceInfo); return false;}
            defaultProfileInfo = valueArray[0];
        }           
        else defaultProfileInfo = CFDictionaryGetValue(factoryInfo, defaultProfileID);
        if (!defaultProfileInfo) {MP_ERR(s, "Cannot get %sdisplay profile info.\n", screenstring); CFRelease(deviceInfo); return false;}
        profileURL = CFDictionaryGetValue(defaultProfileInfo, kColorSyncDeviceProfileURL);
        if (!profileURL) {MP_ERR(s, "Cannot get %sdisplay factory profile URL.\n", screenstring); CFRelease(deviceInfo); return false;}
    }

    result = CFURLGetFileSystemRepresentation(profileURL, true, (UInt8*)pathBuffer, maxPathSize);
    CFRelease(deviceInfo);
    if (!result) MP_ERR(s, "Cannot get %sdisplay profile path.\n", screenstring);
    return result;
}

[UliZappe]

it’s now hopefully as "ColorSync official" as it gets

Well, it still isn’t, as I just realized that kColorSyncDeviceDefaultProfileID is optional if only one factory profile exists (which will probably always be the case for displays). Apple does specify it, but we cannot rely on that (if e.g. some third-party profiling software modifies this setting). Sigh – this makes the whole thing even more complex, as we’ll have to write code to use the first = only value of the dictionaries in case kColorSyncDeviceDefaultProfileID is undefined. I’ll try to supplement the above code with that.

[UliZappe]

I updated the code. As long as I didn’t make some stupid copy&paste error, it should work.

[UliZappe]

As long as I didn’t make some stupid copy&paste error

I did make two, but they are fixed now.

[UliZappe]

Just for the record, let me clear up Apple’s terminology.

Device profiles can be factory, default and current.

For instance, my HP color laser printer has an RGB, a CMYK and a Gray mode, and comes with factory profiles for all modes. By default, it expects CMYK data. I built additional custom profiles for the CMYK and RGB modes which I configured instead of the factory profiles. The result of all this is as follows:

Profile	Factory	Default	Current
HPProfileCMYK	yes	no	no
HPProfileRGB	yes	no	no
HPProfileGray	yes	no	yes
myProfileCMYK	no	yes	yes
myProfileRGB	no	no	yes

Note that there are several current profiles; one for each mode (CMYK, RGB and Gray). However, there’s only one default profile, which is the current profile of the default mode (CMYK in this example), so to speak. So it’s the default profile we’re looking for, not a current one, which might be counter-intuitive for our use case. I hope this makes clear why the code above looks for kColorSyncDeviceDefaultProfileID.

[pigoz]

Great stuff. I've fallen into the trap of thinking Current meant Default. Thanks for the new info.

[UliZappe]

I wrote:

it’s now hopefully as "ColorSync official" as it gets

Well, it still isn’t, as I just realized that kColorSyncDeviceDefaultProfileID is optional if only one factory profile exists (which will probably always be the case for displays).

Hmmm – after a good night’s sleep, and rereading the sparse documentation in the ColorSync header files, I wonder if I overreacted here. Strictly speaking, kColorSyncDeviceDefaultProfileID is marked optional when registering the factory profiles for a device (ColorSyncRegisterDevice), but is unconditionally listed within ColorSyncDeviceCopyDeviceInfo which we use.

It could well be that if you don’t specify kColorSyncDeviceDefaultProfileID when registering the factory defaults for a device because you only specify one factory profile, OS X will automatically generate it so that it’s always present in ColorSyncDeviceCopyDeviceInfo. If so, the last update to the code would have been unnecessary.

I will test tonight what happens if I register a device without specifying kColorSyncDeviceDefaultProfileID and then call ColorSyncDeviceCopyDeviceInfo. If kColorSyncDeviceDefaultProfileID exists, then we could go back to the slightly simpler variant (which unfortunately I overwrote …) before the last update.

Sorry for the confusion. The documentation is … well … suboptimal. ;-)

[UliZappe]

I tested it, and indeed kColorSyncDeviceDefaultProfileID is defined in ColorSyncDeviceCopyDeviceInfo even if it was not specified while registering the device with ColorSyncRegisterDevice. So the latest addition to the code was unnecessary.

So here is the previous version, which hopefully will be the final one. Again, sorry for the confusion.

static Boolean get_screen_profile(struct vo *vo, Boolean fullscreen, char *pathBuffer, size_t maxPathSize)
{
    CFURLRef profileURL;

    struct vo_cocoa_state *s = vo->cocoa;
    NSScreen *screen = fullscreen? s->fs_screen : s->current_screen;
    const char *screenstring = fullscreen? "full screen " : "";

    CGDirectDisplayID displayID = (CGDirectDisplayID)[[[screen deviceDescription] valueForKey:@"NSScreenNumber"] unsignedLongValue];

    CFUUIDRef displayUUID = CGDisplayCreateUUIDFromDisplayID(displayID); // for the current display, use kCGDirectMainDisplay instead of displayID
    if (CFGetTypeID(displayUUID) == CFNullGetTypeID()) {MP_ERR(s, "Cannot get %sdisplay UUID.\n", screenstring); return false;}

    CFDictionaryRef deviceInfo = ColorSyncDeviceCopyDeviceInfo(kColorSyncDisplayDeviceClass, displayUUID);
    CFRelease(displayUUID);
    if (!deviceInfo) {MP_ERR(s, "Cannot get %sdisplay info.\n", screenstring); return false;}

    CFDictionaryRef factoryInfo = CFDictionaryGetValue(deviceInfo, kColorSyncFactoryProfiles);
    if (!factoryInfo) {MP_ERR(s, "Cannot get %sdisplay factory settings.\n", screenstring); CFRelease(deviceInfo); return false;}
    CFStringRef defaultProfileID = CFDictionaryGetValue(factoryInfo, kColorSyncDeviceDefaultProfileID);
    if (!defaultProfileID) {MP_ERR(s, "Cannot get %sdisplay default profile ID.\n", screenstring); CFRelease(deviceInfo); return false;}

    CFDictionaryRef customProfileInfo = CFDictionaryGetValue(deviceInfo, kColorSyncCustomProfiles);
    if (customProfileInfo)
    {
        profileURL = CFDictionaryGetValue(customProfileInfo, defaultProfileID);
        // If profileURL is NULL, the profile URL could not be retrieved although a custom profile was specified.
        // This points to a configuration error, so we should not fall back to the factory profile, but return an error instead.
        if (!profileURL) {MP_ERR(s, "Cannot get %sdisplay profile URL.\n", screenstring); CFRelease(deviceInfo); return false;}
    }
    else    // No custom profile specified; try factory profile for the device
    {
        CFDictionaryRef factoryProfileInfo = CFDictionaryGetValue(factoryInfo, defaultProfileID);
        if (!factoryProfileInfo) {MP_ERR(s, "Cannot get %sdisplay profile info.\n", screenstring); CFRelease(deviceInfo); return false;}
        profileURL = CFDictionaryGetValue(factoryProfileInfo, kColorSyncDeviceProfileURL);
        if (!profileURL) {MP_ERR(s, "Cannot get %sdisplay factory profile URL.\n", screenstring); CFRelease(deviceInfo); return false;}
    }

    Boolean result = CFURLGetFileSystemRepresentation(profileURL, true, (UInt8*)pathBuffer, maxPathSize);
    CFRelease(deviceInfo);
    if (!result) MP_ERR(s, "Cannot get %sdisplay profile path.\n", screenstring);
    return result;
}

[UliZappe]

I changed some variable names in the above code to better reflect Apple’s naming conventions, and thus make it clearer to read.

[pigoz]

I mostly use chromium conventions in mpv (because I think it looks better when mixing C and ObjC), but don't worry, I will change that on my own.

[UliZappe]

Oh, feel free to use whatever naming conventions (i.e. something like my my_name vs. myName) fit well within mpv.

What I referred to in the above post was the actual names wrt/ factory, default and custom – my previous variable names partly added to the confusion I tried to clear up with the "Apple’s terminology" posting above.

[pigoz]

I integrated your code locally and it works quite well, the problem is mpv initializes the 3dlut way before VOCTRL_UPDATE_SCREENINFO is issued (which is responsible for getting the current_screen).

So before integrating this patch I we should make it possible to set the 3dlut much later as well set it multiple times (for each reconfig).

[UliZappe]

A possible workaround for the time being would be to use kCGDirectMainDisplay instead of using displayID, as mentioned in the comment in my final code above.

This would mean that for now, the :icc-profile=auto (or whatever you’ll call it) option won’t work for multiple display configurations, but if that’s clearly documented, it’s better than nothing, I think.

[pigoz]

Yes, that's better than nothing and good enough for the time being. Also it pretty much is enough for all of my use cases (hopefully this doesn't make me lazy).

[UliZappe]

I think that in this case, it should also be mentioned in the documentation that for now, mpv will stick to the OS X system display profile at mpv’s launch time and not change the profile if the OS X system display profile is changed later on.

[pigoz]

I actually had a brainfart before posting, the current window is already available, just not through a VO_CTRL call. A simple function call works though.

[UliZappe]

Luckily, it turns out that the notification code can be extremely simple, because there is one notification (NSWindowDidChangeScreenProfileNotification) that covers exactly our situation.

From Apple’s documentation:

The NSWindowDidChangeScreenProfileNotification is sent when a majority of the window is moved to a different screen (whose profile is also different from the previous screen) or when the ColorSync profile for the current screen changes.

So this is what has to be done:

_1. When a window is created, tell it to send this kind of notification. To do this, add one line in video/out/cocoa_common.m in static void create_window():

  ...
  [s->window setRestorable:NO];
  [s->window setContentView:s->view];
  [s->window setDisplaysWhenScreenProfileChanges:YES];

_2. In the Cocoa init code in osdep/macosx_application.m, register for this notification:

[[NSNotificationCenter defaultCenter] addObserver: self
                                      selector: @selector(new_screen_profile:)
                                      name: NSWindowDidChangeScreenProfileNotification
                                      object:nil];

and add the method that deals with this notification:

- (void) new_screen_profile:(NSNotification*)notification
    {
        NSWindow *window = [notification object]; // the window whose profile has changed
        NSScreen *screen = [window screen]; // the screen where most of the window is on
        mpv_function_to_update_display_profile_for_screen(screen);
    }

That’s it. (I think the switch to a new profile should be logged.)

As far as I know, mpv can open only one window at a time, but since the notification contains the window whose profile has changed, even a multiple window situation could be handled this way.

There is one situation that might not be handled here, and that is fullscreen. Does fullscreen mode get rid of the window (rather than use the window in fullscreen mode)? If so, this case would obviously have to be handled separately. (Basically, check the screen’s profile when fullscreen mode is entered or exited, and register for a NSScreenColorSpaceDidChangeNotification for the fullscreen screen while in fullscreen mode.)

[pigoz]

Nice, this seems simple. Yes, fullscreen uses the NSView API, which under the hood changes the view parent window to a fullscreen window created by the cocoa framework.

[UliZappe]

Hmm, so either use NSScreenColorSpaceDidChangeNotification for the fullscreen screen, or this fullscreen window would also have to be initialized with setDisplaysWhenScreenProfileChanges:YES, or you reimplement fullscreen by keeping the window and use [mpv_window toggleFullScreen:self].

[UliZappe]

It just occurred to me that osdep/macosx_application.m probably will be used only for the bundle application, not for using mpv from the command line. Is this correct?

If so, registering for the notification in this place will only work for the bundle application, so there might be a better place to put it (what is obviously required is that a Cocoa event loop exists).

Well, you know mpv’s architecture much better than I do, so you’ll know where to put it. :-)

[pigoz]

In video/out/cocoa_common.m, I was already working on this on my morning commute. Hopefully I can have something working this evening ;)

[pigoz]

@UliZappe: Took me longer than expected. I pushed my tentative implementation. There's a missing ugly thing: the cache currently supports a single profile, while I'd like to change it to support multiple ones so that mpv can take less time to update the profile at runtime.

I must think about what to use as a cache key and cache invalidation though.

[UliZappe]

@pigoz: Works fine here. :-)

I see your point with the profile cache, but all in all color management is now working as it should. It’s great to have an open source player which does that. :-)

(BTW, thanks for giving credit in your commit!)

[haasn]

but all in all color management is now working as it should

Only for BT.709 sources, I might point out. mpv actually does not handle BT.601 or BT.2020 or any other type of source (XYZ, most RGB, etc.) correctly.

[UliZappe]

Only for BT.709 sources, I might point out.

OK, but now that all the difficult parts are in place, this shouldn’t be much of a problem, shall it?

BT.601 support for SD videos might make sense. Using the video’s metadata or guessing by video resolution (or using an mpv option), you could use the BT.601 primaries (IIRC, those for NTSC based and PAL based sources differ slightly, which might complicate things) instead of those of BT.709 (the transfer function is identical).

Personally, I’ve never encountered any XYZ or RGB video source.

BT.2020 will most probably become important some time in the future, but not now.

But from my POV, BT.709 support currently is by far the most important.

[haasn]

Yeah, BT.601 support is trivial, it just hasn't been merged yet. BT.2020 and XYZ support also implemented in https://github.com/haasn/mpv

That fork has a --primaries option too for specifying the primaries the video is supposed to be interpreted in (for XYZ, this specifies the RGB primaries the source is rendered to), and icc-intent has been generalized to apply to all CMS transformations, eg. the one between BT.709 and BT.2020 and from DCP XYZ-12 (ie. for digital cinema distribution) to the display gamut.

This might be the wrong place to ask but I'd love some independent testing and feedback quality-wise, as it does include some trade-offs especially with regards to the 3DLUT precision. Some test clips at http://www.nand.wakku.to/ramps.tar.xz

BT.709 support currently is by far the most important.

Agreed, but my philosophy is that a user would not necessarily know this - if they find mpv is unreliable even for one particular type of video, the user's trust in it being colorimetrically accurate is diminished. Therefore I think it would be important to make mpv work accurately for as many sources as we can reasonably support, especially ones people familiar with color management could be likely to use (XYZ sources come in mind).

Finding RGB sources is easy, just open a .png file in mpv. We currently decode absolutely every RGB source as sRGB, but the infrastructure in place in my fork allows us to very, very easily support all sorts of color spaces (AdobeRGB, ProPhotoRGB, etc.) with minimal code changes, mainly adding constants and tag detection.

In a perfect world I'd like mpv to be the end-all one-size-fits-everything solution to “display things, with correct colors”.

[UliZappe]

Yeah, BT.601 support is trivial, it just hasn't been merged yet. BT.2020 and XYZ support also implemented in https://github.com/haasn/mpv

Ah, OK. Now this was fast. ;)

That fork has a --primaries option too for specifying the primaries the video is supposed to be interpreted in (for XYZ, this specifies the RGB primaries the source is rendered to)

The latter I don’t understand. XYZ coordinates are absolute, not relative to primaries. I.e. you do not need any primaries to convert XYZ to the display color space.

Wouldn’t an option to set color spaces (in case no tags or other criteria can be found in the video) be more intuitive? Something like --SD-PAL or --SD-NTSC?

This might be the wrong place to ask but I'd love some independent testing and feedback quality-wise, as it does include some trade-offs especially with regards to the 3DLUT precision.

Where/why were these trade-offs necessary? Do they also affect the currently existing BT.709 color reproduction?

Therefore I think it would be important to make mpv work accurately for as many sources as we can reasonably support,

Of course, ideally you are right. For real world usage, SD/NTSC, SD/PAL and BT.2020 are probably sufficient for video.

especially ones people familiar with color management could be likely to use (XYZ sources come in mind).

I’ve yet to encounter a movie in XYZ …

Finding RGB sources is easy, just open a .png file in mpv.

Ah, OK, if you include still images in your reasoning, you are right, of course. (I remember that we indeed tried and looked what would happen if we imported a PNG image when trying to figure out the correct color management in #534)

We currently decode absolutely every RGB source as sRGB, but the infrastructure in place in my fork allows us to very, very easily support all sorts of color spaces (AdobeRGB, ProPhotoRGB, etc.) with minimal code changes, mainly adding constants and tag detection.

Very nice! :)

I would assume that intelligent tag/color space detection will be the bigger issue (for videos) than adding another color space.

In a perfect world I'd like mpv to be the end-all one-size-fits-everything solution to “display things, with correct colors”.

Good luck with PDFs … >8-}

Architectonically, I have no idea whether it makes sense to try and enhance a program like mpv to also be a full-fledged image viewer. A full-fledged video viewer would already be great. But, of course, in a perfect world … ;)

[haasn]

The latter I don’t understand. XYZ coordinates are absolute, not relative to primaries. I.e. you do not need any primaries to convert XYZ to the display color space.

Yes, if we have information about the display color space (:srgb or :icc-profile option set) we skip the intermediate step. --primaries is only relevant here if there is no information about the display color space, we simply convert to whatever primaries the video is tagged with/the --primaries option, or BT.709 if all else fails.

Note as a minor technicality that XYZ input, at least if it comes from SMPTE 428's DCP XYZ-12 style encoding (are there any other XYZ formats in use? I could not find any) then things are taken to be encoded around a non-standard white point (not part of the standard illuminant series, even), so even in the case of --primaries, no --primaries or CMS, we always chromatically adapt from this source white point to ensure white maps to white on whatever display mechanism, except in absolute colorimetric intent. This is why so much logic has to be added, minor technicalities that all constitute “correct behavior”.

Wouldn’t an option to set color spaces (in case no tags or other criteria can be found in the video) be more intuitive? Something like --SD-PAL or --SD-NTSC?

This is basically what --primaries BT.601-625 and --primaries BT.601-525 does, but if you're implicitly suggesting that adding additional aliases or documentation for PAL/NTSC/SECAM/Blu-ray/etc could be useful, that could be considered.

Where/why were these trade-offs necessary?

The problem is that @wm4 does not want 3DLUTs to be dependent on the video profile, so we have to be able to reuse the same 3DLUT for BT.2020 and BT.709 sources. Since BT.2020 is significantly wider than BT.709, there's no way we can scale BT.2020 into BT.709 and pass it through a 3DLUT without clipping profoundly before doing so, since the values will most likely be outside its definition range. However, we can very comfortably fit BT.709 into BT.2020 and pass that through a 3DLUT generated against BT.2020, which is exactly what we do.

I could not notice a perceptual difference personally, especially because we do not round in the 3DLUT step but rather interpolate between adjacent values, the definition loss is extremely subtle rather than as harsh as going from 8-bit BT.709 to 8-bit BT.2020 ought to be.

I still need good verification though to make sure other users feel the same way.

Do they also affect the currently existing BT.709 color reproduction?

Yes, unfortunately. That's part of the reason these haven't been merged in that quickly, until I'm convinced the difference isn't significant.

Note that in spite of this, or perhaps to combat it (and prepare for ultra-wide gamut profiles in general) I've increased the maximum parameter for a 3DLUT to 512, meaning you could double your precision in the green channel while preserving red and blue (which haven't changed much between BT.709 and BT.2020), counteracting any effect this change might have had on extremely noisy wide-gamut profiles and even increasing definition further than it was with the old BT.709 LUTs, of course at the cost of double the memory usage (usually not that high either way).

For real world usage, SD/NTSC, SD/PAL and BT.2020 are probably sufficient for video.

Yes, it's mainly BT.2020 I'm worried about because I feel there is little question at this point it will be moving towards adoption after people grow tired of HD/BT.709. The upcoming Japanese 4K airing specs already include it, for example.

Hence, most of this change is related directly to BT.2020, both constant luminance and non-constant luminance systems as well as the primaries - and all of the other refactorings (including the XYZ/icc-intent stuff) directly affect or improve the way BT.2020 content gets handled.

Good luck with PDFs … >8-}

Don't have to worry about it until mpv is capable of decoding them, fortunately. :)

I have no idea whether it makes sense to try and enhance a program like mpv to also be a full-fledged image viewer.

Yeah, otherwise I'd have gone through with all the RGB stuff already. I'd be willing to accept a compromise for video playback only in mpv and re-engineer a colorimetrically accurate image viewer as a separate project.

Anyway, you're right in that end users are most likely being exposed to YC content, rather than RGB or XYZ - but I feel those are used as part a number of internal processing chains or intermediate files, perhaps more during production than encoding. I could be wrong, but I'd still like to make sure mpv plays intermediate and master files correctly so an end user could compare the results “along the way” and ensure they are correct at every step.

[UliZappe]

@haasn: Thanks for the in-depth explanations! I agree with your assumptions/goals.

Wouldn’t an option to set color spaces (in case no tags or other criteria can be found in the video) be more intuitive? Something like --SD-PAL or --SD-NTSC?

This is basically what --primaries BT.601-625 and --primaries BT.601-525 does, but if you're implicitly suggesting that adding additional aliases or documentation for PAL/NTSC/SECAM/Blu-ray/etc could be useful, that could be considered.

I wasn’t aware of --primaries BT.601-625 and --primaries BT.601-525 at the time of my writing, but now that you say it, less technically named aliases would most probably help users. At the very least, the documentation should make clear which primaries to use for which kind of video; this is most certainly not common knowledge. (Of course, better yet is logic within mpv which mostly gets it right by itself.)

I could not notice a perceptual difference personally […] I still need good verification though to make sure other users feel the same way.

Did you try the ColorTest.mov and QuickTime_Test_Pattern_XX.mov movies from #534? These might deliver more precise results than subjective viewing. (If color management remains a topic for the time being, it might have been a good idea to construct a test movie with a wider variance of colors than I did …)

Arguably, modifications to the current color handling of mpv should not result in a measurable deterioration of the current "standard case" BT.709.

I have no idea whether it makes sense to try and enhance a program like mpv to also be a full-fledged image viewer.

Yeah, otherwise I'd have gone through with all the RGB stuff already. I'd be willing to accept a compromise for video playback only in mpv and re-engineer a colorimetrically accurate image viewer as a separate project.

Personally, I wouldn’t feel that color for movies is less important than for images …

[haasn]

Of course, better yet is logic within mpv which mostly gets it right by itself.

This unfortunately relies on video tags because there's nothing we can really assume. If things are correctly tagged, everything will work out, but in my experience this is rather hit-and-miss among encoders.

Current logic is to assume the BT.601 colormatrix for anything below 720p, and BT.709 colormatrix for anything at-or-above 720p. There is no condition for auto-guessing BT.2020, so tags will absolutely be required for BT.2020 content. Fortunately, at least for CL, the difference is so great from NCL/the other matrices that hopefully most encoders will eventually realize something's wrong. For NCL, I'm not so sure, but it seems reasonable to assume they will notice something's going on if the primaries actually change (everything will be desaturated!)

From this information, the color primaries are always guessed as BT.2020 for videos using the BT.2020-CL or BT.2020-NCL matrix, and always guessed as BT.709 otherwise.

This means that, given no video tags, we actually assume the BT.709 primaries even for BT.601-matrix content, because there's ambiguity between BT.601-525 (NTSC) and BT.601-625 (PAL/SECAM) and I think BT.709 lies pretty much in between them, so it's the better “average case”. It also agrees with sRGB so it displays more or less as if no CMS was being done, for consistency with other players. I'm not sure if this is the right logic though, what's your opinion on it? Should we just assume BT.601-625 (PAL/SECAM) for SD content, instead? What about NTSC? Is there some other heuristic we could use to decide between NTSC and PAL/SECAM?

Did you try the ColorTest.mov and QuickTime_Test_Pattern_XX.mov movies from #534?

I did, actually; but I'm mainly basing my tests off the ramps I generated earlier - especially since those don't need the approx-gamma hack but rather use the encoding actually intended by the ITU-R. Regardless, here's are a few screenshot comparisons between the old BT.709 3DLUT and the new BT.2020 3DLUT, using the QuickTime_Test_Pattern_HD.mov, with a number of profiles:

http://screenshotcomparison.com/comparison/68806

#1 is sRGB.icm (the correct one from ArgyllCMS, before anyone asks ;))
#2 is my own profile (roughly sRGB but with 2.4-ish gamma, iirc)
#3 is some guy's wide gamut profile that he complained about artifacts with (which was the reason we switched to 2.4 rather than generating the lut against BT.709 directly)

I cannot see a difference, can you?

Arguably, modifications to the current color handling of mpv should not result in a measurable deterioration of the current "standard case" BT.709.

Agreed.

Personally, I wouldn’t feel that color for movies is less important than for images …

I'm not sure if you understood me correct; I wasn't implying that mpv should have bad video color, I meant that mpv should only have correct video colors and not necessarily image colors; and image viewing should be done by a different tool with a different goal.

[UliZappe]

This unfortunately relies on video tags because there's nothing we can really assume.

While I agree with the diagnosis of this unfortunate situation, I’m inclined to think mpv should still try and take an educated guess, as the user will usually be at a loss even more.

First of all, I do hope that "high-end" BT.2020 movies will almost always be tagged (and correctly so) from the beginning, so for now I would assume that this poses the smaller problem, if any at all.

The problem are the SD legacy videos with all their norms from the pre color management and pre digital era.

Current logic is to assume the BT.601 colormatrix for anything below 720p, and BT.709 colormatrix for anything at-or-above 720p. There is no condition for auto-guessing BT.2020, so tags will absolutely be required for BT.2020 content.

This would be a reasonable assumption from my POV above.

From this information, the color primaries are always guessed as BT.2020 for videos using the BT.2020-CL or BT.2020-NCL matrix, and always guessed as BT.709 otherwise.

Now I’m confused – "always guessing BT.709 otherwise" directly contradicts the quote before.

This means that, given no video tags, we actually assume the BT.709 primaries even for BT.601-matrix content, because there's ambiguity between BT.601-525 (NTSC) and BT.601-625 (PAL/SECAM) and I think BT.709 lies pretty much in between them, so it's the better “average case”.

That’s not really the case. BT.709 and BT.601-625 (PAL/SECAM) have identical RB primaries; only G differs in its x value (0.29 PAL, 0.30 HD).

BT.709 and BT.601-525 (NTSC, well, actually SMPTE-C), OTOH, differ in all 3 primaries, and each primary differs in x and y. (SMPTE-C G-x is 0.31, so for this coordinate only, HD/BT.709 is indeed "in between"). The visible difference, therefore, is much bigger here than between PAL and HD.

Interestingly, when Apple introduced its "video color profiles" in OS X 10.7, the Automator action that allowed to set these profiles offered "SD" (=SMPTE-C) and "PAL" settings; now in 10.9, the "PAL" setting is gone. My guess would be that Apple felt that "PAL" offered so little difference to "HD" that it was more confusing than helpful (but that’s only a guess).

So the current behavior of mpv (assuming it always uses BT.709 for SD) works probably quite well for users who watch mostly PAL/SECAM SD content, and not so well for those watching NTSC SD content.

It also agrees with sRGB so it displays more or less as if no CMS was being done, for consistency with other players.

Well, that’s not the case as the gamma curve is quite different, and as we found in #534, the movies look quite different because of the gamma difference alone. It’s probably more visible to the "normal" user than the color differences.

I'm not sure if this is the right logic though, what's your opinion on it? Should we just assume BT.601-625 (PAL/SECAM) for SD content, instead? What about NTSC? Is there some other heuristic we could use to decide between NTSC and PAL/SECAM?

I would think of something along the following lines:

1. If tagged, assume the tag’s value; break
2. If equal or above 720 lines vertical resolution, assume HD (BT.709); break
3. If vertical resolution == 480, assume NTSC/SMPTE-C; break
4. If vertical resolution == 576, assume PAL/SECAM; break
5. If aspect ratio ≈ 1.5, assume NTSC/SMPTE-C; break
6. If aspect ratio ≈ 1.3333…, assume PAL/SECAM; break
7. If frame rate == 30fps or 30/1.001fps, assume NTSC/SMPTE-C; break
8. If frame rate == 25fps or 25/1.001fps, assume PAL/SECAM; break
9. Assume HD (BT.709)

Also, I would allow several levels for configuration options to set a color space to use:

1. Always (overwrites 1-9)
2. If untagged (overwrites 2-9)
3. If SD (overwrites 3-9)
4. If SD-guessed (overwrites 5-9)
5. Default (overwrites 9)

Also, I would use the OSD to inform about any assumption being made (2-9), especially (maybe in red?) "wild" assumptions (5-9)

I’m not sure whether the aspect ratio check or the frame rate check are more reliable; anecdotally, I would assume the aspect ratio is more reliable, which is why I used this order. But this might be wrong.

This is certainly not fool-proof, but probably better than anything a user might achieve manually.

I cannot see a difference, can you?

No. :-)

I'm not sure if you understood me correct; I wasn't implying that mpv should have bad video color, I meant that mpv should only have correct video colors and not necessarily image colors; and image viewing should be done by a different tool with a different goal.

Ah, OK. The "different goal" was my original point, too, though it would certainly be nice to get the most popular image formats (jpeg, tiff, png) right in mpv. But in any case, there should be no (bad) "compromise" in video color quality in mpv.

[haasn]

Now I’m confused – "always guessing BT.709 otherwise" directly contradicts the quote before.

I presented the logic for two different fields of information - the color matrix used to decode from Y'CbCr to RGB (irrespective of the actual colors, this is simply the representation method) and the primaries in which these videos live. The issue is that BT.601-525 and BT.601-625 use the same color matrix but different primaries. The only thing we “know” for sure is that SD content uses the BT.601 matrix, and it seems this is what encoders and programs care about most. What the actual primaries are, however, seems to be much more obscure.

Thanks for the clarification on the primaries, I didn't realize BT.601-625 and BT.709 agree in red and blue. Based on this, it's perhaps a better trade-off to always assume either 525 or 625, rather than BT.709, since it offers a (small) deviation to one and a big deviation to the other, rather than just a big deviation to the other.

Well, that’s not the case as the gamma curve is quite different

Yeah, fair point; I guess I meant “if you transform to BT.709 again”, but then again, other players don't do that either. It's not really a fair comparison, and it's not really relevant either, what other players - especially dumb ones, do.

I appreciate your feedback for the 525/625 heuristics, I'll do my best and implement them. What would you feel are acceptable error margins for the resolution/aspect ratio/framerate detection?

Also, I would allow several levels for configuration options to set a color space to use:

I don't really agree with this, partially because it's very complicated and confusing code-wise to implement all these stages of auto-guessing as separate options etc.; but partially also because I struggle to find a use-case. When would you want to set an option for “guess csp X, but only if it's untagged”? Wouldn't it be better to improve mpv's heuristics for this case? Or, even better, wouldn't it be more sensible to write an external tool to tag untagged files as something?

You can easily override the primaries on a per-file basis, if you don't care about them enough to tag. If you do, tag them instead of relying on complicated defaulting logic.

Also, I would use the OSD to inform about any assumption being made (2-9), especially (maybe in red?) "wild" assumptions (5-9)

Currently the OSD shows “Video decoder: (none)” if there's no information, which implies that we're making a guess. Perhaps a warning could be printed for severely wild guesses?

Ps. Maybe it's better to continue this discussion in another issue, as this is not really the right place. Perhaps #668, where the patches are actually relevant?

[haasn]

I decided to go ahead and calculate the actual deviations between the various colorspaces:

Lab values: (L = 100)

	BT.709	BT.601-525	BT.601-625
Red	(134.18, 112.5864)	(124.6131, 113.4519)	(134.18, 112.5864)
Green	(-96.3727, 93.0142)	(-90.7970, 94.5341)	(-100.908, 89.5607)
Blue	(190.193, -259.059)	(162.8294, -233.289)	(190.193, -259.059)

Differences in delta E (CIE 2000):

	BT.709	BT.601-525	BT.601-625
BT.709	(0.0, 0.0, 0.0)	(2.6, 1.4, 2.8)	(0.0, 1.5, 0.0)
BT.601-525	(2.6, 1.4, 2.8)	(0.0, 0.0, 0.0)	(2.6, 2.9, 2.8)
BT.601-625	(0.0, 1.5, 0.0)	(2.6, 2.9, 2.8)	(0.0, 0.0, 0.0)

This means:

• BT.709 gives us an average deviation of 2.3 to BT.601-525, and of 0.5 to BT.601-625, for an overall average of 1.4.
• BT.601-625 gives us an average deviation of 2.8 to BT.601-525, for an overall average of 1.4.

So, assuming 525 and 625 clips are distributed evenly, the net average deviation is the same, but in one case we have a better worst case (with barely perceptible differences to BT.601-625).

Of course, this is just for the actual primaries. Checking an actual screenshot, it seems the deviations arise more from the mid-range tones than from the spectral primaries themselves, especially tones that involve more than one primary.

I'm not sure how to call it, I think it's still important to get good heuristics in.

More food for thought: What about movies which are actually untagged BT.709 but were downscaled to SD by a bad encoder? BT.709 offers the minimal deviation here, followed by BT.601-625 and then BT.601-525.

Edit: I'm looking at your heuristics and I see you picked 1.333 for PAL/SECAM, why is this? At DVD resolution, it would be 720/576 = 1.25, no?

[UliZappe]

FYI, it occurred to me that the task of finding the current screen profile could be much simplified if we used LittleCMS’ cmsOpenProfileFromMem() function instead of cmsOpenProfileFromFile(), i.e. if we don’t insist on reading the ICC profile data from disk.

Because in this case, you could simply use the following two-liner instead of the lengthy get_screen_profile() code from above:

NSData *iccProfileData = [[[mpv_window screen] colorSpace] ICCProfileData];
cmsHPROFILE screen_profile = cmsOpenProfileFromMem([iccProfileData bytes], [iccProfileData length]);

The obvious upside of this solution is that it’s much less code and leaves it to Apple to ensure that the correct profile is used.

The downside is that mpv uses cmsOpenProfileFromFile() otherwise, so this code would have to be replaced for this case. This is why I never thought of the short solution above, because unfortunately, there’s no way to get the file path of the underlying ICC profile from NSColorSpace (which is what [window colorSpace] returns).

Also note that contrary to what you might think, the colorSpace property of NSWindow is not KVO/bindings compliant, so you cannot observe it to be notified of changes in the screen profile; for this, you must still use the NSWindowDidChangeScreenProfileNotification, just as before.

I’ll leave it up to you if you want to change the code to the shorter solution; the original solution works 100% correctly, as far as I can tell. But I wanted to let you know.

[ghost]

Actually, we already open it from memory, and read the file ourselves (for some reason).

I don't know if what you suggest simplifies things much, but maybe it does. In fact, on X11, ICC profiles are (annoyingly) exposed as byte blobs, not as files.

[UliZappe]

Actually, we already open it from memory

Ooops! For some reason, I was convinced you didn’t …

I don't know if what you suggest simplifies things much, but maybe it does.

Well, it replaces some 40 lines of code by 1, actually (if mpv opens profiles from memory, anyway).

On the other hand, there’s this never touch a running system thing …

Sorry this has not occurred earlier to me!

[ghost]

Well, we use the LittleCMS to open from memory, but our internal interface is designed towards taking a filename. It might be not so hard to change this though. The main question is whether @pigoz thinks it would simplify the cocoa code and whether he wants to make the change.

[pigoz]

Wouldn't adapting the core to read from memory benefit X11 as well? I might do it in that case.

[ghost]

Wouldn't adapting the core to read from memory benefit X11 as well? I might do it in that case.

Yes, in theory that could be used to implement #983. The main problem is that only Gnome seems to support this for now, and also the mechanism to get the data is terrible, so I'm not hyped for this.

[pigoz]

I implemented the in memory ICC loading in https://github.com/mpv-player/mpv/compare/memory_icc

Seems to work ok here (I still need to split this in two commits and write proper commit messages).

[UliZappe]

:–)