[vlc-devel] [PATCH v2 2/4] macosx: wait until the View is removed from its parent in Close
Marvin Scholz
epirat07 at gmail.com
Tue Nov 3 12:58:38 CET 2020
On 3 Nov 2020, at 12:46, Steve Lhomme wrote:
> 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.
>
I am talking about close callback called from core. You can not wait on
actions happening on the main
thread in the close callback.
This is not about the display lock but about libVLC blocking the main
thread when its stopping and
Closing the vout. So when you wait for an async dispatch happening on
the main thread you will just
wait forever on this as it can never happen, because the main thread is
blocked.
>> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list