[vlc-devel] [PATCH 1/5] macosx: store the vout_window_t in the VLCOpenGLVideoView

[Steve Lhomme]

On 2020-11-02 17:20, Alexandre Janniaux wrote:
> Hi,
> 
> On Mon, Nov 02, 2020 at 04:18:11PM +0100, Steve Lhomme wrote:
>> This value is not going to change the life of the module so doesn't need any
>> locking.
> 
> Unfortunately like mentioned in previous threads, this is not
> «true» in that mouse callback can happen even after the vout
> window is closed, because the object (~=sys of the module)
> implementing the behaviour might still exist and be waiting to
> be destroyed/removed in the main thread. Storing the vout_window_t
> change nothing to that since there's no refcounting of its lifetime.

OK I see the issue. That's because the 
removeVoutSubview()/removeFromSuperview() are called asynchronously in 
the Close. So the VLCOpenGLVideoView still exists for a short time after 
the Close. An atomic bool disabling mouse handling once the Close is 
called should be enough. It will avoid the lock that is causing the 
deadlock.

Alternatively waiting until the VLCOpenGLVideoView is effectively 
removed from its parent view in Close seems to be an even cleaner 
solution. If the core creates a new macos display right after this one 
is closed it may be added to the parent view before the old one is 
finished removing (even though the calls are likely queued). IMO when 
leaving the Close the display module should not be functional. In 
particular the core should be free to do whatever it wants with the 
window ASAP. This window ("drawable-nsobject") may even not be managed 
by VLC at all.

If there is a way for a VLCOpenGLVideoView to be notified when it's 
removed from the parent view, then it can trigger a condition variable 
that the Close would wait on. It seems to be possible with 
didMoveToSuperview(), the "superview" should be nil when it's removed.


> The proper solution is Marvin's new module. My suggestion is a simple
> workaround splitting the necessary lifetime checker from the lock of
> the global state, avoiding the interleaving of locks, but which cannot
> work for Close() because you need to actually modify the state at this
> point, leaving an opportunity for a deadlock. But it helps shedding
> some light on the locking pattern which your patch seems to ignore.
>  > I agree that it usually works like this because most display are written
> in a synchronous manner with the vout thread, but apple ones and
> especially windowing events are asynchronous and on top of that most code
> interacting with that need to be executed in the main thread.
> 
> For iOS, I will submit the new refactor (much alike Marvin's work) in
> a few days as I've solved the latest issues.
> 
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
> 
>> ---
>>   modules/video_output/macosx.m | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/modules/video_output/macosx.m b/modules/video_output/macosx.m
>> index 9ce48deaafc..bcb71579d68 100644
>> --- a/modules/video_output/macosx.m
>> +++ b/modules/video_output/macosx.m
>> @@ -96,8 +96,10 @@ vlc_module_end ()
>>   @interface VLCOpenGLVideoView : NSOpenGLView
>>   {
>>       vout_display_t *vd;
>> +    vout_window_t *window;
>>       BOOL _hasPendingReshape;
>>   }
>> +- (void)setVoutWindow:(vout_window_t *)aWindow;
>>   - (void)setVoutDisplay:(vout_display_t *)vd;
>>   - (void)setVoutFlushing:(BOOL)flushing;
>>   @end
>> @@ -189,6 +191,7 @@ static int Open (vout_display_t *vd, const vout_display_cfg_t *cfg,
>>               goto error;
>>           }
>>
>> +        [sys->glView setVoutWindow:cfg->window];
>>           [sys->glView setVoutDisplay:vd];
>>
>>           /* We don't wait, that means that we'll have to be careful about releasing
>> @@ -528,6 +531,11 @@ static void OpenglSwap (vlc_gl_t *gl)
>>       [self setFrame:[parentView bounds]];
>>   }
>>
>> +- (void)setVoutWindow:(vout_window_t *)aWindow
>> +{
>> +    window = aWindow;
>> +}
>> +
>>   /**
>>    * Gets called by the Close and Open methods.
>>    * (Non main thread).
>> --
>> 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
>