Thanks for catching the thing with the sizes, I checked on real edid and it was 6 and 0xb (or more like 0xf - 4 since my laptop has some other descriptor in the cta block). I agree there is a race condition here, don't like the solution of using 8 round trips to work around that. (Maybe a fixed number of a few less round trips with some reasoning would be better)

Locally, I found that 2 (more) roundtrips is enough, which makes sense since we need to handle the event handlers only twice.

It's not doing a full 8, it's doing up to 8 as a limit. In practice, it will only do maybe 3, including the existing 2. It depends on how soon the compositor signals the completion event. The 8 limit is just so the startup can't hang on a broken or malicious compositor.

Here's my Mutter MR, feel free to tell me how I could reduce that round trip count from 3 to 2 or less: https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/5068

I know it's something wrong with it, since Mutter is the only implementation I've dealt with that needs so many round trips to work. I welcome any insight on the matter.

Analysis of the events and their synchronous responses:

1. bind wp_color_manager_v1 → supported_* + done
2. get_surface_feedback → preferred_changed (one of my patches sends this, and it's already immediate. If I don't do this step, Chrome/Chromium don't notice HDR is supported.)
3. get_preferred → ready
4. get_information → info events + done

Edit: My analysis shows that Mutter is not using any more round trips than either KWin or wlroots. Wine just needs one extra round trip for all of them. The loop for 8 was just being extremely generous, with a safety margin to stop it from going for too long. The condition check would already be stopping it after one iteration due to the readiness indicator returning.

Edit 2: This looks useful: https://github.com/mpv-player/mpv/blob/master/video/out/wayland_common.c#L3144-L3153

Using roundtrip for this purpose will always be racy, you should move the color management stuff to its own queue and dispatch that queue until the server responds, to eliminate race here. Similar to what mpv does on the section above and here

Using roundtrip for this purpose will always be racy, you should move the color management stuff to its own queue and dispatch that queue until the server responds, to eliminate race here. Similar to what mpv does on the section above and here

Why would it be racy? The code being changed only runs once on startup during the process init before any application code runs so if all the events are dispatched before synchronously then there cannot be a race. Although I would probably need an approach like this for those ready/ready2 events :/

It's not doing a full 8, it's doing up to 8 as a limit. In practice, it will only do maybe 3, including the existing 2. It depends on how soon the compositor signals the completion event. The 8 limit is just so the startup can't hang on a broken or malicious compositor.

Here's my Mutter MR, feel free to tell me how I could reduce that round trip count from 3 to 2 or less: https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/5068

I know it's something wrong with it, since Mutter is the only implementation I've dealt with that needs so many round trips to work. I welcome any insight on the matter.

Analysis of the events and their synchronous responses:

1. bind wp_color_manager_v1 → supported_* + done
2. get_surface_feedback → preferred_changed (one of my patches sends this, and it's already immediate. If I don't do this step, Chrome/Chromium don't notice HDR is supported.)
3. get_preferred → ready
4. get_information → info events + done

Edit: My analysis shows that Mutter is not using any more round trips than either KWin or wlroots. Wine just needs one extra round trip for all of them. The loop for 8 was just being extremely generous, with a safety margin to stop it from going for too long. The condition check would already be stopping it after one iteration due to the readiness indicator returning.

Edit 2: This looks useful: https://github.com/mpv-player/mpv/blob/master/video/out/wayland_common.c#L3144-L3153

the implementation is now using 4 round-trips in total, I guess maybe I miscounted somehow if 3 is enough?
There is nothing wrong with needing the round-trips, kwin needs it too to have the Wayland output data fully initialized before the process init finishes.

Why would it be racy? The code being changed only runs once on startup during the process init before any application code runs so if all the events are dispatched before synchronously then there cannot be a race. Although I would probably need an approach like this for those ready/ready2 events :/

I had assumed your code was using wp_color_management_surface_feedback_v1::get_preferred, rather than wp_color_management_output_v1::get_image_description

It is indeed only run once in that case, since you also ignore image_description_changed event.

Why would it be racy? The code being changed only runs once on startup during the process init before any application code runs so if all the events are dispatched before synchronously then there cannot be a race. Although I would probably need an approach like this for those ready/ready2 events :/

I had assumed your code was using wp_color_management_surface_feedback_v1::get_preferred, rather than wp_color_management_output_v1::get_image_description

It is indeed only run once in that case, since you also ignore image_description_changed event.

Oh wait I completely forgot about that event (winewayland does use it), but I think there isn't a race even with that due to how the event handlers work thankfully.

So should I just close this now? How much of this PR has been worked around by now in a release? I know the EDID changes made it into something, but have any changes been made regarding the need for at least one extra roundtrip? Or perhaps the queue idea instead?

Edit: Sorry, didn't notice the commits that happened 5 hours ago that just appeared in my feed after I made that post.

Sorry for implicitly closing this PR, I didn't think of it when I deleted the branch. This build https://github.com/CachyOS/proton-cachyos/actions/runs/26327916040/artifacts/7175794230 has the changes @Etaash-mathamsetty made in response to this. Would be good to have more confirmation.

Sorry for implicitly closing this PR, I didn't think of it when I deleted the branch. This build https://github.com/CachyOS/proton-cachyos/actions/runs/26327916040/artifacts/7175794230 has the changes @Etaash-mathamsetty made in response to this. Would be good to have more confirmation.

Nope, this build still needs one more roundtrip for preferred format retrieval, or a queue, or something that ensures the format is ready before it is retrieved:

Sorry for implicitly closing this PR, I didn't think of it when I deleted the branch. This build CachyOS/proton-cachyos/actions/runs/26327916040/artifacts/7175794230 has the changes @Etaash-mathamsetty made in response to this. Would be good to have more confirmation.

Nope, this build still needs one more roundtrip for preferred format retrieval, or a queue, or something that ensures the format is ready before it is retrieved:

That is correct, I forgot to account for the image description info also needing another roundtrip. I just corrected this

Crap, I didn't see your reply. Sorry.

You are not going to like what I have to say, but I miscounted again. You should only need 4 roundtrips; now im wondering why the 5th one is needed. Maybe we cannot assume the "timing" (its not really timing but rather at what stage they are queued i guess) of the events beyond just the initial two roundtrips (or even beyond just the first roundtrip), in which case maybe adding a loop was the right solution from the beginning.

edit: We know that we do need at least 4 roundtrips, so we can have those hardcoded with an additional loop in case the compositor hasnt sent enough information yet.

This is my current idea, but it feels kind of hacky. At least it should be low maintenance.

    /* We need at least four roundtrips:
     * 1. Get and bind globals
     * 2. Handle initial events
     * 3. Event handling for zxdg_output and image description
     * 4. Event handling for the created image description infos.
     * However, not all compositors can dispatch the above events within 4 roundtrips,
     * so we do roundtrips until no more events are dispatched with a hard cap at 10 roundtrips. */
    while (i < 10 && ++i)
    {
        ret = wl_display_roundtrip_queue(process_wayland.wl_display,
                                         process_wayland.wl_event_queue);
        if (ret < 0) return FALSE;
        /* There are always two events dispatched when "nothing" happens:
         * 1. wl_display.sync which creates a callback object
         * 2. wl_callback.done */
        if (ret <= 2) break;
    }

Please read what I said in the other PR. I measured and found that 0 additional roundtrips were necessary. But it does need that extra bit of code for assigning the color managed outputs if the service is bound after outputs are created.

The image description is not guaranteed to be ready after any number of roundtrips. If the compositor outsources some things to other threads or processes, it might take 10, 100, 1000, or more roundtrips depending on how fast the compositor is at processing wayland messages and how slow the other process is at processing image descriptions.

Instead of hacking it with a fixed upper bound of 8, you should call wl_display_dispatch_queue until wayland_outputs_color_management_done returns true.