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 691419 - osxvideosink: doesn't close internal window in case of window-id assignment
osxvideosink: doesn't close internal window in case of window-id assignment
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.10.36
Other Mac OS
: Normal normal
: 1.0.8
Assigned To: Andoni Morales
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-01-09 14:02 UTC by Alexey Chernov
Modified: 2013-07-11 16:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
osxvideosink: close the internal window correctly (1.21 KB, patch)
2013-07-07 20:18 UTC, Andoni Morales
none Details | Review
osxvideosink: close the internal window correctly (1.82 KB, patch)
2013-07-07 20:45 UTC, Andoni Morales
accepted-commit_now Details | Review
osxvideosink: close the internal window when setting a new window handle (1.66 KB, patch)
2013-07-09 11:56 UTC, Andoni Morales
none Details | Review
osxvideosink: close the internal window when setting a new window handle (1.65 KB, patch)
2013-07-09 11:58 UTC, Andoni Morales
reviewed Details | Review
0001-osxvideosink-close-the-internal-window-correctly.patch (2.96 KB, patch)
2013-07-09 14:09 UTC, Andoni Morales
committed Details | Review
0002-osxvideosink-default-to-the-main-in-case-we-are-not-.patch (1.10 KB, patch)
2013-07-09 14:09 UTC, Andoni Morales
committed Details | Review
0003-osxvideosink-delegate-the-window-handle-setup-to-the.patch (3.72 KB, patch)
2013-07-09 14:09 UTC, Andoni Morales
committed Details | Review

Description Alexey Chernov 2013-01-09 14:02:31 UTC
osxvideosink doesn't close its internal window if it's already created when another window-id is assigned using GstXOverlay interface.
Comment 1 Alexey Chernov 2013-01-09 14:04:06 UTC
The problem is related to the bug described in bug report #691421
Comment 2 Sebastian Dröge (slomo) 2013-01-16 10:45:20 UTC
Can you provide a patch for this?
Comment 3 Alexey Chernov 2013-01-16 12:51:20 UTC
Not yet, I tried to fix it but it's still not finished.
Comment 4 Tim-Philipp Müller 2013-03-13 10:43:44 UTC
Have you had a chance to cook up a patch yet?
Comment 5 Alexey Chernov 2013-03-14 08:27:42 UTC
No, not really. Hopefully I'll give it another try today and tomorrow, perhaps, would be able to fix it this time.
Comment 6 Andoni Morales 2013-07-07 20:18:12 UTC
Created attachment 248567 [details] [review]
osxvideosink: close the internal window correctly

Make sure to close our internal window properly
Comment 7 Alexey Chernov 2013-07-07 20:22:42 UTC
Thanks, Andoni. Hope to try your patch tomorrow, too.
Comment 8 Andoni Morales 2013-07-07 20:45:27 UTC
Created attachment 248568 [details] [review]
osxvideosink: close the internal window correctly

Fixes a small issue with the autorelease pool
Comment 9 Alexey Chernov 2013-07-08 19:25:08 UTC
I tried to apply the patch to my modified version, it didn't work out of the box (maybe, my fault in applying) but I also added this closing code to *_set_window_handle() function and modify it a little bit so that it finally started working correctly. I hope to prepare a patch this week (I've done some considerable changes so it needed to be sorted out a little bit).
Comment 10 Andoni Morales 2013-07-09 08:22:32 UTC
Actually this patch only guaranties that the window is closed on a state change transition and  might require something more only when adding the window handle.
Comment 11 Sebastian Dröge (slomo) 2013-07-09 09:11:02 UTC
The window handle set by set_window_handle() should only be considered for each switch from READY->PAUSED. Changing the window handle in PAUSED/PLAYING should not have any effect until the next time the sink goes through READY.

Is that the problem here?
Comment 12 Andoni Morales 2013-07-09 11:56:23 UTC
Created attachment 248708 [details] [review]
osxvideosink: close the internal window when setting a new window handle

Fix the case in which we have an internal window and we change the window handle in playing.
Comment 13 Andoni Morales 2013-07-09 11:58:26 UTC
Created attachment 248709 [details] [review]
osxvideosink: close the internal window when setting a new window handle

Fix indentation
Comment 14 Sebastian Dröge (slomo) 2013-07-09 12:51:26 UTC
Review of attachment 248709 [details] [review]:

::: sys/osxvideo/osxvideosink.m
@@ +644,3 @@
+      osxvideosink->osxwindow->closed = TRUE;
+      [osxvideosink->osxwindow->win close];
+      [osxvideosink->osxwindow->win release];

You do realize that this can be called from any thread? Is this a problem? What happens if at the same time the sink wants to render something, shouldn't there be some locking?
Comment 15 Andoni Morales 2013-07-09 13:00:40 UTC
Review of attachment 248709 [details] [review]:

Right, this should be delegated to the main thread like it's done in the other operations done in this function.  No locking would be needed in that case since rendering would than happen in the same thread. Thanks for the review :)
Comment 16 Andoni Morales 2013-07-09 13:01:46 UTC
Review of attachment 248709 [details] [review]:

In fact, some locking is still needed there.
Comment 17 Andoni Morales 2013-07-09 14:09:08 UTC
Created attachment 248730 [details] [review]
0001-osxvideosink-close-the-internal-window-correctly.patch
Comment 18 Andoni Morales 2013-07-09 14:09:31 UTC
Created attachment 248731 [details] [review]
0002-osxvideosink-default-to-the-main-in-case-we-are-not-.patch
Comment 19 Andoni Morales 2013-07-09 14:09:48 UTC
Created attachment 248732 [details] [review]
0003-osxvideosink-delegate-the-window-handle-setup-to-the.patch
Comment 20 Alexey Chernov 2013-07-10 15:41:04 UTC
Thanks, Andoni and Sebastian. I've tested my code with changing window-id during PLAYING state, it works, but I don't have any locks, too, so we will wait for Andoni's implementation be merged.

One thing that, I think, is important in mine, is that it seems better to create GstGLView outside Cocoa's GstOSXVideoSinkWindow as it is used both by external and internal windows and it's usually a pain to look after all these copies generated in osxvideosink.m and in cocoawindow.m.
Comment 21 Andoni Morales 2013-07-10 15:45:55 UTC
commit d57ef52cadd51643dfc812779dac444bf3d9eaae
Author: Andoni Morales Alastruey <ylatuya@gmail.com>
Date:   Tue Jul 9 15:34:04 2013 +0200

    osxvideosink: defer the window handle setup to the main thread

commit 34a5b93637c40b782ab7a768c31e84ed1737981c
Author: Andoni Morales Alastruey <ylatuya@gmail.com>
Date:   Tue Jul 9 15:33:18 2013 +0200

    osxvideosink: default to the main in case we are not setup yet

commit 0e321b87d4df4f86176cb71ff87f68a8a802f015
Author: Andoni Morales Alastruey <ylatuya@gmail.com>
Date:   Sun Jul 7 22:16:05 2013 +0200

    osxvideosink: close the internal window correctly