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.

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

fix the red flashing during live resize #1418

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

allefant
Copy link
Contributor

@allefant allefant commented Mar 8, 2023

I did some more digging about the broken live resize under MacOS reported in #1350. Tracing through a resize in ex_resize2 in lldb the following happened:

  • we call al_clear_to_color()
  • we call al_draw_bitmap() a few times
  • resize happens, this causes MacOS to call setFrameSize on the NSOpenGLView (on the main thread), which in turn calls glViewport() on the main thread (in some internal MacOS code)
    • if this happens during another GL call on the user thread we violate the requirement to serialize all OpenGL calls between threads and MacOS courteously will flash red
    • if this happens outside of another GL call, we get a random glViewport call with a new size in the middle of our drawing and it completely messes up the frame, almost as bad as red (I can actually see this in my game as well, in addition to the red flashes things just wildly jump around sometimes during live resizes)
  • we call al_draw_bitmap() a few more times
  • we call al_flip_display()
  • we handle events and see the ALLEGRO_EVENT_DISPLAY_RESIZE but it is much too late to do anything about it
  • we call al_acknowledge_display(), again, much too late

I think flutter works around this in some complex way involving an off-screen drawable unaffected by the out-of-sync glViewport call, but not really sure. I think SDL works around it by a) not offering live resize and b) somehow not having an OpenGL context on the main thread at all, but I don't fully follow their code either.

I think it would be best if we disable live resize entirely as it's a useless feature, but this PR still seems to be needed even then since MacOS still makes that asynchronous glViewport call for its live-zoom effect... I almost feel we use some setting somewhere which makes MacOS auto-resize the GL viewport, but I couldn't find anything obvious. Anyway, if there's no better ideas I'll try and fix the setFrameSize() issue, probably with a hack using the windowWillResize() notification to know when the next setFrameSize() needs to be ignored...

@allefant allefant changed the title [WIP] proof of concept to fix the red flashing during live resize fix the red flashing during live resize Mar 9, 2023
@allefant
Copy link
Contributor Author

allefant commented Mar 9, 2023

Only deferring setFrameSize after a resize now.

@allefant
Copy link
Contributor Author

After discussing with Mark Oates on IRC I think the reason it works with the SDL port may simply be the fact that Allegro's SDL port is single threaded - only ALLEGRO_MACOS uses the special main addon, ALLEGRO_SDL does not. With a single thread we obviously can never violate the serialized OpenGL commands requirement.

@SiegeLord
Copy link
Member

On my new system I get a repeatable crash in ex_resize2 with or without this change as soon as I resize a window:

