After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 749601 - win32: glimagesink always popup a new window
win32: glimagesink always popup a new window
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-19 20:58 UTC by Xavier Claessens
Modified: 2015-06-19 20:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gl: win32: set the parent window when creating internal window (4.05 KB, patch)
2015-05-19 20:59 UTC, Xavier Claessens
none Details | Review
gl: win32: set the parent window when creating internal window (4.06 KB, patch)
2015-05-20 19:32 UTC, Xavier Claessens
none Details | Review
gl: win32: use a GMainContext to dispatch win32 messages (11.83 KB, patch)
2015-05-20 19:44 UTC, Xavier Claessens
none Details | Review
gl: win32: set the parent window when creating internal window (4.11 KB, patch)
2015-05-20 19:44 UTC, Xavier Claessens
none Details | Review
gl: win32: use a GMainContext to dispatch win32 messages (11.83 KB, patch)
2015-05-20 21:22 UTC, Xavier Claessens
committed Details | Review
gl: win32: set the parent window when creating internal window (4.11 KB, patch)
2015-05-20 21:22 UTC, Xavier Claessens
committed Details | Review
gl: win32: Fix leaked GstGLContext (1.96 KB, patch)
2015-05-20 21:22 UTC, Xavier Claessens
committed Details | Review
gl: win32: fix crash when finalizing GstGLContext (6.14 KB, patch)
2015-05-20 21:22 UTC, Xavier Claessens
none Details | Review
gl: win32: fix crash when finalizing GstGLContext (6.35 KB, patch)
2015-05-21 12:56 UTC, Xavier Claessens
committed Details | Review
gl: win32: remove unused code (1.70 KB, patch)
2015-05-21 19:38 UTC, Xavier Claessens
committed Details | Review
gl: win32: do not call SetParent in release_parent_win_id() (1.66 KB, patch)
2015-05-21 19:38 UTC, Xavier Claessens
committed Details | Review

Description Xavier Claessens 2015-05-19 20:58:37 UTC
On windows, using glimagesink, if I call set_window_handle() in READY state (before it creates its internal window) then when going in PLAYING state it will popup a new window.
Comment 1 Xavier Claessens 2015-05-19 20:59:02 UTC
Created attachment 303624 [details] [review]
gl: win32: set the parent window when creating internal window

When _set_window_handle() was called in READY state, it wasn't
set to the internal window created later.
Comment 2 Xavier Claessens 2015-05-19 21:01:23 UTC
Note that to work it also need to revert commit a12ca13750a15300ab3c718ebde2984dc3d587b3, see bug #745090 comment #14
Comment 3 Xavier Claessens 2015-05-20 14:04:39 UTC
Just tested, and this patch is also needed in the case I set the window from "prepare-window-handle" message.
Comment 4 Xavier Claessens 2015-05-20 19:32:30 UTC
Created attachment 303696 [details] [review]
gl: win32: set the parent window when creating internal window

