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

[Steve Lhomme]

On 2020-11-03 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 ? 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.

Another solution would be to forbid calls to the core from the UI thread 
when the Close starts. That can be done with an atomic bool. But there 
can still be a race condition between the moment locks the display_lock 
and the moment the UI thread checks it can still use it.