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

[Alexandre Janniaux]

Hi,

On Mon, Nov 02, 2020 at 03:14:16PM +0200, Rémi Denis-Courmont wrote:
> 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.

Thank you for precision, but I don't want to incriminate
something in the message, just shed some light on the lock
issue.

Regards,
--
Alexandre Janniaux
Videolabs