[vlc-devel] [PATCH v2 2/4] macosx: wait until the View is removed from its parent in Close

[Steve Lhomme]

On 2020-11-03 10:50, Marvin Scholz wrote:
> Hi,
> 
> On 3 Nov 2020, at 8:46, Steve Lhomme wrote:
> 
>> ---
>>   modules/video_output/macosx.m | 22 +++++++++++++++++++++-
>>   1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/modules/video_output/macosx.m b/modules/video_output/macosx.m
>> index bcb71579d68..7143c2528fd 100644
>> --- a/modules/video_output/macosx.m
>> +++ b/modules/video_output/macosx.m
>> @@ -46,6 +46,7 @@
>>   #include <vlc_vout_display.h>
>>   #include <vlc_opengl.h>
>>   #include <vlc_dialog.h>
>> +#include <vlc_threads.h>
>>   #include "opengl/filter_draw.h"
>>   #include "opengl/renderer.h"
>>   #include "opengl/vout_helper.h"
>> @@ -98,9 +99,12 @@ vlc_module_end ()
>>       vout_display_t *vd;
>>       vout_window_t *window;
>>       BOOL _hasPendingReshape;
>> +    vlc_sem_t removed;
>>   }
>>   - (void)setVoutWindow:(vout_window_t *)aWindow;
>>   - (void)setVoutDisplay:(vout_display_t *)vd;
>> +- (void)didMoveToSuperview;
>> +- (void)waitRemoved;
>>   - (void)setVoutFlushing:(BOOL)flushing;
>>   @end
>>
>> @@ -303,9 +307,11 @@ static void Close(vout_display_t *vd)
>>               /* release on main thread as explained in Open() */
>>               [viewContainer release];
>>               [glView removeFromSuperview];
>> -            [glView release];
>>           });
>>
>> +        [glView waitRemoved];
>> +        [glView release];
>> +
>>           free (sys);
>>       }
>>   }
>> @@ -531,8 +537,22 @@ static void OpenglSwap (vlc_gl_t *gl)
>>       [self setFrame:[parentView bounds]];
>>   }
>>
>> +- (void)didMoveToSuperview:
>> +{
>> +    if (superview != nil)
>> +        vlc_sem_wait(&removed); // not removed anymore
>> +    else
>> +        vlc_sem_post(&removed); // removed again
>> +}
>> +
>> +- (void)waitRemoved:
>> +{
>> +    vlc_sem_wait(&removed);
>> +}
>> +
> 
> Won't this deadlock, just like using a dispatch sync would deadlock?
> As during close vlc blocks the main thread waiting for the Close to finish,

How would it block the main (UI) thread ? In my solution (cleaner patch 
coming as soon as I can confirm they compile, maybe in an hour or two) 
the glView is released *outside* of the UI thread. And only when the 
view is not attached anymore.
(for now it's pushed here 
https://code.videolan.org/robUx4/vlc/-/blob/bd92e048e0f60764730dcb8d96bfc4218e7ab913/modules/video_output/macosx.m#L273)

The only thing done in the UI thread is detaching the view from the rest 
of the view hierarchy. It may take some time but that should not wait 
for anything happening in the core. When calling Close the core is not 
supposed to do any other call. But it may happen, forwarding pending 
mouse events for example. All of these calls should not be blocking in 
the display module. Further patches in the patchset remove all the 
@synchronized left. So the only lock involved is the display_lock from 
the core.

And I think that's the problem you and Alexandre mention and that is not 
solved: The Close is called under display_lock and the pending mouse 
event(s) will also require the display_lock, from the UI thread. That 
will lead to a deadlock.

So for now, until the window/UI thread is detached from the mac display 
module, it's better to revert the patch I did in the core.

> but close waits for an action that needs to happen on the main thread,
> which can never happen, leading to waiting forever on the semaphore.
> 
>>   - (void)setVoutWindow:(vout_window_t *)aWindow
>>   {
>> +    vlc_sem_init(&removed, 1);
>>       window = aWindow;
>>   }
>>
>> -- 
>> 2.26.2
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>