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

[Marvin Scholz]

On 3 Nov 2020, at 11:17, Steve Lhomme wrote:

> 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 ?

How would it not block the UI thread?
On close you dispatch to the main thread to detach the view while the 
core is blocking on the main thread.
The async dispatch to the main thread will never run as the core blocks 
the main thread waiting for your
Close() to finish, which waits on the async dispatch to run, which waits 
on core, which waits on your
Close(), etc…

> 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
>>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel