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

[Steve Lhomme]

On 2020-11-03 12:17, Marvin Scholz wrote:
> 
> 
> 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.

Are you talking about close from the GUI or the Close callback called 
from the core ? I am talking about the latter. The core waits on 
display_lock, not the main/UI/GUI thread.

It would wait with this patch, but the removeSubview is not supposed to 
do anything involving the core (or display_lock). It does however if 
there are pending display_lock waiting in the UI thread. There is the 
deadlock.

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