When _set_window_handle() was called in READY state, it wasn't
set to the internal window created later.
Comment 5 Xavier Claessens 2015-05-20 19:44:52 UTC
Created attachment 303697 [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.
Comment 6 Xavier Claessens 2015-05-20 19:44:59 UTC
Created attachment 303698 [details] [review]
gl: win32: set the parent window when creating internal window

When _set_window_handle() was called in READY state, it wasn't
set to the internal window created later.
Comment 7 Xavier Claessens 2015-05-20 21:22:32 UTC
Created attachment 303714 [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.
Comment 8 Xavier Claessens 2015-05-20 21:22:37 UTC
Created attachment 303715 [details] [review]
gl: win32: set the parent window when creating internal window

When _set_window_handle() was called in READY state, it wasn't
set to the internal window created later.
Comment 9 Xavier Claessens 2015-05-20 21:22:45 UTC
Created attachment 303716 [details] [review]
gl: win32: Fix leaked GstGLContext
Comment 10 Xavier Claessens 2015-05-20 21:22:49 UTC
Created attachment 303717 [details] [review]
gl: win32: fix crash when finalizing GstGLContext

gst_gl_context_finalize() is calling gst_gl_window_win32_quit()
which was posting a message. But then window_proc takes window's
context and get a NULL.

Now that we've got a GMainLoop we can do like other backends and
simply call g_main_loop_quit().

This also remove duplicated code to release the parent window and
potential crash there because parent_proc could be NULL if we never
created the internal window. That could happen for example if setting
state to READY then setting a window_handle, and go back to NULL state.
Comment 11 Matthew Waters (ystreet00) 2015-05-21 04:52:05 UTC
Review of attachment 303714 [details] [review]:

Looks good
Comment 12 Matthew Waters (ystreet00) 2015-05-21 04:52:41 UTC
Review of attachment 303715 [details] [review]:

All looks fine except for.

  CC       libgstgl_win32_la-gstglwindow_win32.lo
gstglwindow_win32.c: In function ‘gst_gl_window_win32_set_window_handle’:
gstglwindow_win32.c:340:8: warning: unused variable ‘parent_id’ [-Wunused-variable]
   HWND parent_id;
Comment 13 Matthew Waters (ystreet00) 2015-05-21 04:53:35 UTC
Review of attachment 303716 [details] [review]:

Looks good.
Comment 14 Matthew Waters (ystreet00) 2015-05-21 04:55:16 UTC
Review of attachment 303717 [details] [review]:

Looks good.
Comment 15 Xavier Claessens 2015-05-21 12:56:18 UTC
Created attachment 303757 [details] [review]
gl: win32: fix crash when finalizing GstGLContext

gst_gl_context_finalize() is calling gst_gl_window_win32_quit()
which was posting a message. But then window_proc takes window's
context and get a NULL.

Now that we've got a GMainLoop we can do like other backends and
simply call g_main_loop_quit().

This also remove duplicated code to release the parent window and
potential crash there because parent_proc could be NULL if we never
created the internal window. That could happen for example if setting
state to READY then setting a window_handle, and go back to NULL state.
Comment 16 Xavier Claessens 2015-05-21 12:57:56 UTC
(In reply to Matthew Waters from comment #12)
>   CC       libgstgl_win32_la-gstglwindow_win32.lo
> gstglwindow_win32.c: In function ‘gst_gl_window_win32_set_window_handle’:
> gstglwindow_win32.c:340:8: warning: unused variable ‘parent_id’
> [-Wunused-variable]
>    HWND parent_id;

That warning actually comes from attachment #303717 [details], now updated.
Comment 17 Nicolas Dufresne (ndufresne) 2015-05-21 18:40:11 UTC
Attachment 303714 [details] pushed as e24bc34 - gl: win32: use a GMainContext to dispatch win32 messages
Attachment 303715 [details] pushed as 0acc18c - gl: win32: set the parent window when creating internal window
Attachment 303716 [details] pushed as e1a8278 - gl: win32: Fix leaked GstGLContext
Attachment 303757 [details] pushed as 247697c - gl: win32: fix crash when finalizing GstGLContext
Comment 18 Xavier Claessens 2015-05-21 19:38:43 UTC
Created attachment 303779 [details] [review]
gl: win32: remove unused code
Comment 19 Xavier Claessens 2015-05-21 19:38:47 UTC
Created attachment 303780 [details] [review]
gl: win32: do not call SetParent in release_parent_win_id()

When called from gst_gl_window_win32_close(), internal window
could not exist, and if it does it's going to be destroyed just
after that anyway. Also it causes window_proc() to be called
and crash because it gets a NULL context.

When called from gst_gl_window_win32_set_window_handle() we are
going to set another parent anyway, and it's probably better to
reparent directly instead of passing by a NULL parent which could
cause the internal window to popup briefly.
Comment 20 Xavier Claessens 2015-05-21 19:39:27 UTC
Reopening since I found another crash in related code.
Comment 21 Nicolas Dufresne (ndufresne) 2015-05-21 19:40:45 UTC
Review of attachment 303779 [details] [review]:

Looks good.
Comment 22 Nicolas Dufresne (ndufresne) 2015-05-21 19:53:09 UTC
Review of attachment 303780 [details] [review]:

Looks Good.
Comment 23 Nicolas Dufresne (ndufresne) 2015-05-21 20:58:46 UTC
Attachment 303779 [details] pushed as ae77afe - gl: win32: remove unused code
Attachment 303780 [details] pushed as 1a98fac - gl: win32: do not call SetParent in release_parent_win_id()