Skip to content
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

Wrong mpv detected refresh rate when fullscreening, with different refresh rate monitors #7932

Closed
dtop129 opened this issue Jan 24, 2024 · 9 comments
Closed

Wrong mpv detected refresh rate when fullscreening, with different refresh rate monitors #7932

dtop129 opened this issue Jan 24, 2024 · 9 comments
Labels
bug Not working as intended
Milestone
1.10

Comments

@dtop129
Copy link

dtop129 commented Jan 24, 2024

  • Sway Version:
    sway version 1.9-dev-a4e85332 (Jan 24 2024, branch 'master')

  • Debug Log: relevant log

  • Configuration File:

  • Description:

    • Open any video with mpv on the higher refresh rate monitor, and press I to see the stats. When first opened, the specified refresh rate should be equal to the monitor's.
    • Fullscreen the mpv window, notice that the specified refresh rate switches to the lower monitor refresh rate.

The issue doesn't appear in the release version 1.8.1, so there is a possibilty of it being a sway issue.
Also, if I send back and forth the fullscreen mpv window between the two monitors, when going back to the higher refresh rate one mpv detects it correctly.
I couldn't bisect as I had trouble with downgrading wlroots when compiling older sway.
@dtop129 dtop129 added the bug Not working as intended label Jan 24, 2024
@emersion
Copy link
Member

Can you post a WAYLAND_DEBUG=1 log of mpv?

@dtop129
Copy link
Author

dtop129 commented Jan 26, 2024 •
edited
Loading

Log of simply fullscreening and unfullscreening mpv
WAYLAND_DEBUG=1 log

@emersion emersion added this to the 1.10 milestone Feb 1, 2024
@Nefsen402
Copy link
Contributor

Unfortunately, I cannot reproduce this issue. Mpv will jump to the correct framerate of the monitor when fullscreen is activated in any arbitrary order on any monitor.

@llyyr
Copy link
Contributor

llyyr commented Feb 2, 2024

I can't reproduce this either but mpv logs (--log-file=mpvsucks.log) would also help in this case to know what it gets from the compositor

@dtop129
Copy link
Author

dtop129 commented Feb 7, 2024

Strange, I can reproduce it consistently even on different machines.

This is the mpv log starting it in the 144hz output, which mpv correctly recognizes, then fullscreening, where mpv thinks it entered the 60Hz output, and unfullscreening.
Even after unfullscreening, mpv still thinks it is on the 60Hz output, as seen from the log.

mpv log

@llyyr
Copy link
Contributor

llyyr commented Feb 7, 2024

There's at least 1 bug here, possibly two. mpv currently doesn't correctly set the current output when it receives a surface leave. This patch for mpv should fix it: https://0x0.st/HkD_.patch and will probably be upstreamed shortly.

However, mpv receives a random surface enter and surface leave on your 60Hz output when you try to fullscreen mpv on the 144Hz which probably shouldn't happen.

[1535813.018]  -> xdg_toplevel@30.set_fullscreen(nil)
[1535813.167] wl_buffer@45.release()
[1535813.171] wl_callback@47.done(2825)
[1535813.402]  -> wl_surface@8.attach(wl_buffer@45, 0, 0)
[1535813.407]  -> wl_surface@8.damage(0, 0, 2147483647, 2147483647)
[1535813.409]  -> wl_surface@8.commit()
[1535813.411]  -> wl_display@1.sync(new id wl_callback@47)
[1535813.608] wl_display@1.delete_id(37)
[1535813.614] wl_display@1.delete_id(47)
[1535813.616] wl_callback@37.done(250887)
[1535813.633]  -> wl_surface@8.frame(new id wl_callback@37)
[1535813.635]  -> wp_presentation@18.feedback(wl_surface@8, new id wp_presentation_feedback@44)
[1535813.637] xdg_toplevel@30.configure(1920, 1080, array[24])
[1535813.673]  -> xdg_surface@29.set_window_geometry(0, 0, 1920, 1080)
[1535813.675] xdg_surface@29.configure(2826)
[1535813.677]  -> xdg_surface@29.ack_configure(2826)
[1535813.683]  -> wl_compositor@7.create_region(new id wl_region@49)
[1535813.685]  -> wl_region@49.add(0, 0, 1920, 1080)
[1535813.687]  -> wl_surface@8.set_opaque_region(wl_region@49)
[1535813.689]  -> wl_region@49.destroy()
[1535813.691]  -> wp_viewport@3.set_destination(1920, 1080)
[1535815.779] wl_callback@47.done(2826)
[1535816.024]  -> zwp_linux_dmabuf_v1@42.create_params(new id zwp_linux_buffer_params_v1@47)
[1535816.031]  -> zwp_linux_buffer_params_v1@47.add(fd 40, 0, 0, 7680, 50331648, 5234708)
[1535816.034]  -> zwp_linux_buffer_params_v1@47.create_immed(new id wl_buffer@51, 1920, 1080, 808665665, 0)
[1535816.037]  -> zwp_linux_buffer_params_v1@47.destroy()
[1535816.039]  -> wl_surface@8.attach(wl_buffer@51, 0, 0)
[1535816.041]  -> wl_surface@8.damage(0, 0, 2147483647, 2147483647)
[1535816.043]  -> wl_surface@8.commit()
[1535816.044]  -> wl_display@1.sync(new id wl_callback@52)
[1535816.471] wl_display@1.delete_id(49)
[1535816.477] wl_display@1.delete_id(47)
[1535816.479] wl_display@1.delete_id(41)
[1535816.480] wl_display@1.delete_id(52)
[1535816.481] wp_presentation_feedback@41.discarded()
[1535816.483] wl_surface@8.enter(wl_output@25)
[1535816.485] wl_surface@8.leave(wl_output@25)