warning: liballegro.5.2.dylib was compiled with optimization - stepping may behave oddly; variables may not be available.
* thread #4, stop reason = EXC_BAD_ACCESS (code=1, address=0x38)
    frame #0: 0x0000000118ac7124 AppleMetalOpenGLRenderer`GLDTextureRec::readTextureDataCore(unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, bool, int, int, GLDPixelModeRec const*, void const*, GLDBufferRec*, bool) + 176
    frame #1: 0x0000000118adfa98 AppleMetalOpenGLRenderer`gldReadFramebufferData + 784
    frame #2: 0x00000001eb04cba4 GLEngine`glReadPixels_Exec + 860
  * frame #3: 0x00000001004bb5a8 liballegro.5.2.dylib`_al_ogl_lock_region_new [inlined] ogl_lock_region_backbuffer(bitmap=0x000000010182d3d0, ogl_bitmap=0x0000600001754900, x=0, gl_y=0, w=659, h=480, format=17, flags=<unavailable>) at ogl_lock.c:223:7 [opt]
    frame #4: 0x00000001004bb52c liballegro.5.2.dylib`_al_ogl_lock_region_new(bitmap=0x000000010182d3d0, x=0, y=<unavailable>, w=659, h=480, format=17, flags=<unavailable>) at ogl_lock.c:159:15 [opt]
    frame #5: 0x0000000100464688 liballegro.5.2.dylib`al_lock_bitmap_region(bitmap=0x000000010182d3d0, x=<unavailable>, y=<unavailable>, width=<unavailable>, height=<unavailable>, format=<unavailable>, flags=<unavailable>) at bitmap_lock.c:108:12 [opt]
    frame #6: 0x00000001004c261c liballegro.5.2.dylib`_al_draw_soft_triangle(v1=0x000000016ff9e9d0, v2=0x000000016ff9e9f4, v3=0x000000016ff9ea18, state=6173616080, init=(liballegro.5.2.dylib`shader_texture_solid_any_init at tri_soft.c:230), first=(liballegro.5.2.dylib`shader_texture_solid_any_first at tri_soft.c:266:39), step=(liballegro.5.2.dylib`shader_texture_solid_any_step at tri_soft.c:282:8), draw=(liballegro.5.2.dylib`shader_texture_solid_any_draw_shade_white_repeat at scanline_drawers.inc:2526)) at tri_soft.c:856:18 [opt]
    frame #7: 0x00000001004c2400 liballegro.5.2.dylib`_al_triangle_2d(texture=0x000000010061f2b0, v1=0x000000016ff9e9d0, v2=0x000000016ff9e9f4, v3=0x000000016ff9ea18) at tri_soft.c:0:17 [opt]
    frame #8: 0x000000010049ad70 liballegro.5.2.dylib`_al_draw_bitmap_region_memory [inlined] _al_draw_transformed_bitmap_memory(src=0x000000010061f2b0, tint=(r = 1, g = 1, b = 1, a = 1), sx=<unavailable>, sy=0, sw=320, sh=<unavailable>, dw=320, dh=<unavailable>, local_trans=0x000000016ff9e990, flags=<unavailable>) at memblit.c:241:4 [opt]
    frame #9: 0x000000010049abb4 liballegro.5.2.dylib`_al_draw_bitmap_region_memory [inlined] _al_draw_transformed_scaled_bitmap_memory(src=0x000000010061f2b0, tint=<unavailable>, sx=<unavailable>, sy=0, sw=320, sh=<unavailable>, dx=<unavailable>, dy=<unavailable>, dw=320, dh=<unavailable>, flags=<unavailable>) at memblit.c:258:4 [opt]
    frame #10: 0x000000010049ab8c liballegro.5.2.dylib`_al_draw_bitmap_region_memory(src=0x000000010061f2b0, tint=(r = 1, g = 1, b = 1, a = 1), sx=<unavailable>, sy=0, sw=320, sh=<unavailable>, dx=<unavailable>, dy=<unavailable>, flags=<unavailable>) at memblit.c:159:4 [opt]
    frame #11: 0x00000001004637c0 liballegro.5.2.dylib`_draw_tinted_rotated_scaled_bitmap_region [inlined] _bitmap_drawer(bitmap=0x000000010061f2b0, tint=<unavailable>, sx=1, sy=1, sw=320, sh=200, flags=0) at bitmap_draw.c:0:13 [opt]
    frame #12: 0x00000001004636ec liballegro.5.2.dylib`_draw_tinted_rotated_scaled_bitmap_region(bitmap=<unavailable>, tint=<unavailable>, cx=<unavailable>, cy=<unavailable>, angle=<unavailable>, xscale=<unavailable>, yscale=2.4000001, sx=1, sy=1, sw=<unavailable>, sh=<unavailable>, dx=0, dy=0, flags=0) at bitmap_draw.c:118:4 [opt]
    frame #13: 0x00000001004639b0 liballegro.5.2.dylib`al_draw_scaled_bitmap [inlined] al_draw_tinted_scaled_bitmap(bitmap=<unavailable>, tint=<unavailable>, sx=<unavailable>, sy=<unavailable>, sw=<unavailable>, sh=<unavailable>, dx=<unavailable>, dy=<unavailable>, dw=<unavailable>, dh=<unavailable>, flags=<unavailable>) at bitmap_draw.c:171:4 [opt]
    frame #14: 0x0000000100463974 liballegro.5.2.dylib`al_draw_scaled_bitmap(bitmap=<unavailable>, sx=<unavailable>, sy=<unavailable>, sw=<unavailable>, sh=<unavailable>, dx=<unavailable>, dy=<unavailable>, dw=<unavailable>, dh=<unavailable>, flags=<unavailable>) at bitmap_draw.c:184:4 [opt]
    frame #15: 0x0000000100003b14 ex_resize2`_al_mangled_main(argc=<unavailable>, argv=<unavailable>) at ex_resize2.c:57:10 [opt]
    frame #16: 0x0000000100493b64 liballegro.5.2.dylib`call_user_main at osx_app_delegate.m:217:10 [opt]
    frame #17: 0x0000000100493b40 liballegro.5.2.dylib`+[AllegroAppDelegate app_main:](self=<unavailable>, _cmd=<unavailable>, arg=<unavailable>) at osx_app_delegate.m:228:5 [opt]
    frame #18: 0x000000019c8ac0d0 Foundation`__NSThread__start__ + 716
    frame #19: 0x00000001003dd5d4 libsystem_pthread.dylib`_pthread_start + 148

@elias-pschernig
Copy link
Member

On my new system I get a repeatable crash in ex_resize2 with or without this change as soon as I resize a window

I only have an old Intel Mac - maybe they replaced the red flash with an outright crash for Apple silicon. I can at least try upgrading to the newest version maybe i can reproduce with that.

@SiegeLord
Copy link
Member

For completeness, sending HALT/RESUME_DRAWING events from windowWillStartLiveResize/viewDidEndLiveResize and respecting them in the ex_resize2 seems to work.

SiegeLord pushed a commit to SiegeLord/allegro5 that referenced this pull request May 21, 2023
Elias investigated the issue in liballeg#1418, showing that there was some concurrent
OpenGL usage between OS's internals and user code. That specific workaround did
not work for me on my Mac, so as an alternative, in this commit we add an
option to bypass live resize altogether. In short, the user opts into drawing
halt/resume events via a config option, and ceases to draw while live resize is
underway. This isn't ideal, but at least is a way to prevent the crashing
behavior until we find some better idea.
@SiegeLord
Copy link
Member

Just found godotengine/godot#54519, but the fix there doesn't seem to work for me (they didn't seem to like it either, as it caused artifacts for them).

SiegeLord pushed a commit that referenced this pull request Sep 13, 2023
Elias investigated the issue in #1418, showing that there was some concurrent
OpenGL usage between OS's internals and user code. That specific workaround did
not work for me on my Mac, so as an alternative, in this commit we add an
option to bypass live resize altogether. In short, the user opts into drawing
halt/resume events via a config option, and ceases to draw while live resize is
underway. This isn't ideal, but at least is a way to prevent the crashing
behavior until we find some better idea.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants