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

[Alexandre Janniaux]

Hi,

On Mon, Nov 02, 2020 at 01:59:51PM +0100, Steve Lhomme wrote:
> On 2020-11-02 13:11, 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.
>
> The issue is that display callbacks are called under sys->display_lock
> (normal). In macos it does a @synchronized on the VLCOpenGLVideoView in a
> few places, which is another kind of lock on the Obj-C object. So far it was
> either locked in the UI thread or in the display thread, under the
> display_lock.
>
> This commit changed things as vout_MouseState() is called by the UI thread,
> under the synchronized VLCOpenGLVideoView, and then it takes the
> display_lock (this patch).
>
> I'm not sure it's possible to remove the synchronized VLCOpenGLVideoView in
> the second case. It's mostly there to make sure the module is not closing.
> It's never going to change during the lifetime of the module.
>
> I'm not sure there is a clean way to handle this. The mouse event is usually
> generated in the UI thread on most (all?) platforms. And sent through this
> code. But the proper way to do it is via the window_t that gets the mouse
> event. This vout_display_SendMouseMovedDisplayCoordinates() seems like
> legacy code, like vout_display_SendEventMousePressed,
> vout_display_SendEventMouseReleased and
> vout_display_SendEventMouseDoubleClick.
>
> The macos code (and other callers of these functions) should store the
> window on init and use that to sent the mouse event. The window will never
> change nor disappear while the display module exists so it can be used
> without a lock.
>
> So the solution is either to revert the patch, or to patch these callers to
> use the window directly and now the "vd" variable. One is fast, but the
> other seems more correct in the long run.

This seems to be a correct solution, but I'm not sure it's completely
correct as-is. vout_display_Send* already just redirect to the window
stored in the `vd` so it's quite similar from what you suggest without
precising how you do it.

The main issue is that the display can be closed while the main thread
is running, thus you can actually remove the vd behind the main thread
back. We could wait to have removed the view to ensure we don't receive
events but then it would deadlock when the player is released in the
main thread before being stopped, which is an very common case in the
VLC executable.

Instead, maybe we could just apply your atomic suggestion to have a
shared state for the vout thread / main thread (could be the
implementation we declared in this file) containing a killed state.
It cannot be only an atomic because of the obvious TOCTOU at the
destruction time so it needs a dedicated locking mechanism that would
not be shared with other method (unlike the self lock synchronization).

It would not solve the deadlock at Close() if you use the mouse, but
would solve all other deadlock so it makes it much more reliable and
usable.

Then, a proper window with Marvin's work would make the final solution
much more reliable.

> Marvin, did you touch that part in your rewriting on the window in macos ?
> (there are still other modules that need some patching as well)
>
> > And if you revert the patch, you'll have to somehow solve the flow control of
> > the video output control queue...
> >
> > --
> > 雷米‧德尼-库尔蒙
> > http://www.remlab.net/
> >
> >
> >
> > _______________________________________________
> > 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