This is either a sway bug or user error, hard to say.

Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue Feb 7, 2024
@Dudemanguy
wayland: set current_output on surface leave if applicable
a18d5c7 
When the mpv surface leaves the output, we no longer mark it as the
current output. However, this implicitly depends on there being a
preceding surface entrance event to a different output. This is not
necessarily the case. Consider moving the window from monitor 1, to
monitor 1-2, and then back to 1 again. mpv gets the entrance event to
monitor 2 and sets that as the current output to work off of. Then when
you move back to only monitor 1, it removes monitor 2 from the current
output. However, monitor 1 is not updated again as the current output
because there is not a new surface entrance event in this case (the
window never left). So the numbers that mpv's core is using are
incorrect and for the wrong monitor. Luckily, we already keep track of
what outputs the mpv surface is currently on no matter how many there
are so it is simply a matter of setting current output again in the
leave event if we have a different output that has the mpv surface.

Ref: swaywm/sway#7932
@Dudemanguy Dudemanguy mentioned this issue Feb 7, 2024
Merged
@Dudemanguy
Copy link
Contributor

Dudemanguy commented Feb 7, 2024 •
edited
Loading

@dtop129: You can try mpv-player/mpv#13433 which hopefully should solve the issue for you. Now as for why there would be a behavior difference between different sway versions, I can't say.

@dtop129
Copy link
Author

dtop129 commented Feb 7, 2024 •
edited
Loading

With mpv-player/mpv#13433 the issue is fixed for me

@dtop129 dtop129 closed this as completed Feb 7, 2024
@emersion
Copy link
Member

emersion commented Feb 7, 2024

Thanks for tracking this down @Dudemanguy!

Dudemanguy added a commit to mpv-player/mpv that referenced this issue Feb 8, 2024
@Dudemanguy
wayland: set current_output on surface leave if applicable
a45518c 
When the mpv surface leaves the output, we no longer mark it as the
current output. However, this implicitly depends on there being a
preceding surface entrance event to a different output. This is not
necessarily the case. Consider moving the window from monitor 1, to
monitor 1-2, and then back to 1 again. mpv gets the entrance event to
monitor 2 and sets that as the current output to work off of. Then when
you move back to only monitor 1, it removes monitor 2 from the current
output. However, monitor 1 is not updated again as the current output
because there is not a new surface entrance event in this case (the
window never left). So the numbers that mpv's core is using are
incorrect and for the wrong monitor. Luckily, we already keep track of
what outputs the mpv surface is currently on no matter how many there
are so it is simply a matter of setting current output again in the
leave event if we have a different output that has the mpv surface.

Ref: swaywm/sway#7932
xrun1 pushed a commit to xrun1/mpv-svp-fix that referenced this issue Mar 12, 2024
@Dudemanguy @xrun1
wayland: set current_output on surface leave if applicable
a3e9988 
When the mpv surface leaves the output, we no longer mark it as the
current output. However, this implicitly depends on there being a
preceding surface entrance event to a different output. This is not
necessarily the case. Consider moving the window from monitor 1, to
monitor 1-2, and then back to 1 again. mpv gets the entrance event to
monitor 2 and sets that as the current output to work off of. Then when
you move back to only monitor 1, it removes monitor 2 from the current
output. However, monitor 1 is not updated again as the current output
because there is not a new surface entrance event in this case (the
window never left). So the numbers that mpv's core is using are
incorrect and for the wrong monitor. Luckily, we already keep track of
what outputs the mpv surface is currently on no matter how many there
are so it is simply a matter of setting current output again in the
leave event if we have a different output that has the mpv surface.

Ref: swaywm/sway#7932
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Not working as intended
Milestone
1.10
Development

No branches or pull requests
5 participants
@emersion @Nefsen402 @Dudemanguy @llyyr @dtop129