libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
8.73k stars 1.64k forks source link

Virgil 3D with SDL2 on macOS #4986

Closed kjliew closed 1 year ago

kjliew commented 2 years ago

A minor change is required for SDL2 to support QEMU Virgil 3D on macOS. Virgil 3D manages OpenGL context with FBO and for Apple OpenGL it seems to require window move/resize events for the screen to be repainted. For borderless fullscreen as in SDL_WINDOW_FULLSCREEN_DESKTOP, it should be legal to allow window position to pass into native backends.

diff -ru orig/SDL2-2.0.16/src/video/SDL_video.c SDL2-2.0.16/src/video/SDL_video.c
--- orig/SDL2-2.0.16/src/video/SDL_video.c  2021-08-10 08:45:04.000000000 -0700
+++ SDL2-2.0.16/src/video/SDL_video.c   2021-11-15 21:59:04.000000000 -0800
@@ -2045,7 +2045,7 @@
         }
     }

-    if ((window->flags & SDL_WINDOW_FULLSCREEN)) {
+    if ((window->flags & SDL_WINDOW_FULLSCREEN_DESKTOP) == SDL_WINDOW_FULLSCREEN) {
         if (!SDL_WINDOWPOS_ISUNDEFINED(x)) {
             window->windowed.x = x;
         }

sdl2.patch OpenGL on Windows 10 and Linux do not require this, but I do not foresee any issue at keeping the same semantics for consistency. If necessary, #ifdef-#endif can be used to guard the change.

sezero commented 2 years ago

Should this go in to 2.0.18, after review of course?

lygstate commented 2 years ago

Maybe it's should be

if ((window->flags & SDL_WINDOW_FULLSCREEN_DESKTOP) == SDL_WINDOW_FULLSCREEN_DESKTOP) {

I am confusing about the change.

slouken commented 2 years ago

Conceptually fullscreen (and fullscreen desktop) windows cover the entire screen. By definition their position comes from the layout of the monitors on the desktop. I'm not sure I understand why this change would be helpful or meaningful?

kjliew commented 2 years ago

Conceptually fullscreen (and fullscreen desktop) windows cover the entire screen. By definition their position comes from the layout of the monitors on the desktop. I'm not sure I understand why this change would be helpful or meaningful?

@slouken I am not sure if the question was directed at me. Here's my take anyway inferring the simplicity of single-monitor scenario. Conceptually, fullscreen isn't quite the same as fullscreen desktop. Your definition is only valid for fullscreen window, which involves a display mode switch, either for real or emulated by the underlaying compositor. So the window position is always the entire display area.

A fullscreen desktop window, on the other hand, is a borderless, stretched window, without display mode switch. The window position may be moved outside of display area and not necessarily visible in the display area, just like an ordinary window with decoration. It is possible that an SDL2 application can manage multiple fullscreen desktop window and implement swipe transitional motion similar to multiple virtual desktop. Hence, window position should be notified into the lower level platform specific window compositor.

The Virgil 3D QEMU macOS use case is really simple, it just need to notify Apple macOS window compositor for repainting with the same window position.

icculus commented 2 years ago

Apple OpenGL it seems to require window move/resize events for the screen to be repainted.

I think this is what's confusing me at the moment. Is the problem that SDL_WINDOW_FULLSCREEN_DESKTOP, which on macOS will generate a new "fullscreen space" (which is an OS-provided virtual desktop), refusing the redraw the window when it isn't in the foreground? Is it that QEMU won't draw to the window when it thinks it hasn't changed?

One can definitely keep drawing to a fullscreen space's window in general and the compositor will notice, so I must be misunderstanding the problem here. I know you've explained this twice now, and I'm grateful for your patience, but I don't understand the problem, and my guessing at what I think is the problem has me believing this isn't the right solution.

kjliew commented 2 years ago

Is the problem that SDL_WINDOW_FULLSCREEN_DESKTOP, which on macOS will generate a new "fullscreen space" (which is an OS-provided virtual desktop), refusing the redraw the window when it isn't in the foreground? Is it that QEMU won't draw to the window when it thinks it hasn't changed?

I have to say that I do not have the answer to what's confusing you. I can only describe the problem, QEMU will draw the window but macOS won't paint it. If I keep QEMU in windowed mode, then I can get macOS to paint it by resizing/moving QEMU window, without any modification to SDL2. This obviously did not work when QEMU was in fullscreen due to the if condition that blocked window position into macOS compositor.

One can definitely keep drawing to a fullscreen space's window in general and the compositor will notice, so I must be misunderstanding the problem here.

You could be right, but that was not always the case. For eg. running $ glxinfo -B will blank out QEMU window in fullscreen. I do not understand well enough QEMU display implementation how it tracked and flushed display. The problem seems to revolve around SDL_GL_DeleteContext(). Here's the macOS-only change that goes into QEMU:

diff -Nru ../orig/qemu-6.2.0/ui/sdl2-gl.c ./ui/sdl2-gl.c
--- ../orig/qemu-6.2.0/ui/sdl2-gl.c     2021-12-14 12:42:01.000000000 -0800
+++ ./ui/sdl2-gl.c      2021-12-15 20:12:01.001487700 -0800
@@ -170,6 +170,13 @@

 void sdl2_gl_destroy_context(DisplayChangeListener *dcl, QEMUGLContext ctx)
 {
+#ifdef CONFIG_DARWIN
+    /* Apple OpenGL FBO blit quirk
+     * Require SDL2 fix in https://github.com/libsdl-org/SDL/issues/4986
+     */
+    struct sdl2_console *scon = container_of(dcl, struct sdl2_console, dcl);
+    SDL_SetWindowPosition(scon->real_window, SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED);
+#endif
     SDL_GLContext sdlctx = (SDL_GLContext)ctx;

     SDL_GL_DeleteContext(sdlctx);

The rest of Virgil 3D SDL2 OpenGL implementation is exactly the same for Windows, Linux & macOS. I had even forced the same codes on Linux (without GBM & EGL), just SDL2 OpenGL and it worked.

Virgil 3D with SDL2 OpenGL for macOS depends on a quirk that requires window position to reach down into macOS compositor for screen updates. IMHO, even without considering such macOS specific quirk, it is conceptually incorrect to block window position for SDL_WINDOW_FULLSCREEN_DESKTOP. The fake fullscreen should possess similar movable property as borderless window.

icculus commented 2 years ago

Thanks, it's helpful to see the exact source line in QEMU.

I'm going to get QEMU building over here and look at this directly, because it seems like some weird interaction beyond the window size issue.

For now, though, I'll move this to 2.0.24, because we're running out of time on 2.0.22.

slouken commented 1 year ago

We've been focused on SDL3 development, so I'm moving this out of the milestone. Please let us know if you have more information or a fix.

kjliew commented 1 year ago

The 1-liner fix had already been offered since the beginning.

slouken commented 1 year ago

The problem is that according to the API contract, setting the window position in fullscreen mode is only supposed to set where the window will be once it leaves fullscreen mode, so regardless of platform, this would be a breaking change.

Have you tracked down the underlying reason this fixes the issue for you?

slouken commented 1 year ago

Could it be that the context is being deleted on the wrong thread?

slouken commented 1 year ago

I'm wondering if it could be related to https://github.com/libsdl-org/SDL/issues/7410?

kjliew commented 1 year ago

If you wished to enforce such API contract, then just send the same window position again and discard the (x, y) inputs for FULLSCREEN_DESKTOP only. This is essentially no-op but achieves the same effect of kicking the macOS underlying compositor.

Have you tracked down the underlying reason this fixes the issue for you? Could it be that the context is being deleted on the wrong thread?

Unfortunately, I highly depend on Windows/Linux for debugging and this issue just applies for macOS. I can't do much over there.

slouken commented 1 year ago

I was able to build qemu for macOS, and I did fix an unrelated issue in 1bd9ebf53, but I wasn't able to reproduce the bug listed here.

I ran qemu-system-i386 -full-screen and watched it successfully start the BIOS in fullscreen mode. Are there other steps required to reproduce this?

kjliew commented 1 year ago

You need Linux with GL acceleration, for eg. Ubuntu AArch64 or ArchLinux AArch64, and qemu for macOS with virglrenderer.

On GNOME/Wayland, this issue reproduces 100% every time after GDM login, or just run glxinfo -B on the terminal.

slouken commented 1 year ago

You mean as a guest OS? Do you have a disk image I can run to reproduce this?

Do I have to do anything special to enable virglrenderer when building or running qemu?

kjliew commented 1 year ago

You mean as a guest OS? Do you have a disk image I can run to reproduce this?

Yes, Linux as a guest OS with GL acceleration. I do, but it's huge, over 4GB in compressed size.

Do I have to do anything special to enable virglrenderer when building or running qemu?

It's an unfortunate "YES" for macOS. The virglrender is solely Linux focus. https://github.com/kjliew/qemu-3dfx/tree/master/virgil3d

slouken commented 1 year ago

Sure, if you give me a download link for the disk image and a qemu configure and run command line, I'll be happy to check it out. I'm applying the patches from your tree now.

kjliew commented 1 year ago

I am sorry that I don't have anything that can host such a huge download. ArchLinux AArch64 setup is quick & easy if you like Linux. Otherwise, Ubuntu server AArch64 has guided setup. It won't take a long time for Apple M1 superb performance and SSD throughput.

slouken commented 1 year ago

Okay, what qemu target do I need to build to run AArch64 Linux?

slouken commented 1 year ago

Okay, I'm configuring with configure --target-list=aarch64-softmmu, and here's my configuration output:

qemu 7.2.50

  Directories
    Install prefix               : /usr/local
    BIOS directory               : share/qemu
    firmware path                : share/qemu-firmware
    binary directory             : /usr/local/bin
    library directory            : /usr/local/lib
    module directory             : lib/qemu
    libexec directory            : /usr/local/libexec
    include directory            : /usr/local/include
    config directory             : /usr/local/etc
    local state directory        : /usr/local/var
    Manual directory             : /usr/local/share/man
    Doc directory                : /usr/local/share/doc
    Build directory              : /Users/valve/projects/qemu/build
    Source path                  : /Users/valve/projects/qemu
    GIT submodules               : ui/keycodemapdb tests/fp/berkeley-testfloat-3 tests/fp/berkeley-softfloat-3 dtc

  Host binaries
    git                          : git
    make                         : make
    python                       : /opt/homebrew/opt/python@3.11/bin/python3.11 (version: 3.11)
    sphinx-build                 : NO
    iasl                         : NO
    genisoimage                  : 

  Configurable features
    Documentation                : NO
    system-mode emulation        : YES
    user-mode emulation          : NO
    block layer                  : YES
    Install blobs                : YES
    module support               : NO
    fuzzing support              : NO
    Audio drivers                : coreaudio sdl
    Trace backends               : log
    D-Bus display                : NO
    QOM debugging                : NO
    vhost-kernel support         : NO
    vhost-net support            : NO
    vhost-user support           : NO
    vhost-user-crypto support    : NO
    vhost-user-blk server support: NO
    vhost-vdpa support           : NO
    build guest agent            : NO

  Compilation
    host CPU                     : aarch64
    host endianness              : little
    C compiler                   : cc
    Host C compiler              : cc
    C++ compiler                 : c++
    Objective-C compiler         : clang
    CFLAGS                       : -g -O2
    CXXFLAGS                     : -g -O2
    OBJCFLAGS                    : -g -O2
    QEMU_CFLAGS                  : -DOS_OBJECT_USE_OBJC=0 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wmissing-format-attribute -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -Wno-psabi -Wno-gnu-variable-sized-type-not-at-end -fstack-protector-strong
    QEMU_CXXFLAGS                : -DOS_OBJECT_USE_OBJC=0 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wmissing-format-attribute -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -Wno-psabi -Wno-gnu-variable-sized-type-not-at-end -fstack-protector-strong
    QEMU_OBJCFLAGS               : -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wmissing-format-attribute -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -Wno-psabi -Wno-gnu-variable-sized-type-not-at-end
    QEMU_LDFLAGS                 : -fstack-protector-strong
    profiler                     : NO
    link-time optimization (LTO) : NO
    PIE                          : NO
    static build                 : NO
    malloc trim support          : NO
    membarrier                   : NO
    debug stack usage            : NO
    mutex debugging              : NO
    memory allocator             : system
    avx2 optimization            : NO
    avx512bw optimization        : NO
    avx512f optimization         : NO
    gprof                        : NO
    gcov                         : NO
    thread sanitizer             : NO
    CFI support                  : NO
    strip binaries               : NO
    sparse                       : NO
    mingw32 support              : NO

  Targets and accelerators
    KVM support                  : NO
    HAX support                  : NO
    HVF support                  : YES
    WHPX support                 : NO
    NVMM support                 : NO
    Xen support                  : NO
    Xen emulation                : NO
    TCG support                  : YES
    TCG backend                  : native (aarch64)
    TCG plugins                  : YES
    TCG debug enabled            : NO
    target list                  : aarch64-softmmu
    default devices              : YES
    out of process emulation     : NO
    vfio-user server             : NO

  Block layer support
    coroutine backend            : sigaltstack
    coroutine pool               : YES
    Block whitelist (rw)         : 
    Block whitelist (ro)         : 
    Use block whitelist in tools : NO
    VirtFS support               : YES
    Live block migration         : YES
    replication support          : YES
    bochs support                : YES
    cloop support                : YES
    dmg support                  : YES
    qcow v1 support              : YES
    vdi support                  : YES
    vvfat support                : YES
    qed support                  : YES
    parallels support            : YES
    FUSE exports                 : NO
    VDUSE block exports          : NO

  Crypto
    TLS priority                 : NORMAL
    GNUTLS support               : NO
    libgcrypt                    : NO
    nettle                       : NO
    AF_ALG support               : NO
    rng-none                     : NO
    Linux keyring                : NO

  Dependencies
    Cocoa support                : YES
    vmnet.framework support      : YES
    SDL support                  : YES
    SDL image support            : YES 2.7.0
    GTK support                  : NO
    pixman                       : YES 0.42.2
    VTE support                  : NO
    slirp support                : NO
    libtasn1                     : NO
    PAM                          : YES
    iconv support                : YES
    curses support               : YES
    virgl support                : NO
    blkio support                : NO
    curl support                 : YES 7.64.1
    Multipath support            : NO
    PNG support                  : YES 1.6.37
    VNC support                  : YES
    VNC SASL support             : YES
    VNC JPEG support             : NO
    CoreAudio support            : YES
    JACK support                 : NO
    brlapi support               : NO
    vde support                  : NO
    netmap support               : NO
    l2tpv3 support               : NO
    Linux AIO support            : NO
    Linux io_uring support       : NO
    ATTR/XATTR support           : NO
    RDMA support                 : NO
    PVRDMA support               : NO
    fdt support                  : internal
    libcap-ng support            : NO
    bpf support                  : NO
    spice protocol support       : NO
    rbd support                  : NO
    smartcard support            : NO
    U2F support                  : NO
    libusb                       : YES 1.0.26
    usb net redir                : NO
    OpenGL support (epoxy)       : NO
    EGL                          : NO
    GBM                          : NO
    libiscsi support             : NO
    libnfs support               : NO
    seccomp support              : NO
    GlusterFS support            : NO
    TPM support                  : YES
    libssh support               : NO
    lzo support                  : NO
    snappy support               : NO
    bzip2 support                : YES
    lzfse support                : NO
    zstd support                 : NO
    NUMA host support            : NO
    capstone                     : NO
    libpmem support              : NO
    libdaxctl support            : NO
    libudev                      : NO
    FUSE lseek                   : NO
    selinux                      : NO
    libdw                        : NO

  User defined options
    Native files                 : config-meson.cross
    prefix                       : /usr/local
    b_pie                        : false
    vfio_user_server             : disabled
kjliew commented 1 year ago

You missed out libepoxy for OpenGL support.

Make sure the configure log has

virgl support                : YES 0.10.4
OpenGL support (epoxy)       : YES 1.5.10
EGL                          : NO
GBM                          : NO
slouken commented 1 year ago

Okay, I have it built and installed. What is the command line to boot the VM?

slouken commented 1 year ago

I'm trying:

qemu-system-aarch64 \
         -machine virt,accel=hvf,highmem=off \
         -cpu cortex-a72 -smp 2 -m 3G \
         -device intel-hda -device hda-output \
         -device qemu-xhci \
         -device usb-kbd \
         -device virtio-mouse-pci \
         -drive "if=pflash,format=raw,file=./edk2-aarch64-code.fd,readonly=on" \
         -drive "if=pflash,format=raw,file=./edk2-arm-vars.fd,discard=on" \
         -drive "if=virtio,format=raw,file=./hdd.raw,discard=on" \
         -cdrom Fedora-Workstation-Live-aarch64-37-1.7.iso

Just to get something running, but it just comes up with the QEMU monitor console and doesn't boot.

I'm about out of time, so I'm going to wait for good instructions on how to get a bootable Linux environment.

kjliew commented 1 year ago

You missed out the GPU and -cdrom won't work on ARM virt machine.

qemu-system-aarch64 \
         -machine virt,accel=hvf,highmem=off \
         -cpu cortex-a72 -smp 2 -m 3G \
         -display sdl,gl=on -device virtio-gpu-gl-pci \
         -device intel-hda -device hda-output \
         -device qemu-xhci \
         -device usb-kbd \
         -device virtio-mouse-pci \
         -drive "if=pflash,format=raw,file=./edk2-aarch64-code.fd,readonly=on" \
         -drive "if=pflash,format=raw,file=./edk2-arm-vars.fd,discard=on" \
         -drive "if=virtio,format=raw,file=./hdd.raw,discard=on" \
         -device usb-storage,drive=fdrw -drive id=fdrw,if=none,media=cdrom,file=Fedora-Workstation-Live-aarch64-37-1.7.iso
slouken commented 1 year ago

That's closer... BdsDxe: failed to load Boot0001 "UEFI Misc Device" from VenHw(...) I don't necessarily need to boot from a Fedora image, I just couldn't find an Arch aarch64 installer and instructions on how to use it.

kjliew commented 1 year ago

When you get to the "Tiano EFI Framework" logo, press "Fn-F2" to enter the UEFI setup. You should be able to select "Boot Manager" and boot from the USB-CD.

Arch AArch64 gist https://gist.github.com/thalamus/561d028ff5b66310fac1224f3d023c12

slouken commented 1 year ago

Okay, I found an archboot image at https://pkgbuild.com/~tpowa/archboot/iso/aarch64/latest/, and I found out how to set up bridged networking with macOS.

Here's my boot command:

qemu-system-aarch64 \
         -machine virt,accel=hvf,highmem=off \
         -cpu cortex-a72 -smp 2 -m 3G \
         -display sdl,gl=on -device virtio-gpu-gl-pci \
         -device intel-hda -device hda-output \
         -device qemu-xhci \
         -device usb-kbd \
         -device virtio-mouse-pci \
         -nic user,model=virtio-net-pci,hostfwd=tcp::50022-:22 \
         -drive "if=pflash,format=raw,file=./edk2-aarch64-code.fd,readonly=on" \
         -drive "if=pflash,format=raw,file=./edk2-arm-vars.fd,discard=on" \
         -drive "if=virtio,format=raw,file=./hdd.raw,discard=on" \
         -device usb-storage,drive=fdrw -drive id=fdrw,if=none,media=cdrom,file=archboot-2023.03.07-20.25-aarch64.iso

... and I successfully installed an Arch Linux system.

slouken commented 1 year ago

So I'm not running fullscreen yet, just trying to get this working, and I have GDM starting at boot, but all I get is a black screen and these warnings:

gl_version 41 - core profile enabled
WARNING: running without ARB/KHR robustness in place may crash
GLSL feature level 410
vrend_check_no_error: context error reported 2 "gnome-shell" Unknown 1280
context 2 failed to dispatch CREATE_OBJECT: 22
vrend_decode_ctx_submit_cmd: context error reported 2 "gnome-shell" Illegal command buffer 591617
vrend_check_no_error: context error reported 3 "Xwayland" Unknown 1280
context 3 failed to dispatch CREATE_OBJECT: 22
vrend_decode_ctx_submit_cmd: context error reported 3 "Xwayland" Illegal command buffer 591617
vrend_check_no_error: context error reported 2 "gnome-shell" Unknown 1280

If I run with -display sdl,gl=off -device virtio-gpu-pci then GDM comes up and I am able to log in.

slouken commented 1 year ago

I am using virglrenderer from here: https://github.com/akihikodaki/virglrenderer/tree/macos

kjliew commented 1 year ago

The other virglrenderer tree was customized for libANGLE/GLES/CocoaGL, AFAICT. I doubt it ever works for SDL2 OpenGL. You should just use the patch from qemu-3dfx for virglrenderer 0.10.4.

slouken commented 1 year ago

Okay, that worked much better. :) I finally have a working installation and can start trying to repro the issue.

slouken commented 1 year ago

So one thing I noticed without even going fullscreen is that glxinfo -B makes the window flash black. Is that normal? I know that macOS is very picky about multi-threading and GL contexts, and that is often a symptom of invalid operations.

kjliew commented 1 year ago

So one thing I noticed without even going fullscreen is that glxinfo -B makes the window flash black. Is that normal? I know that macOS is very picky about multi-threading and GL contexts, and that is often a symptom of invalid operations.

Yes. Congratulation! You have the exact repro now! 😁 Isn't that simple?

slouken commented 1 year ago

Except that I'm running windowed and I've applied your patch. :)

kjliew commented 1 year ago

Did you mean my offered patch for SDL2 or just the misc QEMU/virglrenderer?

Running windowed or fullscreen doesn't matter. QEMU uses FULLSCREEN_DESKTOP and that's just fake borderless window. Without my offered SDL2 patch, you will just get black window on GL acceleration. In windowed mode, you can resize/move the window to kick the compositor to repaint it. You can't do that in fullscreen.

slouken commented 1 year ago

Okay, I tracked down the root cause of this, and fixed it in f5c0760c6. Thanks for your patience!

slouken commented 1 year ago

You can delete the code in qemu that does the macOS setposition hack, it's no longer necessary. :)

sezero commented 1 year ago

I was able to build qemu for macOS, and I did fix an unrelated issue in 1bd9ebf53,

That and the other (https://github.com/libsdl-org/SDL/commit/e0e79419b68ce80bb15621e736e37e6f6ae365e5) aren't applicable / not needed in SDL3, yes?

slouken commented 1 year ago

No, I don't believe so. That code has been entirely reworked for SDL3.

kjliew commented 1 year ago

I know you just released 2.26.4. Any hope we can see this in 2.26.5 instead of 2.28.0?

sezero commented 1 year ago

in 2.26.5 instead of 2.28.0?

I actually hope that 2.28.0 is scheduled for release soon, instead of 2.26.x

kjliew commented 1 year ago

I cherry-picked https://github.com/libsdl-org/SDL/commit/63e6c19b7d74212ff2e7018963332ff408d10d97 and https://github.com/libsdl-org/SDL/commit/f5c0760c6bf465ffd9fcbb48ca0bcffa748c5d66 into 2.26.4 and everything looks great, including all the tricks involving SDL2 I made for dosbox and qemu on macOS.

Just FYI.

slouken commented 1 year ago

We'll have one more 2.26.5 before 2.28.0. We don't have a firm date for 2.28 yet, but it'll likely be toward the end of April. I'm glad the patches work well for you, but they're a big enough behavior change that I'd like them to get more testing before they go into a point release.

slouken commented 1 year ago

Also, as an aside, the mouse warping behavior was really frustrating, so I made a quick patch to improve it. As noted, the right way to handle this is for qemu to render the cursor at the guest position instead of warping the system cursor, but this is a bit better.

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 8cb77416af..4cd5081108 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -51,6 +51,9 @@ static SDL_Cursor *sdl_cursor_hidden;
 static int absolute_enabled;
 static int guest_cursor;
 static int guest_x, guest_y;
+static int mouse_warped;
+static int mouse_warp_x;
+static int mouse_warp_y;
 static SDL_Cursor *guest_sprite;
 static Notifier mouse_mode_notifier;

@@ -224,6 +227,9 @@ static void sdl_grab_start(struct sdl2_console *scon)
     if (guest_cursor) {
         SDL_SetCursor(guest_sprite);
         if (!qemu_input_is_absolute() && !absolute_enabled) {
+            mouse_warped = 1;
+            mouse_warp_x = guest_x;
+            mouse_warp_y = guest_y;
             SDL_WarpMouseInWindow(scon->real_window, guest_x, guest_y);
         }
     } else {
@@ -295,16 +301,13 @@ static void sdl_send_mouse_event(struct sdl2_console *scon, int dx, int dy,
         qemu_input_queue_abs(scon->dcl.con, INPUT_AXIS_Y,
                              y, 0, surface_height(scon->surface));
     } else {
-        if (guest_cursor) {
-            x -= guest_x;
-            y -= guest_y;
-            guest_x += x;
-            guest_y += y;
-            dx = x;
-            dy = y;
+        if (mouse_warped && x == mouse_warp_x && y == mouse_warp_y) {
+            /* Ignore mouse warp motion */
+            mouse_warped = 0;
+        } else {
+            qemu_input_queue_rel(scon->dcl.con, INPUT_AXIS_X, dx);
+            qemu_input_queue_rel(scon->dcl.con, INPUT_AXIS_Y, dy);
         }
-        qemu_input_queue_rel(scon->dcl.con, INPUT_AXIS_X, dx);
-        qemu_input_queue_rel(scon->dcl.con, INPUT_AXIS_Y, dy);
     }
     qemu_input_event_sync();
 }
@@ -715,7 +718,26 @@ static void sdl_mouse_warp(DisplayChangeListener *dcl,
         if (gui_grab || qemu_input_is_absolute() || absolute_enabled) {
             SDL_SetCursor(guest_sprite);
             if (!qemu_input_is_absolute() && !absolute_enabled) {
-                SDL_WarpMouseInWindow(scon->real_window, x, y);
+                /*
+                 * Only warp if we're far away from the desired position,
+                 * this prevents continually warping and jittering.
+                 *
+                 * The real solution is to use relative mouse motion
+                 * and have qemu render the cursor itself at the expected 
+                 * location if the input isn't absolute.
+                 */
+                const int WARP_THRESHOLD_SQUARED = 100;
+                int current_x, current_y, delta_x, delta_y;
+
+                SDL_GetMouseState(&current_x, &current_y);
+                delta_x = (current_x - x);
+                delta_y = (current_y - y);
+                if ((delta_x * delta_x + delta_y * delta_y) > WARP_THRESHOLD_SQUARED) {
+                    mouse_warped = 1;
+                    mouse_warp_x = x;
+                    mouse_warp_y = y;
+                    SDL_WarpMouseInWindow(scon->real_window, x, y);
+                }
             }
         }
     } else if (gui_grab) {