GNOME Bugzilla – Bug 745090
android: Fix changing GL window handle
Last modified: 2015-05-20 19:43:37 UTC
When we change the window handle, we need to make sure the backing EGL surface is properly recreated
Created attachment 297768 [details] [review] gl/egl: Detect window handle changes When (re)activating the context, the backing window handle might have changed. If that happened, destroy the previous surface and create a new one
Created attachment 297769 [details] [review] glwindow: Deactivate window before changing handle When setting a new window handle, we need to ensure all implementations will detect the change. For that we deactivate the context before setting the window handle, then reactivate the context
commit a12ca13750a15300ab3c718ebde2984dc3d587b3 Author: Edward Hervey <bilboed@bilboed.com> Date: Tue Feb 24 14:01:04 2015 +0100 glwindow: Deactivate window before changing handle When setting a new window handle, we need to ensure all implementations will detect the change. For that we deactivate the context before setting the window handle, then reactivate the context https://bugzilla.gnome.org/show_bug.cgi?id=745090 commit 7075b2528884b8425ce78153a34e253359950b16 Author: Edward Hervey <bilboed@bilboed.com> Date: Tue Feb 24 13:58:26 2015 +0100 gl/egl: Detect window handle changes When (re)activating the context, the backing window handle might have changed. If that happened, destroy the previous surface and create a new one https://bugzilla.gnome.org/show_bug.cgi?id=745090
Commit a12ca13750a15300ab3c718ebde2984dc3d587b3 broke EAGL. EAGL does not refresh its private fields when it gets a new window handle. When gst_gl_context_eagl_create_context is called, the (async) window handle didn't arrive yet, so: if (window_handle) { ... } else { priv->eagl_layer = NULL; // <-- OUCH! priv->framebuffer = 0; priv->color_renderbuffer = 0; priv->depth_renderbuffer = 0; } This obviously causes everything else down the line not to work, starting with: void gst_gl_context_eagl_resize (GstGLContextEagl * eagl_context) { int width, height; glBindRenderbuffer (GL_RENDERBUFFER, eagl_context->priv->color_renderbuffer); [eagl_context->priv->eagl_context renderbufferStorage:GL_RENDERBUFFER fromDrawable:eagl_context->priv->eagl_layer]; // ^ = nil ... P.S. Sebastian, eventually the way to find this bug was simply to add an "Open GL ES Error" breakpoint (that's an Xcode feature)
Created attachment 300503 [details] [review] Revert "glwindow: Deactivate window before changing handle"
Can't we fix iOS/OSX instead of reverting ?
(In reply to Nicolas Dufresne (stormer) from comment #6) > Can't we fix iOS/OSX instead of reverting ? I think we should fix. The revert patch was just for people to use in the meanwhile.
This is caused by a number of things. 1. this patch 2. bc7a7259f357b0065dd94e0668b5a895d83fa53a fixing up the send_message_async calling onto a NULL GMainContext resulting in performing the set_window_handle on the main context which is racy. However that moved the set_window_handle to after the context_class->create_context call. 3. eagl context class assumes that the view is always available by the context_class->create_context call. One fix is to bring back the old behaviour of immediately setting the window handle if the window main loop is not running. This will only be non-racy iff set_window_handle is synchronised with context_create (either calling thread or gl thread will work) which mostly happens already. The only time that doesn't hold is when the app explicitly sets the window handle while we're creating the context but that's currently racy anyway. The other more involved fix is to get the layer from the window/view whenever it needs the access the layer so it always has the latest layer and can detect layer/view changes. The layer should also not be context state as it currently is.
commit 6981a8d15b17094cde8a2cec0a2c821c6eaa9547 Author: Alessandro Decina <alessandro.d@gmail.com> Date: Thu Apr 2 18:05:55 2015 +1100 libgstgl: fix rendering on iOS Stop assuming that the handle has been set by the time ->create_context is called. After bc7a7259f357b0065dd94e0668b5a895d83fa53a set_window_handle always happens after ->create_context in fact. See also https://bugzilla.gnome.org/show_bug.cgi?id=745090
What's the status here? Was this accidentially fixed by one of the recent commits or ...?
I reopened this due to the EAGL regression which was since fixed (see comment 9), so I'm resolving.
I'm suspecting this patch to also break win32. I set a hWnd but it still popup a new window. I checked that gst_gl_window_set_window_handle() is called, but _set_window_handle_cb is never called back. Shouldn't it be using the sync call instead?
We can't use the sync callback as set_window_handle can be called before the window loop is running and when we try to wait for the loop to run, the callback is never executed. Please check if this breaks win32 and reopen the bug if necessary.
Reopening. I confirm that this breaks win32, at least if I set the window handle in READY state. To make it work I have to revert commit a12ca13750a15300ab3c718ebde2984dc3d587b3 AND apply the patch from bug #749601
Just tested by setting the window from "prepare-window-handle" message and it both patch from bug #749601 and revert of commit a12ca13750a15300ab3c718ebde2984dc3d587b3 are still needed to make it work on windows. To be clear, I'm not suggestion to actually revert, it would be better to actually find why it doesn't work.
Ok, understood what happens here. In glimagesink's _ensure_gl_setup() it calls gst_video_overlay_prepare_window_handle() and gst_gl_window_set_window_handle() BEFORE gst_gl_context_create(). That means that when it set the window handle, it arrives in gst_gl_window_win32_send_message_async() and window_win32->internal_win_id is 0 because it didn't create it yet.
Created attachment 303695 [details] [review] gl: win32: use a GMainContext to dispatch win32 messages gst_gl_window_win32_send_message_async() could be called before the internal window is created so we cannot use PostMessage there. x11 and wayland backends both create a custom GSource for this, so there is no reason to not do that for win32.
Let's close this one and handle both patches in bug #749601.