[vlc-devel] [PATCH] macosx: call input_changed in extensions

[Josh Watzman]

This is obnoxiously complicated -- see comments in code. If anyone cares about
playing_changed or meta_changed, something similar will probably have to be
done.
---
 modules/gui/macosx/intf.h |    2 +-
 modules/gui/macosx/intf.m |   73 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/modules/gui/macosx/intf.h b/modules/gui/macosx/intf.h
index cf9edde..5fa2490 100644
--- a/modules/gui/macosx/intf.h
+++ b/modules/gui/macosx/intf.h
@@ -83,7 +83,7 @@ struct intf_sys_t
 @interface VLCMain : NSObject <NSWindowDelegate, NSApplicationDelegate>
 {
     intf_thread_t *p_intf;      /* The main intf object */
-    input_thread_t *p_current_input;
+    input_thread_t *p_current_input, *p_input_changed;
     id o_mainmenu;              /* VLCMainMenu */
     id o_prefs;                 /* VLCPrefs       */
     id o_sprefs;                /* VLCSimplePrefs */
diff --git a/modules/gui/macosx/intf.m b/modules/gui/macosx/intf.m
index fbfd1dd..b917076 100644
--- a/modules/gui/macosx/intf.m
+++ b/modules/gui/macosx/intf.m
@@ -62,6 +62,7 @@
 #import "CoreInteraction.h"
 #import "TrackSynchronization.h"
 #import "VLCVoutWindowController.h"
+#import "ExtensionsManager.h"
 
 #import <AddressBook/AddressBook.h>         /* for crashlog send mechanism */
 #import <Sparkle/Sparkle.h>                 /* we're the update delegate */
@@ -249,6 +250,7 @@ void WindowClose(vout_window_t *p_wnd)
  * Run: main loop
  *****************************************************************************/
 static NSLock * o_appLock = nil;    // controls access to f_appExit
+static NSLock * o_plItemChangedLock = nil;
 
 static void Run(intf_thread_t *p_intf)
 {
@@ -256,12 +258,14 @@ static void Run(intf_thread_t *p_intf)
     [VLCApplication sharedApplication];
 
     o_appLock = [[NSLock alloc] init];
+    o_plItemChangedLock = [[NSLock alloc] init];
 
     [[VLCMain sharedInstance] setIntf: p_intf];
     [NSBundle loadNibNamed: @"MainMenu" owner: NSApp];
 
     [NSApp run];
     [[VLCMain sharedInstance] applicationWillTerminate:nil];
+    [o_plItemChangedLock release];
     [o_appLock release];
     [o_pool release];
 
@@ -378,7 +382,60 @@ static int PLItemChanged(vlc_object_t *p_this, const char *psz_var,
                          vlc_value_t oldval, vlc_value_t new_val, void *param)
 {
     NSAutoreleasePool * o_pool = [[NSAutoreleasePool alloc] init];
-    [[VLCMain sharedInstance] performSelectorOnMainThread:@selector(PlaylistItemChanged) withObject:nil waitUntilDone:NO];
+
+    // This is a pretty bizarre two-step system to inform the extension manager
+    // that the input has changed, but it's necessary to avoid a series of
+    // possible deadlocks and other issues. Here are other possible approaches
+    // that don't work:
+    //
+    // - Just call into the extension manager in -PlaylistItemChanged on the
+    // main thread. This can pretty easily cause a deadlock if we call
+    // -PlaylistItemChanged twice in quick succession. The first call will poke
+    // the condvar the extension is waiting on, causing the extension thread to
+    // wake up and run extension code; many parts of it -- including the dialog
+    // code -- must be run on the main thread. The extension thread goes back to
+    // sleep while blocking on the main thread to become available, while
+    // holding the extension lock. Meanwhile the main thread goes into the
+    // second call of -PlaylistItemChanged, attempts to lock the extension, and
+    // that's a deadlock.
+    //
+    // - Restructure the dialog manager to never block on the main thread while
+    // holding the extension lock. This should work, but as it turns out doesn't
+    // because the main thread will attempt to lock the same lock twice. What
+    // happens is that -performSelectorOnMainThread works by injecting an event
+    // into the main event loop of the main thread. For some unknown reason, as
+    // part of its processing, when creating an NSAttributedString with HTML, it
+    // runs the main event loop, which means we can end up executing one
+    // -performSelectoOnMainThread as part of another. Since the dialog manager
+    // uses attributed strings with HTML (since dialogs are HTML), we deadlock
+    // here too. This seems strictly like a flaw in NSAttributedString and/or
+    // in -performSelectorOnMainThread and is documented elsewhere:
+    //   http://mrrsoftware.com/blog/tag/nsattributedstring/
+    //   https://www.bluestatic.org/blog/2010/05/31/nsattributedstring-spins-a-nested-run-loop/
+    //
+    // - Change around this bit of code to not force it to run on the main
+    // thread. This would probably work, but, as a newcomer to VLC, I don't
+    // quite know the implications of doing this, particularly since a lot of
+    // code here seems to serailize on the main thread as a way of thread
+    // safety; it would likely require some somewhat intricate restructuring
+    // and adding of locks.
+    //
+    // - Let the extension manager deal with listening for events the same way
+    // that we do here. That would work, but would require duplicating a
+    // nontrivial amount of code from here to deal with tracking the current
+    // input.
+    //
+    // - So, instead, we just serialized all calls to -PlaylistItemChanged (so
+    // we make sure to process them in order, with no one trampling
+    // p_input_changed), do most of the work on the main thread as before, and
+    // then actually inform the extension manager out here where we don't block
+    // the main thread. It seems likely that there are other pre-existing
+    // deadlock possibilities here -- the main thread can't lock an extension!
+    // -- but it at least tends to work in my testing.
+    [o_plItemChangedLock lock];
+    [[VLCMain sharedInstance] performSelectorOnMainThread:@selector(PlaylistItemChanged) withObject:nil waitUntilDone:YES];
+    [[VLCMain sharedInstance] informInputChanged];
+    [o_plItemChangedLock unlock];
 
     [o_pool release];
     return VLC_SUCCESS;
@@ -579,7 +636,7 @@ static VLCMain *_o_sharedMainInstance = nil;
         _o_sharedMainInstance = [super init];
 
     p_intf = NULL;
-    p_current_input = NULL;
+    p_current_input = p_input_changed = NULL;
 
     o_msg_lock = [[NSLock alloc] init];
     o_msg_arr = [[NSMutableArray arrayWithCapacity: 600] retain];
@@ -1274,7 +1331,7 @@ static VLCMain *_o_sharedMainInstance = nil;
 {
     if (p_current_input && (p_current_input->b_dead || !vlc_object_alive(p_current_input))) {
         var_DelCallback(p_current_input, "intf-event", InputEvent, [VLCMain sharedInstance]);
-        vlc_object_release(p_current_input);
+        p_input_changed = p_current_input;
         p_current_input = NULL;
 
         [o_mainmenu setRateControlsEnabled: NO];
@@ -1288,6 +1345,7 @@ static VLCMain *_o_sharedMainInstance = nil;
             [o_mainmenu setRateControlsEnabled: YES];
             if ([self activeVideoPlayback] && [[o_mainwindow videoView] isHidden])
                 [o_mainwindow performSelectorOnMainThread:@selector(togglePlaylist:) withObject: nil waitUntilDone:NO];
+            p_input_changed = vlc_object_hold(p_current_input);
         }
     }
 
@@ -1297,6 +1355,15 @@ static VLCMain *_o_sharedMainInstance = nil;
     [self updateMainMenu];
 }
 
+- (void)informInputChanged
+{
+    if (p_input_changed) {
+        [[ExtensionsManager getInstance:p_intf] inputChanged:p_input_changed];
+        vlc_object_release(p_input_changed);
+        p_input_changed = NULL;
+    }
+}
+
 - (void)updateMainMenu
 {
     [o_mainmenu setupMenus];
-- 
1.7.10.2 (Apple Git-33)



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 896 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20121230/7574b0f9/attachment.sig>