[vlc-devel] [PATCH 02/17] video_output: translate the mouse coordinates to display coordinates early

[Rémi Denis-Courmont]

Le maanantaina 2. marraskuuta 2020, 14.51.37 EET Alexandre Janniaux a écrit :
> Hi,
> 
> On Mon, Nov 02, 2020 at 02:11:39PM +0200, Rémi Denis-Courmont wrote:
> > Le maanantaina 2. marraskuuta 2020, 13.46.58 EET zhilizhao(赵志立) a écrit :
> > > > On Nov 2, 2020, at 6:15 PM, Alexandre Janniaux <ajanni at videolabs.io>
> > > > wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Mon, Nov 02, 2020 at 10:35:09AM +0100, Steve Lhomme wrote:
> > > >> It seems the issue is that code in macosx.m
> > > >> 
> > > >>    @synchronized(self) {
> > > >>    
> > > >>        _hasPendingReshape = NO;
> > > >>    
> > > >>    }
> > > >> 
> > > >> It might use an atomic bool for this variable. Using less locks is
> > > >> always
> > > >> a
> > > >> good idea when possible. I don't know if/how Objective-C can handle
> > > >> atomics.
> > > >> 
> > > >> On the other hand the original patch is not mandatory and doesn't
> > > >> improve
> > > >> anything much. So if necessary I'm OK with reverting it.
> > > > 
> > > > I read the issue as «you cannot get the display lock for mouse update,
> > > > since display can currently report the mouse state, thus already under
> > > > display lock».
> > > > 
> > > > Is that different from reality?
> > > > 
> > > > Using the work from Marvin, we'll be able to remove the mouse API from
> > > > display on MacOSX, so finally solve this issue.
> > > 
> > > So it’s OK to just revert? The master branch is totally unusable now.
> > 
> > I don't think so. It seems that there's a lock inversion in the MacOS
> > video
> > display plugin that just happens to become (more) visible with the patch.
> 
> Somehow,
> 
>     display lock -> display callback -> lock(self)
>         vs
>     main thread callback -> lock(self) -> vout_MouseState -> new display
> lock()
> 
> The lock inversion didn't exist because the last display lock didn't exist.

The display lock was there before, only behind the control event, instead of 
up-front. The code is relying on luck and on an implementation detail of the  
core (i.e., that the mouse event is processed asynchronously). And AFAIK, the 
locking problem is not even new: it is already observable with splitters for a 
long time. There are probably already time-dependent scenarii to trigger it 
without involving splitters.

I maintain that this just makes it more visible, and this is not a problem in 
the core. This is really a general rule that you can't send an events while 
holding a lock that you require to handle the event, namely the internal lock 
of the MacOS plugin. Modules should not send events to themselves, but if they 
do anyway, then they're responsible for handling doing so asynchronously and 
handling recursion.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/